Repository navigation
fix: the build follower waits on facts it can read, not on a notification that cannot cross processes - #1450
Merged
Merged
Conversation
…tion that cannot cross processes `BuildProtocolDriver.FollowGo` subscribed to `ObserveBuildGo(fingerprint).Take(1)` and nothing else. That is a projection of THIS cluster's mirror of `Admin/Build`, so a follower in another cluster was waiting for an event that could not be delivered to it: the builder writes the same durable row, but no change feed crosses the process boundary — `PostgreSqlChangeListener` is registered and never started in either partitioned-PG overload. It had also stopped observing its own claim, which closed the second door. A registration outlives the grant wait and the arbiter grants it the moment the build falls free, so the follower was handed builds it never ran — holding the claim at `Planning`, never heartbeating, and never taken over because its process is alive and the takeover rule defends a live holder by design. Both a builder that DIED (nobody will ever publish that GO) and a builder that FINISHED (the follower's stranded registration then locks the next image out of the build) ended in a wait with no way out. The follower now ends on either of two real events, both level-triggered on durable state: - the GO becomes VISIBLE — `ReadBuildGo` reads it off the durable build root, the same witness the claim arbiter decides on (#1437), merged with this cluster's mirror; and - the arbiter hands us the claim — on which the durable witness is re-read: a GO already there means the winner finished and this process stands down without re-baking; no GO means the builder went away mid-build and this process bakes. `WithdrawBuildClaim` gives back the candidacy (and a claim that raced it) when the follower reaches its answer some other way. `Run`'s `Catch` now bounds the grant wait only — it used to wrap the bake, so a TimeoutException anywhere in the sweep demoted the winner to a follower still holding its own claim. No timer and no bound on the wait: a bound would end it by guessing, and a follower that guesses "the build finished" certifies a share it never probed. Starting the PG change listener is deliberately NOT part of this — it changes behaviour for every partitioned-PG deployment and wants its own justification. The follower is correct without it. Both tests drive the public `BuildProtocolDriver.Run`, compile unchanged against the previous implementation, and time out there (45 s and 32 s). Fixes #1440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… guarantee Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a cross-process build-following hang by making BuildProtocolDriver stop waiting on an in-process GO notification and instead end on level-triggered, durable facts (durable GO visibility or claim grant), ensuring followers either stand down cleanly or take over the bake.
Changes:
- Refactors
BuildProtocolDriver.Run/FollowGoto distinguish grant-wait timeout from bake execution and to end follow on durable GO visibility or a claim grant. - Adds durable witness read (
ReadBuildGo) and explicit stand-down (WithdrawBuildClaim) helpers to prevent stranded claim registrations from wedging later builds. - Adds regression tests covering follower takeover-without-GO and follower stand-down-after-GO, plus documentation/WhatsNew updates.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/BuildCoordinationTest.cs | Adds regression tests for follower takeover and stand-down behavior. |
| src/MeshWeaver.Hosting/BuildProtocolDriver.cs | Implements the new follower termination logic and narrows the timeout catch to the grant wait only. |
| src/MeshWeaver.Graph/BuildCoordinationExtensions.cs | Adds durable GO read and claim withdrawal helpers used by the follower. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-follower-that-can-never-be-told.md | Adds a Fix whats-new entry describing the operational impact. |
| src/MeshWeaver.Documentation/Data/Architecture/BuildCoordination.md | Updates architecture docs to reflect the lack of cross-process change feed and the new follower “two doors” behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Test Results (shard 3) 11 files ±0 11 suites ±0 5m 25s ⏱️ +20s Results for commit ca6c682. ± Comparison against base commit 4fd5e45. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 4)1 924 tests ±0 1 914 ✅ ±0 6m 58s ⏱️ - 1m 17s Results for commit ca6c682. ± Comparison against base commit 4fd5e45. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Contributor
Test Results 66 files ±0 66 suites ±0 44m 0s ⏱️ -58s Results for commit ca6c682. ± Comparison against base commit 4fd5e45. This pull request removes 2 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…s doc promising a distinction it does not make Copilot on #1450: - NoDynamicTypes was a mutable Dictionary behind an IReadOnlyDictionary — ImmutableDictionary.Empty, per the repo's collections policy. - ReadBuildGo returns null for 'no GO', 'no store' AND 'read failed', so the doc's claim that a failed read is distinguishable was wrong. Reworded to say what is actually true and WHY collapsing them is safe: every caller's null branch is the conservative one (it bakes, into content-addressed stores), the same fail-open asymmetry ArbitrateDurably applies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #1440.
What the wait was waiting on, and why it could not arrive
BuildProtocolDriver.FollowGowas a singlemesh.ObserveBuildGo(fingerprint).Take(1). That is aprojection of this cluster's mirror of
Admin/Build. A builder in another process writes thesame durable row, but no change feed crosses the process boundary —
PostgreSqlChangeListeneris registered and never started in either partitioned-PG overload (
AddPartitionedPostgreSqlPersistencehas the
AddHostedServicecommented out; the Aspire overload never had one). A mirror activatedbefore that write is never told, so the follower was waiting for an event that could not be
delivered to it.
It had also stopped observing its own claim, which closed the second door.
RequestBuildClaim'sregistration outlives the grant wait and the arbiter grants it the moment the build falls free — so
the follower was handed builds it never ran. It then held the claim at
Planning, neverheartbeating, and was never taken over, because its process is alive and
HolderStillHoldsItdefends a live holder by design (a stopped heartbeat on a live process means busy, not dead).
Both terminal states were therefore un-endable:
The fix
The follower ends on either of two real events, both level-triggered on durable state — the same
property #1437 gave the arbiter, and for the same reason:
ReadBuildGoreads it off the durable build root — the one witness apeer cluster shares with the builder — merged with this cluster's mirror.
there means the winner finished and this process stands down without re-baking; no GO means the
builder went away mid-bake and this process bakes.
WithdrawBuildClaimgives back the candidacy (and a claim that raced it) when the follower reachesits answer some other way.
Run'sCatchnow bounds the grant wait only — it used to wrap thebake, so a
TimeoutExceptionanywhere in the sweep demoted the winner to a follower still holdingits own claim.
No timer, and no bound bolted onto the wait. A bound would end it by guessing, and a follower
that guesses "the build finished" certifies a share it never probed — a silent wrong answer where
the hang at least announced itself. (#1366 removed a staleness clock for exactly this reason.)
Deliberately NOT in scope
Starting the PG change listener. It would make the GO propagate promptly across clusters rather
than at the arbiter's next pass, but it changes behaviour for every partitioned-PG deployment and
wants its own justification and measurement — the issue says so explicitly. The follower is correct
without it.
Verification — fail before, pass after
Both tests drive the public
BuildProtocolDriver.Run, so they compile unchanged against theprevious implementation. Reverting only the two
src/files and re-running:With the fix, the whole class and then the whole project:
dotnet build -c Release -warnaserror --no-incrementalclean onMeshWeaver.Hosting,MeshWeaver.GraphandMeshWeaver.Documentation;DocumentationLinkIntegrityTestgreen.Noted in passing, not fixed here
BuildNodeType.ArbitrateDurablybuilds its decision input from the claim lock node(
Admin/Build/_Claim), andArbitrate'sGrantSettleWindowguard testsnode.Path == RootPath—which is never true on that path. So #1424's convergence window is inert whenever a storage adapter
exists. Out of scope here; worth its own issue.
🤖 Generated with Claude Code