Skip to content

Conversation

@MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Oct 29, 2025

This is an alternative, and I think a more footgun-proof fix for #1014

Up to now, Salsa increments the iteration count for all cycle heads participating in a query before starting a new iteration. However, queries with cycle handling that aren't cycle heads never increment their iteration count. Instead, they always use zero (or the iteration count from the last iteration).

However, we use the iteration count in multiple places (e.g. validate_same_iteration, validate_provisional) to assert whether the most recent memo for a given key is "the same" as when a query dependent on it. For this to work, it's essential that the iteration count uniquely identify a query execution, which isn't the case now.

The way #1014 works around this is by adding an extra check to validate_same_iteration that always skips if a query has become a non-cycle head. This works, but it always requires remembering to check both the iteration count and whether the query is still in the cycle heads. In fact, we currently don't perform this check within validate_provisional because we can't; the cycle heads of a finalized memo are often empty. I'm not sure if that's the cause of the flaky test failures but it might as well be.

This PR ensures that salsa either:

  • Increments the iteration count as part of the outer cycle if the query is a cycle head
  • Increments the iteration count when a query with cycle handling completes (that doesn't depend on itself)

Test plan

I tested that the regression test from #1014 still passes and I ran the parallel tests with 10k iterations

@netlify
Copy link

netlify bot commented Oct 29, 2025

Deploy Preview for salsa-rs ready!

Name Link
🔨 Latest commit ec0d26f
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/69026dbabffe520008f78e9f
😎 Deploy Preview https://deploy-preview-1017--salsa-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 29, 2025

CodSpeed Performance Report

Merging #1017 will not alter performance

Comparing MichaReiser:cycle-recovery-dependencies (ec0d26f) with master (671c3dc)

Summary

✅ 13 untouched

@MichaReiser MichaReiser changed the title Track recovery function dependencies as part of the cycle head Experiment with iteration count Oct 29, 2025
@MichaReiser MichaReiser force-pushed the cycle-recovery-dependencies branch 4 times, most recently from 4219e79 to 32f217a Compare October 29, 2025 19:31
@MichaReiser MichaReiser changed the title Experiment with iteration count Increment iteration count for all queries with cycle handling participating in a cycle Oct 29, 2025
@MichaReiser MichaReiser force-pushed the cycle-recovery-dependencies branch from 32f217a to ec0d26f Compare October 29, 2025 19:40
@MichaReiser MichaReiser requested review from carljm and ibraheemdev and removed request for ibraheemdev October 29, 2025 19:40
@MichaReiser MichaReiser marked this pull request as ready for review October 29, 2025 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants