fix(runtime): quiesce client route publication before shutdown - #10917
fix(runtime): quiesce client route publication before shutdown#10917ReubenBond wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
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 (1)
| Severity | Finding |
|---|---|
 |
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — PublishUpdates builds a pruned builder to minimize data transfer, but the remote call still… |
What changed in this PR
This PR adjusts Orleans.Runtime.GrainDirectory.ClientDirectory shutdown behavior so client route publication is quiesced earlier in the silo lifecycle, avoiding in-flight route updates during membership shutdown (addressing the flaky timeout described in #10904). It also adds a deterministic lifecycle test to verify publication is stopped before membership shutdown begins and that publication cannot be rescheduled once shutdown starts.
Changes:
- Quiesce client route publication at the
Activelifecycle stage boundary, reject new publish scheduling during shutdown, and avoid error logs on normal shutdown cancellation. - Track and drain/complete the internal publish work during shutdown sequencing so lifecycle stop can complete deterministically.
- Add a lifecycle-focused unit test which blocks a publication, verifies shutdown ordering, and verifies later updates cannot schedule another publish.
| File | Description |
|---|---|
| src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs | Quiesces route publication earlier in lifecycle shutdown and prevents new publication work from being scheduled after shutdown begins. |
| test/Orleans.Core.Tests/Directory/ClientDirectoryTests.cs | Adds a deterministic lifecycle test validating publication quiescence occurs before membership shutdown and that later updates cannot schedule publication. |
💡 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
🔵 Needs a closer look
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — PublishUpdates builds a pruned builder to minimize data transfer, but the remote call still… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs:580
ShouldPublishusesVolatile.Read(ref _isStopping)to short-circuit work, but_isStoppingis set via a normal write. To make the stop signal reliably visible to the volatile read (and communicate the intended synchronization), set_isStoppingviaVolatile.Write/Interlocked.Exchange.
This issue also appears on line 606 of the same file.
lock (_lockObj)
{
beginStopping = _isStopping == 0;
_isStopping = 1;
runTask = _runTask;
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs:608
TestAccessor.PublishTasksCompletedreads_runTask/_nextPublishTaskwithout synchronization, but those fields are updated concurrently. This can yield inconsistent results (and make the new lifecycle test flaky). Read them under the same lock used elsewhere inClientDirectory.
public bool PublishTasksCompleted =>
instance._runTask is not { IsCompleted: false }
&& instance._nextPublishTask is not { IsCompleted: false };
bd520db to
7bd71a4
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 |
|---|---|
 |
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — The shutdown cancellation catch block is empty, which makes it unclear that cancellation is… |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — The shutdown cancellation catch block is empty, which makes it unclear that cancellation is… View comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs:527
WaitAsync(_shutdownCts.Token)can throw during shutdown, causing the method to stop awaitingpublishTaskwhile the underlying RPC task may still fault later (eg, timeout). To avoid potentialUnobservedTaskExceptionand ensure faults are observed even if shutdown cancels the wait, attachIgnore()topublishTaskafter scheduling it.
publishTask = remote.OnUpdateClientRoutes(update);
}
await publishTask.WaitAsync(_shutdownCts.Token);
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 |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — The shutdown cancellation catch block is empty, which makes it unclear that cancellation is… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
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 (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — PublishUpdates calls remote.OnUpdateClientRoutes(update) while holding _lockObj. Calling out… |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/ClientDirectory.cs — PublishUpdates calls remote.OnUpdateClientRoutes(update) while holding _lockObj. Calling out… View resolved comment |
ClientDirectory continued publishing until the RuntimeGrainServices shutdown stage, after membership shutdown had already begun. A route update could therefore remain in flight from a retiring silo, and its response could time out after the source and successor left the cluster. This was the failure captured in #10904.
Quiesce client route publication at the Active-stage boundary, drain tracked publication work before the membership transition, and reject new publication scheduling once shutdown begins. Normal shutdown cancellation now completes without an error log.
Add a deterministic lifecycle test which blocks a publication, verifies it completes before the membership shutdown stage, and verifies later updates cannot schedule another publication.
Fixes #10904
Microsoft Reviewers: Open in CodeFlow