Skip to content

test(zookeeper): make close ordering assertion deterministic - #11347

Open
ReubenBond wants to merge 1 commit into
dotnet:mainfrom
ReubenBond:rb-issue-11346-zookeeper-close-race
Open

ReubenBond wants to merge 1 commit into
dotnet:mainfrom
ReubenBond:rb-issue-11346-zookeeper-close-race

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 22, 2026

Copy link
Copy Markdown
Member

Problem

ReadOwner_CanceledCaller_JoinsNativeTasksBeforeClose treated the fake native request's AfterRequest notification as a scheduling barrier. Once that notification fires, the operation owner can legitimately begin closing before the test continuation resumes, making the intermediate closeStarted assertion flaky.

Change

Capture at close entry whether the native request completion notification has already occurred, then assert that recorded ordering after observing close start. This deterministically verifies that the owner joins the admitted native request before close while preserving the cancellation-token, no-second-read, and close-completion lifecycle checks.

Closes #11346

Microsoft Reviewers: Open in CodeFlow

Copilot AI lite review requested due to automatic review settings September 22, 2026 08:16

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

🟢 Approval recommended

The revised assertion correctly verifies ordering without relying on a scheduling barrier.

Review effort: Lite
Findings: None

What changed in this PR

Makes the ZooKeeper close-ordering test deterministic by recording native request completion at close entry.

Changes:

  • Removes the racy intermediate closeStarted assertion.
  • Verifies the native completion notification preceded close initiation.
File Description
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperReadRetryTests.cs Updates close-ordering assertions in the ZooKeeper retry test.

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

@github-actions

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request Current main Variance
Lines 82.91% (115,362 / 139,149) 82.89% (115,344 / 139,149) +0.0129 pp
Branches 72.20% (33,474 / 46,361) 72.18% (33,463 / 46,361) +0.0237 pp

Report-only conclusion: improved.

The current-main baseline is commit 8bc9fd2444 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

This branch has not been deployed

No deployments
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.

test(zookeeper): close-start assertion races after the native-read completion barrier

2 participants