Repository navigation
fix(#4588): the boot install holds an UNPROVEN ref where another writer keeps the partition current - #4619
Conversation
…er keeps the partition A partition can have two writers, and #4259 made the boot default install safe by pinning it to the sealed commit — so both land the SAME tree. That pin has a residue: `ProvenRef` can only attribute a seal to a REPOSITORY, so a source with no `RepoPath` (a remote registry) is listed at its configured ref. The control instance's plugin source is exactly that, and its ref is `HEAD`. Measured on memex.systemorph.com 2026-09-17: `Plugins/Hosting` was stamped at 14:34:30Z with `installedFromRef: HEAD` and a 222-file map, having written `main`'s tree into `Hosting` — whose `_GitSync` re-imports the same subdirectory at the commit sealed for the running framework (then `061976bc`, which does not carry `Hosting/Issue/Source/FleetWatchCadence.cs`). Thirteen minutes later the record claimed that file and the node was gone; three Hosting NodeTypes could not compile. At 16:03Z the seal advanced to `d98fc2ac` and its import — a delta from the PREVIOUS seal — wrote `Issue/Source/IssueLayoutAreas.cs` (changed between the two seals) and left `Issue/Test/IssueTests.cs` alone (unchanged between them), where the installer's `main` copy was still sitting: `CS0117 'IssueLayoutAreas' does not contain a definition for 'Facts'`. That last step is why gate 2 alone cannot close this class. Gate 2 makes the INSTALLER's delta full; this mix was produced by the OTHER writer's delta, which has the identical blind spot. The remedy the invariant already names is one writer. `InstanceAutoRegistrationService.Install` now asks `PartitionContentOwnership` for a candidate whose ref this boot could NOT prove, and holds where the installer does not own the content. `Undetermined` holds too: a hold that was wrong is re-derived and lifted at the next boot, an install that was wrong cannot be taken back. A proven ref is not held — the two writers land the same tree, which is #4259's design untouched — and a mesh with no tracking layer is unchanged. The hold fetches nothing and adopts no module, still re-asserts the package's declared access (create-only, writes nothing in the steady state), and is recorded as a SKIP with its reason, once, at Warning. Never a failure: no retry can make a ref provable, and nothing polls. Pinned by `TheBootInstall_HoldsAnUnprovenRef_WhereAnotherWriterKeepsThePartitionCurrent` — a real boot lane, the real GitHub tracking seam over a real `_GitSync` node, and the prune the sealed reconcile performs. Without the gate the test reproduces the loop itself: the pruned node is written back at the branch tip on the very next pass. Its control is the same sync-owned partition once the repository IS sealed, where the install proceeds and lands the sealed commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect ownership provenance, install security handling, reporting, and error propagation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a boot-install ownership gate preventing unproven package references from overwriting synchronized partitions.
Changes:
- Adds provenance-aware install holds and access reassertion.
- Adds tests for ownership and sealed-reference behavior.
- Documents the fix, incident, and user-facing change.
File summaries
| File | Reviewed changes and findings |
|---|---|
test/Memex.Portal.Shared.Test/BootInstallLandsOnTheSealedCommitTest.cs |
Tests unproven holds and sealed-reference controls. Nit (1 vote): replace 50 ms polling with a direct live-provider emission to avoid warning storms and masked failures. |
src/MeshWeaver.PluginCatalog/InstanceAutoRegistrationService.cs |
Implements the ownership gate and hold reporting. Moderate (2 votes): make tracking direction-aware for ExportOnly. Critical (3 votes): verify target sync provenance matches the candidate repository/tree. Moderate (3 votes): distinguish ownership holds from authorization skips. Critical (1 vote): replace the remaining Observable.Using(ImpersonateAsSystem) path with RunAsSystem. Moderate (1 vote): resolve ownership once per target partition. Nit (2 votes): avoid duplicate warnings. Nit (1 vote): assert durable ledger recording in the hold test. Moderate (1 vote): preserve or retry access-establishment failures. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-17-a-boot-install-no-longer-writes-a-space-that-syncs-its-own-content.md |
Adds the user-facing release note. |
src/MeshWeaver.Documentation/Data/Architecture/OnePartitionOneBookkeeping.md |
Documents the ownership invariant, incident, and new gate. |
Review details
Suppressed comments (4)
src/MeshWeaver.PluginCatalog/InstanceAutoRegistrationService.cs:1794
InstallAllconcatenates candidates, so this starts a fresh 30-second ownership budget for every unproven package. If the tracking provider is unavailable, N default packages targeting one partition serialize into N×30s of boot delay while repeatedly asking the same question, rather than producing one bounded decision for the partition. Resolve ownership once per target partition per pass and share that conservative verdict across candidates.
: PartitionContentOwnership.Observe(hub, partition, candidate.Package.Id, logger)
src/MeshWeaver.PluginCatalog/InstanceAutoRegistrationService.cs:1889
- The end-to-end hold test checks
second.Skippedand the package install record, but never readsPlugins/_DefaultInstallLedger. SinceRecordSeededdeliberately catches ledger-write failures and still returns the summary, the test would stay green if this new hold were not durably recorded with its reason as promised. Add an authoritative ledger read/assertion for the held package and reason.
.Select(_ => DefaultInstallSummary.Empty with
{
Skipped = [new DefaultInstallSkip(package.Id, reason)],
});
src/MeshWeaver.PluginCatalog/InstanceAutoRegistrationService.cs:1880
EnsureDeclaredAccessdocuments that access-establishment failures must propagate, but this catch converts them toUnitand then reports an intentional ownership skip. The boot summary stays atFailed == 0and the ledger records only the hold reason even though the partition may remain unreadable. Preserve a distinct access failure or otherwise make the re-assertion failure retryable instead of swallowing it.
.Catch((Exception exception) =>
{
// A hold is not an install, so an access re-assert that could not run must not turn
// it into a FAILED one — it is said, at Warning, and the hold stands.
logger.LogWarning(exception,
test/Memex.Portal.Shared.Test/BootInstallLandsOnTheSealedCommitTest.cs:341
- This 50 ms interval repeatedly re-subscribes to the one-shot ownership decision until its eventually-consistent query happens to include the new config. If the provider is faulting or never emits
SyncSource, the test creates a warning/resubscription storm until its timeout and can hide the provider failure behind polling. Wait on the provider's livetrueemission directly (then letRunDefaultInstallexercisePartitionContentOwnership.Observe) so a broken seam fails once instead of being retried.
var verdict = await Observable.Interval(TestTimeouts.Quick / 20).StartWith(0L)
.SelectMany(_ => PartitionContentOwnership.Observe(Mesh, Package))
.Where(v => v.Owner == PartitionContentOwner.SyncSource)
.FirstAsync()
.Timeout(TestTimeouts.CrossSilo)
.Await(TestContext.Current.CancellationToken);
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // the branch tip, and that is what may not be written into a partition somebody else keeps | ||
| // current. Asking the seam for every candidate would put a bounded read in front of | ||
| // installs that were never in question. | ||
| return candidate.RefIsProven |
There was a problem hiding this comment.
Correct, and it is the one finding I have not closed in code — it is now #4625, with the two ways it could be closed and why neither is a one-liner.
The gap is real: RefIsProven says the SEAL named a commit for the package's source, and nothing compares that repository with the one the partition's own writer syncs. Two pinned writers can still land two trees. What makes it awkward is that the ownership seam is a BIT by design — IPartitionSourceTracking answers IsTracked/ImportsContent, never an identity — so that the compiler layer needs no reference to the sync layer above it. Closing it properly means either extending that seam to name what it tracks (with a default answering "unknown", so no implementer is obliged), or comparing at a level that already knows both. Holding every proven install instead would take #4259's lane — and the fleet's normal shape, where a package and its partition sync the same repository — offline, which is a bigger behaviour change than this PR should make while it is a blocker fix.
On the test: right that the two repositories differ there, and I have said so in the arm rather than leaving it implied. The mismatch is deliberate for a mechanical reason — the fake transport answers only for the package's repository, so a sync of any other one cannot fetch through it and pollute the fetch witness — and the arm is about the REF, not the repo. The comment now says exactly that and points at #4625, so nobody reads the control as a claim that a mismatched repository is safe.
| /// <param name="candidate">The candidate to land.</param> | ||
| /// <param name="partition">Its target partition.</param> | ||
| /// <returns>A cold observable emitting this package's summary exactly once.</returns> | ||
| private IObservable<DefaultInstallSummary> Land(InstallCandidate candidate, string partition) |
There was a problem hiding this comment.
Fixed — Land now runs through accessService.RunAsSystem(...) too, so the boot install's two arms have the same scope shape.
You are right that this site is the worse of the six in the file: its inner sequence is a CROSS-HUB install, so the terminating thread is reliably not the subscribing one. The whole cold pipeline (InstallOrUpdate → EnsureDeclaredAccess → AdoptPrebuilt) is composed inside the factory, so what is issued is still issued impersonated and the emission-time behaviour is unchanged.
The ratchet moves with it, in this diff: test/ImpersonationScopeSites.allow 6 → 5 for this file, TotalBudget 64 → 63, and a dated note on the budget saying which site went and why. Verified by the guard itself (3/3) plus the install-lane suite.
| // installs that were never in question. | ||
| return candidate.RefIsProven | ||
| ? Land(candidate, partition) | ||
| : PartitionContentOwnership.Observe(hub, partition, candidate.Package.Id, logger) |
There was a problem hiding this comment.
Valid, and fixed — but not by narrowing IsTracked, because the two consumers need opposite answers for ExportOnly.
The compile control plane (#3583) asks "does this partition's content track an external source?". For a mesh → repo partition the mesh IS the source of truth, so its live source is current and the type must COMPILE rather than park — narrowing that bit would park types that are fine. The installer asks "does anything else WRITE this partition's content?", and there the answer is no: ExportOnly rejects imports, so it can neither revert nor prune what an install wrote.
So the seam gained a second member, IPartitionSourceTracking.ImportsContent, with a default of IsTracked — the conservative answer, so a provider that cannot tell the directions apart keeps the pre-#4588 behaviour and no implementer is obliged (no Implementers: gate). GitHubPartitionSyncSourceProvider overrides it and excludes ExportOnly alone, reading the same config nodes through one shared helper so the two questions can never disagree about which sources exist. PartitionContentOwnership now asks ImportsContent; NodeTypeCompilationHelpers still asks IsTracked.
Regression added: OnePartitionOneBookkeepingTest.AnExportOnlySourceIsNotASecondWriter takes BOTH readings from the real provider over one real _GitSync node — IsTracked true, InstallerOwnsTheContent true — then flips the direction to Bidirectional and asserts SyncSource, so it fails if the two questions are ever collapsed back into one bit. Negative control run: with the ownership call reverted to IsTracked, that arm fails on THE assertion.
| }) | ||
| .Select(_ => DefaultInstallSummary.Empty with | ||
| { | ||
| Skipped = [new DefaultInstallSkip(package.Id, reason)], |
There was a problem hiding this comment.
Right, and fixed with a distinct outcome rather than by re-wording the skip.
DefaultInstallSummary and DefaultInstallLedger now carry Held (DefaultInstallHold) beside Skipped, and the summary renders it in its own words: HELD (another writer owns the partition; re-derived next boot) against the existing SKIPPED (authorization, not retried). The ledger records holds with the same snapshot semantics as the skips — computed from THIS pass only, never merged with the previous ledger, so a hold that has lifted disappears by itself.
Two smaller consequences came with it: the "a pass that attempted nothing knows nothing" guard now counts a hold-only pass as knowing something (a hold is derived from a listed candidate and a partition the pass actually asked about, and a lifted hold has to be able to leave the ledger), and the change comparison includes the held list so a lift is written.
The distinction is exactly the #2536 one in the other direction: that lane was taught not to say "failed" about a standing decision; this one must not say "authorization, not retried" about a transient one. The boot test asserts both halves — the package appears in Held, and Skipped is empty.
| // installs that were never in question. | ||
| return candidate.RefIsProven | ||
| ? Land(candidate, partition) | ||
| : PartitionContentOwnership.Observe(hub, partition, candidate.Package.Id, logger) |
There was a problem hiding this comment.
Fixed — the gate now calls the non-logging PartitionContentOwnership.Observe(hub, partition), and HoldForThePartitionsOwnWriter says the whole thing once, with the verdict's own reason inside its sentence. A contract that says "said once" has to mean it.
…p — review round Four of the five review findings on #4619, fixed; the fifth is filed as #4625. 1. EXPORT-ONLY IS NOT A SECOND WRITER. `SyncDirection.ExportOnly` is mesh → repo and rejects imports, so nothing it configures can revert or prune what an installer wrote — holding an install there was a false positive, the quiet "my plugin never installs" this gate must not create. Fixed WITHOUT narrowing the shared bit, because the two consumers need opposite answers for it: the compile control plane asks "does this content track a source?" (#3583) and for a mesh-as-truth partition the live source IS current, so narrowing it would PARK types that are fine. `IPartitionSourceTracking` gains `ImportsContent` with a DEFAULT of `IsTracked` — the conservative answer, so no implementer is obliged — the GitHub provider overrides it to exclude `ExportOnly` alone, off one shared reading of the config nodes, and `PartitionContentOwnership` asks the new one. 2. A HOLD IS NOT A SKIP. `DefaultInstallSkip`'s contract is an authorization this lane can never obtain, and the summary renders it "authorization, not retried". A hold is a fact about THIS boot's ref and THIS partition's writer, re-derived from scratch next pass. The summary and the ledger now carry `Held`/`DefaultInstallHold` separately, rendered "HELD (another writer owns the partition; re-derived next boot)", with the skips' snapshot semantics so a hold that has lifted disappears by itself — and the "a pass that attempted nothing knows nothing" guard counts a hold-only pass as knowing something, or a lifted hold could never leave the ledger. 3. THE LANDING PATH TAKES THE SAME SCOPE SHAPE. `Land` ran `Observable.Using(ImpersonateAsSystem)` over a CROSS-HUB install, so the disposing thread was reliably not the subscribing one (#1790). It now runs through `RunAsSystem`, with the whole cold pipeline inside the factory. The ratchet moves in the same change: the allow file 6 → 5 for this file, `TotalBudget` 64 → 63. 4. ONE WARNING PER HOLD. The gate calls the non-logging ownership overload; the hold says the whole thing once, with the verdict's reason inside its sentence. Not fixed, filed as #4625: `RefIsProven` says the seal named a commit for the PACKAGE's source and nothing compares that repository with the one the partition's own writer syncs. Closing it needs a seam that can name what it tracks rather than answer a bit; holding every proven install instead would take #4259's lane — and the fleet's normal shape — offline. The control arm now says in place what it does and does not claim, and the doc names the assumption as a residue. New arm `AnExportOnlySourceIsNotASecondWriter` takes BOTH readings from the real provider over one real `_GitSync` node and then flips the direction as its control, so it fails if the two questions are ever collapsed back into one bit. Negative control run: with the ownership call reverted to `IsTracked`, it fails on THE assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ℹ️ Merge-queue steward: no action — removed from the queue with reason |
|
ℹ️ Merge-queue steward: no action — removed from the queue with reason |
…→ 62) #4618 lowered it 64 → 63 by removing PluginGateRunner's line; this branch lowered it 64 → 63 by taking InstanceAutoRegistrationService from 6 to 5. Both are real, so the merged budget is 62 — exact against the merged allow file (33 entries, sum 62). Taking either side alone would have left the constant one ABOVE the file, which is the free slot the exact seed exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #4588.
The defect, measured end to end (memex.systemorph.com, 2026-09-17)
The install did not fail to write the file. It wrote it, and the partition's OTHER writer removed it.
Plugins/Hosting1.22.1,installedFromRef: HEAD,installedAtUtc 14:34:30Z, 222 files — claimsHosting/Issue/Source/FleetWatchCadence.cs = 7e8e1d69…Hosting/_GitSynclastSyncCommitSha 061976bc(2026-09-15), re-importing every few minutes,lastSyncOutcome: Imported, v548061976bcdoes not carry the file (contents?ref=061976bc→ 404;?ref=main→ 200). The fivefiles
mainhas that the seal did not are exactlyDispatchSeam.md,InMeshWarningRatchets.md,InstanceAction/Test/DispatchSeamTests.cs,Issue/Source/FleetWatchCadence.cs,StatusFreshness.md— and all five appeared in the mesh at 16:03:45–46Z, each at version 1, theminute
Hosting/_GitSyncadvanced tod98fc2ac(v549, 16:04:02Z), which carries them. Apartition-shaped absence that ends the moment the seal moves is a prune, not a lost write; the
issue's open question ("refused, lost, or never attempted") is answered by that.
The shape has changed, and the new shape is the same defect one step later
Hosting/DeploymentStatusandHosting/InstanceActionare green.Hosting/Issueis the only typeleft at Error (1 of 1 over the
hostingpartition,namespace:Hosting scope:descendants nodeType:NodeType content.compilationStatus:Error):Measured against the two trees:
d98fc2acmain061976bc…d98fc2ac?Issue/Source/IssueLayoutAreas.csIssue/Test/IssueTests.csSo the sealed import is a delta from the previous seal: it rewrote the view and left the test
alone, where the installer's
maincopy from 14:34:30Z was still sitting. The partition holds onefile from each tree — the mix
Doc/Architecture/OnePartitionOneBookkeepingdescribes, with the twowriters' roles swapped.
🚨 That is why gate 2 alone cannot close this class. Gate 2 (#4355) makes the INSTALLER's delta
full on a synced partition; this mix came from the other writer's delta, which has the identical
blind spot — it diffs its own baseline, not the mesh. Whichever writer diffs second lands a mix.
Why the install was at
HEADat all#4259 pins the boot install to the sealed commit, and its test asserts the point exactly: "the two
unattended writers of this partition now agree on its tree".
ProvenRefcan only attribute a sealto a repository, so a source with no
RepoPath(a remote registry, a registeredIPackageSource) answers "no repository to attribute a seal to" and is listed at its configuredref —
HEADby default. That residue is the hole this falls through.Not the #3659/#4525 class: the file is a clean node file (16,710 bytes, valid
// <meshweaver>header, no NUL, no BOM), and the running image (
afde4eab) predates #4525 in any case.The fix — gate 1c
InstanceAutoRegistrationService.InstallasksPartitionContentOwnershiponly for a candidatewhose ref this boot could not prove, and holds where the installer does not own the content.
8 of 19 Hosting NodeTypes are at compilationStatus:Error on memex-cloud — three declared source nodes never landed and the install logged nothing #4259's design is untouched, and the gate keys on provenance rather than on "this partition has a
second writer", so it cannot become "the boot install stopped installing".
Undeterminedholds, asymmetrically and on purpose: a hold that was wrong is re-derived andlifted at the next boot; an install that was wrong has already put a tree into a partition it does
not own.
providerCount == 0— local meshes, CI meshes,the bake host).
DefaultInstallSkipwith its reason on the seed ledger and said once at Warning — never afailure (no retry can make a ref provable) and never a poll.
the hold cannot trade a content defect for an access one. Through
AccessService.RunAsSystem, notObservable.Using(ImpersonateAsSystem)— ActivityLogLogger.SealOverflowLocked latches system-security on the script thread (same shape as #1784's review finding) #1790's ratchet is why.Named residue: a local checkout source keeps today's behaviour (#3359 — no commit to pin,
the operator is the authority), so an operator mirroring a working tree into a partition they also
connected to git still has two writers. That is a configuration a person chose twice, like a human's
Update click; it is written down in the doc rather than left implicit.
Verification
dotnet build src/MeshWeaver.PluginCatalog -c Release -warnaserror0 Warning(s), 0 Error(s)Memex.Portal.Shared.Test—BootInstall*,OnePartitionOneBookkeeping*,PartitionContentOwnershipSeam*AnInstallHoldsItsRoot,InstallCompleteness, …)MeshWeaver.Documentation.Test(link integrity, topic map, the #1790 ratchet)Observable.Usingsite and it was convertedTheBootInstall_HoldsAnUnprovenRef_…fails reproducing the defect: the node the partition's own writer does not carry is written back at the branch tip on the very next pass (# Quiz, only on main, created during the pass)The live arm drives the real boot lane, the real GitHub tracking seam over a real
{partition}/_GitSyncnode, and the prune a sealed reconcile performs; its control is the same sync-owned partition once
the repository IS sealed, where the install proceeds and lands the sealed commit.
Work conserved in
Doc/Architecture/OnePartitionOneBookkeeping(gate 1c + the 2026-09-17 Hostingcase + why gate 2 alone is not enough), plus a What's New entry.
🤖 Generated with Claude Code
Follow-up filed: #4620 — the mirror image of gate 2 on the SYNC side (its import diffs two commits, so a partition another writer touched is left a mix). Not fixed here: with the unattended second writer gone, the paths that can still reach it are human-authored (an Update click, a local-checkout mirror), and the choice between "detect", "full re-import" and "rely on one writer" is a maintainer call.
Review round (all five findings valid; four fixed in code, one filed)
Observable.Using(ImpersonateAsSystem)Landruns throughRunAsSystemtoo, and the #1790 ratchet moves with it:ImpersonationScopeSites.allow6 → 5 for this file,TotalBudget64 → 63, dated note on the budget.truewas read as a competing WRITER, butExportOnly(mesh → repo) rejects importsIPartitionSourceTrackinggainedImportsContentwith a default ofIsTracked(no implementer obliged, so noImplementers:gate); the GitHub provider excludesExportOnlyalone;PartitionContentOwnershipasks the new one and the compile control plane keeps askingIsTracked— narrowing that would PARK types whose sources are current. New armAnExportOnlySourceIsNotASecondWritertakes both readings from the real provider over one real_GitSync, then flips the direction as its control.Skipped, whose contract is "authorization, not retried"Held/DefaultInstallHoldon the summary AND the ledger, renderedHELD (another writer owns the partition; re-derived next boot). Ledger snapshot semantics, so a lifted hold disappears by itself; the "a pass that attempted nothing knows nothing" guard now counts a hold-only pass as knowing something.Observe(hub, partition)overload; the hold says it once, with the verdict's reason inside.RefIsProvendoes not prove the partition syncs the SAME repositoryRe-verified after the round: gate + ownership + seam + install-root suites 14/14, the #1790 ratchet 3/3,
MeshWeaver.Documentation.Test562/562, and the export-only arm's own negative control (revert the ownership call toIsTracked→ it fails on THE assertion).