fix(reminders): prevent ticks after unregister - #10296
Merged
ReubenBond merged 1 commit intoJul 22, 2026
Merged
Conversation
Keep stopped reminders and absent-local deletion markers sequence-stamped until a newer table refresh confirms removal, preventing stale reads from restarting deleted reminders. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3e1201f4-2286-4cd5-bb60-3690810a4810
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a race where reminder table refresh results can be stale relative to an unregister operation, allowing a previously unregistered reminder to be recreated locally and fire again after quiescence was reported. It does so by retaining stopped reminders/tombstones with sequence stamps until a newer refresh confirms the deletion, while still allowing immediate stop/restart.
Changes:
- Retain unregistered reminders locally as sequence-stamped stopped entries (and add identity tombstones when no local entry exists) to suppress stale table refresh results.
- Purge tombstones on newer refresh confirmation and on range changes; clear tombstones on successful re-registration.
- Add a regression test covering stop/restart behavior before the removed reminder is purged.
Show a summary per file
| File | Description |
|---|---|
| test/Orleans.Reminders.Tests/TimerTests/ReminderTests_TableGrain.cs | Adds a test for restarting a reminder after unregister but before purge. |
| src/Orleans.Reminders/ReminderService/LocalReminderService.cs | Implements tombstone/stopped-entry tracking and refresh suppression logic to avoid resurrecting reminders from stale reads. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/Orleans.Reminders/ReminderService/LocalReminderService.cs:378
localRemindersis iterated and mutated in the same loop. If any reminder is out-of-range,localReminders.Remove(...)duringforeach (var r in localReminders)will throwInvalidOperationException(collection modified). The new tombstone loop correctly usesKeys.ToArray(), so the reminders loop should follow the same safe pattern.
removedReminderTasks.Add(r.Value.StopAsync(ReminderEvents.LocalReminderStopReason.RemovedFromRange));
localReminders.Remove(r.Key);
}
foreach (var key in localReminderTombstones.Keys.ToArray())
- Files reviewed: 2/2 changed files
- Comments generated: 2
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Azure Table reminder refreshes can complete with a stale snapshot after a reminder has been unregistered. Removing the local entry immediately allowed that stale row to recreate the reminder and deliver another tick after quiescence was reported.
Retain stopped reminders as sequence-stamped tombstones until a newer refresh confirms deletion. Unregisters without a local entry now record an identity tombstone as well, and successful re-registration replaces the stopped entry so immediate stop/restart remains supported.
Microsoft Reviewers: Open in CodeFlow