Repository navigation
fix: on Windows, stop a late host after a failed create in more cases, and name the session in text mode - #324
Conversation
…air at the first read must heal it
…the marked repair needs no liveness check
…ils after its marker write
…d a list that dies before its repair
A marker that appears while the creator runs stops a serving host. One that appears after the creator has exited does not: the watch has stopped. Each case fails without the code that it covers.
…t serving The new give-up case fails its first marker write with EIO. create then sees no program in the manifest, sends nothing, and the host that starts late finds no marker and serves. The unit cases ask create to retry the write, to stop the host once the manifest names the program, and to stop retrying after 50 writes or once the host has exited.
… the program record lands A create that gives up on Windows sends no SIGTERM while the manifest names no program, so only the marker can stop a host that starts late. When the marker write fails, create now writes it again every 100 ms, up to 50 times, and reads the manifest after each write. It stops when the marker lands, stops the host once the manifest names the program, and stops retrying once the host has exited. The unit case for the stop now finds the program by the number of marker writes, because the readiness loop also reads the manifest.
On Windows the marker write in CreateGivesUp can now fail. With the new guard CreateRetriesMarker, which facts.mjs reads from the give-up catch, CreateStopsHostOnWindows goes on to "retry", and CreateRewritesMarker writes and reads again until the marker lands, the read names the program, or the host is gone. A create whose last write failed ends as "exhausted", which R17 does not cover. New runs: the win32 mutant breaks R17, and a win32 witness reaches the retry. Docs: SKILL.md, USAGE and the commands README.
With the host's watch, a marker that always landed off Windows made the SIGTERM redundant, so mutant-CreateStopsOnGiveUp passed. The write can fail on any platform, and off Windows the SIGTERM is then what stops the host: the mutant breaks R17 again.
|
@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. 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. |
The host resolves its Home to the real path and derives the socket path from it. On macOS tmpdir() is a symlink, so the tests looked for the socket at another path, and the two cases that need a serving host failed there. A TMPDIR that is a symlink reproduces it on Linux.
|
@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. |
![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: 1159cb73b7
ℹ️ 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".
When a process of another user takes the creator's pid, the liveness probe of the marker watch throws EPERM inside its timer. In the host that is an uncaught exception, and here vitest reports it as unhandled errors, so the run fails.
The marker watch probes the creator's pid inside its timer. A process of another user that took that pid answers EPERM, isProcessAlive throws it, and the uncaught error ended the host. The watch now counts any probe error as an exited creator, checks for the marker once more and stops. facts.mjs reads the new watch, and the docs say how it treats the error.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3b156226b
ℹ️ 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".
The two cases where the watch must not stop the host waited 300 ms and 500 ms. They now spy on the timers, wait until clearInterval runs on the host's 100 ms watch, write the marker and check at once that the host still serves: a stopped watch never checks again. With a watch that never stops, or a probe error that leaves the timer, they fail.
# 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 |
# Conflicts: # .agents/skills/verify-session-lifecycle/SKILL.md # .agents/skills/verify-session-lifecycle/coverage.json # .agents/skills/verify-session-lifecycle/scripts/check.sh
|
Codex Review: Didn't find any major issues. Breezy! 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. |
…ir comment recordDestroyed writes it for a terminal manifest too, where destroy stops no host, so the host can still be in its shutdown tail. It is ending either way, so the repair needs no liveness check.
… zombie creator A creator that writes the marker and exits between the watch's probe and its check must still stop the host: with the check before the probe, the watch stops without the marker. A zombie creator has exited, but kill(pid, 0) still finds it, so the watch ticks for as long as its parent does not reap it. The zombie case fails on the head.
kill(pid, 0) finds a zombie until its parent reaps it, so the watch ticked at 10 Hz for as long as a parent that never waits lived. The watch now also counts a creator that readProcStat shows as Z as exited. facts.mjs reads the new probe, and the docs name it.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 600068767c
ℹ️ 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".
…runs at once A timeout raise alone leaves #324's 48 min share at the merge queue's 60 min limit. check.sh waits for each batch of CHECK_JOBS runs, so a share took the sum of its longest runs. With 8 shares and CHECK_JOBS 12, every share starts all its runs at once and takes about as long as its longest run. The job timeout goes to 45 min.
…share times On the model of #324 the 8 shares took 15 to 43 min. The 43 min share holds only the longest run, which TLC checks with 1 worker, so no share split makes it shorter, and 45 min left too little room.
Why
ThomasK33 asked to ship every valid review finding of this autopilot run in a follow-up PR, with no parking issues. This PR ships the lifecycle follow-ups from #284, #288, #294 and #312 (#264):
createthat gives up sends no SIGTERM while the manifest names no program, so two cases left a host that started late serving the failed session. Both are closed here:create's manifest read fails for a reason such as EIO or an antivirus lock. The read then returns null, socreatesends nothing, and the host's start-up check may have run before the marker existed. The host's new marker watch closes this.createclose this.createdid not name its session. JSON mode hasdetails.sessionId.What changed
runHostinsrc/host/hostMain.ts,watchCreateFailed). Right after the start-up check, the host checks every 100 ms whether its creator still runs and then whether thecreate-failedmarker exists. On the marker it callsstartShutdown, as for a SIGTERM. One check after the creator has exited finds no marker, and then the watch stops. A probe that throws, such as EPERM from a process of another user that took the creator's pid, and a zombie creator (readProcStatshowsZon Linux) count as an exited creator. This closes the case where the host's start-up check came beforecreategave up andcreate's manifest read then failed.create(src/cli/commands/create.ts). The give-up catch writes the marker through the newwriteMarkerOrLog, which logs a failed write and reports whether it landed. On Windows, while the write fails, the manifest names no program and the host runs (hostRuns, where a probe that throws counts as an exited host),createwaits 100 ms, writes the marker again and reads the manifest, up to 50 times. The stop after the loop ends a host that the last read names. A marker that lands reaches the host by its start-up check or its watch, becausecreatestill runs.createfirst writesSession <id> failed to start.to stderr (writeHumanErrorLinesinsrc/cli/output.ts), then the error line.socketRoot.ts,lifecycle.ts,sessionPaths.tsandrpcClient.ts. The marked repair inreconcileSessionsays that onlyrecordDestroyedwrites the marker, for a terminal manifest or once the host and the program's group are dead, so the host has ended or is ending and the repair needs no liveness check..agents/skills/verify-session-lifecycle):ListDiesBeforeRepair: a reconcilinglistcan die between its write and its repair. R18 allows the destroyed record to be lost only until the next reconcile reads the manifest.mutant-ReconcileRepairsMarkednow breaks R18, andWitnessDeadRepairRepairedshows the heal.HostPollsCreateFailed(the watch), with the Windows manifest read that can fail.mutant-HostPollsCreateFailed-win32breaks R17.CreateRetriesMarker(the retries). On Windows the marker write can fail,CreateStopsHostOnWindowsgoes on to"retry", andCreateRewritesMarkerwrites and reads again. Acreatewhose last write failed ends as"exhausted", and R17 does not cover it (SKILL.md Scope).mutant-CreateRetriesMarker-win32breaks R17, andwitness-WitnessMarkerRetried-win32reaches the retry.docs/USAGE.md, SKILL.md,src/cli/commands/README.md,src/cli/README.mdandsrc/host/README.md.Behavior changes for CLI and JSON users
createprintsSession <id> failed to start.on stderr before the error line.createwhose marker write fails can take up to about 5 s longer (50 retries, 100 ms apart) while its host runs.createends the session in more cases: whencreate's manifest read fails, and when its first marker writes fail.lifecycle-modelshare takes longer than the 13 to 17 min that the comment inci.ymlstates, and I expect about 27 to 36 min, within the job's 50 min timeout. A follow-up PR will put the times measured on this PR into that comment and into SKILL.md.Not verified
Real Windows behavior is not verified. CI has no Windows job. The Windows paths run on Linux with preloads in the
createprocess only:test/fixtures/fake-win32-create.tsmakesprocess.platformreadwin32, andChildProcess.killsends SIGKILL, asTerminateProcessdoes.fail-manifest-read-after-marker.tsfails the manifest reads once the marker exists, andfail-first-marker-write.tsfails the first marker write with EIO.When all 51 marker writes fail, for example on a full disk, a Windows host that the manifest does not name yet can still serve the session.
docs/USAGE.mdand the SKILL.md Scope section state this.Validation
00b3d5ee(text mode):create.test.ts"names the session in text mode when the host never answers" andoutput.test.ts"writes the lines to stderr, without color when color is off" fail.bfbf4664(the watch): the integration casea create on Windows whose manifest read fails after it gave upfails. The host and the program are still alive 20 s aftercreateexited.6d2c217a(the retries): the 4 newcreate.test.tscases fail: they find 1 marker write where 2, 3 or 51 were due, or no liveness check of the host. The give-up caseon Windows, when its first marker write failsfails: the host and the program are still alive 20 s later.test/unit/host/hostMarker.test.tsrunsrunHostin the test process. WithoutwatchCreateFailed(), "stops a serving host when the marker appears while its creator runs" fails. With a watch that never stops, "keeps serving when the marker appears after its creator has exited" fails.facts.mjsmutation checks ofCreateRetriesMarker: the read before the write in the loop, noisProcessAlive(host.pid), no loop, a throw before the stop, the first read before the first write, and awriteMarkerOrLogthat always reports a failure. Each reads FALSE (the last two also makeHostChecksCreateFailedFALSE, and the throw also makesCreateStopsOnGiveUpFALSE).check.sh --update-coverageon the head, after the merges of fix(gc): keep a session whose program group still runs, and model-check the lifecycle in CI #302 and fix(gc): freeze a fork chain behind its zombies before gc reads it ended #326: all 85 runs as expected.current64,052,288 distinct states,current-no-token63,127,914,current-win3253,200,220. No guard reads FALSE (38 guards). Eleven runs explore more than 20 million states each. New and changed runs of this PR:mutant-ReconcileRepairsMarkedbreaks R18 in 24 states, andwitness-WitnessDeadRepairRepairedis reached in 21.mutant-HostPollsCreateFailed-win32breaks R17 in 11 states, andwitness-WitnessMarkerPolledis reached in 11.mutant-CreateRetriesMarker-win32breaks R17 in 6 states, andwitness-WitnessMarkerRetried-win32is reached in 6.mutant-CreateStopsOnGiveUpstill breaks R17 (5 states). An earlier run of this branch had it pass, because the model let the marker write fail only on Windows, and with the watch a marker that always landed made the SIGTERM redundant.9658e3d4lets the write fail on every platform.60006876.check.sh --drift-only: covered code matches the sync record.lifecycleandcliintegration 148/148 on the merge of fix(gc): freeze a fork chain behind its zombies before gc reads it ended #326 (vitest run --maxWorkers=1 --retry=2). CI runs the full suite.c8f43008. CI on macOS failed inhostMarker.test.ts: the host resolves its Home to the real path and derives the socket path from it, and on macOStmpdir()is a symlink. So the two cases that need a serving host looked for the socket at another path.1159cb73gives the tests the real Home path. ATMPDIRthat is a symlink reproduces the failure on Linux, and the cases pass with the fix. The same macOS run also failedtest/unit/evals/scheduler.test.ts(input-order settlement), which this PR does not touch.1159cb73(security lane clean): the watch's liveness probe throws EPERM when a process of another user takes the creator's pid, and that uncaught error in the timer ends the host. The red testc4490868makes the probe answer EPERM. Before the fix, vitest reported 3 unhandled EPERM errors from the timer. The fixe3b15622counts any probe error as an exited creator, andfacts.mjsreads the new watch. A catch that keeps the creator alive, and a marker branch without the shutdown, both read FALSE.e3b15622. The probe of the host increate's retry loop can throw EPERM in the same way, and then the error left the give-up catch, socreatereportedINTERNAL_ERRORwithout the session id. The red testcb5c98e0got the raw EPERM error. The fixaf541001addshostRuns, which counts any probe error as an exited host, andfacts.mjsreads it forCreateRetriesMarker. AhostRunsthat returns true on an error reads FALSE. Unit 2016/2016.test/unit/evals/scheduler.test.tsfailed once in a full local unit run under load and passed 5 of 5 times alone. This PR does not touch it.af541001:coverage.jsonlacked thehostRunsfingerprint. The sync commit was still due, and the next sync commit added it.8d742ffc, which merges fix(gc): keep a session whose program group still runs, and model-check the lifecycle in CI #302.f2f2a7cereplaced the fixed 300 ms and 500 ms waits inhostMarker.test.ts: the cases spy on the timers and wait untilclearIntervalruns on the host's 100 ms watch.8d742ffc, folded in before round 6:f64e17a1corrects the marked-repair comment:recordDestroyedalso writes the marker for a terminal manifest, wheredestroystops no host.7563fe19adds the order test. The creator writes the marker and exits between the watch's probe and its check. With the check before the probe, the case fails. It also adds a zombie-creator case, which fails on8da31541.e030d75dcounts a zombie creator as exited, andfacts.mjsreads the new probe. A probe that tests another state reads FALSE.7c9e06f8).Refs #284, #288, #294, #264
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh