Fix scheduler fresh matches after subsumption

[saulshanabrook]

Context

Custom schedulers collect fresh candidate matches before deciding which scheduler rows should run. Fresh collection was still allowed to use subsumed body witnesses, so a scheduled rule could run actions for a row that analysis had already subsumed before the scheduler step.

Concrete fix

• Collect fresh scheduler matches from non-subsumed rows by querying the rule body with subsumed rows excluded.
• Add a regression where analysis subsumes (Add (Mul (Num 0) (Num 1)) (Num 2)) before the scheduled ruleset runs. The scheduled rule would match the old affine shape, but the scheduler now sees no fresh valid match, inserts no Hit row, and reports no user-visible update.

Scope

This narrowed PR only fixes fresh scheduler match collection after subsumption. It does not add held-backlog validation or cleanup for matches that were already saved by a persistent scheduler in an earlier iteration.

Validation

• cargo test --lib scheduler
• cargo fmt --check
• make nits (passed; emitted existing rustdoc warnings)
• git diff --check

[codspeed-hq]

Merging this PR will not alter performance

✅ 30 untouched benchmarks
⏩ 232 skipped benchmarks¹

---

_{Comparing saulshanabrook:codex/split-scheduler-stale-match-cleanup (f295052) with main (42a4fb6)}

Footnotes

1. 232 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[codecov-commenter]

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.71%. Comparing base (42a4fb6) to head (f295052).

Files with missing lines	Patch %	Lines
src/scheduler.rs	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #885      +/-   ##
==========================================
+ Coverage   86.68%   86.71%   +0.03%     
==========================================
  Files          90       90              
  Lines       27088    27114      +26     
==========================================
+ Hits        23480    23512      +32     
+ Misses       3608     3602       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[saulshanabrook]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

The scheduler executes each rule in four phases per iteration: query (build worklist), validation (recheck chosen keys, dedupe witnesses), action (run user actions only on validated keys), and cleanup (remove stale internal rows). RunReport semantics were updated so query/validation/cleanup don't count as DB progress; reports are unioned per-iteration.

Changes

Custom Scheduler Validation & Cleanup

Layer / File(s)	Summary
Scheduler Iteration & Report Semantics CHANGELOG.md, src/scheduler.rs	Added validation and cleanup phases to step_rules_with_scheduler; marked validation and cleanup as non-database-progress; derived action-phase can_stop from actions + scheduler decision; combined query/validation/action/cleanup reports.
Scheduler Rule Construction & Worklists src/scheduler.rs	Extended SchedulerRuleInfo to add validation_rule and cleanup_rule; built a validated table; changed the query rule to skip subsumed rows; added a validation rule that collapses multiple body-only witnesses to one keyed validated row; action rule now reads only from validated; added cleanup rule to remove stale decided/validated rows; adjusted Matches::instantiate to dedupe chosen keys and residuals.
Scheduler Tests & Helpers src/scheduler.rs	Added test schedulers (HoldFirstMatchScheduler, CountingScheduler, ChooseFirstScheduler) and tests ensuring subsumed rows aren't scheduled, validation prevents duplicate actions and residual backlog is deduped across iterations, and held matches that become stale are cleaned without running actions; minor imports and explanatory comments added.

Sequence Diagram

sequenceDiagram
  participant Scheduler
  participant QueryRule
  participant ValidateRule
  participant ActionRule
  participant CleanupRule
  Scheduler->>QueryRule: build decided worklist from scheduler matches
  QueryRule->>Scheduler: return query_report
  Scheduler->>ValidateRule: recheck chosen keys in rule body
  ValidateRule->>Scheduler: populate validated, return validation_report
  Scheduler->>ActionRule: execute actions on validated keys only
  ActionRule->>Scheduler: return action_report
  Scheduler->>CleanupRule: remove stale decided/validated entries
  CleanupRule->>Scheduler: return cleanup_report
  Scheduler->>Scheduler: union all four reports

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

