fix(testing): stabilize reminder topology boundaries - #10933
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Review tier: Lite
Findings: None
What changed in this PR
Adjusts reminder lifecycle test synchronization to wait on the latest reminder-table reconciliation after cluster topology stabilizes, preventing obsolete in-flight reads from consuming the scenario deadline and causing flaky timeouts.
Changes:
- Reorders lifecycle harness reconciliation flow to perform an explicit refresh after liveness/manifest stabilization, then wait for the current reconciliation generation.
- Updates
LocalReminderServicetest-only reconciliation tracking so explicit test refreshes participate in the same generation/barrier mechanism as range changes. - Adds a regression test which blocks an obsolete range-change read, performs a newer refresh, and asserts the barrier follows the refresh instead of waiting for the obsolete work.
| File | Description |
|---|---|
| test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs | Refreshes after topology stabilization before waiting on the reconciliation barrier, ensuring the barrier reflects the stable refresh generation. |
| test/Orleans.Reminders.Tests/TimerTests/LocalReminderServiceTests.cs | Adds a controlled regression validating that the reconciliation barrier follows the latest refresh when an obsolete range-change read is stalled. |
| src/Orleans.Reminders/ReminderService/LocalReminderService.cs | Unifies test-only tracking for range-change reconciliations and explicit refreshes under a single reconciliation generation/barrier mechanism. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3061118 to
c20935d
Compare
c20935d to
cb1a19d
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/LocalReminderServiceTests.cs — Now that BlockingSiloStatusListener can be disposed, prefer a using declaration at the creation… |
Suppressed comments (1)
test/Orleans.Reminders.Tests/TimerTests/LocalReminderServiceTests.cs:598
- BlockingSiloStatusListener allocates a ManualResetEventSlim but never disposes it. Since this listener can be created repeatedly across test runs, disposing avoids leaking OS resources associated with the event (especially if its wait handle is materialized).
private sealed class BlockingSiloStatusListener(SiloAddress localSilo) : ISiloStatusListener
{
private readonly TaskCompletionSource _blocked = new(TaskCreationOptions.RunContinuationsAsynchronously);
private readonly ManualResetEventSlim _release = new();
private int _hasBlocked;
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/LocalReminderServiceTests.cs — Now that BlockingSiloStatusListener can be disposed, prefer a using declaration at the creation… View resolved comment |
1e5b63c to
6f5aceb
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
It introduces a breaking change to a public interface and hard-codes a stabilization timeout which may prematurely fail slow provider runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — IReminderServiceLifecycleHarness is a public interface, and adding RegisterOnSiloAsync is a… |
|
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs — WaitForStartupReadinessAsync/WaitForTopologyReconciliationAsync now impose a fixed 30s… |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are internally consistent, remain test-focused (via test-only barriers/diagnostics), and the updated tests align with the stated goal of deterministic topology boundaries without introducing evident behavioral regressions.
Review tier: Lite
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs — WaitForStartupReadinessAsync/WaitForTopologyReconciliationAsync now impose a fixed 30s… View resolved comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — IReminderServiceLifecycleHarness is a public interface, and adding RegisterOnSiloAsync is a… View resolved comment |



Problem
Reminder lifecycle tests treated service startup, membership convergence, and reminder reconciliation as separate partial signals. A silo could report ready while its current membership version had not yet flowed through all status listeners, or a newer range or periodic refresh could supersede the reconciliation being awaited. Tests then refreshed stale ranges, observed duplicate owners, blocked deterministic oracle reads, or timed out reminder registration under provider load.
The stale-owner scenario also used 16 concurrent registrations during uncontrolled topology propagation, making provider throttling and routing timing part of the assertion.
Solution
Rationale
The failures are synchronization defects introduced by the lifecycle and TestKit scenarios, not reminder ownership regressions. The runtime already guarantees that a non-owner registration is persisted without starting a stale local schedule; the revised tests exercise that invariant directly while using explicit lifecycle boundaries for topology convergence.
Fixes #10928
Fixes #10934
Fixes #10938
Fixes #10940
Fixes #10943
Fixes #10955
Fixes #10962