Accumulator bindings can remain after the facts that created those bindings are retracted

[WilliamParker]

I'd expect the ?temperature binding go away in the following case:

(defn invalid-accum-bindings-downstream []
                    (let [r (dsl/parse-rule [[?result <- (acc/all) :from [Cold (= ?temperature temperature)]]]
                                            (insert! (->TemperatureHistory [?result ?temperature])))
                          q (dsl/parse-query [] [[TemperatureHistory (= ?temps temperatures)]])]
                      (-> (mk-session [r q] :cache false)
                          (insert (->Cold 10))
                          fire-rules
                          (retract (->Cold 10))
                          fire-rules
                          (query q))))

However, it does not:

clara.test-rules> (invalid-accum-bindings-downstream)
({:?temps [() 10]})

I believe the reason for this is that when we retract an element
from an AccumulateNode we repropagate the new result, if any exists,
with the same bindings as the token that was retracted. [1]
The same behavior exists for the AccumulateWithJoinFilterNode [2].
However, if the original elements that created these bindings in the
first place are retracted it is no longer valid for these bindings to exist.

I discovered this while writing tests for [3] but the behavior is the same both before
and after those changes; this is a different concern.

1. https://github.com/rbrush/clara-rules/blob/0.11.1/src/main/clojure/clara/rules/engine.cljc#L822
2. https://github.com/rbrush/clara-rules/blob/0.11.1/src/main/clojure/clara/rules/engine.cljc#L1005
3. Accumulators perform unnecessary retractions when right-(activate/retract) doesn't change the final result #182

[rbrush]

Good find. This is similar enough to #189 that I'll comment on both here, at least for now.

I think we need to add a check when the last value is retracted and include logic similar to the logic at [1], where we only send the initial value if all bindings are satisfied. (Not clear how to do this check that the "last" value is retracted for an accumulator. I'm going to take a closer look at that.)

As for the discussion in #102, I had forgotten about that issue but think that we shouldn't propagate down the network unless all variables can be bound, which I think is what I was trying to do with the logic at [1]. That would actually simplify the fix to this problem, since we'd propagate the initial value per distinct set of bindings in the accumulator, which fits in nicely with the existing flow since we activate the accumulator with each distinct set of bindings. Furthermore, I like the guarantee that a rule can only be activated if everything is bound...eliminates errors from subsequent tests and the rule itself.

[1]
https://github.com/rbrush/clara-rules/blob/0.11.1/src/main/clojure/clara/rules/engine.cljc#L717

[WilliamParker]

I see two broad approaches on how to check that the last value is retracted:

• Store appropriate things in the memory so that the accumulator can know this. I see no reason why this couldn't be done, but I haven't though thought deeply about the consequences for recalculation of things and the resulting computational complexity. It also wouldn't work for streaming accumulators. This may relate to the discussion at Rethinking AccumulateNode #190
• Treat retracting back to the initial value as a full retraction. I see some merit to this, in that if the retracted value is equal to the initial value, the accumulator is arguably stating that this retraction makes the new value logically equivalent to the absence of facts.

[rbrush]

Is it safe to say that we can close this issue, given the discussion in #102 and the remaining edge case being tracked in #218?

[WilliamParker]

@rbrush I agree. Closing.