fix(reminders): decouple reconciliation barrier from scheduler - #10838
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses reminder-suite test flakiness by removing a circular dependency where the reconciliation test barrier required the reminder service scheduler to be available in order to observe/await reconciliation progress. It does this by publishing range-change reconciliation generation state under a lock so the test barrier can snapshot it from the calling thread, and adds a regression test to validate behavior when the scheduler is intentionally blocked.
Changes:
- Publish range-change generation/task state under a dedicated lock so the test barrier can read it off-scheduler.
- Update
TestOnlyWaitForRangeChangeReconciliationto use the new locked snapshot approach instead of scheduler-queued snapshots. - Add a regression test which blocks the reminder-service scheduler and verifies the barrier remains observable.
Show a summary per file
| File | Description |
|---|---|
| test/Orleans.Reminders.Tests/TimerTests/LocalReminderServiceTests.cs | Adds a regression test that blocks the reminder service scheduler to prove the reconciliation barrier remains observable. |
| src/Orleans.Reminders/ReminderService/LocalReminderService.cs | Introduces a lock-protected generation/task snapshot for range-change reconciliation and updates the test-only barrier to use it off-scheduler. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Code coverage77.57% line coverage - 97,533 / 125,731 lines Coverage details
|
|
CI is failing on a PR-specific compatibility test: RangeChangeBarrier_StopsWaitingWhenCanceled returns rue after cancellation, failing the immediate Assert.False at LocalReminderServiceTests.cs:317 in run 32961916933. Current main includes #10815 in the same reconciliation area, so I am rebasing this branch and will update the test/implementation as needed before pushing a fresh build. |
9d6539e to
8d7d2da
Compare
|
Fixed and force-pushed with lease in 8d7d2da. The failure was a publication race: the provider read could report that it was blocked before OnRangeChange published the new reconciliation task, allowing the off-scheduler barrier to observe the previous completed generation. The branch now publishes a proxy reconciliation task before starting provider reads and completes it with the actual refresh task. The three range-change barrier tests pass on net8.0 and net10.0, the cancellation test passed 20/20 stress iterations, and Orleans.slnx builds successfully. |
772c7e0 to
d99913f
Compare
Problem
The newest reminder-suite recurrence after #10815 timed out at the first queued state snapshot in LocalReminderService.TestOnlyWaitForRangeChangeReconciliation. The test barrier depended on the reminder-service scheduler being available while it was specifically waiting for that service to finish reconciliation, so scheduler or provider pressure could prevent the barrier from observing the task it needed to await.
Solution
Publish range-change generation state under a lock and let the test barrier snapshot that state directly from the calling thread. The barrier continues to follow newer generations, awaits the current reconciliation, and propagates failures from the current generation.
Add a regression test which holds the reminder-service scheduler unavailable and verifies that the reconciliation barrier remains observable.
Rationale
This removes the circular scheduler dependency from the test synchronization boundary while preserving the reminder service's reconciliation behavior and generation supersession guarantees.
Fixes #10499
Microsoft Reviewers: Open in CodeFlow