Collect cycle heads transitively#1050
Merged
MichaReiser merged 11 commits intosalsa-rs:masterfrom Jan 5, 2026
Merged
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
✅ Deploy Preview for salsa-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
CodSpeed Performance ReportMerging #1050 will not alter performanceComparing Summary
|
MichaReiser
changed the title
MichaReiser
force-pushed
the
micha/cycle-participant-keep-lock
branch
from
d9c263e to
797f85b
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an unsoundness issue in Salsa's cycle detection by transitively collecting cycle heads instead of only going one level deep. The key changes ensure that queries participating in nested cycles properly track all outer cycle heads, which is necessary for correct memoization invalidation and cycle finalization.
- Refactored
collect_all_cycle_headsto recursively collect all cycle heads transitively - Added panic in
complete_cycle_participantwhen outer_cycle is None for non-self-dependent cycle participants - Added regression test demonstrating the necessity of transitive cycle head collection
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/parallel/cycle_nested_deep_conditional.rs |
Fixed thread span naming to correctly identify threads t2, t3, and t4 |
tests/cycle_stale_cycle_heads.rs |
New regression test demonstrating the stale cycle heads scenario with deeply nested cycles |
src/function/execute.rs |
Core logic changes: reordered condition check in outer_cycle, refactored collect_all_cycle_heads to use recursive collection, and added panic for missing outer cycle |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MichaReiser
force-pushed
the
micha/cycle-participant-keep-lock
branch
from
e5e840f to
56c62b9
Compare
MichaReiser
force-pushed
the
micha/cycle-participant-keep-lock
branch
from
5039f35 to
b92dc45
Compare
ibraheemdev
approved these changes
Dec 31, 2025
| for (head_key, iteration_count) in missing_heads { | ||
| max_iteration_count = max_iteration_count.max(iteration_count); | ||
| depends_on_self |= head_key == database_key_index; | ||
| let mut missing_heads: SmallVec<[(DatabaseKeyIndex, IterationCount); 4]> = SmallVec::new(); |
Member
There was a problem hiding this comment.
I assume the choice of 4 is ~arbitrary here?
Co-authored-by: Ibraheem Ahmed <ibraheem@ibraheem.ca>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I started looking into fixing the
maybe_changed_afterunsoundness when I noticed that I don't fully understand how it can happen thatouter_cycleisNonefor a query that has cycle heads but doesn't depend on itself. It's essential that there's always an outer cycle in that case that finalizes the cycle participant once it converges.My first change was to make Salsa panic if we encounter the situation that
outer_cycleisNone(and the query has cycle heads but doesn't depend on itself), but we then started seeing frequent panics in ty. So I dug a bit deeper and found that we need to transitively collect the cycle heads (only going one level deep isn't enough).I added a regression test that demonstrates why transitively collecting all cycle heads is necessary.
Knowing the outer cycle is not only necessary so that we can transfer the head's lock to the proper outer cycle, it's also necessary so that the memo's considered stale when the iteration count of the outer-most cycle changes (
validate_same_iteration).