Skip to content

fix(transactions): await recovery cleanup tasks - #11188

Merged
ReubenBond merged 3 commits into
dotnet:mainfrom
ReubenBond:rb-fix-transaction-recovery-lifetime
Sep 8, 2026
Merged

ReubenBond merged 3 commits into
dotnet:mainfrom
ReubenBond:rb-fix-transaction-recovery-lifetime

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 7, 2026 •

Copy link
Copy Markdown
Member

Problem

Transaction recovery tests could let participant observation, silo shutdown, or transaction production continue after the resources they use became eligible for disposal. Enabling CA2025 for the complete Orleans.Transactions.TestKit.Base project exposed three lifetime paths in TransactionRecoveryTestsRunner.

Solution

  • Keep the optional participant cleanup gate alive until its observation completes, cancelling and awaiting the observation during failure cleanup.
  • Await the original silo shutdown task on timeout so fixture-owned cluster and silo resources remain alive through shutdown completion.
  • Pass a cancellation token to transaction production and asynchronously cancel and await the producer before disposing its linked token source.
  • Add deterministic normal and cancellation-path coverage for producer and phase-gate lifetime ordering.
  • Enable CA2025 as a warning for all C# sources in Orleans.Transactions.TestKit.Base.

Rationale

The recovery runner now owns every spawned operation through completion. Cancellation stops new work while the current operation settles, phase gates retain the disposal behavior established in #11167, and primary recovery failures retain precedence over a concurrent cleanup watchdog timeout. Two narrow CA2025 suppressions document guarantees the analyzer cannot follow across the structured cleanup paths: each task is awaited before the fixture or gate can dispose its resources.

Microsoft Reviewers: Open in CodeFlow

Copilot AI lite review requested due to automatic review settings September 7, 2026 15:59

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

ProducerCancellationScope.DisposeAsync can throw if the producer faults, potentially masking the original test failure during await using cleanup.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 Medium severity

New issues introduced by this change (1)
Severity Finding
Medium severity src/​Orleans.Transactions.TestKit.Base/​TestRunners/​TransactionRecoveryTestsRunner.cs — ProducerCancellationScope.DisposeAsync currently awaits producer directly. If producer…
What changed in this PR

This PR tightens task and cancellation lifetime management in TransactionRecoveryTestsRunner to ensure recovery scenarios don’t outlive fixture-owned resources, aligning the test kit with CA2025 guidance and preventing cleanup races during transaction recovery tests.

Changes:

  • Made producer shutdown/cleanup explicitly async-owned by the runner (cancel + await producer before disposing its token source).
  • Ensured participant cleanup observation and silo shutdown tasks are cancelled/awaited so gates/fixtures aren’t disposed while work is still running.
  • Added focused lifetime-ordering tests and enabled CA2025 warnings for Orleans.Transactions.TestKit.Base.
File Description
test/​Transactions/​Orleans.Transactions.Tests/​TransactionRecoveryTestsRunnerLifetimeTests.cs Adds deterministic tests covering producer cancellation-scope disposal ordering and phase-gate observation lifetime.
src/​Orleans.Transactions.TestKit.Base/​TestRunners/​TransactionRecoveryTestsRunner.cs Refactors recovery runner cleanup to cancel/await owned tasks (producer, cleanup observation, shutdown) and introduces async disposal scope.
.editorconfig Enables CA2025 warnings for Orleans.Transactions.TestKit.Base sources.

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

Copilot AI review requested due to automatic review settings September 8, 2026 09:41
@ReubenBond
ReubenBond force-pushed the rb-fix-transaction-recovery-lifetime branch from c48a035 to 4e075db Compare September 8, 2026 09:41

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

The new lifetime test CreateTransition() mixes named and positional arguments in a way that will not compile.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity

New issues introduced by this change (1)
Severity Finding
High severity test/​Transactions/​Orleans.Transactions.Tests/​TransactionRecoveryTestsRunnerLifetimeTests.cs — CreateTransition() mixes named and positional arguments in the RecoveryTransition constructor…
Issues resolved since last review (1)
Severity Finding
Medium severity src/​Orleans.Transactions.TestKit.Base/​TestRunners/​TransactionRecoveryTestsRunner.cs — ProducerCancellationScope.DisposeAsync currently awaits producer directly. If producer… View resolved comment

Copilot AI review requested due to automatic review settings September 8, 2026 14:14

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 new timeout cleanup path can mask the original TimeoutException if the shutdown task faults, which can make failures misleading.

Review tier: Lite
Findings: None

Issues resolved since last review (1)
Severity Finding
High severity test/​Transactions/​Orleans.Transactions.Tests/​TransactionRecoveryTestsRunnerLifetimeTests.cs — CreateTransition() mixes named and positional arguments in the RecoveryTransition constructor… View resolved comment
Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Orleans.Transactions.TestKit.Base/TestRunners/TransactionRecoveryTestsRunner.cs:326

  • In the TimeoutException handler, await shutdown; throw; can mask the original timeout if shutdown faults (the await will throw instead of rethrowing the timeout). If the intent is to ensure shutdown completes for lifetime reasons but still report the timeout as the primary failure, suppress exceptions from the shutdown task in this path.

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