Repository navigation
fix(build): the build claim is exclusive across clusters, not just within one - #1437
Merged
Merged
Conversation
…thin one Two Orleans clusters sharing one database each granted themselves the build claim and each ran the full 268-type bake (#1424). Three things lined up: - the arbiter decided inside the owning hub's serialised Update lambda, which is exclusive within ONE cluster — a second cluster activates its own hub for Admin/Build, arbitrates against its own mirror, and grants its own candidate; - both grants were minted at the same next MeshNode.Version and the store's monotonic condition APPLIES at equal versions, so both landed last-write-wins and neither writer was told it lost (the refusal signal is a strictly greater stored version, and the versions were equal); - nothing propagates a rival's write: Orleans membership and its memory streams are per-cluster, and PostgreSqlChangeListener is never started in either partitioned-PG wiring. The fix, in two halves. IStorageAdapter.WriteIfVersion — an atomic compare-and-set, the write-side twin of DeleteIfExists. Applies only while the durable row still carries the version the caller read (0 = "must not exist"), so at most one of N concurrent callers is told true. Postgres implements it as one statement whose row count is the verdict (ON CONFLICT ... WHERE target.version = @expected, or DO NOTHING); in-memory via TryAdd/TryUpdate. Every decorator forwards explicitly — the interface default is a non-atomic read-compare-write, correct only for single-writer backends, exactly like DeleteIfExists. The claim moved onto a lock node, Admin/Build/_Claim. Making the grant exclusive was necessary and not sufficient: every cluster mirrors Admin/Build and flushes it wholesale through the persistence sampler, and MeshNode.Version is a per-node counter rather than a cross-cluster clock — so a LOSING cluster's flush overwrote the winner's ClaimedBy with its own null and the next arbitration pass found a free build. The cross-cluster test caught exactly that. The lock has no hub, no mirror and no sampler; it is written only by the compare-and-set grant, a compare-and-set heartbeat, and a rowcount-gated DeleteIfExists on release. BuildState.ClaimedBy stays as the observable projection ObserveBuildClaim reads. Order matters: the mirror commits ~200 ms before the sampler reaches storage, so the durable decision has to come first — adjudicating afterwards finds the loser already baking. A refused claim writes nothing and retries nothing; the arbiter is level-triggered and re-decides on the next real change (the lock's own change feed is now one of its triggers, which is what hands a released claim on without waiting for the slow tick). Fails OPEN: with no adapter owning the path the grant is taken on the mirror as before. Being wrong that way costs one duplicated bake, bounded and non-corrupting because every downstream write is content-addressed; being wrong the other way leaves the fingerprint with no GO and stalls the rollout. Also: NodeTypeBatchBake.WriteStamp now uses the same primitive. Its racing writer is the type's own per-node hub — not a baker, so the claim does not and cannot exclude it — but both sides read version v and minted v+1, which the store applied at equal versions, silently destroying one stamp while the "REFUSED" log stayed quiet because it tests for a strictly greater version. The loser's outcome is unchanged (bytes durable, record pending, probe re-bakes) and now loud. No compare-and-re-apply loop: that would be a second write path into a record this driver does not own. Verified: CrossClusterBuildClaimTest fails with 2 grants before, passes with 1 after; BuildCoordinationTest, the prewarm/bake suites, Hosting.Test (163), Graph.Test (1104) and Documentation.Test all green; Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a deleted row Postgres implemented the expectedVersion > 0 case as INSERT … ON CONFLICT DO UPDATE … WHERE target.version = @expected, which reports SUCCESS when the row is absent — the INSERT simply lands. "The row still carries exactly v" is false when there is no row, and for the build claim the difference is a live defect: a heartbeat racing a release re-creates the lock its holder just dropped, blocking the next candidate for the whole staleness budget. That case is now a plain UPDATE with the version predicate, so the row count is the verdict and an absent row is a refusal. The SET list mirrors the ON CONFLICT one exactly, so created_by/created_date stay untouched and authorship survives a compare-and-set as it does an upsert. The in-memory adapter already refused this case; the two backends now agree on what compare-and-set means, which the new tests pin. CompareAndSetWriteTests covers the primitive through the PRODUCTION decorator chain: 8 concurrent creates yield exactly one winner and seven told losers, the same for 8 concurrent successions of a held row, a mismatched version is refused and leaves the row, and a delete is not undone by a later compare-and-set. The chain matters — the interface default is a non-atomic read-compare-write, so a decorator that forgets to forward silently downgrades every caller to the behaviour the primitive replaces, and nothing else would notice. MeshWeaver.Hosting.Test 167/167; Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-Orleans-cluster build-claim collisions by introducing an atomic compare-and-set storage primitive and moving build claim ownership to a dedicated durable lock node, ensuring only one server proceeds with a given build across all clusters sharing the same backing store.
Changes:
- Add
IStorageAdapter.WriteIfVersion(CAS) and thread it through persistence decorators/routing adapters (Postgres, in-memory, file-system via default). - Move build claim ownership to
Admin/Build/_Claim(and per-chunk equivalents) and update the arbiter/heartbeat/release paths to operate on the lock. - Add a new cross-cluster regression test and update architecture/“What’s New” docs to reflect the new coordination model.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/CrossClusterBuildClaimTest.cs | Adds a regression test modeling two independent clusters sharing one durable store. |
| src/MeshWeaver.Mesh.Contract/Services/IStorageAdapter.cs | Introduces the WriteIfVersion CAS API with a default implementation and contract docs. |
| src/MeshWeaver.Hosting/Persistence/VersionWritingStorageAdapter.cs | Forwards WriteIfVersion through the version-history decorator. |
| src/MeshWeaver.Hosting/Persistence/SubtreeDeletionGuardStorageAdapter.cs | Guards and forwards CAS writes through the subtree-deletion decorator. |
| src/MeshWeaver.Hosting/Persistence/PersistenceService.cs | Adds a sequential try-then-claim chain for CAS writes across writable providers. |
| src/MeshWeaver.Hosting/Persistence/PathFilteringStorageAdapter.cs | Adds path-scoped forwarding for CAS semantics. |
| src/MeshWeaver.Hosting/Persistence/MonotonicWriteGuardStorageAdapter.cs | Forwards CAS writes and updates high-water tracking only on applied writes. |
| src/MeshWeaver.Hosting/Persistence/InMemoryStorageAdapter.cs | Implements atomic CAS via TryAdd/TryUpdate for the in-memory backend. |
| src/MeshWeaver.Hosting/Persistence/CachingStorageAdapter.cs | Routes CAS writes to a fresh inner filesystem adapter to avoid stale-cache comparisons. |
| src/MeshWeaver.Hosting/NodeTypeBatchBake.cs | Uses CAS to make compile-state stamp writes deterministic and refusal-visible. |
| src/MeshWeaver.Hosting/BuildProtocolDriver.cs | Ensures chunk close-out explicitly releases the durable claim lock. |
| src/MeshWeaver.Hosting.Snowflake/SnowflakePathRoutingAdapter.cs | Routes CAS writes to the owning per-schema adapter. |
| src/MeshWeaver.Hosting.PostgreSql/PostgreSqlStorageAdapter.cs | Implements atomic CAS as a single-statement rowcount-gated upsert in Postgres. |
| src/MeshWeaver.Hosting.PostgreSql/PostgreSqlPathRoutingAdapter.cs | Routes CAS writes to the owning per-schema adapter. |
| src/MeshWeaver.Graph/Configuration/BuildNodeType.cs | Moves arbitration to durable-first logic and introduces the _Claim lock path helpers. |
| src/MeshWeaver.Graph/BuildCoordinationExtensions.cs | Extends heartbeat/completion/failure flows to maintain and release the durable lock. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-two-servers-no-longer-run-the-same-build-twice.md | Adds a release note describing the user-visible impact of cross-cluster exclusivity. |
| src/MeshWeaver.Documentation/Data/Architecture/BuildCoordination.md | Updates architecture documentation to reflect durable lock + CAS behavior and known limitations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+64
to
+78
| public IObservable<bool?> WriteIfVersion( | ||
| MeshNode node, long expectedVersion, JsonSerializerOptions options) | ||
| { | ||
| var write = inner.WriteIfVersion(node, expectedVersion, options); | ||
| if (versionQuery is null) | ||
| return write; | ||
|
|
||
| return write.SelectMany(applied => applied is true | ||
| ? versionQuery.WriteVersion(node, options) | ||
| .Catch<MeshNode, Exception>(_ => Observable.Empty<MeshNode>()) | ||
| .DefaultIfEmpty(node) | ||
| .LastAsync() | ||
| .Select(_ => applied) | ||
| : Observable.Return(applied)); | ||
| } |
Comment on lines
+181
to
+185
| public static IObservable<System.Reactive.Unit> ReleaseBuildClaim( | ||
| this IMessageHub hub, string holder, string path) | ||
| => OnOwnLock( | ||
| hub, holder, path, | ||
| (storage, held, _, _) => storage.DeleteIfExists(held.Path).Select(_ => (bool?)true)); |
…grant The pending-candidates check ran after the arbitration pass had already read the lock, so the periodic tick cost a storage round-trip on every quiet Build node for a decision that was always "nothing to do". Checking the mirror's RequestedClaims first makes the quiet path free. No behaviour change; the doc-comments that still described the pre-lock design are brought in line. Build/bake filter 76/76; Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Contributor
Test Results (shard 2)2 453 tests - 122 2 452 ✅ - 122 5m 37s ⏱️ - 1m 28s Results for commit f55dbdd. ± Comparison against base commit c323673. This pull request removes 335 and adds 213 tests. Note that renamed tests count towards both. |
Contributor
Test Results (shard 1)1 489 tests +123 1 489 ✅ +123 7m 44s ⏱️ +22s Results for commit f55dbdd. ± Comparison against base commit c323673. This pull request removes 213 and adds 336 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Author
|
Two refinements landed after the opening description, both found by reviewing the primitive's edges:
Verification on the final tree: full |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1424.
What was wrong
The claim arbiter's exclusivity came from the owning per-node hub serialising its
Updatelambdas — which holds per cluster. A second Orleans cluster over the same database activates its own hub forAdmin/Build, arbitrates against its own mirror, and grants its own candidate. Three things then made the collision invisible:MeshNode.Version, and the store's monotonic condition applies at equal versions (re-persisting an unchanged node is a legitimate shape), so both landed last-write-wins;saved.Version > written.Version, and the versions were equal;PostgreSqlChangeListeneris registered but never started in either partitioned-PG wiring.On top of that, the grant is observed off the RAM mirror, which commits ~200 ms before the persistence sampler reaches storage — so adjudicating after the write lands can never stop the second builder starting.
The fix
1.
IStorageAdapter.WriteIfVersion— an atomic compare-and-set. The write-side twin ofDeleteIfExists: applies only while the durable row still carries the version the caller read (0= "must not exist"), so at most one of N concurrent callers is toldtrue. Postgres implements it as one statement whose row count is the verdict (ON CONFLICT … DO UPDATE … WHERE target.version = @expected, orDO NOTHING); in-memory viaTryAdd/TryUpdate. Every decorator forwards explicitly — the interface default is a non-atomic read-compare-write, correct only for single-writer backends, exactly the contractDeleteIfExistsalready documents.2. The claim moved onto a lock node,
Admin/Build/_Claim. Making the grant exclusive turned out to be necessary and not sufficient, and the test caught it: every cluster mirrorsAdmin/Buildand flushes it wholesale through the persistence sampler, andMeshNode.Versionis a per-node counter rather than a cross-cluster logical clock — so a losing cluster's flush overwrote the winner'sClaimedBywith its ownnull, and the next arbitration pass found a free build. The lock has no hub, no mirror and no sampler; it is written only by the compare-and-set grant, a compare-and-set heartbeat, and a rowcount-gatedDeleteIfExistson release.BuildState.ClaimedBystays as the observable projectionObserveBuildClaimreads — a clobber there now costs a stale view, never a second builder.A refused claim writes nothing and retries nothing: the arbiter is level-triggered and re-decides on the next real change. The lock's own change feed is now one of its triggers, which is what hands a released claim on without waiting for the slow tick.
Fails OPEN. With no adapter owning the path (or no persistence at all — monolith, test, dev box) the grant is taken on the mirror exactly as before. Being wrong that way costs one duplicated bake — bounded and non-corrupting, because every downstream write is content-addressed. Being wrong the other way leaves the fingerprint with no GO at all, which holds every silo's readiness probe down and stalls the rollout. Same asymmetry the bake gate applies; the opposite of
AssemblyCacheRetention, where the wrong answer deletes bytes a running pod needs.Also, since we were in this code
NodeTypeBatchBake.WriteStampnow uses the same primitive. Its racing writer is the type's own per-node hub — not a baker, so the claim does not and cannot exclude it, and the existing comment was right that election cannot close this. But the comment's conclusion ("the guard above stays the thing that makes it safe") was wrong at equal versions: both sides readvand mintedv+1, the store applied both, one stamp was silently destroyed, and the "REFUSED" warning never fired because it tests for a strictly greater version. Conditioning on the version we read makes the loser deterministic and loud. Its outcome is unchanged and still correct (bytes durable and content-addressed, record pending, level-triggered probe re-bakes). Deliberately no compare-and-re-apply loop — that would be a second write path into a record this driver does not own.Verified
CrossClusterBuildClaimTest— two independent meshes (own hubs, own mirrors, own adapter instances, so no change feed crosses) over one shared backing store, both claiming concurrently. Fails before withfound 2; passes after with exactly 1, and the durable lock names the winner. Run withDOTNET_PROCESSOR_COUNT=4.BuildCoordinationTest(incl. the full protocol-driven bake), prewarm/bake/write-integrity filter: 80/80.MeshWeaver.Hosting.Test163/163,MeshWeaver.Graph.Test1104/1104,MeshWeaver.Documentation.Test20/20.-c Release -warnaserrorclean on every touched project plusMemex.Portal.Distributed.Follow-up found while here (not fixed)
BuildCoordination.mdclaimed the GO broadcast rides the PGLISTEN/NOTIFYfeed "cross-process and therefore cross-ServiceId by construction". That listener is never started in either partitioned-PG overload, so across clusters nothing propagates the GO, andBuildProtocolDriver.FollowGosubscribes with no bound. The doc now says so. This does not affect the fix (the arbiter reads the lock rather than waiting to be told), but it means a separate-cluster bake host should still be treated as "publishes the GO earlier", not "spares its peers the bake". Starting the listener, or giving the follower a bounded fall-back to its own sweep, is the outstanding work.🤖 Generated with Claude Code