fix(reminders): reconcile stale-owner registrations - #10612
Conversation
Persist registrations received by stale owners without creating transient local schedules. Refresh active reminder services before deterministic test clock advances so owner readiness is explicitly observed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a reminders ownership race in Orleans where a reminder registration/update could be persisted on a silo which no longer owns the grain’s ring range and incorrectly be reconciled into that stale owner’s local schedule. The change restores the invariant that only the current ring-range owner schedules and ticks reminders locally, and updates deterministic reminder tests to explicitly drive storage reconciliation before advancing fake time or asserting ownership.
Changes:
- Gate
LocalReminderService.RegisterOrUpdateReminderlocal reconciliation so it only schedules reminders when the service currently owns the grain’s ring range. - Update reminder test helpers to actively refresh reminder services and synchronize local schedules before/after fake-time advancement and before waiting on tick/counter progress.
- Improve timeout diagnostics for schedule synchronization to include reminder identity, owners/silos, tick count, and current fake time.
Show a summary per file
| File | Description |
|---|---|
| test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs | Adds a refresh/synchronization barrier so deterministic reminder tests drive reconciliation explicitly and get better diagnostics on timeout. |
| src/Orleans.Reminders/ReminderService/LocalReminderService.cs | Prevents stale ring-range owners from creating local schedules for newly persisted reminder registrations/updates. |
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: 0
- Review effort level: Lite
| { | ||
| entry.ETag = newEtag; | ||
| ReconcileLocalReminder(entry, _timeProvider.GetUtcNow().UtcDateTime); | ||
| // A request can arrive on a stale owner. Persist it here, but let the current owner load it. |
There was a problem hiding this comment.
It would be better to forward to the new owner, but that is a bigger change. We should open an issue for it - we could use the Virtual Synchrony approach, similar to the DistributedGrainDirectory
Problem
Reminder registration requests can arrive on a silo which no longer owns the reminder's ring range. The service persisted the registration and also created a local schedule on that stale owner. The real owner only discovered the row during a later refresh, producing transient duplicate owners or leaving deterministic fake-time tests waiting for an owner transition which could not occur until time advanced.
This explains the recurring Azure suite pattern: missing local schedule-update diagnostics, tick-readiness waits timing out, and duplicate owner assertions after topology changes.
Solution
Only reconcile a newly persisted registration into the local schedule when the receiving reminder service currently owns the grain's ring range. Non-owner registrations remain durable and are loaded by the current owner during reconciliation.
Make the deterministic reminder tests explicitly refresh active reminder services before waiting for owner schedules or advancing fake time. Timeout diagnostics now include reminder identity, owner silos, tick count, and fake time.
Rationale
The runtime change restores the existing ownership invariant instead of masking the race with longer timeouts. The test barrier drives the same storage reconciliation used in production, removing the circular dependency between periodic refresh and fake-time advancement.
Fixes #10499
Microsoft Reviewers: Open in CodeFlow