Skip to content

fix: drain admitted client observer invocations during shutdown - #11264

Merged
ReubenBond merged 6 commits into
dotnet:mainfrom
ReubenBond:rb-fix-drain-client-observer-invocations
Sep 15, 2026
Merged

ReubenBond merged 6 commits into
dotnet:mainfrom
ReubenBond:rb-fix-drain-client-observer-invocations

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 15, 2026 •

Copy link
Copy Markdown
Member

Problem

Client-observer invocations run on separate per-object pumps and interleaved tasks. Stopping the hosted dispatch pump or external message center leaves accepted observer work running, including work using the hosted client's dependency-injection scope.

Solution

Account for each accepted observer message with AdmissionGate through invocation processing and response handling. Preserve FIFO queuing and cancellation semantics, release removed queued requests exactly once, and reject late requests with SiloUnavailableException. Cancellation controls remain admitted while application work drains, then their own admission phase closes and drains.

Explicit TryEnterUnscoped/Exit ownership keeps observer queues and interleaved task state free of admission-token fields. The same stable generated-interface classifier selects the gate at entry and release. The existing scoped API wraps the shared atomic entry implementation and remains available to other callers. On a 64-bit runtime, queue element storage drops from two references to one: 16 bytes to 8 bytes, excluding array overhead.

The hosted client drains its incoming queue and observer execution before releasing its owned scope. External clients drain immediately before connection-manager closure using a client-only lifecycle-adapter hook, preserving earlier lifecycle networking and cleanup. Outstanding outbound callbacks complete before the drain so observers awaiting them can finish.

Shutdown policy

Host cancellation bounds the wait, while actual observer execution remains accounted for. Owned scope/message-center disposal follows actual drain, direct disposal stays nonblocking and idempotent, and deferred disposal failures are logged. Root-provider and singleton disposal remain owned by the host, including abort behavior.

Deterministic regressions cover queued and interleaved ownership, rejection and cancellation paths, terminal response handling, hosted scope lifetime and exactly-once disposal, upper-stage external lifecycle cleanup before the drain, and connection closure after the drain. Gate regressions exercise mixed scoped/unscoped ownership, exceptional exits, and entry/close races. Shutdown guidance describes the resulting guarantees.

Microsoft Reviewers: Open in CodeFlow

Copilot AI lite review requested due to automatic review settings September 15, 2026 20:55

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

🟡 Changes recommended

Focused deterministic tests for hosted and external shutdown races, callback faults, queued invocations, and late-request rejection are still needed.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Updates client-observer shutdown to reject late work and drain admitted invocations before disposing client resources.

Changes:

  • Tracks admissions through observer invocation, cancellation, queuing, and response handling.
  • Drains hosted and external clients before scope or transport disposal.
  • Adds lifecycle hooks and documents shutdown behavior.
File Summary
src/​Orleans.Runtime/​Core/​HostedClient.cs Drains hosted observer work before scope disposal.
src/​Orleans.Core/​Runtime/​OutsideRuntimeClient.cs Drains external observer work before transport shutdown.
src/​Orleans.Core/​Runtime/​InvokableObjectManager.cs Adds admission tracking, cancellation handling, FIFO processing, and late-request rejection. Moderate finding (2 votes): focused deterministic shutdown regression tests are needed.
src/​Orleans.Core/​Networking/​ConnectionManagerLifecycleAdapter.cs Supports pre-connection-close draining hooks.
src/​Orleans.Core/​Core/​DefaultClientServices.cs Registers the external-client drain hook.
docs/​site/​src/​content/​docs/​host/​configuration-guide/​shutting-down-orleans.md Documents observer draining during shutdown.
docs/​site/​src/​content/​docs/​host/​client.md Documents client observer shutdown semantics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Orleans.Core/Runtime/InvokableObjectManager.cs
Copilot AI review requested due to automatic review settings September 15, 2026 21:04

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

🔵 Needs a closer look

Unresolved critical shutdown-ordering and moderate disposal-timing findings require human review.

Review tier: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread src/Orleans.Runtime/Core/HostedClient.cs
Copilot AI review requested due to automatic review settings September 15, 2026 21:13

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

🔵 Needs a closer look

Shutdown callback ordering can delay observer draining and requires correction plus human review.

Review tier: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

Copilot AI review requested due to automatic review settings September 15, 2026 21:25
@ReubenBond
ReubenBond marked this pull request as ready for review September 15, 2026 21:26
@github-actions

github-actions Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request
Lines 81.94% (111,338 / 135,883)
Branches 71.01% (31,890 / 44,912)

Report-only conclusion: current-main baseline stale.

The newest successful coverage run tested f64f1f1, not current main 3654f04.

Coverage combines every CI test matrix job, including providers, CodeGen, .NET 8/10, Linux, Windows, and macOS, using canonical physical source and branch identities.

The comparison remains report-only while normal line and branch variance is calibrated.

Coverage details

@ReubenBond
ReubenBond force-pushed the rb-fix-drain-client-observer-invocations branch from fc0def3 to dd6279a Compare September 15, 2026 21: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

🔵 Needs a closer look

Hosted shutdown ordering can leave observer draining waiting on outbound calls and permits new calls during the drain.

Review tier: Lite
Findings: None

Resolved since last review (2)

Copilot AI review requested due to automatic review settings September 15, 2026 21:36

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

🟡 Changes recommended

Coordinate callback-monitor expiry with forced completion to prevent incorrect timeout results during shutdown.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1 High severity

Open (1)

Comment thread src/Orleans.Core/Runtime/OutsideRuntimeClient.cs
Copilot AI review requested due to automatic review settings September 15, 2026 22:45
@ReubenBond
ReubenBond force-pushed the rb-fix-drain-client-observer-invocations branch from dd6279a to 75f58e5 Compare September 15, 2026 22:45

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

🔵 Needs a closer look

The shutdown, admission, and lifecycle changes span multiple components and require final human validation.

Review tier: Lite
Findings: None

Resolved since last review (1)

@ReubenBond
ReubenBond merged commit 8c4887a into dotnet:main Sep 15, 2026
73 checks passed
@ReubenBond
ReubenBond deleted the rb-fix-drain-client-observer-invocations branch September 15, 2026 23:10
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.

2 participants