Repository navigation
fix(#3461): the publication layout belongs to the PREFIX — phase 4 no longer has to be atomic across two repos - #4249
Merged
meshweaver-cloud[bot] merged 2 commits intoSep 13, 2026
Conversation
… pointer it invalidated Phase 4's stated precondition — "plugins must flip in ONE change set across BOTH producers" — is not available: those producers live in two repositories, which cannot merge atomically, and one publishes on every core build. The flip must pass through a prefix where one producer writes generations and the other still writes flat. That state was unhandled, and it is this issue's own failure mode one layout further in. MECHANISM, at file:line on the merge base. `resolve_publication_dir` runs on every run regardless of layout (publish-bake-bundles.sh:1178), so a FLAT run on a prefix carrying a live _current read every decision — already published? which architecture? which source commit? does it carry a module set? — off the GENERATION the pointer names (:1208-:1317, all $LIVE), and then wrote the FLAT directory (publish_publication's flat branch). Either it published into a directory no reader resolves and reported success while every consumer went on serving the older generation, or it skipped against a directory it was not writing. ShippedPrebuiltBundles.ResolvePublicationPointer follows a pointer whose generation exists and is sealed, so the stale serve is indefinite. THE RULE: the layout a run publishes in decides which directory is live FOR THAT RUN. A flat run reads and writes the prefix, and the pointer it found is retired once its publication stands — which returns the prefix to last-writer-wins, i.e. exactly today's semantics. - retired LAST, from the ONE success exit of a target (publish_to_target now wraps publish_to_target_inner): five success paths return 0 and every refusal exits the subshell, so it runs exactly when the publication stands and no future path can forget to thread it through. Until that line consumers resolve the generation — whole and sealed — not a directory mid-replace. - a POSTCONDITION, not check-then-act: the delete is attempted and the absence read back. A concurrent producer deleting it first is a success for this run; only a pointer still standing fails the target, loudly. - a ROUTING statement, never content: every generation directory stays, and retention collects unnamed ones by age, by its own rules. - announced as ::warning:: on both sides — a half-flipped fleet must not read as intentional. WATCHED IT FAIL. test-publish-bake-overlap.py grows two mixed-layout cases (a flat publisher that republishes over a pointed-at prefix, and one that skips). Against origin/main's script: 87 passed, 3 failed — exactly the three new assertions, _current reading Systemorph-MeshWeaver-3501-1 and -3601-1 — while both cases' non-vacuity checks passed, so they exercised the path. Against this script: 90 passed, 0 failed. Consequence: plugins can be flipped ONE PRODUCER AT A TIME, in whichever order the two repositories merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Address the critical pointer-retirement and stale-content races, and add the missing interleaving regression.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes mixed flat/generation publication prefixes safer during the phased rollout.
Changes:
- Retires generation pointers after successful flat publishing.
- Adds mixed-layout publish and skip regression tests.
- Updates rollout documentation and verification details.
File summaries
| File | Review findings |
|---|---|
.github/scripts/publish-bake-bundles.sh |
Critical (3 votes): unconditional pointer deletion can remove a newer pointer. Critical (1 vote): skip logic can expose stale flat content. |
.github/scripts/test-publish-bake-overlap.py |
Moderate (1 vote): lacks coverage for pointer replacement during retirement. |
src/MeshWeaver.Documentation/Data/Architecture/SealedPublicationGenerations.md |
Nit (1 vote): contains a stale assertion count. Nit (2 votes): rollout guidance contradicts the caller-facing workflow contract. |
Review details
Suppressed comments (3)
.github/scripts/publish-bake-bundles.sh:1108
- The wrapper calls
retire_pointerafter every successful inner path, including already-published/converged skips that return without callingpublish_publication. Consequently these messages claim that this run "published" or "sealed" the flat publication even when it wrote no bytes, making the new skip-path diagnostics inaccurate. Use wording that describes successful flat-layout handling rather than claiming this run sealed content.
echo "::warning::retired $account/$share/$dest/$POINTER — this run published the FLAT directory, so the flat publication is the live one again. Consumers resolve $dest itself until a generation publisher writes a new pointer."
return 0
fi
echo "::error::$dest/$POINTER could not be retired (exists=$still) under $account/$share. This run sealed the FLAT publication, but every consumer resolves the pointer, so they would keep serving the generation it names and this run's bytes would reach nobody. Remove $POINTER by hand, or flip this caller to publication-layout: generation."
.github/scripts/test-publish-bake-overlap.py:1153
- The new mixed-layout cases run the generation publisher to completion and only then invoke the flat publisher. They never replace
_currentafter the flat run's initial resolution but beforeretire_pointer; that interleaving makes the new unconditional delete remove a newer generation pointer, so the regression can pass while the production race remains. Add a hook/race fixture for that ordering and assert the newer pointer is preserved (or the target fails).
h.publish(core, "Systemorph/MeshWeaver", "3501", gen)
tok_gen = "Systemorph-MeshWeaver-3501-1"
r = h.publish(sat, "Systemorph/MeshWeaver.Plugins", "3502") # flat — the default, unflipped
s = h.shelf()
check("the fixture really is mixed (a generation exists AND this run sealed the flat directory)",
s.generations() == [tok_gen] and s.sealed()
and set(s.bakes_present()) - {"<marker>"} == {"satellite"},
src/MeshWeaver.Documentation/Data/Architecture/SealedPublicationGenerations.md:580
- The preceding
16 of the 67claim is from an older version of this harness and contradicts the next paragraph's current pre-change result (87 passed, 3 failed) and the 90-assertion total above. Please remove or update this stale count so the documented negative control is reproducible for this PR.
🚨 **Negative control:** run against the pre-change script, **16 of the 67 fail**. For the
- Files reviewed: 3/3 changed files
- Comments generated: 3
- 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
+1099
to
+1103
| az storage file delete --account-name "$account" --share-name "$share" \ | ||
| --path "$dest/$POINTER" --auth-mode login --backup-intent --only-show-errors > /dev/null 2>&1 || true | ||
| still=$(az storage file exists --account-name "$account" --share-name "$share" \ | ||
| --path "$dest/$POINTER" --auth-mode login --backup-intent --query exists -o tsv \ | ||
| --only-show-errors 2>/dev/null || echo "unknown") |
Comment on lines
+1239
to
+1243
| if [ "$LIVE" != "$DEST" ] && [ "$PUBLICATION_LAYOUT" = "flat" ]; then | ||
| echo "::warning::$ACCOUNT/$SHARE: $DEST/$POINTER names generation '${LIVE##*/}', but this run publishes the FLAT layout (publication-layout: flat). Reading and writing $DEST, and retiring the pointer once this publication is sealed — otherwise consumers would keep resolving '${LIVE##*/}' and serve bytes this run replaced. Another producer of this prefix has flipped to the generation layout; flip this caller too (MeshWeaver#3461, phase 4)." | ||
| LIVE="$DEST" | ||
| RETIRE_POINTER=true | ||
| elif [ "$LIVE" != "$DEST" ]; then |
Comment on lines
+390
to
+392
| Set `publication-layout: generation` on every producer of one prefix. The multi-producer prefix is | ||
| `plugins` and nothing else: core CD's `plugins-bake` and MeshWeaver.Plugins' `publish-bake`. The | ||
| single-producer prefixes each flip in their own repository's PR. |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
…n, never retire the pointer Three Copilot findings, all correct and all fatal to the first mechanism: 1. `retire_pointer` deleted `_current` after resolving it, so a generation publisher moving the pointer in between lost its newer publication. 2. With LIVE switched to DEST the skip decisions stopped proving anything about the generation: the pointer moves BEFORE the flat compatibility copy is refreshed, so a run seeing an older flat copy could take the already- published (or not-sealing-backwards) skip, retire a NEWER generation, and expose the older bytes. Content going BACKWARDS — a worse defect than the one being fixed. 3. The lane's own `publication-layout` description still told operators the prefix must flip in ONE change set, contradicting the change. Rewritten, and it is smaller: a live `_current` means the PREFIX is on the generation layout, so a run that resolves one publishes a GENERATION whatever its caller asked for (loudly, naming the caller to flip). One assignment; no delete, no conditional delete, no ETag, no new ordering. - finding 1 disappears — nothing is deleted, and move_pointer is still the last write of a proven publication; - finding 2 disappears — LIVE stays the generation, so every already-published / architecture / source-commit / module-set decision reads the publication consumers are actually served, exactly as before; - never backwards: a flat caller whose content IS the live publication skips and changes nothing, asserted as its own case; - finding 3: the input description now states that the layout belongs to the prefix and that the two producers may flip in either order, and its stale "retention is not implemented" precondition is corrected (#3959). Watched it fail: against origin/main's script, 87 passed / 3 failed — the three assertions about the flat caller publishing a generation. The "earlier generation is untouched" control and the whole never-backwards case pass on main too, which is what makes them regression controls. Against this script, 90/90. Siblings green: pointer-readers 14/14, sealed-module-compose 6/6, bundle-registry 92/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi
removed this pull request from the merge queue due to a manual request
Sep 13, 2026
Contributor
|
ℹ️ Merge-queue steward: no action — removed from the queue with reason |
This was referenced Sep 14, 2026
rbuergi
added a commit
that referenced
this pull request
Sep 14, 2026
…reconcile the migration prose Three findings from the Copilot review of the phase-4 default move. 1. The fleet-wide behaviour change had NO test. Reverting `node-repo-publish-bake.yml`'s `publication-layout` default to `flat` would leave every existing test green while all six satellite lanes silently returned to in-place publication. `NodeRepoLaneHostGuard. ThePublishBakeLane_DefaultsToTheGenerationLayout` asserts the input's `default: 'generation'` AND that the step exports it as `BAKE_PUBLICATION_LAYOUT` (the script's own fallback is `flat`, so a lane that stopped exporting the variable would publish flat with the default still reading `generation`). Watched failing in both directions: default reverted to `flat` → red on the first assertion; the env export renamed → red on the second; both restored → green. 2. The transition residual the review named is real, is documented rather than patched, and is kept at zero by a second guard. `NoCallerInThisRepository_PublishesFlat` refuses any caller in this repository that would take the flat arm (watched failing by flipping `main-cd.yml`'s `plugins-bake` to `flat`). The remaining way to reach that arm is a run whose `publish-bake-bundles.sh` predates #4249 — which no code added to today's script can help, because such a run is not running it, and which leaves pointer-followers on the NEWER generation, the safe direction. A new table in Doc/Architecture/SealedPublicationGenerations states the residual, its three entry conditions and why there is no check on the flat arm. 3. The migration prose contradicted itself after the flip. Corrected: - phase 2's heading and its "what actually shipped" bullet said the selector defaults to `flat` — now labelled as this phase's own history, with the phase-4 value beside it; - "Its pin half, and phases 4-5, are open" — phase 4 is landed, and the "until the writer flips, the window is shrunk not closed" sentence is now stated as the half-truth it became (closed for pointer-followers, open for the flat copy); - the Verification section reported 90 assertions and only #4249's negative control; it now reports 98/0 and labels EVERY negative control with the change that measured it, which is what keeps such a section from going stale; - SealedPublicationReads said the selector "defaults to flat, so nothing anywhere writes a generation", and separately that the two Azure-direct readers still address the prefix — the latter was fixed in phase 3 and is executed by test-publication-pointer-readers.py; - BundleDeliveryStages carried "open — phase 4/5 unlanded" and the "phase 4 unreachable from every producer" reading of 2026-09-13. Verified: MeshWeaver.Documentation.Test link integrity + topic map + What's New + NodeRepoLaneHostGuard, 14 passed / 0 failed; build -c Release -warnaserror, 0 Warning(s) 0 Error(s). No workflow or script behaviour changed by this commit — `.github/` is byte-identical to the previous one. 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.
Refs #3461— the part of phase 4 that is a defect rather than a scope call. It flips nothing; it removes the reason the flip had to be atomic across two repositories.What was established first
src/composes underShippedPrebuiltBundles.PublicationDirectoryOfand so resolves_current— the boot seeder, the same-major adoption fallback,SealedPublicationIndex,PrebuiltBundleRetention,PublishedBundleCatalogue,SealedLaneBundles,ServedModuleBytes, the portal's HTTP endpoints — and so do the two Azure-direct readers fixed in phase 3. Phase 4 needs no reader change.pluginspublish only their own modules already (core CD passesupstream-sources: '', the satellite is a root), so the 4-vs-5 module-set difference is untouched.The defect
The
publication-layoutinput's own description says: "DO NOT FLIP A PREFIX UNTIL EVERY PRODUCER OF IT CAN WRITE GENERATIONS … the new one moves the pointer, the old one replaces the flat copy in place and never touches it, so a pointer-following reader keeps serving its generation and never sees the old writer's NEWER publication — a stale serve with nothing red anywhere.plugins… flips in ONE change set.""One change set" is not available. Those two producers live in two repositories, which cannot merge atomically, and one publishes on every core build. The flip must pass through the forbidden state; forbidding it leaves it unhandled.
The fix
A live
_currentmeans the prefix is on the generation layout, so a run that resolves one publishes a GENERATION whatever its caller asked for — announced as a::warning::naming the caller to flip. One assignment inpublish_to_target; no delete, no conditional delete, no ETag, no new ordering.pluginsmay be flipped one producer at a time, in whichever order the two repositories merge;_currentis moved before the flat compatibility copy is refreshed, so a run finding an older flat copy could otherwise skip, displace a newer generation and expose older bytes. A flat caller whose content is already live skips and changes nothing — its own case.What changed after review
Copilot raised three findings on the first mechanism (retire the pointer). All three were correct:
_currentin between, losing its newer publication;LIVEswitched toDEST, the skip decisions stopped proving anything about the generation — so a stale flat copy could take the already-published (or not-sealing-backwards) skip and displace a newer generation. Content going backwards, a worse defect than the one being fixed;publication-layoutdescription still told operators the prefix must flip in one change set.The mechanism above replaces it and is strictly smaller: (1) and (2) disappear because nothing is deleted and
LIVEis untouched; (3) is fixed in the input description, which now states that the layout belongs to the prefix and that the two producers may flip in either order — and its stale "retention is not implemented" precondition is corrected (#3959).The negative control — watched it fail on
mainThe same two cases' other assertions — "the earlier generation is untouched" and the whole never-backwards case — pass on main too, which is what makes them regression controls rather than new behaviour, and shows the cases reached the path.
Verification (each step able to fail)
test-publish-bake-overlap.py(this branch)origin/main's scripttest-publication-pointer-readers.pytest-sealed-module-compose.pytest-bundle-registry.pybash -n .github/scripts/publish-bake-bundles.shcheck-workflow-shell.pycheck-workflow-yaml-keys.py/check-workflow-timeouts.pydotnet build test/MeshWeaver.Documentation.Test -c Release -warnaserrordotnet test … --filter DocumentationLinkIntegrity --no-buildDocs:
Doc/Architecture/SealedPublicationGenerations→ phase 4 gains "The layout belongs to the PREFIX, not to the caller", the "Where this stands" bullet is corrected, and Verification records the new count and this negative control.Blast radius today: none. No caller passes
publication-layout, so no_currentexists on any share; the new branch cannot be entered until a producer flips. No What's New entry for the same reason. No public type or member leavessrc/; no interface gains a member; no i18n catalog key changes.🤖 Generated with Claude Code