test(reminders): enable provider failover coverage - #10895
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs — The startAdditionalSiloOnNewPort argument is misleading here because StartAdditionalSilosAsync… |
|
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs — GetGrainOwnedBySiloAsync starts a reminder on each candidate grain and only stops it on the… |
What changed in this PR
This PR re-enables previously skipped reminder-service failover coverage across Azure Table and Cosmos providers by centralizing the 1-failure/1-join multi-grain scenario in ReminderTestsBase with explicit startup/reconciliation barriers and robust topology cleanup, and adds an in-memory equivalent test.
Changes:
- Add a shared
Test_Reminders_GT_1F1J_MultiGrainscenario toReminderTestsBase, including explicit reminder-service startup and range-reconciliation waiting plus baseline-topology restoration infinally. - Enable the Azure Table and Cosmos GT failover tests by switching them to call the shared base scenario; add the same scenario to the in-memory TableGrain suite.
- Make Cosmos fixture reminder-service startup explicit by waiting for reminder-service-started diagnostics with a timeout.
| File | Description |
|---|---|
| test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs | Adds the shared GT failover scenario plus new ownership/schedule assertions and improved cleanup/reconciliation helpers. |
| test/Orleans.Reminders.Tests/TimerTests/ReminderTests_TableGrain.cs | Adds an in-memory provider test which runs the shared GT failover scenario. |
| test/Extensions/Orleans.Cosmos.Tests/ReminderTests_Cosmos.cs | Adds explicit reminder-service startup waiting and re-enables the GT failover test via the shared base scenario. |
| test/Extensions/Orleans.Azure.Tests/Reminder/ReminderTests_AzureTable.cs | Re-enables the GT failover test by delegating to the shared base scenario. |
Suppressed comments (1)
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs:324
- Same as above:
StartAdditionalSilosAsyncignoresstartAdditionalSiloOnNewPort, so passing the named argument here suggests behavior which is not implemented.
var stopTask = StopSiloAsync(failedSilo);
failoverJoinTask = StartAdditionalSilosAsync(1, startAdditionalSiloOnNewPort: true);
await Task.WhenAll(stopTask, failoverJoinTask).WaitAsync(cts.Token);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code coverage78.52% line coverage - 99,409 / 126,605 lines Coverage details
|
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs — This test performs cluster churn (join + silo stop + join + convergence), but the linked CTS uses… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs — GetGrainOwnedBySiloAsync starts a reminder on each candidate grain and only stops it on the… View resolved comment |
|
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs — The startAdditionalSiloOnNewPort argument is misleading here because StartAdditionalSilosAsync… View resolved comment |
a8681e4 to
7b2865d
Compare
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTests_TableGrain.cs — Avoid creating an exception instance via RuntimeHelpers.GetUninitializedObject. That bypasses the… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs — This test performs cluster churn (join + silo stop + join + convergence), but the linked CTS uses… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTests_TableGrain.cs — Avoid creating an exception instance via RuntimeHelpers.GetUninitializedObject. That bypasses the… View comment |
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/Orleans.Reminders.Tests/TimerTests/ReminderTests_TableGrain.cs — Avoid creating an exception instance via RuntimeHelpers.GetUninitializedObject. That bypasses the… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs:576
HostedCluster.GetActiveSilos()returns a lazy enumerable (with internal locking/logging). InAssertReminderOwnershipAndSchedulesAsync, re-enumerating it inside theforeachviaAssert.Contains(...)can observe different silo sets if the cluster is still changing and adds unnecessary repeated enumeration/log noise. Materialize the active silo snapshot once before the loop.
await SynchronizeReminderSchedulesAsync(cancellationToken, reminders);
var activeSilos = HostedCluster.GetActiveSilos();
foreach (var reminder in reminders)
{
var grainId = reminder.Grain.GetGrainId();
var actualOwner = Assert.Single(observer.GetActiveReminderSilos(grainId, reminder.ReminderName));
Assert.Equal(1, observer.GetActiveReminderCount(grainId, reminder.ReminderName));
Assert.Contains(activeSilos, silo => silo.SiloAddress == actualOwner);
Assert.Equal(expectedTickCount, (long)observer.GetTickCount(grainId, reminder.ReminderName));
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8c2fa16 to
feb9de0
Compare


Closes #4319
Problem
The Azure Table and Cosmos generic-type reminder failover tests remained skipped because their original worker tasks kept calling grains while a silo was shutting down. The tests also raced reminder-service startup, membership convergence, and ownership reconciliation, then left joined silos in their shared fixtures.
Solution
Move the scenario into
ReminderTestsBasewith one fake-time driver and explicit startup, ownership, schedule, tick, and counter barriers. Add a fail-closed TestingHost barrier which requires the expected membership, gateway, grain-directory, and manifest views before reminder reconciliation; a timeout throws before any grain call runs. After convergence, invoke each grain exactly once and propagate any rejection. Select a reminder actually owned by the disposable joined silo, replace that silo during failover, and restore the exact baseline topology infinally. Enable the Azure Table and Cosmos cases, add the in-memory equivalent, and cover rejection and timeout propagation directly.Rationale
This preserves the intended one-failure/one-join topology while removing shutdown-call races without hiding the original failure mode. The scenario validates real ownership recovery across both grain implementations without weakening assertions or contaminating later fixture tests.
Microsoft Reviewers: Open in CodeFlow