Skip to content

fix(testing): cancel draining after fatal silo errors - #11294

Merged
ReubenBond merged 2 commits into
dotnet:mainfrom
ReubenBond:rb-fix-testing-cancel-fatal-silo-draining
Sep 17, 2026
Merged

ReubenBond merged 2 commits into
dotnet:mainfrom
ReubenBond:rb-fix-testing-cancel-fatal-silo-draining

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 17, 2026 •

Copy link
Copy Markdown
Member

Fatal silo errors represent process loss. Graceful draining in the test host can wait on unreachable peers and delay cleanup.

Stop the affected host with a pre-canceled drain budget while preserving in-process cleanup. Log single and aggregated cancellation at Debug and actual stop failures at Error. The regression probe verifies canceled drain tokens, one host stop for repeated fatal notifications, and log classification for single, multiple, and nested stop failures.

Extracted from commit 48302ff632bbc7f99eadc0ce366b08c2ca4d9b88 in #10236 as a standalone prerequisite for that PR. Addresses the fatal-drain policy implicated in #11286; end-to-end reproduction of that streaming shutdown remains a separate verification gap.

Copilot AI lite review requested due to automatic review settings September 17, 2026 06:31

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

Cancellation aggregates may be logged at Error, and the regression test does not verify the required logging behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Updates test-host fatal-silo handling to cancel graceful draining promptly while preserving cleanup.

Changes:

  • Uses a pre-canceled token for fatal-error shutdown.
  • Adds regression coverage for cancellation and duplicate stop scheduling.
File Review summary
test/​TestInfrastructure/​Orleans.TestingHost.Tests/​TestClusterFatalErrorHandlerTests.cs Tests cancellation and single-stop behavior; logging assertions are still needed (nit, 1 vote).
src/​Orleans.TestingHost/​TestClusterFatalErrorHandler.cs Needs aggregate cancellation handling to preserve Debug logging while retaining Error logging for real failures (moderate, 2 votes).

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

Comment thread src/Orleans.TestingHost/TestClusterFatalErrorHandler.cs Outdated
@github-actions

github-actions Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request Current main Variance
Lines 82.10% (112,247 / 136,719) 82.12% (112,272 / 136,709) -0.0243 pp
Branches 71.29% (32,298 / 45,305) 71.32% (32,306 / 45,295) -0.0334 pp

Report-only conclusion: regressed.

The current-main baseline is commit 2e40fa8a3b and uses the same reviewed coverage matrix.

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

Copilot AI review requested due to automatic review settings September 17, 2026 09:18

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 nested aggregate test case has an assertion mismatch that must be corrected before approval.

Review effort: Lite
Findings: None

Resolved since last review (1)

@ReubenBond
ReubenBond merged commit 4bd1df8 into dotnet:main Sep 17, 2026
73 checks passed
@ReubenBond
ReubenBond deleted the rb-fix-testing-cancel-fatal-silo-draining branch September 17, 2026 15:17
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