fix(zookeeper): make membership reads sequential and resilient

[ReubenBond]

Problem

ZooKeeper membership reads can fail with ConnectionLossException. The provider historically starts every row read concurrently, and recent consistency fencing makes a snapshot perform more requests and replay the complete pass when canonical membership changes during the read. Large concurrent snapshots can amplify the pinned SDK's existing transport instability.

The SDK's ZooKeeper.Using also retries an entire callback. Replaying a conditional insert or update after a lost acknowledgment can turn a committed transaction into a false conflict.

Solution

• Read membership rows sequentially in ZooKeeper's returned child order. Each row reads its membership record and then its heartbeat before advancing to the next row.
• Evaluate cleanup rows sequentially while preserving row-scoped evidence checks and conditional deletes.
• Preserve the opening and closing table-version/child-version fence around each complete snapshot.
• Bound canonical snapshot churn and cleanup BadVersion contention to five total attempts. Exhaustion reports an explicit consistency or contention failure instead of looping indefinitely, returning stale rows, or silently skipping cleanup.
• Retry surfaced ConnectionLossException failures at the native Sync, GetChildren, and GetData boundaries using one Polly owner: four retries with exponential 250/500/1000/2000 ms delays.
• Wait for the operation-owned session's next connected generation before reissuing a failed native read, so retry attempts are not consumed while the same session is reconnecting.
• Keep one operation-owned ZooKeeper session alive until its callback and asynchronous close complete, including after the caller cancels its wait.
• Submit conditional insert and update transactions once. A lost acknowledgment propagates an unknown-outcome exception rather than replaying the transaction into false.

Initialization, heartbeat updates, and recursive deletion retain their existing SDK callback behavior. Mutation payloads, CAS preconditions, public APIs, the ZooKeeper SDK version, and configuration remain unchanged.

This deliberately trades snapshot latency for lower ZooKeeper request concurrency and improved reliability. It mitigates load amplification without claiming that recent consistency changes introduced the underlying SDK socket-reset behavior.

Scope

This PR contains the ZooKeeper-specific resilience work extracted from #11332. Shared membership-provider conformance wiring remains there.

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Copilot review overview

🟡 Changes recommended

Unbounded diagnostic console output remains unresolved; the test-name typo is minor.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Fix typo in test name: Refences to References · New

What changed in this PR

Improves ZooKeeper read resilience with scoped retries, session ownership, snapshot fencing, and expanded tests.

Changes:

• Adds Polly retries for connection-loss reads.
• Preserves safe cancellation and conditional-write semantics.
• Adds resilience, diagnostic, and unit-test coverage.

File	Summary
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperReadRetryTests.cs	Tests retries, cancellation, fencing, and mutations.
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperReadResilienceTests.cs	Adds functional resilience scenarios.
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperNativeDiagnostics.cs	Captures native diagnostics.
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperBasedMembershipTableUnitTests.cs	Adds membership and diagnostics tests.
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​Orleans.Clustering.ZooKeeper.Tests.csproj	Adds test dependencies.
src/​Orleans.Clustering.ZooKeeper/​ZooKeeperSession.cs	Manages native operations and asynchronous close.
src/​Orleans.Clustering.ZooKeeper/​ZooKeeperReadRetryPolicy.cs	Defines connection-loss retry behavior.
src/​Orleans.Clustering.ZooKeeper/​ZooKeeperGatewayListProvider.cs	Applies resilient gateway reads.
src/​Orleans.Clustering.ZooKeeper/​ZooKeeperBasedMembershipTable.cs	Integrates sessions, retries, and fenced reads.
src/​Orleans.Clustering.ZooKeeper/​Orleans.Clustering.ZooKeeper.csproj	Adds the Polly dependency.

---

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

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve process-wide logger isolation and ensure unavailable ZooKeeper environments are skipped rather than failed.

Review effort: Lite
Findings: 1

Open (1)

• Fix typo in test name: Refences to References

[Copilot]

Copilot review overview

🔵 Needs a closer look

Five unresolved moderate review findings remain.

Review effort: Lite
Findings: 1

Open (1)

• Fix typo in test name: Refences to References

[Copilot]

Copilot review overview

🟡 Changes recommended

A Polly API usage currently prevents compilation, and diagnostics still need bounded console emission.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Fix ResilienceContextPool.Get call to compile with operation key · New
• Fix typo in test name: Refences to References

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate review findings remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Avoid callbacks accessing uninitialized listener state · New
• Fix ResilienceContextPool.Get call to compile with operation key
• Fix typo in test name: Refences to References

[Copilot]

Copilot review overview

🟡 Changes recommended

