fix(zookeeper): preserve membership consistency

[ReubenBond]

ZooKeeper membership reads can mix canonical row states with a different table version. Membership mutations need atomic row/table preconditions, while each owning silo publishes its heartbeat through an independent native column update.

Synchronize once at the start of full and point reads, then fence canonical state with the deployment node's data and child versions. Same-session retries retain the observed order. Update membership using one native atomic multi containing conditional writes of only the deployment node and membership row. Heartbeat data updates preserve both canonical tokens, so a status or vote update can use the tokens read before an owner heartbeat. Reads retain raw heartbeat values independently of the canonical snapshot fence.

Owner-only UpdateIAmAlive performs one native setData of the existing /IAmAlive node with wildcard version -1. The membership row and table version are preserved, and native errors, including missing-node errors, propagate. Cleanup enumerates children once, reads each selected row's eligibility, and conditionally deletes its heartbeat and row in one atomic multi. It retries only the contested row, preserves the table version, and respects start time, heartbeat, every suspicion vote, and the exact UTC cutoff. Acknowledged initialization and deletion writes use their native completion guarantees.

Preserve the persistent-node layout, supported options, read-only connection eligibility, and cancellation/completion ownership. Focused native-operation mocks cover exact heartbeat and canonical transaction shapes, token reuse after heartbeat writes, field isolation, read fences, canonical conflicts and atomicity, cleanup request counts and row-scoped retries, concurrent failures, and cancellation.

Includes only the tiny legacy cleanup-baseline prerequisite from #11308 (bede893d290a9035f196f4c3ed58213d102efc6c), kept as a separate commit. The ZooKeeper implementation is independently based on main; conformance-kit wiring follows separately.

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Copilot review overview

🔵 Needs a closer look

It makes substantial concurrency/consistency changes to a production membership provider and alters operational connection behavior, warranting final human review before approval.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Read-only ZooKeeper connection option dropped from read paths · New
• Test fake depends on non-public ZooKeeperNetEx Op internals via reflection · New

What changed in this PR

This PR hardens the ZooKeeper-based membership provider against inconsistent reads and liveness regressions under concurrent updates by adding version-fenced read loops, atomic multi-operations, and compare-and-set heartbeats, with extensive native-operation unit tests to validate race behavior.

Changes:

• Implement coherent read loops for full and point reads, fenced using ZooKeeper node version and child-version checks.
• Make membership mutations atomic using native multi operations and CAS heartbeat updates to preserve the maximum IAmAlive timestamp across races.
• Update cleanup semantics and add focused unit tests (plus a small shared cleanup-baseline test adjustment) to cover read/write races, cleanup eligibility, failures, and cancellation.

File	Description
test/​Orleans.Runtime.Internal.Tests/​MembershipTests/​MembershipTableTestsBase.cs	Adjusts the legacy cleanup baseline expectations to align with “retain Active, prune eligible Dead” behavior.
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperNativeFake.cs	Adds a deterministic in-memory fake for ZooKeeper native operations (reads, children, multi, CAS writes) to exercise race scenarios.
test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperBasedMembershipTableUnitTests.cs	Adds comprehensive unit tests for coherent reads, CAS heartbeats, atomic mutations, cleanup eligibility, and infrastructure/cancellation failures.
src/​Orleans.Clustering.ZooKeeper/​ZooKeeperBasedMembershipTable.cs	Refactors provider internals to native-ops wrappers, version-fenced read loops, atomic multi-based insert/update/cleanup, and CAS heartbeat logic.

---

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

[github-actions]

Code coverage

Metric	Pull request
Lines	82.29% (112,655 / 136,903)
Branches	71.53% (32,477 / 45,403)

Report-only conclusion: current-main baseline stale.

The newest successful coverage run tested 3604a08, not current main 0f9537d.

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: 309ab40b8e
• View the pull request test run and coverage artifacts
• View the trusted coverage reporting run

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes core membership/heartbeat/cleanup concurrency behavior in the ZooKeeper provider and, despite strong tests, warrants final human review due to the correctness-critical nature of membership semantics.

Review effort: Lite
Findings: None

Resolved since last review (2)

• Read-only ZooKeeper connection option dropped from read paths
• Test fake depends on non-public ZooKeeperNetEx Op internals via reflection

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes core membership consistency/atomicity logic in a distributed coordination backend (ZooKeeper) and should receive final human review despite strong unit-test coverage.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

ZooKeeperNativeFake.GetChildren returns nondeterministic child order

test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperNativeFake.cs:62

GetChildren derives the child list from Dictionary.Keys enumeration order, which is not guaranteed and can make tests nondeterministic when multiple children exist. Sorting the returned child names (ordinal) makes the fake deterministic without changing semantics of the membership code under test.

[Copilot]

Copilot review overview

🔵 Needs a closer look

It modifies critical membership consistency/concurrency behavior in the ZooKeeper provider and merits final human validation despite strong targeted test coverage.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

ZooKeeperNativeFake should avoid xUnit assertion for API mismatch

test/​Extensions/​Orleans.Clustering.ZooKeeper.Tests/​ZooKeeperNativeFake.cs:179

ZooKeeperNativeFake.CreateResult() currently uses Assert.IsType(...) to validate the reflection-created result. If ZooKeeperNetEx changes constructor visibility/signature, this will fail as an xUnit assertion rather than the clearer InvalidOperationException diagnostics used elsewhere in this fake (e.g., ApiMismatch/GetValue). Consider replacing the assertion with an explicit type check and throw InvalidOperationException/ApiMismatch so failures point directly to an SDK/API mismatch.

[ReubenBond]

Addressed the body-only suggestion in review 5249963064 with 59390e0. Constructor visibility/signature mismatches occur at constructor lookup, before an assertion can inspect the result. CreateResult<T> now explicitly locates the matching non-public constructor and uses the existing ApiMismatch / InvalidOperationException diagnostic when it is unavailable. Invoking that constructor guarantees the requested result type, so the fake no longer needs an xUnit assertion or a redundant type fallback. Added a missing-constructor diagnostic regression; all 111 focused net10 membership tests pass. This is a test-helper-only change; native membership and heartbeat behavior is unchanged.

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes core membership consistency and concurrency behavior (reads, atomic writes, cleanup, and heartbeat semantics) in the ZooKeeper provider and warrants final human review despite strong unit test coverage.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Cleanup baseline no longer exercises retention of non-dead non-active rows

test/​Orleans.Runtime.Internal.Tests/​MembershipTests/​MembershipTableTestsBase.cs:490

This cleanup baseline now only inserts/validates Active and Dead rows. Since IMembershipTable.CleanupDefunctSiloEntriesAsync is documented as deleting dead entries, it would be useful for this shared test to also include at least one non-dead, non-Active status (eg, Joining) and assert that it is retained after cleanup. Without that, a provider could regress and start pruning other statuses and this test would not catch it.

[ReubenBond]

Regarding the shared-baseline coverage suggestion in review 5250121734: this PR already covers the requested retention behavior in Cleanup_OnlyEligibleDeadRows_DeletesWithNativeVersionChecks. Its matrix is None, Created, Joining, Active, ShuttingDown, Stopping, and Dead. For every non-Dead status it asserts all three nodes remain, the retained row has the expected status, no delete transaction was issued, and the table version is unchanged. For Dead it asserts exactly the two conditional native deletes and unchanged table version. Disposition: covered by the existing provider regression. The conformance-kit owner agrees that the frozen shared prerequisite should remain unchanged and duplicate shared-baseline work is unnecessary for this suggestion.

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes core membership correctness paths and includes a confirmed cleanup edge case which can silently retain corrupted rows, warranting additional human review.

Review effort: Lite
Findings: 1

Open (1)

• Cleanup swallows NoNode even when heartbeat node is missing · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

It makes substantial, correctness-critical changes to membership read/write/cleanup semantics with intricate retry and native-operation behaviors which warrant final human verification despite strong test coverage.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Cleanup swallows NoNode even when heartbeat node is missing