🤖 fix: migrate off shared adoption copies by installing first, drop take-over renames and marker-key reliance - #4254
Closed
ThomasK33 wants to merge 2 commits into
Closed
🤖 fix: migrate off shared adoption copies by installing first, drop take-over renames and marker-key reliance#4254ThomasK33 wants to merge 2 commits into
ThomasK33 wants to merge 2 commits into
Conversation
… plain receipt, drop take-over renames and marker-key reliance Review of the previous layer found the two mechanisms it added for shared-receipt migration to be the problem: taking the creator's copy over by rename left every other descendant's receipt pointing at nothing (their fast path read that as settled), left the owner metadata keyed by the old path, and — after a crash between the pending manifest write and the rename in a full store — a permanent capacity skip that also blocked forced removal; and retaining the reliance receipt under the superseded-marker control key was tombstoned by the previous build's unlisted-key reconciliation on downgrade, after which the creator could delete the only shared copy. Both are removed. A migration now installs the descendant's own copy FIRST while the manifest still holds the plain created: false receipt naming the shared copy — the form every build reads, and the sibling's reason to keep that copy (siblingReliesOn) — and only then flips the record to created: true for the new target, in the pass's single manifest write. No pending record precedes the install (it would replace the receipt), so a retry recognizes a copy installed before the flip by its identical bytes at this descendant's own import slot and claims it instead of writing a second one; a crash before the install leaves nothing to recover. A downgrade in the window sees the receipt exactly as before plus, at worst, one redundant file this build claims on return. The install is exempt from the capacity check — at most one extra file per outstanding shared receipt, bounded by prior state — and the creator's file is never renamed, so its normal deletion (removeKeys included) returns the slot once nobody relies on it. The sibling scan reads the config strictly: an unreadable or malformed config.json no longer answers "no other descendants" (which let the deletion and superseded-copy paths remove a generation a sibling still names); callers treat the throw as transient. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$153.06`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=153.06 -->
…any identical occupied candidate A migration retry that found an identical file at the descendant's own import slot marked it adoption-created. No receipt was recorded before that install, so the file was indistinguishable from one the owner wrote there — the slot is inside the writable notebook — and after the legacy source was deleted, reconciliation trusted that provenance and removed the owner's file. The claim is dropped. The retry now follows the same rule as first-time placement for a byte-identical occupied candidate: reused non-destructively with a plain created: false receipt naming it, never claimed. Destructive provenance is only ever taken from a receipt recorded before an install (targetStamp), never from a byte match. The scenario still converges in one pass with no second copy; the bounded outcome of a crash between install and flip is one redundant copy that adoption never deletes. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$171.20`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=171.20 -->
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
added this pull request to stack #4255
September 14, 2026 12:17
Member
Author
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64bb582c9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
removed this pull request from stack #4255
September 14, 2026 13:49
Member
Author
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Sep 14, 2026
…rden guarded reads, removal pinning and legacy adoption (coder#4250) ## Summary Final layer of the split of coder#4139. coder#4210–coder#4245 landed on `main` through the merge queue; this PR carries the two remaining layers as one diff (the queue's head-branch auto-delete closed coder#4248 and dropped this PR mid-stack, so they were rebased onto `main` and collapsed here): the install-before-flip migration that replaced take-over-by-rename (former coder#4248 / coder#4254), the post-rebase hardening of guarded reads, removal pinning and legacy adoption, and the recovery round from this PR's own review against `main`. ## Implementation **Migration off a shared adoption copy** (former coder#4248/coder#4254) 1. *Install before flip.* A note migrating off a sibling-owned copy stages and installs its own copy at `imported/<child>/<rel>` while the manifest still holds the plain `created: false` receipt naming the shared copy — the shape every build reads and what `siblingReliesOn` checks. Only after the install does the record flip, in the pass's single manifest write. No control key is ever written for a migration, so a downgrade in the window sees exactly the receipt it already understands. 2. *Retry.* Crash before install: nothing changed. Crash after install, before flip: an identical file at the child's own import slot is reused non-destructively (plain `created: false` receipt) — never claimed as adoption-created, because without a receipt recorded before the install it cannot be told from an owner-authored note. If a downgraded build then edits the legacy note in that window, the pass fails closed (documented limitation at `legacyImportTarget`): the edit stays in the legacy session store, the receipt keeps protecting the shared copy, and the non-forced handover rejects until the redundant slot is cleared by hand. 3. *Capacity.* A migration at the per-scope cap is a transient, unmemoized skip: the note keeps standing on the shared copy, the creator's deletion waits, and the migration completes once a slot frees. The store never exceeds the cap; take-over by rename and the earlier cap exemption are both gone, so no receipt is ever left naming an absent target and no owner metadata is stranded. 4. *Strict sibling scan.* `descendantAdoptionRecords` reads the config with `throwOnError`; an unreadable or malformed `config.json` is a transient skip, never "no siblings". **Hardening** (former layer 7 + this PR's review rounds) 5. Guarded reads: the acting/`guardedWorkspaceId` tombstone check runs before and after reads in every scope, with a final all-scope gate over the assembled hot set, index and root listing; a late tombstone first observed in the second gate also clears global/project items. 6. Descendant pinning during removal is a config-only edit that runs before the metadata lookup (phantom-cleanup path included); a non-forced removal aborts when the pin cannot be verified. 7. Legacy adoption: names the parser only admits after normalization are a reported permanent skip; a non-regular entry at the old source counts as deletion; manifest keys and receipt targets are validated for containment before any probe; excluded dot entries skip `DT_UNKNOWN` classification. 8. Sibling checks use file identity (`ino:size:mtimeNs`) rather than spelling, with the receipt identities probed once per pass (cache evicted for paths the pass mutates). A same-inode match counts as relying when it is a single directory entry (`nlink 1`) or the spellings fold to the same name; an independent hard link does not block the creator's deletion. 9. Read-side re-ranking emits the memory change event with `reason: "access"` (optional field on the in-memory/IPC event) so `MemoryBrowser` no longer labels unchanged notes as agent-edited. No new persisted fields, files or modules. ## Validation Every fix has a test that was red on the code it replaced, including: three descendants at cap migrate one by one as slots free and the store never exceeds the cap; a crash between stage and install leaves the manifest byte-identical; a copy installed before the flip is reused without being claimed and an owner-authored identical file survives the later source deletion; an edit made after a crash between install and flip is left unfolded with nothing overwritten; hot set, index and root listing all withhold already-accumulated global items on a late tombstone; receipt-identity probes are bounded by the receipt count; hard link vs case alias. ## Known limitations (later work) - Crash between a migration's install and its receipt flip followed by a downgraded edit of the same note: fail-closed, manual slot clean-up (see `legacyImportTarget`). - Cross-process ownership revalidation of an `AgentSession`'s cached memory context after another backend removes the owner (multi-backend layer). - Rollback owner-root resolution for adopted rows; throttled full-store fingerprint; sidecar locking; concurrent removal across backends (as replied on the earlier layers' threads). ## Risks Low to medium, confined to the adoption pass, manifest, and read gating (fail-closed in both directions). No destructive decision is ever taken from byte identity alone; every destructive step requires a receipt recorded before the install. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$1260.46`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=high costs=1260.46 -->
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
1 participant
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.
Summary
Replaces the two mechanisms that #4245's second review round introduced for migrating a descendant off a shared adoption copy — take-over-by-rename at the file cap, and a reliance record under the superseded-marker control key — with a simpler install-before-flip migration. The five findings from #4245's third review round (multi-sibling take-over, crash before the take-over rename, marker tombstoned on downgrade, tolerant config read in a destructive check, stale owner metadata after take-over) disappear by construction;
memoryService.tsshrinks by 51 lines.Stack: layer 6 of the split of #4139. Re-opened copy of #4248 (identical diff, rebased onto
mainafter the merge queue landed #4210–#4245 and GitHub auto-closed #4248 when its base branch was deleted). Closes the review threads that were open on #4245; the remaining bundle isthis PR + #4250.Implementation
imported/<child>/<rel>while the manifest still holds the plaincreated: falsereceipt naming the shared copy — the shape every build reads and exactly whatsiblingReliesOnchecks. Only after the install does the record flip tocreated: truefor the new target, in the pass's single manifest write. No pending record precedes the install, so no control key is ever written for a migration.created: falsereceipt naming it) — it is never claimed as adoption-created, because without a receipt recorded before the install it cannot be told from an owner-authored note. Destructive provenance comes only from atargetStamprecorded before an install.imported/<child>/that this build reuses (never deletes) on return; older builds do not reconcile unlisted files underimported/.remainingCapacityis still decremented). The creator's file is never renamed, so its normal deletion loop (removeKeys+remove) returns the slot once no receipt relies on it. Migration skips are transient only.descendantAdoptionRecordsreads the config withthrowOnError, so an unreadable or malformedconfig.jsonthrows (callers already treat that as a transient skip) instead of yielding an empty project map that reports "no siblings" to the deletion paths.Removed:
siblingOwner/siblingSourceGone/migratingFrom/takeOver, the take-over staging/installable/restore branches, theDescendantAdoptionRecordscan shape, and the reliance-marker write. Kept from #4245: the deletion-loop reliance guard and superseded-copy cleanup, marker preserved when its generation is unreadable, boundedDT_UNKNOWNclassification, malformed sibling manifest → quarantine, transient null stamps.Validation
Four new tests, each red on
memory-sharing-l3a3: three descendants at cap all migrate off one shared copy and the creator's deletion then clears the copy and its pin; a crash between stage and install leaves the manifest byte-identical (no pending record, no control keys) and the creator's deletion keeps the copy until the retry finishes; a copy installed before the flip is reused without being claimed, and an owner-authored identical file at the import slot survives the migration and the later source deletion; an unreadable config leaves everything in place and the pass unmemoized. The two #4245 tests that encoded take-over and marker reliance are replaced by these scenarios.Risks
Low to medium, confined to the adoption pass and manifest. The one residual outcome is a single redundant identical file under a descendant's own import slot after a crash between install and flip; adoption reuses it as a non-owned receipt and never deletes it.
Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:high• Cost:$1197.33