Skip to content

fix: a release request is not answered until a release names the build it asked for - #3010

Closed
rbuergi wants to merge 1 commit into
mainfrom
fix/781-release-names-the-build
Closed

rbuergi wants to merge 1 commit into
mainfrom
fix/781-release-names-the-build

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes Systemorph/MeshWeaver.Plugins#781.

The mechanism

LastReleaseRequestHandledAt is stamped at dispatch, on the same commit that flips to Pending. That is correct as a re-fire guard and wrong as a delivery receipt — and the field was doing both jobs. Between the dispatch and a Release node existing there are three ways to end up with nothing, and all three were silent and indistinguishable from "no release was asked for":

  1. TryCreateReleaseNode's bound expired — a bare Timeout(_, Observable.Return(null)) with no log line at all.
  2. The create faulted or was refused. It runs under the REQUESTER's identity for attribution, so a partition the requester cannot write refuses it — logged at Warning, swallowed.
  3. The request was answered by the build already in hand (Compile cleanup: MeshWeaver.Compiler, dependency-scoped rebuilds, consume-prebuilt-everywhere (maintainer-directed) #1707 slice 3), which dispatches nothing and therefore creates nothing.

In every case ApplyCompileSuccess stamps LatestReleasePath = releasePath ?? def.LatestReleasePath — the PREVIOUS build's release — and the trigger is spent.

Measured on Publish/Deck, 2026-08-27: compilationStatus: Ok, compiledSources identical to currentSourceVersions, lastCompiledVersion 575, latestAssemblyPath naming v575's bytes — and latestReleasePath naming a release cut the previous morning. Every single-field read is healthy; only holding the release up against the build shows it. So every consumer following LatestReleasePath (the build protocol, a release-pinned activation, the Configuration pane's "→ release" link) served the previous assembly under a green Ok.

And the state was unrepairable from the outside. A second, ordinary release request hit case 3 — the bytes ARE current — so it was consumed again without producing anything. Only RequestedReleaseForce escaped, and only if someone knew to look.

The invariant restored

LatestReleasePath must never name a build older than LastCompiledVersion once RequestedReleaseAt has been consumed.

Answerable from the node alone, no store probe: a release version is {yyyyMMddHHmmss}-{hash} where the hash is minted from the build's DURABLE store coordinates — exactly what LatestAssemblyCollection / LatestAssemblyPath carry.

  • ReleaseVersionHash — the mint and the check are now ONE function, so they cannot drift into disagreeing about what a release version means.
  • ReleaseNamesBuild — the cheap comparison, pure. Inconclusive answers TRUE, three deliberate cases: no release at all (absence is not staleness — an ADOPTED build has never had one, and calling that stale would put a compile back on every install), no durable coordinates (the Null-store path mints from a process-local location), and a version shape this mint did not produce.
  • IsSatisfiedByCurrentBuild = BuildInHandAnswersRequest and ReleaseNamesBuild.
  • TryCutReleaseForBuildInHand — when the bytes are right but the release is not, the release is cut from those bytes, no compile, under the same System execution scope the compile's own release create uses. A cut that does not land leaves the clause false, so the request falls through to the compile path and gets its release from the compile's terminal create; it is never consumed for nothing.
  • The bound now delivers its diagnostic (ERROR naming the type and the attempted release) — the same rule ReadBudget states for nested read bounds: the inner bound is the only one that knows WHICH write starved.

No window was widened and no timeout was raised. ReleaseCreateBound is the same 10s, now named and documented as a bound nested inside the hub's request timeout.

Failing-then-passing evidence

ReleaseNamesTheBuildTest on the unfixed tree:

[FAIL] ARequestIsNotSatisfiedWhileTheReleaseNamesOtherBytes
  Expected value to be false ... but found True.
Failed!  - Failed: 1, Passed: 9, Total: 10

The nine siblings pass on the unfixed tree — including AnAdoptedBuildWithNoReleaseIsStillSatisfied and NoReleaseAtAll_IsInconclusive_NotStale, which are what stop this fix from quietly putting the #1707 compile back on the install path. After the fix: Passed! - Failed: 0, Passed: 17 (with RecompileStormAbsorptionTest + ForcedReleaseIsSpentTest), and 593/593 for the whole MeshWeaver.Compiler.Pipeline.Test project.

