fix(testing): isolate reminder event continuations - #10899
ReubenBond merged 5 commits into
Conversation
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.Testing.Reminders/ReminderDiagnosticObserver.cs — WaitForEventAsync subscribes with only OnNext/OnError. If the observable completes without… |
What changed in this PR
This PR addresses a flaky reminders test by ensuring that waits on reminder diagnostic events do not resume continuations inline on the diagnostic event emitter thread, aligning reminder diagnostic wait behavior with other lifecycle-state waiters.
Changes:
- Replace Rx
FirstAsync(...).ToTask(...)conversions with aTaskCompletionSource-based wait usingRunContinuationsAsynchronously. - Ensure cancellation and error propagation disposes subscriptions and completes the returned task appropriately.
- Add a regression test asserting that an
ExecuteSynchronouslycontinuation attached toWaitForReminderServiceStartedAsyncdoes not run inline.
| File | Description |
|---|---|
| test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs | Introduces WaitForEventAsync using TaskCompletionSource(...RunContinuationsAsynchronously) and refactors reminder/service wait helpers to use it. |
| test/Orleans.Reminders.Tests/Diagnostics/ReminderEventsTests.cs | Adds a regression test to validate that service-start wait continuations do not run inline on the event emission thread. |
💡 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: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — WaitForEventAsync subscribes with only OnNext/OnError. If the observable completes without… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs:335
WaitForEventAsynccallspredicate(value)inside the subscription callback without handling exceptions. If the predicate throws, the exception will escape the RxOnNextdelegate (potentially crashing the event producer) and the returned task will never complete or clean up its registration/subscription. This is a semantic regression compared toFirstAsync(predicate)which faults the sequence when the predicate throws.
value =>
{
if (!predicate(value))
{
return;
f6040e8 to
758a6ac
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.Testing.Reminders/ReminderDiagnosticObserver.cs — The cancellation callback disposes the subscription and cancels the task, but the… |
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.Testing.Reminders/ReminderDiagnosticObserver.cs — WaitForEventAsync evaluates predicate(value) directly inside the observer OnNext callback. If… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs — The cancellation callback disposes the subscription and cancels the task, but the… View resolved 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.Testing.Reminders/ReminderDiagnosticObserver.cs — WaitForEventAsync evaluates predicate(value) directly inside the observer OnNext callback. If… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.Reminders.Tests/Diagnostics/ReminderEventsTests.cs:58
- The assertion
Assert.NotEqual(emitterThread, continuationThread)is currently comparing the continuation thread against the test thread (the one callingEmitReminderServiceStarted).TaskCreationOptions.RunContinuationsAsynchronouslyguarantees the continuation is not invoked inline on completion, but it does not guarantee it will run on a different managed thread than the caller (a ThreadPool thread can dequeue its own queued work later). This can make the regression test flaky and fail even when continuations are correctly asynchronous.
A more robust regression is to emit the event from a dedicated non-ThreadPool Thread and assert the continuation does not run on that emitter thread.
ReminderEvents.EmitReminderServiceStarted(siloAddress);
Assert.True(continuationRan.Wait(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken));
Assert.NotEqual(emitterThread, continuationThread);
}

Problem
ReminderTestKit_StaleRefreshCannotRestoreUnregisteredRemindercan resume directly on theLocalReminderServicediagnostic-event thread after awaitingReminderServiceStarted. The failed CI artifact shows the grain turn starting immediately and then waiting 30 seconds for its nested local reminder-service request, without reaching reminder storage. The existing RxToTaskconversion permits synchronous caller continuations at this scheduler boundary.Solution
Complete diagnostic event waits through task sources configured with
RunContinuationsAsynchronously, while preserving replay, filtering, cancellation, error propagation, and subscription cleanup. Add a regression which verifies that a service-start waiter cannot run anExecuteSynchronouslycontinuation on the event emitter thread.Rationale
Reminder diagnostic waits are phase barriers. Resuming consumers independently from the runtime event producer keeps subsequent grain calls outside the reminder-service scheduler turn and applies the same continuation guarantee already used by the observer's lifecycle-state waiters.
Fixes #10893
Microsoft Reviewers: Open in CodeFlow