Skip to content

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 into
mainfrom
fix/3461-flat-publisher-retires-pointer
Sep 13, 2026
Merged

meshweaver-cloud[bot] merged 2 commits into
mainfrom
fix/3461-flat-publisher-retires-pointer

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

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.

🚨 The branch name is stale. The first mechanism here retired the pointer; Copilot's review showed that can move consumers backwards, and it was replaced (see "What changed after review"). The branch is fix/3461-flat-publisher-retires-pointer; the change does the opposite of retiring.

What was established first

  • The readers are ready. Every reader in src/ composes under ShippedPrebuiltBundles.PublicationDirectoryOf and 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.
  • fix(#3732): a downstream publication seals only its own modules — upstream copies are composed, never re-sealed #4172 did not shrink it. Both producers of plugins publish only their own modules already (core CD passes upstream-sources: '', the satellite is a root), so the 4-vs-5 module-set difference is untouched.
  • Phase 4 alone still does not stop the reds. The flat compatibility copy is replaced in place under the generation layout too, and the postcondition covers both copies identically. Only phase 5 removes that window.

The defect

The publication-layout input'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 _current means 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 in publish_to_target; no delete, no conditional delete, no ETag, no new ordering.

  • the half-migration is unreachable rather than forbidden, so plugins may be flipped one producer at a time, in whichever order the two repositories merge;
  • every decision the run makes — already published? which architecture? which source commit? does it carry a module set? — still reads the generation the pointer names, i.e. the publication consumers are actually served. Unchanged;
  • 🚨 never backwards: a flat caller does not take the prefix back. _current is 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:

  1. the delete raced a generation publisher moving _current in between, losing its newer publication;
  2. with LIVE switched to DEST, 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;
  3. the lane's publication-layout description 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 LIVE is 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 main

$ python3 .github/scripts/test-publish-bake-overlap.py --script <origin/main's copy>
  FAIL the run publishes a GENERATION although its caller asked for flat
       — rc=0, generations=['Systemorph-MeshWeaver-3501-1']
  FAIL …and the pointer names THIS run's publication, so consumers resolve what it sealed
       — _current='Systemorph-MeshWeaver-3501-1': 0/10 file(s) present, sealed=False
  FAIL …and it says so out loud, naming the caller that should be flipped
87 passed, 3 failed, 90 assertion(s) total.

$ python3 .github/scripts/test-publish-bake-overlap.py            # this branch
90 passed, 0 failed, 90 assertion(s) total.

The 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)

step result
test-publish-bake-overlap.py (this branch) 90 passed, 0 failed
…against origin/main's script 87 passed, 3 failed — the three new ones
test-publication-pointer-readers.py 14 cases over 2 readers — all green
test-sealed-module-compose.py 6/6 cases
test-bundle-registry.py 92 assertions, 0 failures
bash -n .github/scripts/publish-bake-bundles.sh OK
check-workflow-shell.py 1 finding, 0 live, 0 stale — identical to main
check-workflow-yaml-keys.py / check-workflow-timeouts.py 36 workflows, 0 violations / 87 jobs, 0 violations
dotnet build test/MeshWeaver.Documentation.Test -c Release -warnaserror 0 Warning(s), 0 Error(s), 22.4 s
dotnet test … --filter DocumentationLinkIntegrity --no-build Passed! Failed: 0, Passed: 1

Docs: 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 _current exists 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 leaves src/; no interface gains a member; no i18n catalog key changes.

🤖 Generated with Claude Code

… 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>
Copilot AI lite review requested due to automatic review settings September 13, 2026 22:08
@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

262 tests  ±0   262 ✅ ±0   2m 20s ⏱️ +24s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

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

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_pointer after every successful inner path, including already-published/converged skips that return without calling publish_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 _current after the flat run's initial resolution but before retire_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 67 claim 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 thread .github/scripts/publish-bake-bundles.sh Outdated
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 thread .github/scripts/publish-bake-bundles.sh Outdated
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.
@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 446 tests  ±0   1 446 ✅ ±0   2m 13s ⏱️ -2s
    2 suites ±0       0 💤 ±0 
    2 files   ±0       0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

898 tests  ±0   898 ✅ ±0   3m 22s ⏱️ -3s
  2 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 844 tests  ±0   1 652 ✅ ±0   4m 46s ⏱️ +7s
    4 suites ±0     192 💤 ±0 
    4 files   ±0       0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    6 files  ±0      6 suites  ±0   4m 31s ⏱️ -8s
2 100 tests ±0  2 100 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 104 runs  ±0  2 104 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

1 055 tests  ±0   1 053 ✅ ±0   6m 10s ⏱️ -7s
    3 suites ±0       2 💤 ±0 
    3 files   ±0       0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   18 files  ±0     18 suites  ±0   23m 25s ⏱️ +11s
7 605 tests ±0  7 411 ✅ ±0  194 💤 ±0  0 ❌ ±0 
7 609 runs  ±0  7 415 ✅ ±0  194 💤 ±0  0 ❌ ±0 

Results for commit 67e5293. ± Comparison against base commit 69244c6.

♻️ This comment has been updated with latest results.

@meshweaver-cloud
meshweaver-cloud Bot added this pull request to the merge queue Sep 13, 2026
…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
rbuergi removed this pull request from the merge queue due to a manual request Sep 13, 2026
@meshweaver-cloud

Copy link
Copy Markdown
Contributor

ℹ️ Merge-queue steward: no action — removed from the queue with reason MANUAL — the steward takes no action for this reason.

@rbuergi rbuergi changed the title fix(#3461): a flat publisher owns the prefix it writes — phase 4 no longer has to be atomic across two repos fix(#3461): the publication layout belongs to the PREFIX — phase 4 no longer has to be atomic across two repos Sep 13, 2026
@meshweaver-cloud
meshweaver-cloud Bot added this pull request to the merge queue Sep 13, 2026
Merged via the queue into main with commit 77451a1 Sep 13, 2026
36 checks passed
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>
@rbuergi
rbuergi deleted the fix/3461-flat-publisher-retires-pointer branch October 10, 2026 13:10
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