Repository navigation
fix(storage): stop writeManifest from recreating a removed session folder - #200
Conversation
These tests fail on main. writeManifest into a missing session folder creates the folder. A reconcile whose write follows gc's rm brings the folder back, and so does destroy --force, which then reports success. Refs #196 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
…lder writeManifest went through writeTextFileAtomic, which runs mkdir before every write. So a manifest write that landed after gc removed a session folder brought the folder back, holding only session.json, and list --all showed that stub until the next gc. Now writeManifest writes the temp file and renames it with no mkdir. Only allocateSession creates a session folder. In a removed folder the write fails with SESSION_NOT_FOUND. reconcileSession then returns, because the session has nothing left to reconcile. destroySession reports SESSION_NOT_FOUND, as it does when its reads find the manifest gone. A host write fails as before, and the host ends. writeTextFileAtomic keeps its mkdir for the Home Registry, exports, snapshots and bundle manifests. Tests that seed a session now create its folder first, as allocateSession does. Refs #196 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
The writeManifest test pins that a write into a missing folder fails, and the lifecycle model's ManifestWriteNeedsFolder fact reads it from the code. The test helper's name says what it does. Refs #196 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
The session lifecycle model let every manifest write bring back a removed session folder, and its Scope left that rule out (#196). Now: - R10 RemovedFolderStaysRemoved: once gc removed the folder, no write brings it back. - ManifestWriteNeedsFolder, the 13th guard: facts.mjs reads it as true when writeManifest reaches a rename and no mkdir. - A write into a removed folder fails. reconcileSession returns, and destroy ends with an error. A host write there counts as a crash, which only a state that already breaks R1 reaches. - mutant-ManifestWriteNeedsFolder breaks R10: destroy of an exited session writes destroyed after gc's rm. check.sh: 25 runs, all as expected, 438,636 distinct states. With mkdir put back into writeManifest in a scratch edit, the fact reads FALSE and the current run breaks R10 (UNEXPECTED). Artifact commands still bring back a removed folder through ensureArtifactsDir; Scope now names #198. Refs #196 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
…lder The storage README said every manifest write goes through writeTextFileAtomic, which creates the parent folder, and that write failures throw STORAGE_WRITE_ERROR. The host README did not say what reconcileSession and destroySession do when gc removed the folder before their write. Refs #196 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
|
@codex review |
|
@codex security review |
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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity 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. |
A failed resize write fails only its RPC, and the host keeps running. Only a failed start-up or exit write ends the host. The Scope note about gc's rm said the order of deleted files does not matter. That holds for host writes. A reconcile or destroy write that lands during the rm makes it fail with ENOTEMPTY and leaves a session.json stub until the next gc. #202 tracks that race. Refs #196 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity 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. |
Brings in #200 (writeManifest no longer recreates a removed session folder) and its rule R10 RemovedFolderStaysRemoved with guard ManifestWriteNeedsFolder. This branch's rule becomes R11 DestroyedProgramStopped. The merged model keeps both: WriteLands gates every manifest write, and Write records the program's pid. In reconcileSession the group kill stays right after the re-read, before the write that can now return on SESSION_NOT_FOUND. check.sh: 27 runs as expected, 910,528 distinct states for the current model. coverage.json is regenerated after that check. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Why
This PR lands the #196 follow-up on main. A session manifest write that landed after
gcremoved the session folder brought the folder back with onlysession.json, andlist --allshowed that stub. NowwriteManifestruns nomkdir, so that write fails and each caller treats the session as removed.What changed
writeManifestwrites the temp file and renames it with nomkdir. In a removed folder it throws the existingSESSION_NOT_FOUNDcode.writeTextFileAtomickeeps itsmkdirfor its other callers.reconcileSessionreturns on that error.destroyreports it, as its reads already do. A host write fails as before.verify-session-lifecyclemodel gets rule R10 (a folder gc removed stays removed), theManifestWriteNeedsFolderguard and a mutant run for it.Scope
session.jsonwriter:create, the host, reconcile anddestroy.ensureArtifactsDirstill brings back a removed folder, tracked in artifact commands recreate a session folder that gc removed #198.rmmakes thermfail and leaves a stub until the next gc, tracked in gc leaves a session.json stub when a manifest write lands during its rm #202.destroycan now exit 3 withSESSION_NOT_FOUNDwhen gc removes the session during the destroy. Before, it reported success and left the stub.Tradeoffs
writeTextFileAtomickeeps itsmkdir. The Home Registry needs it. The artifact writes run afterensureArtifactsDir, so removing it there would leave the artifact commands recreate a session folder that gc removed #198 bug as it is.destroyfails instead of reporting success when gc removed the session first. Success would break the model's R9 (destroy succeeds only after it wrote or read destroyed), and destroy's reads already fail this way.Blast Radius
This touches every session manifest write. A write into an existing folder behaves as before. Only a write into a missing folder changes, from creating the folder to failing.
Verification
destroy --forcewith its second read held 3 s,gcduring the hold): main exits 0 andlist --allshows the stub. This branch exits 3 withSESSION_NOT_FOUND, and the folder stays removed. The same replay withlistheld at reconcile's re-read: main recreates afailedstub, this branch does not.npm run verifypasses.verify-session-lifecycle/scripts/check.shexits 0: 25 runs as expected, 438,636 distinct states. Withmkdirput back intowriteManifest, the current run breaks R10.Fixes #196
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh