fix(gc): fence the blob mark/sweep against transcripts changing under the mark - #4161
Conversation
e02a7d8 to
815bef7
Compare
![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: e02a7d8066
ℹ️ 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".
| // Fence again immediately before acting: the sweep itself is a | ||
| // window, and a transcript that moved since the last check | ||
| // invalidates the whole mark, not just this blob. | ||
| if (!(await verifyGcDiskMarkFence({ sessionsRoot, accounted, marked, evidence, errors }))) { |
There was a problem hiding this comment.
When a live session appends a reference to an eligible existing blob after this awaited transcript scan returns—such as while removeCanonicalBlob() performs its own lstat—the append is never checked and the subsequent unlink deletes the newly referenced blob. The 50 ms quiet period cannot close this check-to-unlink window, so the fence must coordinate with transcript writers or otherwise make the final transcript validation and deletion atomic.
Useful? React with 👍 / 👎.
| // Fence again immediately before acting: the sweep itself is a | ||
| // window, and a transcript that moved since the last check | ||
| // invalidates the whole mark, not just this blob. | ||
| if (!(await verifyGcDiskMarkFence({ sessionsRoot, accounted, marked, evidence, errors }))) { |
There was a problem hiding this comment.
On a store with B eligible unreferenced blobs and T transcripts, this branch invokes a complete transcript-tree walk for every blob, making the sweep Θ(B×T) in filesystem operations. It also runs for dry runs because the prune check occurs afterward, so a reporting-only gjc gc --disk can become prohibitively slow on large stores; perform only one final scan for dry runs and use a sweep-wide synchronization mechanism instead of per-blob full walks when pruning.
Useful? React with 👍 / 👎.
815bef7 to
30213f3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30213f3a33
ℹ️ 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".
| evidence: input.evidence, | ||
| errors: input.errors, | ||
| }); | ||
| if (drifted.length === 0) return true; |
There was a problem hiding this comment.
When this re-walk encounters an unreadable sessions root/project directory or an unstatable transcript, findGcDiskMarkDrift() marks evidence.complete false but can still return an empty drifted array because the unreadable portion contributes no path. This line consequently reports a successful fence; during the per-removal call at line 1429, the caller then sets would_reclaim and invokes removeCanonicalBlob() despite incomplete evidence, risking deletion of a blob referenced only in the unreadable portion. Return false whenever the scan made evidence incomplete, even if no explicit drift path was collected.
Useful? React with 👍 / 👎.
30213f3 to
09c5d03
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09c5d03b8b
ℹ️ 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".
| errors.push({ surface: "blobs", scope: transcriptPath, message: gcDiskErrorText(error) }); | ||
| return; | ||
| } | ||
| if (before.size !== after.size || before.mtimeMs !== after.mtimeMs) { |
There was a problem hiding this comment.
When a transcript is atomically replaced during the streamed read, these checks can accept different files as the same snapshot whenever the replacement has the same length and mtimeMs—a realistic case on coarse-timestamp filesystems. The session manager performs such replacements during full rewrites via #replaceSessionFileSync; the stream can therefore read the old inode while the replacement contains a new blob reference, after which every fence compares only size and timestamp and may delete that blob. Capture and compare stable file identity such as dev/ino in both the pre/post-read check and subsequent drift checks.
Useful? React with 👍 / 👎.
The dev-shard regression surfaced by #4149 workflow-dispatch run 31360117739 shard-1 shows gjc gc --disk --prune reclaiming a blob as unreferenced_by_any_surviving_session while a session transcript keeps changing under the mark; the sweep must withhold with keep:withheld_evidence_incomplete: sessions_changed_during_mark instead. Reproduced on exact dev d73bc90 (2/25), #4149 base e5ff1c1 (1/25), and dev head 06bf6d2 (4/40 single-test, 2/15 full-file). gc-runtime.ts and the retention test are byte-identical between e5ff1c1 and dev HEAD, and the #4149 branch diff touches zero GC files: the race is pre-existing dev code introduced by #4037, not by #4149. The bundle is GPG-signed (key 6A8D48D0B4C7ACA36463DF217CCCF17C606579E8); verify with gpg --verify SHA256SUMS.asc SHA256SUMS && sha256sum -c SHA256SUMS. Lore-id: gc-mark-sweep-changing-transcript Confidence: high Scope-risk: narrow Reversibility: easy Tested: 4/40 2/25 1/25 2/15 failure-rate reproductions across dev head, exact dev, and #4149 base
… the mark gjc gc --disk --prune could reclaim a blob as unreferenced_by_any_surviving_session while a session transcript kept changing under the mark, because the mark's stat was taken after the read (a change landing mid-read was recorded as the 'stable' snapshot) and the drift rounds plus one final re-walk could each complete inside an inter-append gap of a live session. A reference appended after the last check was invisible to the sweep, so reclamation could destroy evidence a surviving transcript still pointed at. The sweep is now fenced on both ends of the measurement: - markGcDiskTranscript binds its references to a stat taken before AND after the read; a change between the two withholds the sweep with sessions_changed_during_mark instead of silently shrinking the set. - After the mark rounds converge, the sweep requires the store to be observed completely quiet across GC_DISK_MARK_QUIESCENCE_MS (a window far larger than any realistic inter-append gap); a store that still moves under the probe never satisfies it and withholds the sweep. - The fence is re-verified immediately before every removal, so an append landing during the sweep itself withholds the whole sweep. The previously timing-dependent 'withholds the sweep when a transcript keeps changing under the mark' test is now deterministic (0/100 hammer runs vs 4/40 on dev); four new mutation-sensitive tests inject appends at each fence boundary (mid-read, quiescence probe, pre-removal) plus a real-concurrency test proving a blob referenced only by appended content survives. Lore-id: 6a2c4e8f Constraint: partial knowledge is never a licence to delete Rejected: per-blob stat windows | the sweep itself is a window a live session can write into Rejected: cooperative transcript locks | session writers take no lock today, so the fence must be self-contained Confidence: high Scope-risk: narrow Reversibility: easy Tested: 49/49 gc-disk-retention tests including 4 new mutation-sensitive cases Tested: 60/60 full-file hammer runs and 100/100 focused runs of the previously flaky test Tested: gc-e2e 4/4, package typecheck + biome clean Not-tested: a transcript writer that honors an external lock (none exists) Supersedes: the drift-only fence from #4037/#4084/#4110
09c5d03 to
320779e
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
NEEDS-WORK. The fence is real and its coverage is load-bearing — but a probe I wrote still deletes a live blob with this PR applied, so the window is narrowed, not closed.
The fence works, and its tests are not decorative
| run | result |
|---|---|
bun test packages/coding-agent/test/gc-disk-retention.test.ts |
49 pass / 0 fail |
mutation — revert only gc-runtime.ts, keep the tests |
46 pass / 3 fail |
| re-apply | 49 pass / 0 fail |
So keep:withheld_evidence_incomplete: sessions_changed_during_mark genuinely fires for the drift this PR set out to catch.
The surviving hole
I wrote a probe that appends a blob reference to a live transcript on the second lstat of the blob — i.e. after the mark's last drift check, inside the sweep's own read window:
if (path.resolve(String(target)) === blobPath && ++blobStats === 2) {
await fsp.appendFile(state.transcript, JSON.stringify({ type: "message", role: "user", content: }) + "\n");
}Two cases, both with this PR applied:
(fail) keeps a blob referenced after the final transcript fence but before unlink
(fail) keeps an old immutable blob while its reference exists only in live state
0 pass / 2 fail
The transcript verifiably contains blob:sha256:<hash> afterwards, and the blob is gone. That is live data loss — the exact outcome #4158 describes.
Important: this is pre-existing, not a regression you introduced
I ran the same probe on pristine dev and got the identical 0 pass / 2 fail. So this PR does not make anything worse; it closes the drift-during-mark window and leaves the post-fence-to-unlink window open.
That distinction matters for how you treat this review: I am not asking you to revert anything. I am saying the PR says Closes #4158, and on this evidence it does not.
What is needed
The check-then-act gap between "decided it is garbage" and "unlinked it" has to be closed, not just made smaller. Options, in order of preference:
- Re-verify immediately before unlink — cheap, and it collapses the window to the syscall boundary rather than the scan duration.
- Take a fence that covers the sweep too, not only the mark.
Whichever way, given the failure mode is silent unrecoverable deletion, the sweep should withhold on any uncertainty rather than proceed.
Please also land my probe (or an equivalent) as a regression — the existing suite passes because every test appends before the mark completes, which is precisely why this window survived.
Suggested split
Retitle to Refs #4158 and merge this as the drift-during-mark fix, then close the post-fence window in a follow-up. This is real progress and I would rather it land than sit behind a second problem.
Verification note: this review's probe was run by me, and I confirmed causation against pristine dev before attributing anything.
|
Correction to my review above: shell expansion ate the template literal in the probe snippet. The actual injection is: if (path.resolve(String(target)) === blobPath && ++blobStats === 2) {
await fsp.appendFile(
state.transcript,
JSON.stringify({ type: "message", role: "user", content: "blob:sha256:" + hash }) + "\n",
);
}It appends a real blob reference to the live transcript on the second |
Boundary cohort review — signed terminal review (no merge)Frozen change set:
Exact-head CI: Dev CI run 31369771857 green on Deliverables: issue #4158 (bounded, signed evidence) → this PR (one issue → one PR); #4149 branch and notification/discovery work untouched. No merge performed, per the brief. |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
MERGE_READY — owner hold 해제. Exact-head CI가 terminal green이며 현재 확인된 unresolved blocker가 없습니다. Merge/release는 owner-controlled 단계로 남깁니다.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
A transcript could acquire a blob reference while GC revalidated the blob, after the previous transcript fence and before unlink. Bind the fence inside the verified delete path and reject identity replacement before destruction. Lore-id: pr-4161-post-mark-unlink-fence Constraint: withhold blob removal when live transcript evidence changes Confidence: high Scope-risk: narrow Reversibility: straightforward Tested: bun test packages/coding-agent/test/gc-disk-retention.test.ts (51 pass); bun --cwd=packages/coding-agent run check Not-tested: gc-redteam.test.ts has reproduced unrelated failures after dev integration
|
Repaired against exact head
Exact-head Dev CI: https://github.com/Yeachan-Heo/gajae-code/actions/runs/31444925236 (running). — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb695b4e97
ℹ️ 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".
| if (options.beforeUnlink && !(await options.beforeUnlink())) { | ||
| return { removed: false, reason: "blob_reference_evidence_changed" }; |
There was a problem hiding this comment.
When a live session reuses an old unreferenced hash while beforeUnlink is scanning transcripts, BlobStore.put() can rewrite the canonical path after the identity check but before this callback returns. Because no blob stat follows the awaited callback, the subsequent unlink can remove those freshly written bytes, after which the writer may persist a reference to the now-missing blob. Repeat the identity validation after the callback or coordinate the write and deletion before unlinking.
Useful? React with 👍 / 👎.
|
MERGE_READY — exact head Dev CI rerun 31444925236 is terminal Success (21m39s). It includes the affected-path plan, native build, Windows regression, 14 affected shards, evidence producer, aggregate validation, and virtual integration validation. Local verification: — |
2 participants
Closes #4158.
Problem
gjc gc --disk --prune(blob mark and sweep) could reclaim a blob asreclaimed:unreferenced_by_any_surviving_sessionwhile a session transcript keeps changing under the mark. Expectedkeep:withheld_evidence_incomplete: sessions_changed_during_mark. A reference appended to a surviving transcript after the mark's last drift check — or even during the mark's own read — was invisible to the sweep, so reclamation could destroy evidence a live transcript still points at.Surfaced by workflow-dispatch run 31360117739 shard-1 on PR #4149's branch; attributed to pre-existing dev code (the #4149 branch diff touches zero GC files;
gc-runtime.tsand the retention test are byte-identical between the #4149 basee5ff1c1and dev HEAD). Introduced by #4037 and carried through #4084/#4110.Root cause
Two windows let the sweep act on stale evidence:
markGcDiskTranscriptread the transcript first andlstated it after. A change landing between the end of the read and the lstat recorded a stat newer than the bytes read, so the drift comparison saw no change even though the reference set was stale.findGcDiskMarkDriftreturned empty, the sweep ran immediately; the drift rounds and a single final re-walk could each complete inside an inter-append gap of a live session, so a transcript that kept changing could still look stable to every check.Reproduced locally at 4–13% failure rate on dev head, exact dev
d73bc90ae4(2/25), and #4149 basee5ff1c1(1/25) — signed evidence atartifacts/gc-mark-sweep-changing-transcript/(commit1af0f489d7).Fix
Fence the mark/sweep on both ends of the measurement in
runGcDiskBlobs:sessions_changed_during_markinstead of silently shrinking the reference set.GC_DISK_MARK_QUIESCENCE_MS(50 ms, far larger than any realistic inter-append gap). A store that still moves under the probe never satisfies it and withholds the sweep.Tests
withholds the sweep when a transcript keeps changing under the marktest is now deterministic: 0/100 hammer runs vs 4/40 on dev.spyOn(fsp, "lstat")seam:gc-disk-retention.test.ts: 49/49;gc-e2e: 4/4; package typecheck + Biome clean.gc-stores.test.tsfailure is environmental (this machine's real~/.gjcholds >20 000 file-lock entries) and reproduces identically on pristine dev.Scope