Fix Machine.deleteRule isolation for shared wildcard sub-patterns (#255)

[sanchezdale]

Issue #, if available:

#255

Description of changes:

We found a bug where cloning a rule (same rule, but different name) and then deleting one of them would cause a couple of non-deterministic behaviors but only when one of the rules had a wildcard.

The two possibilities were:

• a ghost rule: a rule that would keep matching even though it was supposed to be deleted, or
• an *orphaned rule: a rule that would stop matching even though it was still in the Trie and never got deleted.

We reported this in #255, and this PR is one option to fix it.

What we narrowed it down to:

When two rules share a wildcard clause (like *bar*), Ruler shares the trie transitions between them, but each rule still gets its own match marker at the end of that shared path (its own ByteMatch to NameState).

The delete path didn't handle that:

1. ByteMachine.findPattern(...) only returned one NameState for the pattern, and which one depended on HashSet iteration order (that's the non-determinism). Half the time delete grabbed the surviving rule's NameState, didn't find the rule we were deleting, and did nothing so a ghost.
2. When it did grab the right NameState, it emptied it and then tore down the shared transitions but the teardown guard (doesNameStateContainPattern) only checked that one NameState, never the sibling still using the shared path so an orphan.

The fix (delete/teardown path only, no changes to addRule or matching):

• Added ByteMachine.findAllPatterns which returns all NameStates a pattern leads to. Delete now removes the sub-rule from the NameState that actually holds the rule being deleted → kills the ghost.
• Teardown now only removes the shared byte transitions once no NameState references the pattern anymore (noNameStateContainsPattern) → kills the orphan.
• The common single-NameState case (exact, prefix, numeric, $or, etc.) runs the original logic unchanged (deleteStepForNameState), so nothing else moves.

Testing:

• Added repro tests for both ghost and orphan, plus the 3 exact reproductions from Machine.deleteRule() silently fails or corrupts sibling rules when ruleJson contains wildcard patterns #255. They're non-deterministic (~50%/trial), so each runs over 200 fresh machines and requires a 100% pass rate — they fail on main and pass with the fix.
• Added boundary tests to keep already-safe cases safe: different wildcards on the same field, shared prefix, and rules sharing nothing.
• Full suite green (767 tests).

Benchmark / Performance (for source code changes):

Perf note (grain of salt): I ran this on my laptop, not a clean/isolated CI box, so please treat these as directional rather than authoritative. This change only touches the add/delete path, the matching path (rulesForJSONEvent) is untouched, so I wouldn't expect any real effect on match throughput.

Ran on: Apple M4 Pro (8P + 4E), 48 GB RAM, macOS 26.4.1, OpenJDK 26.0.1. StableBenchmarks, warmup=10 / measure=30, origin/main vs fix/wildcard-delete-isolation. Metric is events/sec, higher is faster.

rule_type	before eps (±stddev)	after eps (±stddev)	delta %
EXACT	635,982 (±0.65%)	616,302 (±0.68%)	-3.09%
WILDCARD	491,418 (±1.61%)	479,476 (±1.36%)	-2.43%
PREFIX	616,881 (±2.48%)	630,169 (±0.60%)	+2.15%
PREFIX_EQUALS_IGNORE_CASE	615,477 (±2.70%)	631,194 (±0.61%)	+2.55%
SUFFIX	611,150 (±1.93%)	618,821 (±0.67%)	+1.26%
SUFFIX_EQUALS_IGNORE_CASE	606,296 (±1.53%)	618,413 (±0.82%)	+2.00%
EQUALS_IGNORE_CASE	550,053 (±2.40%)	566,541 (±0.54%)	+3.00%
NUMERIC	398,969 (±1.27%)	408,582 (±0.35%)	+2.41%
ANYTHING_BUT	365,110 (±1.74%)	360,499 (±1.61%)	-1.26%
ANYTHING_BUT_IGNORE_CASE	359,776 (±1.17%)	355,480 (±1.32%)	-1.19%
ANYTHING_BUT_PREFIX	374,925 (±1.29%)	374,804 (±1.18%)	-0.03%
ANYTHING_BUT_SUFFIX	365,927 (±1.93%)	364,181 (±0.58%)	-0.48%
ANYTHING_BUT_WILDCARD	391,149 (±1.02%)	409,018 (±0.66%)	+4.57%
COMPLEX_ARRAYS	89,871 (±1.37%)	90,018 (±1.21%)	+0.16%

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[sanchezdale]

Hey @fym-rgb! Hope you don't mind the tag! saw you merged here recently so figured you'd be a good person to flag this to. I ran into a deleteRule bug with shared wildcard patterns and took a stab at a fix here. we also filed a bug #255 Would be great to get your eyes on it!

[fym-rgb]

Hi @sanchezdale Thanks for the excellent report and fix — the writeups in #255 and this PR are some of the clearest I've seen. Also appreciate the validation details.

I verified this independently, the issue is real and the fix does what it claims to do. The mechanism analysis matches my own as well.

A few items before merge:

1. Required licensing confirmation. The PR body is missing the confirmation line from our PR template. Could you reply here confirming:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

2. Another gap I found while investigating. anything-but: {wildcard: ...} has the same defect and isn't covered: findAllPatterns routes ANYTHING_BUT_WILDCARD (and the other ANYTHING_BUT* types) to the default: single-NameState path, and since anything-but-wildcard values are wildcard-parsed into the ByteMachine, the same duplicate-NameState mechanism applies.

A sample test to confirm this if you are interested:

@Test
public void anythingButWildcard_sharedClause_deleteIsolation() throws Exception {
    String ruleJson = "{\"name\":[\"test\"],\"properties.foo\":[{\"anything-but\":{\"wildcard\":\"*bar*\"}}]}";
    String event = "{\"name\":\"test\",\"properties\":{\"foo\":\"nomatchhere\"}}";

    for (int i = 0; i < 200; i++) {
        Machine machine = Machine.builder().build();
        machine.addRule("rule1", ruleJson);
        machine.addRule("rule2", ruleJson);
        assertEquals(2, machine.rulesForJSONEvent(event).size());

        machine.deleteRule("rule1", ruleJson);

        List<String> after = machine.rulesForJSONEvent(event);
        assertFalse("rule1 should be deleted", after.contains("rule1"));
        assertTrue("rule2 should still match", after.contains("rule2"));
    }
}

The ANYTHING_BUT* find paths are multi-value, so extending findAllPatterns there is a bit more involved.

I'll open a follow-up issue and fix that separately — no action needed from you.

3. And another issue I found as I was reviewing this: In the old code, when candidateSubRuleIds trimmed to empty in the non-terminal case, deleteStep returned immediately, skipping the remaining patterns for that key; the new multi-NameState branch ignores the boolean from deleteStepForNameState, so later patterns for the same key still get processed. I probed this and I'm accepting the new behavior deliberately: value lists in a rule are OR semantics, so continuing to later values is the OR-consistent direction, and the re-initialization is safely bounded since it can only admit sub-rules that match the target rule name and are registered under a provided pattern at a NameState reachable through the earlier keys' provided patterns.

4. Nit. findAllPatterns duplicates findPattern's type switch. Consider implementing findPattern on top of findAllPatterns (return the single element or null) so the two switches can't drift when a new pattern type is added.

Can you confirm No. 1, and I will also add No. 4 on top.

[sanchezdale]

Thanks @fym-rgb!really appreciate the thorough review and verification. Good catch on anything-but: {wildcard: ...}, and thanks for taking that on. I’ve updated the PR description. Thanks again!

[fym-rgb]

No problem @sanchezdale. CI benchmarks are running now. Once they're clean I'll merge this, and I'll cut a tagged pre-release on GitHub you can build against right away, so you're not blocked on the adjacent gaps I'm fixing separately (they don't (or shouldn't) affect your patterns since plain wildcard is fully covered by this PR). A Maven Central release will follow once the remaining fixes land.

[fym-rgb]

Approving — all pre-merge items are closed:

• Licensing: Apache 2.0 confirmation added to the PR description. Thanks!
• Correctness: independently verified — the 8 new tests fail on unmodified main (both ghost and strand reproduce) and the full suite is 767/767 green on this branch. The single-NameState delete path is byte-identical to the previous logic, so non-shared patterns are untouched.
• Performance: CI green on all 4 JDKs (8/11/17/21). I compared the CI benchmark output against the last main run: no rule type regresses consistently across JDKs — deltas are within cross-runner noise, as expected since none of the changed methods are reachable from rulesForJSONEvent.
• Item 4 (nit): landed on the branch as 4f714af (findPattern now delegates to findAllPatterns, single type switch).
• Item 3 (multi-NameState candidate flow): accepted deliberately as reviewed above.
• Item 2 (anything-but wildcard gap): pre-existing, tracked separately; fix coming as a follow-up on top of this.

Merging with rebase to keep authorship of the fix and the nit distinct. A tagged pre-release you can build against follows right after; Maven Central once the remaining fixes land.

Thanks again for a model contribution.