tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6075733957
Thanks for the work on this, @Rikkola . Here are five issues identified by
AI.
---
## 1. Infinite loop: veto reset with multiple anchors
**File:** `VetoSignalProcessor.java:89` / `DynamicFilter.java:61-65`
When two left tuples activate `sequence(not(X), D)` and X is inserted,
`fireAllRules()` never returns.
`DynamicFilter.assertObject()` iterates the `signalAdapters` linked list.
For each adapter it calls `receive()`, which flows into
`VetoSignalProcessor.consume()` — that deactivates the current step, resets to
step 0, and re-activates step 0 synchronously. Re-activation re-registers the
adapter at the **tail** of the same list that is being iterated. With two
anchors the two adapters keep chasing each other.
The pre-captured `next` pointer guards against removal during iteration but
does not guard against tail-append.
Reproduction:
```java
List<String> results = new ArrayList<>();
Rule rule = rule("multiple-anchors").build(
pattern(declarationOf(String.class))
.expr("anchor", s -> s.startsWith("anchor")),
sequence(not(event("x")), event("d")),
execute(() -> results.add("fired"))
);
KieSession session = KieBaseBuilder.createKieBaseFromModel(
new ModelImpl().addRule(rule)).newKieSession();
session.insert("anchor1");
session.insert("anchor2");
session.fireAllRules();
session.insert(new Event("x"));
session.fireAllRules(); // Expected: returns. Actual: infinite loop.
```
Stack trace from a 2-second watchdog:
```
LogicGate.activate(LogicGate.java:136)
LogicCircuitStep.activate(LogicCircuitStep.java:41)
VetoSignalProcessor.consume(VetoSignalProcessor.java:89)
LogicGate.propagate(LogicGate.java:117)
LogicGate.consume(LogicGate.java:102)
SignalAdapter.receive(SignalAdapter.java:46)
DynamicFilter.assertObject(DynamicFilter.java:63)
```
**Suggested fix:** defer re-subscription until the current event's delivery
completes, so newly registered adapters are not visited for the in-flight event.
---
## 2. Binary incompatibility: `DSL.not()` return type change
**File:** `DSL.java:558`
This is the same one as Copilot suggested. I think you haven't fixed it.
The return type of `public static ExprViewItem not(ViewItemBuilder<?>,
ViewItemBuilder<?>...)` changed to `ExistentialExprViewItem`. This is
source-compatible but **binary-incompatible** — the JVM method descriptor
includes the return type, so pre-compiled callers get `NoSuchMethodError`.
This is not limited to sequence users; any existing executable model that
calls `DSL.not()` is affected. e.g., a KJAR built by executable-model.
Reproduction — compile this client against the parent commit's DSL, then run
it with the PR's DSL on the classpath (without recompiling the client):
```java
import org.drools.model.DSL;
import org.drools.model.view.ExprViewItem;
import org.drools.model.view.ViewItemBuilder;
public class Pr7160BinaryCompatibilityProbe {
public static void main(String[] args) {
ExprViewItem result = DSL.not(
DSL.expr("true", true), new ViewItemBuilder<?>[0]);
System.out.println("Previous API client returned " + result);
}
}
```
Result:
```
java.lang.NoSuchMethodError:
'org.drools.model.view.ExprViewItem org.drools.model.DSL.not(
org.drools.model.view.ViewItemBuilder,
org.drools.model.view.ViewItemBuilder[])'
```
**Suggested fix:** keep the existing method descriptor (`ExprViewItem`
return type). Expose the `SequenceStep` capability through a separate method or
interface.
---
## 3. Nested XNOR activation evaluates stale zero state
**File:** `LogicGate.java:139`
This is similar to what Copilot suggested, but a little more complex.
```java
if (predicate.test(0L, allMatched)) {
```
`activate()` always evaluates against `0L` (empty input). For a standalone
XNOR this is correct — zero inputs means XNOR is satisfied. But in a nested
circuit like `and(xnor(xnor(A, B), C), D)`:
1. The inner `xnor(A, B)` activates first and propagates a match bit to the
outer XNOR.
2. The outer XNOR then activates but ignores the bit already written to its
gate memory — it evaluates `0L` instead of the actual memory.
3. The outer XNOR sees zero inputs and reports "matched," so D alone fires
the rule.
Reproduction:
```java
check(
"nested-xnor",
0,
and(
xnor(xnor(event("a"), event("b")), event("c")),
event("d")
),
"d"
);
// nested-xnor expected=0 actual=1
```
**Suggested fix:** read the actual gate memory in `activate()` instead of
hard-coding `0L`, so bits propagated by child gates during earlier activation
are respected.
---
## 4. Reversion does not re-evaluate the parent gate
**File:** `LogicGateOutputSignalProcessor.java:58-69`
`clearParentBit()` removes the child's contribution bit from the parent and
unconditionally sets the parent to unmatched. It does not re-evaluate the
parent's predicate after clearing the bit.
For `and(xor(xor(A, B), C), D)` with arrival order A, C, B, D:
1. A matches inner XOR (1 input) — matched. Outer XOR gets 1 input — matched.
2. C matches outer XOR (2 inputs) — unmatched (XOR requires exactly 1).
3. B matches inner XOR (2 inputs) — unmatched, reverts. Outer XOR now has
only C — should be matched again.
4. But `clearParentBit` sets the outer XOR to unmatched without
re-evaluating. D cannot fire.
Reproduction:
```java
check(
"nested-xor-reversion",
1,
and(
xor(xor(event("a"), event("b")), event("c")),
event("d")
),
"a", "c", "b", "d"
);
// nested-xor-reversion expected=1 actual=0
```
**Suggested fix:** after clearing the bit, re-evaluate the parent's
predicate. If the result is a state transition (unmatched -> matched),
propagate upward. This needs to be recursive through the circuit.
---
## 5. `resetPrior()` clears parent AND/OR memory, preventing recovery
**File:** `LogicGate.java:120-128`
When a gate propagates a match, `resetPrior()` calls
`memory.resetLogicGateMemory(gateIndex, ...)` for non-reverting gates. This
clears the AND/OR parent's record of **other** inputs that already matched.
For `and(and(xnor(A, B), C), D)` with arrival order C, A, B, D:
1. XNOR starts matched (zero inputs). C arrives — inner AND has both inputs,
propagates, and its memory is reset (C's bit is cleared).
2. A arrives — XNOR becomes unmatched (1 input).
3. B arrives — XNOR is matched again (2 inputs). But the inner AND lost C's
bit, so it sees only XNOR and waits for C again.
4. D cannot fire.
Reproduction (case A — AND recovery):
```java
check(
"and-xnor-recovery",
1,
and(
and(xnor(event("a"), event("b")), event("c")),
event("d")
),
"c", "a", "b", "d"
);
// and-xnor-recovery expected=1 actual=0
```
Same pattern breaks OR (case B — OR retention):
```java
check(
"or-reversion",
1,
and(
or(xor(event("a"), event("b")), event("c")),
event("d")
),
"a", "c", "b", "d"
);
// or-reversion expected=1 actual=0
```
**Suggested fix:** for circuits that contain reverting children, the parent
gate must retain its input memory across propagation rather than resetting it.
`statusCanRevert` on the child alone is not sufficient — the ancestor chain
needs to know it may be re-evaluated.
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