• yihozhang

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly reflects the subsumption-related scheduler fix, which is a major part of the changeset.
Description check	✅ Passed	The description is related to the changeset because it describes the subsumption-related scheduler bug fix.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/scheduler.rs`:
- Around line 424-445: The scheduler currently leaves duplicate free_vars keys
in the backlog so duplicates in rule_info.matches are revalidated across
iterations; before persisting residual matches (where rule_info.matches or the
worklist is updated), deduplicate/canonicalize entries by their free_vars key
(e.g., collapse identical Vec/Hashable representations of free_vars) so only one
witness per scheduler key is kept, ensuring the scheduler does not enqueue
duplicates; update the code that pushes residual matches into rule_info.matches
to use a HashSet or sort+dedup on the free_vars key, and add a regression test
that invokes step_rules_with_scheduler twice with ChooseFirstScheduler and
asserts the user action runs only once.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f4009b83-94aa-4f34-8014-8235764ef56c

📥 Commits

Reviewing files that changed from the base of the PR and between 376ccf9 and 2c55caf.

📒 Files selected for processing (2)

• CHANGELOG.md
• src/scheduler.rs

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)

• GitHub Check: build
• GitHub Check: benchmark (ubuntu-latest, rust_rule_tableaction_hot_path)
• GitHub Check: benchmark (ubuntu-latest, rust_rule_match_with_serialize)
• GitHub Check: benchmark (ubuntu-latest, luminal-llama)
• GitHub Check: benchmark (ubuntu-latest, rectangle)
• GitHub Check: benchmark (ubuntu-latest, extract-vec-bench)
• GitHub Check: benchmark (ubuntu-latest, proof_testing_typecheck)
• GitHub Check: benchmark (ubuntu-latest, cykjson)
• GitHub Check: benchmark (ubuntu-latest, conv1d_128)
• GitHub Check: benchmark (ubuntu-latest, taylor51)
• GitHub Check: benchmark (ubuntu-latest, conv1d_32)
• GitHub Check: benchmark (ubuntu-latest, eggcc-2mm)
• GitHub Check: benchmark (ubuntu-latest, herbie)
• GitHub Check: benchmark (ubuntu-latest, python_array_optimize)
• GitHub Check: benchmark (ubuntu-latest, stresstest_large_expr)
• GitHub Check: benchmark (ubuntu-latest, math-microbenchmark)
• GitHub Check: benchmark (ubuntu-latest, eggcc-extraction)
• GitHub Check: coverage
• GitHub Check: test

🧰 Additional context used

🪛 LanguageTool

CHANGELOG.md

[style] ~7-~7: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...subsumption, or deletion from firing. - Fix custom schedulers so multiple body-only...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...es like (rule ((A x y)) ((Hit x))). - Fix custom scheduler cleanup so stale inter...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🔇 Additional comments (1)

CHANGELOG.md (1)

5-8: LGTM!

[saulshanabrook]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[yihozhang]

I'm worried that this will be slow because we now run the query twice, and because the conjunction of (worklost ...) and query may yield a different plan, performance becomes unpredictable.

[saulshanabrook]

Yeah that makes sense. I wonder if there is a way to fix this bug while still not degrading performance... if this does in fact lead to slowdowns.

Some ideas from LLM but wanna look deeper

The key invariant we need is narrower:

A held scheduler key must not cross a subsume / delete boundary and later run actions without either being revalidated or discarded.

So the options are:

1. Keep current PR: always validate chosen keys

   • Correct and simple.
   • Preserves held valid work if another witness still exists.
   • Performance concern remains: every chosen key path has the second body query.

2. Drop held backlog after non-monotonic changes

   • Track a global “invalidation epoch” bumped by subsume / delete.
   • Store the epoch with residual scheduler keys.
   • If a held key is from an older epoch, discard it instead of validating.
   • Passes your repro: analysis subsumes, epoch changes, second copy drops held keys, Hit stays 0.
   • Fastest and simplest alternative, but conservative: it can drop still-valid held matches.

3. Epoch-gated validation

   • Same epoch tracking, but only old held keys go through validation.
   • Fresh keys, or held keys from the current epoch, run through the cheap direct action path.
   • In your repro, validation runs only because analysis happened between scheduler runs.
   • This is probably the best semantic/performance compromise.

4. Per-relation invalidation epochs

   • Like option 3, but only validate/drop if a relation used by that rule body had non-monotonic changes.
   • More precise and faster for unrelated analysis, but more plumbing: rule dependency tracking plus table-level invalidation.

5. Experimental-only workaround

   • Change back-off in egglog-experimental to clear held matches after analysis/non-monotonic phases.
   • Smallest if the only goal is Python param_eq.
   • But it leaves core scheduler semantics footgunny, and other persistent schedulers can still hit the stale held-key bug.

[yihozhang]

What motivates this PR? At first glance, I am more or less okay with the current semantics in the presence of non-monotonic ops like subsume or delete...

[saulshanabrook]

What motivates this PR? At first glance, I am more or less okay with the current semantics in the presence of non-monotonic ops like subsume or delete...

Basically that a backoff scheduler can hold onto a deleted/subsumed value... So then some analysis runs, it gets subsumed, then it runs again and still triggers. This to me breaks my mental semantic model and breaks a lot of reasoning about subsumption. I would assume that after something has been subsumed in one round, nothing in later rounds can match it.

In my Python work it depends on this behavior... Like ones round, runs analysis to saturation (which subsumes for constant folding like stuff among other things), then runs another round. If the next round picks up on things that were subsumed, certain assertions break in my domain model... I can look for a bigger example, but here is a small reproduction:

Held Scheduler Match Repro

The smallest experimental test that needs held-key validation is:

#[test]
fn test_backoff_drops_held_matches_subsumed_by_analysis() {
    let mut egraph = egglog_experimental::new_experimental_egraph();

    egraph.parse_and_run_program(None, r#"
        (ruleset analysis)
        (ruleset copy)
        (datatype Math
          (Add Math Math)
          (Num i64))
        (relation Hit (Math))

        (let expr1 (Add (Num 0) (Num 1)))
        (let expr2 (Add (Num 0) (Num 2)))

        (rewrite (Add (Num 0) x) x :subsume :ruleset analysis)
        (rule ((= e (Add (Num 0) x))) ((Hit e)) :ruleset copy :name "copy-add")

        (let-scheduler bo (back-off :match-limit 1 :ban-length 1))
    "#).unwrap();

    egraph.parse_and_run_program(None, "(run-schedule (run-with bo copy))").unwrap();
    assert_eq!(egraph.get_size("Hit"), 0);

    egraph.parse_and_run_program(None, "(run-schedule (run analysis))").unwrap();

    egraph.parse_and_run_program(None, "(run-schedule (run-with bo copy))").unwrap();
    assert_eq!(egraph.get_size("Hit"), 0);
}

I verified it against experimental origin/main with the current pinned egglog, without held-key validation. It fails:

assertion `left == right` failed: held scheduler keys whose body witnesses were subsumed must not fire
left: 2
right: 0

Mechanism:

1. First (run-with bo copy) sees two matches.
2. back-off :match-limit 1 refuses to run them and keeps them in the scheduler backlog.
3. (run analysis) subsumes both (Add (Num 0) ...) witnesses.
4. Second (run-with bo copy) reuses the held backlog.
5. Without held-key validation, core runs actions from the held scheduler key without rechecking the rule body, so it inserts two stale Hit rows.
6. With held-key validation, validation rechecks decided + rule.body against non-subsumed rows, finds no valid witness, and cleanup drops the stale keys.

This maps to Python param_eq: each loop does scheduled rewrite work, then the next loop runs analysis before scheduled rewrite again. If back-off held rewrite matches, analysis can subsume their witnesses before the scheduler tries them later.

[saulshanabrook]

@yihozhang this is ready for another review. I tightened it up to only handle on thing, correctness of subsumption. I also improved performance, so it only re-checks held over rows to see if they had been subsumed or deleted.

[saulshanabrook]

OK from feedback from @yihozhang and @oflatt that its unclear if this was the intended semantics and also based on performance concerns, I have stripped this PR down to just fixing an even more basic bug, that previously it would find subsumed rows even before they were deferred. I believe this fixes that bug, but let me know if somehow this was intentional for some reason.

I tested my upstream code against this and it works fine.

Sorry for all the time spent reviewing these more complicated fixes, when the simpler one was enough at least for me!

[yihozhang]

Thank you!