test(reminders): add lifecycle conformance scenarios - #10896
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs — WaitForOwnerCountAsync mixes two different owner-count notions: the fast-path uses GetOwners()… |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — The "no local owner before entering the loading window" assertion only checks immediately after… |
What changed in this PR
Adds a shared, deterministic reminder-service lifecycle conformance suite to reduce provider-specific drift and make ownership/recovery/churn semantics verifiable with consistent diagnostics across providers.
Changes:
- Introduces
ReminderServiceLifecycleTestRunner+IReminderServiceLifecycleHarnessfor deterministic lifecycle/churn scenarios inOrleans.Reminders.TestKit. - Adds an in-process
ReminderServiceLifecycleHarnessand extendsReminderDiagnosticObserverto track local start/stop and schedule-change counts needed by the new scenarios. - Wires the new scenarios into existing reminder provider test bases and adds TestKit self-tests (including fault-injection “oracle” checks).
| File | Description |
|---|---|
| test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs | New harness adapting InProcessTestCluster + ReminderTestClock + diagnostics to the shared lifecycle runner. |
| test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs | Adds lifecycle counters and a schedule-change waiter to support deterministic lifecycle assertions. |
| test/Orleans.Testing.Reminders/Orleans.Testing.Reminders.csproj | Adds a project reference to Orleans.Reminders.TestKit so test utilities can use the shared runner/harness contract. |
| test/Orleans.Reminders.Tests/TimerTests/ReminderTestsBase.cs | Integrates lifecycle conformance scenarios into provider reminder suites via a per-provider runner. |
| test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs | New TestKit conformance tests for lifecycle scenarios plus fault-injection self-tests. |
| src/Orleans.Reminders/Orleans.Reminders.csproj | Adds InternalsVisibleTo for Orleans.Testing.Reminders to enable test-only range-reconciliation barriers. |
| src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs | New shared deterministic lifecycle/churn runner and harness contract used by provider suites and TestKit tests. |
| src/Orleans.Reminders.TestKit/README.md | Documents the new lifecycle runner and harness usage pattern. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
 |
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… |
 |
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — The "no local owner before entering the loading window" assertion only checks immediately after… View resolved comment |
|
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs — WaitForOwnerCountAsync mixes two different owner-count notions: the fast-path uses GetOwners()… View resolved comment |
Suppressed comments (4)
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:263
- The XML docs say "stopped for an identity", but this API counts stops for the (grainId, reminderName) reminder, not for a particular identity instance.
/// <summary>Gets the number of local reminder instances stopped for an identity.</summary>
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:272
- The XML docs say "schedule changes for an identity", but the method counts schedule-change events for the reminder (grainId + reminderName) rather than for a particular identity.
/// <summary>Gets the number of local schedule changes for an identity.</summary>
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:281
- The XML docs say schedule-change waits are "for an identity", but the waiter is keyed by (grainId, reminderName) and waits for schedule changes for the reminder overall.
/// <summary>Waits for the requested number of local schedule changes for an identity.</summary>
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs:62
- These interface docs mention "for an identity", but the methods are keyed by (grainId, reminderName) and count events for the reminder overall.
/// <summary>Gets the number of local reminder instances stopped for an identity.</summary>
int GetLocalStopCount(GrainId grainId, string reminderName);
/// <summary>Gets the number of local schedule changes for an identity.</summary>
int GetScheduleChangeCount(GrainId grainId, string reminderName);
baf92fa to
22e6d4b
Compare
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
test/Extensions/Orleans.AdoNet.Tests/Reminders/ReminderServiceLifecycleTests_AdoNet.cs — using UnitTests.General; is unused in this file and will trigger IDE0005 (remove unnecessary… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… View comment |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… View comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… View comment |
Suppressed comments (5)
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:254
- The XML summary says this count is "for an identity", but the method signature (and underlying key) only scopes by (grainId, reminderName). This is misleading for consumers and for debugging lifecycle scenarios.
/// <summary>Gets the number of local reminder instances started for an identity.</summary>
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:263
- The XML summary says this count is "for an identity", but the method does not accept an identity and aggregates by (grainId, reminderName). Update the summary to match the actual behavior.
/// <summary>Gets the number of local reminder instances stopped for an identity.</summary>
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:272
- The XML summary refers to schedule changes "for an identity", but the count is keyed only by (grainId, reminderName). This can confuse readers trying to interpret the diagnostics.
/// <summary>Gets the number of local schedule changes for an identity.</summary>
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:281
- The XML summary says this waits for schedule changes "for an identity", but the API aggregates by (grainId, reminderName). Please align the documentation with what is actually being waited on.
/// <summary>Waits for the requested number of local schedule changes for an identity.</summary>
src/Orleans.Reminders.TestKit/README.md:189
- The README sample creates a ReminderDiagnosticObserver but never disposes it. Since the observer subscribes to global diagnostic streams, the sample should either use
using varor useclock.DiagnosticObserver(which is disposed with the clock) to avoid leaking subscriptions in real-world usage.
var observer = ReminderDiagnosticObserver.Create(); // create before deployment
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
test/Extensions/Orleans.AdoNet.Tests/Reminders/ReminderServiceLifecycleTests_AdoNet.cs — using UnitTests.General; is unused in this file and will trigger IDE0005 (remove unnecessary… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… View comment |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… View comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… View comment |
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs:159
- IsActiveSilo currently allocates a new ActiveSilos array on every predicate invocation (ActiveSilos => Order().ToArray()), which can add significant overhead since the predicate is evaluated repeatedly while waiters poll counts. You can avoid the per-call allocation by checking the cluster’s active-silo lookup directly.
private bool IsActiveSilo(SiloAddress? siloAddress)
=> siloAddress is not null && ActiveSilos.Contains(siloAddress);
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs:122
- If fixture.InitializeAsync() throws (e.g., deployment failure), DisposeAsync() is never called because initialization happens before the try/finally. Moving InitializeAsync inside the try ensures cleanup on partial initialization (same pattern applies to RunFaultAsync).
var fixture = new ReminderServiceLifecycleFixture();
await fixture.InitializeAsync();
try
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs:59
- These APIs don’t accept a local reminder identity and appear to be counted per (grainId, reminderName). The XML docs currently say “for an identity”, which is misleading for consumers of the harness contract.
This issue also appears on line 61 of the same file.
/// <summary>Gets the number of local reminder instances started for an identity.</summary>
int GetLocalStartCount(GrainId grainId, string reminderName);
/// <summary>Gets the number of local reminder instances stopped for an identity.</summary>
int GetLocalStopCount(GrainId grainId, string reminderName);
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs:62
- The XML docs currently say “for an identity”, but this count is keyed by (grainId, reminderName). Consider updating the wording to match the actual parameters.
/// <summary>Gets the number of local schedule changes for an identity.</summary>
int GetScheduleChangeCount(GrainId grainId, string reminderName);
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
test/Extensions/Orleans.AdoNet.Tests/Reminders/ReminderServiceLifecycleTests_AdoNet.cs — using UnitTests.General; is unused in this file and will trigger IDE0005 (remove unnecessary… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… View comment |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… View comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… View comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs:162
- Most lifecycle scenarios assume the reminder service is already started, but they never call _harness.WaitForStartupReadinessAsync(cancellationToken) before registering/updating reminders. Since xUnit does not guarantee running the StartupReadiness fact first, and BaseInProcessTestClusterFixture.DeployAsync does not await ReminderServiceStarted events, other scenarios can race reminder-service startup and intermittently time out/flap under load or slower providers.
Consider adding a startup-readiness barrier at the start of every RunReminderService_* scenario (or centralizing it in a helper) so scenario order cannot affect determinism.
async () =>
{
var expectedStart = _harness.UtcNow.UtcDateTime + due;
await grain.RegisterOrUpdateAsync(Name, due, Period).WaitAsync(cancellationToken);
await _harness.WaitForOwnerCountAsync(grain.GetGrainId(), Name, 1, cancellationToken);
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
test/Extensions/Orleans.AdoNet.Tests/Reminders/ReminderServiceLifecycleTests_AdoNet.cs — using UnitTests.General; is unused in this file and will trigger IDE0005 (remove unnecessary… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… View comment |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… View comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… View comment |
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs:160
- IsActiveSilo allocates and enumerates ActiveSilos (which calls GetActiveSilos().ToArray()) every time the predicate is invoked. Since this predicate is evaluated under ReminderDiagnosticObserver's lock (e.g., while processing LocalReminderStarted/Stopped events via GetActiveReminderCountCore), calling back into the cluster and allocating arrays inside that lock can add avoidable overhead and increases the risk of lock contention during churn-heavy scenarios.
private bool IsActiveSilo(SiloAddress? siloAddress)
=> siloAddress is not null && ActiveSilos.Contains(siloAddress);
test/Orleans.Testing.Reminders/ReminderTestClock.cs:33
- ReminderTestClock now always creates a ReminderDiagnosticObserver (Replay() buffer + subscriptions) even for tests which already create their own observer, resulting in duplicate buffering/subscription overhead. For example, ReminderTestKitClusterIntegrationTests creates both ReminderTestClock.Attach(...) and ReminderDiagnosticObserver.Create() (test/Orleans.Reminders.TestKit.Tests/ReminderTestKitClusterIntegrationTests.cs:172-177). Consider making observer creation opt-in or allowing an observer to be supplied so existing tests can reuse one observer instead of instantiating two.
{
TimeProvider = new FakeTimeProvider(initialTime);
DiagnosticObserver = ReminderDiagnosticObserver.Create();
MinimumReminderPeriod = minimumReminderPeriod;
RefreshReminderListPeriod = refreshReminderListPeriod;
|
The current CI failures are deterministic and specific to the reminder lifecycle scenarios added by this PR. Across Windows/Linux and multiple provider matrices, ReminderService_OneSiloJoinLeaveTransfersOwnership and ReminderService_UpdateDoesNotRestartLocalOwner time out after two minutes with TaskCanceledException from ReminderServiceLifecycleTestRunner (including lines 464 and 209). This needs a branch synchronization/timeout fix before another retry. Failed run: https://github.com/dotnet/orleans/actions/runs/33161717052 |
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
test/Extensions/Orleans.AdoNet.Tests/Reminders/ReminderServiceLifecycleTests_AdoNet.cs — using UnitTests.General; is unused in this file and will trigger IDE0005 (remove unnecessary… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… View comment |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… View comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… View comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… View comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs:650
- RemovePersistedRowAsync uses row.ETag! when calling IReminderTable.RemoveRow. ReminderEntry.ETag is nullable, so a provider which returns a null/empty ETag (or a partially-initialized row during failure) will cause a NullReferenceException/ArgumentNullException during cleanup, which is harder to diagnose than a structured conformance failure.
if (!await _harness.ReminderTable.RemoveRow(grainId, name, row.ETag!).WaitAsync(cancellationToken))
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:255
- The XML summaries for GetLocalStartCount/GetLocalStopCount/GetScheduleChangeCount say "for an identity", but these counters are keyed by (grainId, reminderName) and do not accept an instance identity argument. This wording is ambiguous given the LocalReminderStarted/Stopped events also carry an identity object.
/// <summary>Gets the number of local reminder instances started for an identity.</summary>
public int GetLocalStartCount(GrainId grainId, string reminderName)
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — This adds new public API surface (IReminderServiceLifecycleHarness and… |
Issues resolved since last review (5)
| Severity | Finding |
|---|---|
|
test/Extensions/Orleans.AdoNet.Tests/Reminders/ReminderServiceLifecycleTests_AdoNet.cs — using UnitTests.General; is unused in this file and will trigger IDE0005 (remove unnecessary… View resolved comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — ownerFilter is invoked while holding the observer's private lock. Since ownerFilter is… View resolved comment |
|
src/Orleans.Reminders.TestKit/README.md — The lifecycle README example creates a separate ReminderDiagnosticObserver, but ReminderTestClock… View resolved comment |
|
src/Orleans.Reminders.TestKit/ReminderServiceLifecycleTestRunner.cs — These interface docs mention "for an identity", but the harness methods are keyed by (grainId,… View resolved comment |
|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The XML docs say "started for an identity", but this API is keyed by (grainId, reminderName) and… View resolved comment |
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs:123
- If fixture.InitializeAsync() throws, the fixture is never disposed because it's called before the try/finally. Moving InitializeAsync inside the try ensures partial cluster resources are cleaned up even when deployment fails.
This issue also appears in the following locations of the same file:
- line 163
- line 188
var fixture = new ReminderServiceLifecycleFixture();
await fixture.InitializeAsync();
try
{
using var cancellation = CancellationTokenSource.CreateLinkedTokenSource(
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs:166
- Same disposal hole here: if fixture.InitializeAsync() throws, the fixture won't be disposed. Wrapping InitializeAsync in the existing try/finally ensures cleanup even on partial initialization failures.
var fixture = new ReminderServiceLifecycleFixture();
await fixture.InitializeAsync();
try
{
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs:191
- Same pattern in RunFaultAsync: fixture.InitializeAsync() is outside the try/finally, so a failure during initialization can leak the in-process cluster. Put InitializeAsync inside the try so fixture.DisposeAsync() always runs.
var fixture = new ReminderServiceLifecycleFixture();
await fixture.InitializeAsync();
try
{
|
Follow-up on run 33161717052: the missing transitions were causal, not timeout-budget issues. A grain call can persist registration/update/removal on a silo which is not the reminder's ring owner, so the owner transition is only observable after the next reminder-table refresh. The runner now arms the owner/schedule wait before advancing exactly one refresh. Join/leave had also consumed one refresh before its final delivery advance, so advancing the original due time overshot the persisted first due; it now advances only the exact remaining interval. Cleanup removes only scenario rows by current ETag and advances one refresh for quiescence. Controlled TestKit regressions gate update reconciliation on that refresh and assert the join/leave advance sequence reaches, rather than passes, the due boundary. The later full matrix run is green across Windows/Linux and all provider jobs. |
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>
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>
790dbb5 to
40193c7
Compare
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs — This file uses TestSuite/TestProvider/TestArea/TestCategory and TestContext.Current, but it does… View resolved comment |
|
test/Orleans.Reminders.Tests/TimerTests/ReminderServiceLifecycleTestsBase.cs — ReminderServiceLifecycleTestsBase uses ReminderOptions but does not import its namespace… View resolved comment |
|
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs — DisposeAsync returns early when _cluster is null, so a partially-initialized fixture can leak… View resolved comment |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The current MySQL failures are deterministic and specific to the lifecycle conformance assertions added by this PR. MariaDB/MySQL persists reminder Run 33181707452 shows the same 500 ms truncation on both target frameworks:
Ownership, ETag rotation, periods, and delivery behavior reach the expected states; the exact persisted timestamp comparison needs to account for the provider's storage precision (or the MySQL schema needs matching fractional precision) before retrying these jobs. Failed jobs: https://github.com/dotnet/orleans/actions/runs/33181707452 |
|
The DynamoDB jobs now confirm the same provider-precision issue from the MySQL failures. Both net8.0 and net10.0 persist the half-second lifecycle timestamps at whole-second precision (and materialize them with Examples from run 33181707452:
The lifecycle contract needs an explicit timestamp-precision normalization supplied by each provider, or the scenarios need fake-clock values aligned to the common persisted precision. Retrying the current commit will reproduce these failures. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: None
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
test/Orleans.Testing.Reminders/ReminderServiceLifecycleHarness.cs:55
- ActiveSilos uses Enumerable.Order(), which is not consistently available across target frameworks and is unusual compared to the rest of the codebase. Use OrderBy with the element itself (SiloAddress implements IComparable) to keep deterministic ordering without depending on Order().
/// <inheritdoc />
public IReadOnlyList<SiloAddress> ActiveSilos
=> _cluster.GetActiveSilos().Select(silo => silo.SiloAddress).Order().ToArray();
test/Orleans.Reminders.TestKit.Tests/ReminderServiceLifecycleConformanceTests.cs:3
- The first two using directives are unused in this file and will trigger IDE0005 (remove unnecessary usings) with EnforceCodeStyleInBuild/TreatWarningsAsErrors enabled.
using Microsoft.Extensions.DependencyInjection;
using Orleans.Configuration;
using Orleans.Reminders.TestKit;
|
Final synchronization result: the lifecycle runner now uses the merged #10891 explicit TestOnlyRefresh and cluster-manifest/range-reconciliation barriers, so update/removal/owner loading no longer depend on advancing fake time or timer ordering. Join/leave now selects one deterministic grain whose hash is inside the joined silo's actual IConsistentRingProvider range, registers it after join, and observes its transfer on leave instead of assuming a random 32-grain sample must move. Exact-due recovery probes one tick before the loading boundary, explicitly refreshes at the boundary, and then advances exactly to the persisted due time. Controlled tests assert the update refresh gate and exact join/leave advance. Each blocking scenario passed 10/10 focused stress runs; the complete TestKit suites and final matrix run 33184417895 are green. |
Problem
Reminder provider conformance currently verifies table operations but leaves service startup, local ownership, recovery, and topology churn to provider-specific suites. That permits lifecycle semantics to drift and makes timing failures difficult to diagnose.
Solution
Add a shared deterministic lifecycle runner and in-process harness covering readiness, single-owner registration, in-place updates, removal quiescence, exact-due recovery, stale-owner reconciliation, one-silo join/leave transfer, and cleanup isolation. The harness centralizes fake-time advancement, per-instance diagnostic ownership, explicit startup events, and membership/range-reconciliation barriers. Dedicated setup-only adapters run the same inherited scenarios for in-memory, Azure Table, Cosmos, SQL Server, PostgreSQL, MySQL, Redis, DynamoDB, and Firestore providers. Fault-injection self-tests prove duplicate local instances, owner restarts, and cancellation-safe cleanup are detected.
Rationale
Keeping identities, schedules, assertions, churn orchestration, and independently bounded scenario cleanup in TestKit limits provider code to backend configuration, produces exact owner/tick diagnostics, and prevents provider-specific retries, whole-table clearing, or timing allowances from concealing semantic failures.