The real-mesh release suites moved to MeshWeaver.Plugins, so nothing in this repo's CI drives the watcher end to end. They were run locally against this branch via -p:MeshWeaverRoot=<this worktree>: NodeTypeReleaseGateTest, ReleaseRequestSatisfiedByCurrentBuildTest, ReleaseRequestWatcherHighWaterTest, ReleaseArtifactLinkTest, BuildCoordinationTest, CodeEditRecompileTest, CompileSourceSnapshotWedgeTest — 35/35 passed.

Same family as #2992?

Partly, and worth saying precisely. #2992 was two nested bounds that were EQUAL, so the inner one could never fire first. Here the inner bound does fire first — and then throws its diagnostic away, degrading to a null the caller cannot tell from "nothing was asked for". Same underlying rule from ReadBudget.cs (a nested bound is the only one that knows WHICH read starved), failing at the delivery step rather than the ordering step. Two instances of one rule in two weeks is worth a sweep for Timeout(_, Observable.Return(<empty>)) across the pipeline.

Not reproduced

The 2026-08-27 dump cannot say WHICH of the three swallow paths ran that night (requestedReleaseForce: true was still standing, which today's ApplyCompileSuccess would have cleared, so the node predates several fields). All three are closed here, and the third one — the only one that was permanent — now has a regression test.

…d it asked for

Systemorph/MeshWeaver.Plugins#781. `LastReleaseRequestHandledAt` is stamped at
DISPATCH, which is correct as a re-fire guard and wrong as a delivery receipt —
and the field was doing both jobs. Between the dispatch and a Release node
existing there are three ways to end up with nothing, and all three were silent
and indistinguishable from "no release was asked for": the create's bound
expired through a bare `Timeout(_, Observable.Return(null))` with no log line at
all; the create faulted or was refused (it runs under the REQUESTER's identity,
so a partition the requester cannot write refuses it) and was swallowed at
Warning; or the request was answered by the build already in hand (#1707 slice
3), which dispatches nothing and therefore creates nothing. In every case
`ApplyCompileSuccess` stamps `releasePath ?? def.LatestReleasePath` — the
PREVIOUS build's release — and the trigger is spent.

Measured on `Publish/Deck`, 2026-08-27: status Ok, sources clean,
lastCompiledVersion 575, latestAssemblyPath naming v575's bytes, and
latestReleasePath naming a release cut the previous morning. Every single-field
read is healthy; only holding the release up against the build shows it. Every
consumer following LatestReleasePath — the build protocol, a release-pinned
activation, the "→ release" link — therefore served the previous assembly under
a green Ok. And the state was unrepairable from the outside: a second, ordinary
request was absorbed by the same satisfied-branch, so asking again could not
fix it.

The invariant, from the issue: LatestReleasePath must never name a build older
than LastCompiledVersion once RequestedReleaseAt has been consumed. It is
answerable from the node alone — a release version is {yyyyMMddHHmmss}-{hash}
where the hash is minted from the build's durable store coordinates, exactly
what LatestAssembly{Collection,Path} carry.

- `ReleaseVersionHash` — the mint and the check are now ONE function, so they
  cannot drift into disagreeing about what a release version means.
- `ReleaseNamesBuild` — the cheap comparison, pure. Inconclusive answers TRUE in
  three deliberate cases: no release at all (absence is not staleness — an
  ADOPTED build has never had one, and calling that stale would put a compile
  back on every install), no durable coordinates (the Null-store path), and a
  version shape this mint did not produce.
- `IsSatisfiedByCurrentBuild` = `BuildInHandAnswersRequest` AND
  `ReleaseNamesBuild`.
- `TryCutReleaseForBuildInHand` — when the bytes are right but the release is
  not, the release is cut FROM THOSE BYTES, no compile, under the same System
  execution scope the compile's own release create uses. A cut that does not
  land leaves the clause false, so the request falls through to the compile path
  and gets its release there; it is never consumed for nothing.
- The bound now DELIVERS its diagnostic (ERROR naming the type and the attempted
  release) — the same rule ReadBudget states for nested read bounds.

Red-then-green: `ReleaseNamesTheBuildTest` fails on the unfixed tree with
`ARequestIsNotSatisfiedWhileTheReleaseNamesOtherBytes ... found True` while its
nine sibling cases pass, including the two that pin the #1707 non-regression.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 1, 2026 19:48
@rbuergi
rbuergi enabled auto-merge September 1, 2026 19:48
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

216 tests   216 ✅  2m 27s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 0d21a51.

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.

🟡 Changes recommended

It introduces an Observable.Using(access.ImpersonateAsSystem, ...) identity scope in TryCutReleaseForBuildInHand, which is a known AsyncLocal-latch hazard and conflicts with established RunAsSystem sealing patterns.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a correctness gap in the NodeType release-request watcher: a release request could be marked “handled” even when no release existed for the current build, leaving LatestReleasePath pointing at an older build under a green CompilationStatus.Ok. It restores the invariant that once a release request is consumed, the node must have (or cut) a release that names the bytes the request asked to publish.

Changes:

  • Adds a pure “release names these bytes” check (ReleaseVersionHash + ReleaseNamesBuild) and uses it to tighten IsSatisfiedByCurrentBuild.
  • When the build bytes are already current but the release is stale, cuts the missing release from the build-in-hand (no recompile) before consuming the request.
  • Adds a regression test suite for the #781 shape and documents the incident/fix in What’s New + the release redesign postmortem.
File summaries
File Description
test/MeshWeaver.Compiler.Pipeline.Test/ReleaseNamesTheBuildTest.cs New regression tests that pin the “release must name the build” invariant and the “build-in-hand” satisfaction rules.
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs Updates the release request watcher to pre-cut a release from the current build when needed and only satisfy/consume requests when the release names the build.
src/MeshWeaver.Compiler.Pipeline/NodeTypeBuildState.cs Centralizes release hash minting/checking and adds TryCutReleaseForBuildInHand; improves timeout diagnostics for release creation.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-01-a-release-now-names-the-build-you-asked-it-to-publish.md User-facing What’s New entry describing the silent-stale-release defect and the behavioral fix.
src/MeshWeaver.Documentation/Data/Architecture/Postmortems/NodeTypeReleaseRedesign.md Postmortem addendum documenting the measured failure mode, invariant, and design rule left behind.
Review details

Suppressed comments (2)

src/MeshWeaver.Compiler.Pipeline/NodeTypeBuildState.cs:212

  • This failure log message may now come from a non-compile release cut (TryCutReleaseForBuildInHand), so wording that asserts a compile succeeded / that the previous build is still referenced can be inaccurate. Prefer a message that describes only the failed release create and the possible consequence (LatestReleasePath may remain stale).
                    logger?.LogError(ex,
                        "CompileWatcher: failed to create Release node at {ReleasePath} — the "
                        + "compile SUCCEEDED but no release was cut, so LatestReleasePath still "
                        + "names the PREVIOUS build", releasePath);

src/MeshWeaver.Compiler.Pipeline/NodeTypeBuildState.cs:220

  • TryCreateReleaseNode can now be used for a release cut without a compile, so this log message shouldn’t assume a successful compile or that a previous release necessarily exists. Prefer wording that states the release cut failed and the type’s LatestReleasePath may remain stale.
            logger?.LogError(ex,
                "CompileWatcher: TryCreateReleaseNode threw for {NodeTypePath} — no release was "
                + "cut, so LatestReleasePath still names the PREVIOUS build", nodeTypePath);
  • Files reviewed: 5/5 changed files
  • Comments generated: 2
  • Review effort level: Lite

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

Comment on lines +342 to +359
var accessService = hub.ServiceProvider.GetService<AccessService>();
return Observable.Using(
() => accessService?.ImpersonateAsSystem()
?? (IDisposable)System.Reactive.Disposables.Disposable.Empty,
_ => TryCreateReleaseNode(
hub,
nodeTypePath,
new NodeCompilationResult(
AssemblyLocation: null,
NodeTypeConfigurations: [],
CompiledSources: def.CompiledSources as ImmutableDictionary<string, long>
?? def.CompiledSources?.ToImmutableDictionary(),
Collection: def.LatestAssemblyCollection,
ContentPath: def.LatestAssemblyPath,
Version: def.LastCompiledVersion),
node,
activityPath: def.LastCompilationActivityPath,
logger));
Comment on lines +200 to +204
logger?.LogError(
"CompileWatcher: the Release node create for {ReleasePath} did not land "
+ "within {Bound}s — the compile SUCCEEDED but no release was cut, so "
+ "LatestReleasePath still names the PREVIOUS build",
releasePath, ReleaseCreateBound.TotalSeconds);
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

531 tests   528 ✅  1m 30s ⏱️
  4 suites    2 💤
  4 files      1 ❌

For more details on these failures, see this check.

Results for commit 0d21a51.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

586 tests   586 ✅  50s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit 0d21a51.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    3 files      3 suites   1m 17s ⏱️
1 218 tests 1 026 ✅ 192 💤 0 ❌
1 219 runs  1 027 ✅ 192 💤 0 ❌

Results for commit 0d21a51.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

419 tests   419 ✅  1m 28s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 0d21a51.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 703 tests   1 703 ✅  1m 50s ⏱️
    3 suites      0 💤
    3 files        0 ❌

Results for commit 0d21a51.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results

   14 files     14 suites   9m 24s ⏱️
4 673 tests 4 478 ✅ 194 💤 1 ❌
4 674 runs  4 479 ✅ 194 💤 1 ❌

For more details on these failures, see this check.

Results for commit 0d21a51.

@rbuergi

rbuergi commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by its own sibling branch, which landed on main while this sat DIRTY. Evidence first, because "superseded" is a claim that deserves proof rather than a judgement call.

What is already on main

fix/781-release-postcondition merged and brought src/MeshWeaver.Compiler.Pipeline/ReleasePostCondition.cs, ReleasePostConditionTest.cs (10 cases) and the settle-side wiring in NodeTypeCompilationHelpers.cs. It enforces the same invariant this PR was written for — LatestReleasePath must not name a build older than LastCompiledVersion once RequestedReleaseAt is consumed — and it does the full job, not just detection:

  • detects the violation at the settle;
  • re-cuts from the bytes the compile just produced — ReleasePostCondition.cs:161 calls NodeTypeBuildState.TryCreateReleaseNode under .RunAsSystem(...), with the requester cleared so the create is not re-attempted under an identity that was already refused;
  • stays loud either way — an ERROR naming the type, the stale path and the build that moved, whether or not the re-cut succeeds, written to the compile _Activity;
  • is pinned on a real mesh too, by ReleasePostConditionAtSettleTest in MeshWeaver.Plugins, which reproduces the incident end to end: an Editor holding Compile asks for a release on a type whose Release subtree denies Create, the compile succeeds as System, the attributed create is refused and swallowed, and the settle must still leave a release naming lastCompiledVersion.

Why this branch should not be merged on top

Merging would add a second, parallel mechanism for one invariant. Two remedies racing to cut a release for the same build is worse than either alone — that is the dual-writer shape that produces split-brain state, and it is the class of defect this very postmortem exists to document.

It would also reintroduce a pattern main just removed. Copilot's review here asked this branch to replace Observable.Using(access.ImpersonateAsSystem, …) in TryCutReleaseForBuildInHand with AccessService.RunAsSystem — an AsyncLocal identity-leak hazard, since the scope factory and the dispose can land on different threads and leave a hub thread latched as system. ReleasePostCondition.cs on main already uses RunAsSystem. The review was right, and the merged implementation is the one that took the advice. test/ImpersonationScopeSites.allow carries src/MeshWeaver.Compiler.Pipeline/NodeTypeBuildState.cs 1 as budgeted debt that may only shrink; this branch would have spent that budget re-adding a site rather than paying it down.

The second review finding — the timeout log wording claiming "the compile SUCCEEDED" / "PREVIOUS build" on a path where there may have been neither — was also correct, and is moot once the code carrying it is not merged.

What was genuinely different here, and why it is not lost

This branch's distinctive piece was the cheap pure comparison (ReleaseNamesBuild / ReleaseVersionHash — mint and check as the same function applied twice, so they cannot drift about what a release version means) plus ReleaseNamesTheBuildTest. Main's ReleasePostCondition answers the same question at the same seam. If a reviewer later wants the pure-comparison factoring specifically, it is on fix/781-release-names-the-build at 0d21a515e — nothing needs re-deriving.

The postmortem is not lost either: Doc/Architecture/Postmortems/NodeTypeReleaseRedesign on main carries the fuller 2026-09-02 "the post-condition at the settle" write-up, including the "What was NOT the cause" section — InstallReleaseRequestWatcher had gated on a settled status since well before the incident, so the obvious "a request fired mid-compile" reading is wrong and would have produced a gate that already existed. This branch's earlier draft of the same postmortem was the merge conflict that kept it DIRTY.

Housekeeping

This PR was DIRTY, and a DIRTY PR runs no CI at all — so its last red (Run tests (shard 2)) attests to a merge base 81 commits stale and means nothing about the current tree. Its worktree was abandoned; I confirmed with the other active session that it is not live work before touching it.

Reopen if I have misread the overlap — the branch is intact.

@rbuergi rbuergi closed this Sep 2, 2026
auto-merge was automatically disabled September 2, 2026 07:30

Pull request was closed

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.

2 participants