fix: report session-workspace failures faithfully and prime inline files atomically - #44
Open
danny-avila wants to merge 2 commits into
Open
fix: report session-workspace failures faithfully and prime inline files atomically#44danny-avila wants to merge 2 commits into
danny-avila wants to merge 2 commits into
Conversation
…les atomically Three fixes to how session mode reports and protects workspace state, all found reviewing the subtree import of this code into a downstream monorepo. Inline priming destroyed before it wrote. writeFile unlinked the destination and then wrote the replacement, so a write that failed partway (ENOSPC/EIO) left the previous turn's bytes gone and the workspace dirty — forcing a recycle/restore that loses warm state when no checkpoint exists yet. By-reference priming already avoids exactly this: it keeps a regular file in place and lets a rename replace it atomically. Inline priming now does the same, and still clears a squatting symlink or directory so a prior turn cannot redirect the write. Checkpoint/restore collapsed two different bind failures into one generic 409. A missing header (a caller error) and a REJECTED bind (this runner is pinned to a different session, so it must be recycled) both answered "Missing runtime session header" with no error code, leaving the control plane unable to see the conflict. The conflict case now returns the same session_workspace_dirty signal /execute already uses for this condition, so an older service fronting a newer runner still recycles the VM. A malformed or duplicated header now returns 400 instead of propagating SessionWorkspaceBindingError out of the route. A validation failure after priming reported the workspace as dirty. Any error thrown once priming completed took the dirty branch, so a ValidationError — a deterministic rejection of the request, after which nothing ran — answered session_workspace_dirty instead of 400. That cost a needless restore and hid the reason the caller needed to fix its request. Each fix is covered by a test that fails without it.
Collaborator
Author
|
@codex review |
![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: e130c736e4
ℹ️ 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".
Addresses review feedback on the previous commit. Rejecting a request with no runnable source AFTER priming is too late, and answering it with a clean 400 was worse than the behavior it replaced. `getJob`'s gate accepted any utf8 file, including the `.dirkeep` sentinel, while `Job.execute` required a utf8 file that is NOT `.dirkeep`. A request carrying only `.dirkeep` plus binary inputs therefore passed the gate, reached `prime()` — replacing files in the session workspace and recording priming metadata — and only then failed. Session cleanup deliberately preserves the workspace, so the rejected request's writes stayed visible to the next execution. The previous commit made that case return 400 by moving the ValidationError branch above the dirty branch, which traded a false-dirty for a false-clean: the workspace really had been written to. Both checks now call a single `hasRunnableSource` predicate, and the request gate runs it before any Job is built, so such a request is rejected without touching the workspace. The catch block is restored to its original order: once priming has completed, any later failure — validation included — reports the workspace as dirty, which is the honest answer. Tests: a request with nothing runnable is rejected with prime() never called, and a genuine post-prime failure still reports dirty. Both fail without this change.
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
Three fixes to how session mode reports and protects workspace state. All three surfaced while reviewing the subtree import of this code into a downstream monorepo — the code is unchanged from
mainthere, so these are pre-existing here rather than import artifacts.Inline priming destroyed before it wrote.
writeFileunlinked the destination and then wrote the replacement, so a write that failed partway (ENOSPC/EIO) left the previous turn's bytes gone and the workspace dirty — forcing a recycle/restore that loses warm state when no checkpoint exists yet. By-reference priming already avoids exactly this and says so in a comment: it keeps a regular file in place and letsstreamToDisk's rename replace it atomically. Inline priming now does the same, while still clearing a squatting symlink or directory so a prior turn cannot redirect the write.Checkpoint/restore collapsed two different bind failures into one generic 409. A missing header (a caller error) and a rejected bind (this runner is pinned to a different session, so it must be recycled) both answered
Missing runtime session headerwith no error code, leaving the control plane unable to see the conflict. The conflict case now returns the samesession_workspace_dirtysignal/executealready uses for this condition, so an older service fronting a newer runner still recycles the VM. A malformed or duplicated header now returns 400 instead of propagatingSessionWorkspaceBindingErrorout of the route.A validation failure after priming reported the workspace as dirty. Any error thrown once priming completed took the dirty branch, so a
ValidationError— a deterministic rejection of the request, after which nothing ran — answeredsession_workspace_dirtyinstead of 400. That cost a needless restore and hid the reason the caller needed to fix its request.Change Type
Testing
apisuite: 367 passed, 0 failed (28 files), andtsc --noEmitreports the same 3 pre-existing errors asmain, none in the touched files.Each fix has a test that fails without it — verified by stashing the change and re-running:
src/inline-prime-atomicity.test.ts(new) — replacement without unlinking, symlink/directory squatting still cleared, and the regression itself: an injectedENOSPCat the write leaves the previous turn's bytes intact. Worth noting for future readers: making the workspace directory read-only does not reproduce the bug, because that blocks the unlink too — the write has to fail while the destination is still removable.src/api/v2-checkpoint-binding.test.ts(new) — missing header stays a plain 409 with no recycle signal; a rejected bind returnssession_workspace_dirty; malformed and duplicated headers return 400.src/api/v2-session-binding.test.ts— adds the post-primeValidationErrorcase.Checklist