Serialize global diagnostic logger state, skip unavailable ZooKeeper functional tests, and correct the test-name typo.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 1

Open (4)

• Serialize tests that mutate global ZooKeeper logging settings · New
• Avoid callbacks accessing uninitialized listener state
• Fix ResilienceContextPool.Get call to compile with operation key
• Fix typo in test name: Refences to References

[ReubenBond]

Native validation remains blocked at b865783

Exact-head CI run 35529047552 completed with the two ZooKeeper jobs and aggregate CI failing. There were no unrelated failed jobs. This PR remains draft.

Framework	Passed/total	Full-scenario repetitions	Point-scenario repetitions
net8.0	215/216	First failed; two unstarted	Three completed
net10.0	214/216	First failed; two unstarted	First failed; two unstarted

The fixed workload result is 3 completed, 3 failed, 6 unstarted.

The new immutable primary-exception snapshots identify all three observed failures as full ReadAll:

• net10 point scenario: post-round SameHandles full ReadAll, after the reader/writer tasks completed (Read:491 -> SameHandles:502 -> ConcurrentReads:765); heartbeat GetData failed.
• net8 full scenario: the same post-round full ReadAll check; heartbeat GetData failed.
• net10 full scenario: a concurrent full-table reader (Reader:731, with pointReads=false, observed at the worker join on line 764); membership GetData failed.

All three primary exceptions are ConnectionLossException from native GetData through Polly. No actual ReadRow failure was observed among these three failures; this is not a universal ReadRow reliability claim. No conditional-write conflict or canonical mismatch was reported.

Framework	Captured SDK resets	SDK/native detail caps	Retry warnings	Fourth-retry warnings
net8.0	224	0 / 0	12838	74
net10.0	79	0 / 0	4839	111

Fourth-retry warnings record scheduled retries, not independent failures or exhausted-operation counts. Reset signatures alone do not establish a common transport cause.

Artifacts: net8 diagnostics and TRX, net10 diagnostics and TRX.

Earlier exact-head results remain part of the record, in completed/failed/unstarted order: 51fb59a655 9/2/1, 2af4afe8cd 0/4/8, e995372a54 1/4/7. These are separate code-changing heads; the retry budget and workload stayed fixed. The earlier point-scenario failures lacked enough preserved outer stack context to identify the actual failed RPC.

The 199 focused unit tests pass on both frameworks. Scoped reviews of the owner fixes and primary-snapshot follow-up found no significant issues; all four published inline review threads have been answered and resolved. Those results do not supersede the failed native gate or constitute human approval.

The implementation retains one native-read retry owner, operation/close lifetime, full snapshot fences, and single-attempt conditional writes. No SDK change, retry-budget/workload tuning, unchanged rerun, broader tracker, or merge has been used to turn this gate green. Further implementation is on hold pending a material decision.

[ReubenBond]

PR feedback follow-up

Rechecked the live review threads, review bodies, and discussion at b865783dab. All four inline threads already have evidence-backed replies and are resolved; there is no newer inline feedback. The review overview's “Open (4)” list predates those resolutions.

The review summary also recommends skipping functional tests when ZooKeeper is unavailable. I am not applying that suggestion: this PR's native validation requires the configured ZooKeeper service and explicit failures for unavailable infrastructure. Adding a skip would weaken the required gate and conflict with the agreed scope.

No additional source change or CI rerun is warranted by this feedback. The draft and head remain unchanged. The recorded native validation failure remains a blocker; resolved review threads do not turn that gate green or constitute human approval.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The native-session, cancellation, retry, and conditional-write semantics are concurrency-sensitive and warrant final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (4)

• Serialize tests that mutate global ZooKeeper logging settings
• Avoid callbacks accessing uninitialized listener state
• Fix ResilienceContextPool.Get call to compile with operation key
• Fix typo in test name: Refences to References

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes retry ownership, session lifetime, cancellation, and snapshot concurrency across production operations, warranting final human validation.

Review effort: Balanced
Findings: None

[ReubenBond]

8dda CI classification and follow-up

Run 35635796336 tested 8dda170160. The reconnect-generation barrier improved the fixed workload to 5 completed, 3 failed, 4 unstarted, but it exposed synchronized recovery waves: retry-warning peaks reached 579/second on net10 and 401/second on net8, with 351 and 143 captured resets respectively. All three primary failures remained post-round full ReadAll heartbeat GetData failures with terminal ConnectionLossException.

