Repository navigation
fix(host): a create that gives up leaves a create-failed marker, and its host stops on it - #312
Conversation
…its host stops on it
… SIGTERM ends the host
|
@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. What shall we delve into next? 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. |
# Conflicts: # .agents/skills/verify-session-lifecycle/coverage.json
… explore the whole model
The integration case holds the host's event-log open, between its manifest read and its spawn, until a fake-Windows create has given up. Without the check before the spawn it fails: the host spawns the program. replay-hold.mjs learns to hold open for it, because a held readFile does not hold a read that fails before create's write. The unit cases run runHost in the test process. One finds the marker before the spawn, the other gets it during the start-up write.
f03f551 moved the write of the destroy marker into writeEmptyMarker, so ReconcileRepairsDestroyed read FALSE and check.sh failed. The guard now reads that writeDestroyMarker calls the helper, and that the helper renames the empty file into place with no mkdir.
|
@codex review |
|
@codex security review |
🛡️ 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. |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
# Conflicts: # .agents/skills/verify-session-lifecycle/SKILL.md # .agents/skills/verify-session-lifecycle/coverage.json # .agents/skills/verify-session-lifecycle/scripts/check.sh
…ter the merge of main
|
@codex review |
|
@codex security 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: bdeffb2993
ℹ️ 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".
🛡️ 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. |
Why
ThomasK33 asked to work every deferred follow-up of the first autopilot round until none is left. This PR fixes item 1 of #264, the last open item after #294 fixed items 2 to 5. It is option A of the approved proposal: a stop file on every platform.
On Windows,
ChildProcess.kill('SIGTERM')ends the host at once, without its shutdown. So acreatethat gives up there sends SIGTERM only once the manifest names the program, and the next reconcile then kills the program and recordsfailed. Before the host's start-up write,createsent nothing, and a host that started late listened later and served a session whosecreatehad failed. That is the #209 bug in its most likely case.What changed
create(src/cli/commands/create.ts): when the readiness wait fails, the catch first writes an empty marker,<session>/create-failed, with the newwriteCreateFailedMarker(src/storage/manifests.ts, which shares the newwriteEmptyMarkerwithwriteDestroyMarker, path fromcreateFailedMarkerPathinsrc/storage/sessionPaths.ts). Only then does it read the manifest for the platform rule and stop the host as before. A failed marker write is logged at debug level and changes neither the error nor the stop.runHostinsrc/host/hostMain.ts) checks for the marker twice:initiateShutdown, which stops the program and recordsdestroyed.create's marker write comes before its manifest read. So either the host finds the marker, orcreatereads the program's pid and sends the SIGTERM.createwrites the marker on every platform. On Linux and macOS the SIGTERM also stops the host, so there the marker is a second path to the same shutdown..agents/skills/verify-session-lifecycle):HostChecksCreateFailed, whichfacts.mjsreads: the marker write right before the manifest read in the give-up catch, the check right before the SIGTERM skip, and the check right after the start-up write and before the listen.writeCreateFailedMarkeris covered, and the pre-spawn check is arunHostpart.Win32, for the environment: a SIGTERM ends the host at once, andcreatesends it only when the manifest names the program.cmark, the marker write inCreateGivesUp,CreateStopsHostOnWindows(withWin32, the manifest read and the host crash), the check inHostOpensLogAndSpawns, and the new stepHostChecksMarker.current-win32(no token,Win32, R1 to R14 and R17 to R19) passes;mutant-HostChecksCreateFailed-win32breaks R17;mutant-HostChecksCreateFailedpasses (redundant off Windows).src/storage/README.md,src/host/README.md,src/cli/commands/README.md.Behavior changes for CLI and JSON users
createthat fails after the session was allocated leaves an empty internal file,create-failed, in the session folder.gcremoves it with the folder.createnow ends the session itself, and recordsdestroyed, instead of serving it.Not verified
Real Windows behavior is not verified: the marker's atomic rename on Windows, and
TerminateProcessending the host before the marker check. CI has no Windows job. The Windows path runs on Linux with a preload in thecreateprocess only (test/fixtures/fake-win32-create.ts):process.platformreadswin32, andChildProcess.killsends SIGKILL, asTerminateProcessdoes.Validation
Red first. At the test-only commit (
59b6533e, which I ran as9c9ab1efbefore I rebuilt the branch on main), the new integration casea create that gives up on Windows, where SIGTERM ends the host at oncefailed: the host served the session, and its program's group stayed alive past the deadline. The Linux cases with and without a token passed. All three pass on the head.Live, with the CLI (
node --import tsx src/cli/main.ts), isolatedAGENT_TTY_HOMEandXDG_STATE_HOME,createfaking Windows (fake-win32-create.ts), no session token (denied-autogroup.ts), and the host's start-up write held 10 s (REPLAY_HOLD='_host:rename:/session.json:1:10000'):createexits 6 (HOST_UNREACHABLE)374937f9create-failedmarkerrunningdestroyedinspectreaches itcreateunit tests: the marker is written once, with the session's marker path, before the last manifest read, and the host is stopped. A failed marker write keeps theHOST_UNREACHABLEerror withdetails.sessionIdand still stops the host. A create that succeeds writes no marker. 23/23.facts.mjsmutation checks: the marker write moved after the manifest read readsHostChecksCreateFailedFALSE; the check after the start-up write removed reads FALSE; the check before the spawn removed makesfacts.mjsthrow (check.shexit 2), as for any missingrunHostpart.Lifecycle check (
check.sh --update-coverage) on the head: all 74 runs as expected.current27,970,364 distinct states,current-no-token27,080,736,current-win3214,815,680.mutant-HostChecksCreateFailed-win32breaks R17 (FailedCreateStopped) in 6 steps. Coverage synced in880419de.Gates on the head: typecheck, lint and format pass. Unit 1940/1940.
lifecycleandcliintegration 140/140 (vitest run --maxWorkers=1 --retry=2). CI runs the full suite.Receipts:
win32-giveup.sh,fake-win32-create.tsandresults.txtin the trail folder of this autopilot round.Merge of main
4357e94f(fix(host): the marked repair also removes the old socket of an older host #305, test: keep the lifecycle tests stable on a slow runner and cut fixed waits #311 and others; merge66468559, coverage90adb5e9), no Codex round (round 1 on880419dewas clean in both lanes). Onlycoverage.jsonconflicted.check.sh: all 74 runs as expected, same state counts.facts.mjsdrift 0. Unit 1944/1944. Typecheck, lint and format pass.lifecycleandcliintegration: 138/141 under load 68; the 3 that timed out at 5 s (doctor node-runtimeon Node 22.0.0 and 27.0.0,uses --home instead of AGENT_TTY_HOME) pass alone and are outside this change.Round 2, after the verifier's notes on
880419de:writeDestroyMarkerandwriteCreateFailedMarkershare one helper,writeEmptyMarker(f03f5510).test/unit/storage/markers.test.tspins both writers first (4a65660f).ReconcileRepairsDestroyedread FALSE after that change, so the firstcheck.shfailed.facts.mjsnow reads the guard through the helper (75e1e300). Amkdirin the helper, or awriteDestroyMarkerthat does not call it, reads FALSE.a create that gives up on Windows before its host spawns leaves no program, holds the host's event-log open until a fake-Windowscreatehas given up. That is between the host's manifest read and its spawn.replay-hold.mjscan now holdopenfor this, because a heldreadFiledoes not hold a read that fails beforecreate's write. The case checks thatcreateexits 6 withdetails.sessionId, that the host exits, that noprogram.jsonappears and thatinspectreadsfailed. Without the check before the spawn, it fails: the host spawns the program.test/unit/host/hostMarker.test.tsrunsrunHostin the test process. Case 1 puts the marker in place before the spawn: no program, and the manifest is unchanged. Case 2 writes the marker during the start-up write, through awriteProgramRecordhook: the host never listens, and it recordsdestroyedwith the program group gone. Without the check before the spawn, case 1 fails. Without the check after the start-up write, case 2 fails.fe67124f).check.sh --update-coverage: all 74 runs as expected, same state counts (current27,970,364). No guard reads FALSE. Coverage synced ine12cabee.facts.mjsdrift 0.lifecycleandcliintegration 142/142 (vitest run --maxWorkers=1 --retry=2). A trial merge with mainfb100de4merges cleanly, withfacts.mjsdrift 0, unit 1981/1981 and typecheck passing.Merge of main
96e432ea(fix(host): record the host at create, read a token without host.json as unshared, freeze again #303, fix(storage): free a dead breaker's lock at once, keep a stopped waiter alive, skip the sweep on a free lock #299 and others; merge7a017bc2, syncbdeffb29). fix(host): record the host at create, read a token without host.json as unshared, freeze again #303 changed the lifecycle skill too, soSKILL.md,check.shandcoverage.jsonconflicted. The guard list now holds both new guards,HostChecksCreateFailedand fix(host): record the host at create, read a token without host.json as unshared, freeze again #303'sCreateRecordsHost: 33 guards, none reads FALSE.SKILL.mdcounts 75 runs.check.sh --update-coverage: all 75 runs as expected,current27,013,946 distinct states,current-no-token26,822,518,current-win3214,684,648. Ten runs still explore more than 20 million states each.facts.mjsdrift 0. Unit 1994/1994. Typecheck, lint and format pass.lifecycleandcliintegration 143/143. A trial merge with mainc7ef603ais clean, with drift 0.Closes #264
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh