Skip to content

fix(testing): isolate reminder event continuations - #10899

Merged
ReubenBond merged 5 commits into
dotnet:mainfrom
ReubenBond:rb-fix-reminder-stale-refresh-timeout-ecb
Aug 28, 2026
Merged

ReubenBond merged 5 commits into
dotnet:mainfrom
ReubenBond:rb-fix-reminder-stale-refresh-timeout-ecb

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Aug 28, 2026

Copy link
Copy Markdown
Member

Problem

ReminderTestKit_StaleRefreshCannotRestoreUnregisteredReminder can resume directly on the LocalReminderService diagnostic-event thread after awaiting ReminderServiceStarted. 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 Rx ToTask conversion 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 an ExecuteSynchronously continuation 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

Copilot AI lite review requested due to automatic review settings August 28, 2026 08:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: 1 Medium severity

New issues introduced by this change (1)
Severity Finding
Medium severity 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 a TaskCompletionSource-based wait using RunContinuationsAsynchronously.
  • Ensure cancellation and error propagation disposes subscriptions and completes the returned task appropriately.
  • Add a regression test asserting that an ExecuteSynchronously continuation attached to WaitForReminderServiceStartedAsync does 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.

Comment thread test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs
Copilot AI review requested due to automatic review settings August 28, 2026 09:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: None

Issues resolved since last review (1)
Severity Finding
Medium severity 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

  • WaitForEventAsync calls predicate(value) inside the subscription callback without handling exceptions. If the predicate throws, the exception will escape the Rx OnNext delegate (potentially crashing the event producer) and the returned task will never complete or clean up its registration/subscription. This is a semantic regression compared to FirstAsync(predicate) which faults the sequence when the predicate throws.
            value =>
            {
                if (!predicate(value))
                {
                    return;

Copilot AI review requested due to automatic review settings August 28, 2026 14:30
@ReubenBond
ReubenBond force-pushed the rb-fix-reminder-stale-refresh-timeout-ecb branch from f6040e8 to 758a6ac Compare August 28, 2026 14:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: 1 Medium severity

New issues introduced by this change (1)
Severity Finding
Medium severity test/​Orleans.Testing.Reminders/​ReminderDiagnosticObserver.cs — The cancellation callback disposes the subscription and cancels the task, but the…

Comment thread test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs
Copilot AI review requested due to automatic review settings August 28, 2026 14:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: 1 Medium severity

New issues introduced by this change (1)
Severity Finding
Medium severity test/​Orleans.Testing.Reminders/​ReminderDiagnosticObserver.csWaitForEventAsync evaluates predicate(value) directly inside the observer OnNext callback. If…
Issues resolved since last review (1)
Severity Finding
Medium severity test/​Orleans.Testing.Reminders/​ReminderDiagnosticObserver.cs — The cancellation callback disposes the subscription and cancels the task, but the… View resolved comment

Comment thread test/Orleans.Testing.Reminders/ReminderDiagnosticObserver.cs
Copilot AI review requested due to automatic review settings August 28, 2026 14:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: None

Issues resolved since last review (1)
Severity Finding
Medium severity test/​Orleans.Testing.Reminders/​ReminderDiagnosticObserver.csWaitForEventAsync 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 calling EmitReminderServiceStarted). TaskCreationOptions.RunContinuationsAsynchronously guarantees 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);
    }

This was referenced Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: ReminderTestKit_StaleRefreshCannotRestoreUnregisteredReminder times out

2 participants