The barrier correctly prevented retrying while disconnected, but one reconnect released every failed row at once. Commit 2b295d69c6 therefore keeps first-attempt all-row pipelining unchanged and serializes only native recovery requests on that operation-owned session. Each waiter acquires the recovery lease, revalidates the connected generation, and releases the lease on success, cancellation, timeout, or fault; an earlier retry which disconnects the session sends later waiters back to the next reconnect without consuming another Polly attempt.

Deterministic tests cover concurrent first attempts, maximum recovery concurrency of one, reconnect-before-registration, failure-before-Disconnected, a second disconnect while waiters are queued, canceled-waiter isolation, reconnect timeout preserving native ConnectionLossException, and unchanged attempt/backoff counts. The focused suite passes 208/208 on both net8 and net10. A scoped review found no significant issues. A fresh exact-head native gate is pending; the PR remains open and unmerged.

[Copilot]

Copilot review overview

🟡 Changes recommended

Timed-out recovery requests bypass retry admission and can recreate a concurrent request storm.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Retry loop bypasses admission after connection wait failure · New

[ReubenBond]

Sequential access revision

The all-row ReadAll fanout predates the recent consistency work: it was present in the original ZooKeeper provider in June 2015. #11316 retained that cross-row fanout, changed each row from paired member/heartbeat reads to member-then-heartbeat, and added Sync, opening/closing version+cversion fences, and whole-pass replay. The newer 128-row concurrent conformance workload substantially amplifies that request/replay shape. This is a reliability mitigation, not a claim that #11316 introduced the underlying SDK socket-reset defect.

At the user's direction, 5856ccd484 removes cross-row fanout entirely:

• ReadAll consumes ZooKeeper's existing child order sequentially.
• Each row reads membership, then heartbeat, before the next row starts.
• Cleanup evaluates rows sequentially as well; recursive deletion and writes are unchanged.
• The reconnect-generation barrier remains, but the retry-admission semaphore and herd-specific machinery are removed because there is no row-read fanout.
• Snapshot fence mismatches and cleanup BadVersion conflicts are bounded to five total attempts, matching the bounded membership-snapshot pattern used by other Orleans providers. Exhaustion throws explicit consistency/contention errors and never returns stale rows or silent cleanup success.

Tests prove one row-native request is active at a time, exact member-before-heartbeat ordering, cancellation before the next row, full-pass fence restart, success on the final allowed attempt, exact exhaustion counts, no stale snapshot/deletion, and cleanup stopping before later rows after exhausted contention. The focused suite passes 206/206 on net8 and net10. A scoped review found no significant issues.

Historical fully sequential stress completed the fixed 12/12 workload while socket-reset diagnostics remained, supporting lower load as a mitigation rather than an SDK cure. A fresh exact-head native gate is pending.

[github-actions]

Code coverage

Metric	Pull request
Lines	82.90% (115,230 / 139,005)
Branches	72.14% (33,395 / 46,291)

Report-only conclusion: current-main baseline stale.

The newest successful coverage run tested 5858077, not current main 2dace6e.

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

• Pull request commit: 0972cfa6a8
• View the pull request test run and coverage artifacts
• View the trusted coverage reporting run

[Copilot]

Copilot review overview

🔵 Needs a closer look

Distributed consistency and session-lifetime behavior require human validation, and two retry remarks overstate the implemented reconnection guarantee.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Retry loop bypasses admission after connection wait failure
• Document retry behavior after reconnect wait timeout · New

[ReubenBond]

Sequential native gate is green

Run 35640986948 tested runtime head 5856ccd484 and both ZooKeeper jobs passed:

Framework	Passed/total	Fixed workload
net8.0	223/223	6 full + 6 point repetitions completed
net10.0	223/223	6 full + 6 point repetitions completed

Result: 12 completed, 0 failed, 0 unstarted.

Sequential access substantially reduced both load and duration:

• Stable 126-row snapshots: approximately 0.098–0.110 seconds.
• Point scenarios: 4.80–6.54 seconds.
• Full scenarios: 5.38–8.23 seconds.
• Polly retry warnings: 0 on both frameworks.
• Fourth-retry warnings: 0.
• Captured SDK SocketContext.GetResult reset stacks: 0.

The diagnostic listener reached its native-event detail cap in some instances (2 net8, 1 net10), but there were no SDK reset-stack captures or read retry warnings in this run. This is a load-shape reliability result, not a claim that the pinned SDK transport defect is cured.

The latest 0972cfa6a8 commit changes XML remarks only to describe the bounded reconnect wait requested in review. Runtime behavior is identical to the green head. All inline review threads are resolved; latest-head checks are running.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Distributed consistency, retry, cancellation, and session-lifetime changes warrant final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Retry loop bypasses admission after connection wait failure
• Document retry behavior after reconnect wait timeout