Harden Codex stop gate supervision - #662
Conversation
![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: 5379993feb
ℹ️ 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".
| const JOBS_DIR_NAME = "jobs"; | ||
| const MAX_JOBS = 50; | ||
| const STATE_LOCK_FILE_NAME = ".state.lock"; | ||
| const STATE_LOCK_WAIT_MS = 5000; |
There was a problem hiding this comment.
When SessionEnd encounters a live, invalid, or unreapable state lock, this loop can wait the full 5 seconds before throwing, but the inspected plugins/codex/hooks/hooks.json also gives the SessionEnd hook exactly 5 seconds. The hook can therefore be terminated before cleanupSessionJobs records tombstones or kills the session's workers, allowing jobs to continue after session shutdown. Use a shorter lock deadline for this path or increase the lifecycle-hook timeout with enough margin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e92ff66: SessionEnd now fences and stops the session's workers from a lock-free snapshot before taking the state lock, the lock wait on that path is 1s, the broker shutdown request times out after 2s, and the SessionEnd hook timeout is raised to 15s with a test guarding the margin.
The SessionEnd hook and the state lock both had a 5 second deadline, so a live, invalid, or unreapable lock could let the hook be killed before it fenced tombstones or stopped the session's workers. - fence and terminate session workers from a lock-free snapshot first, then prune state under a 1 second lock deadline - give the broker shutdown request a 2 second socket timeout so a silent broker cannot hold the hook - raise the SessionEnd hook timeout to 15 seconds and guard the margin with a test - keep cleanup going past unreadable jobs or state instead of aborting Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lbfiwm6KGT2FVh1emKpBdM
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e92ff66859
ℹ️ 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".
| const claimed = claimFile(resolveAdmissionFile(workspaceRoot, job.id), { status: terminal.status, completedAt }); | ||
| const winner = claimed ? { status: terminal.status, completedAt } : readAdmissionClaim(workspaceRoot, job.id); | ||
| if (winner?.status !== "admitted") { | ||
| return { job: applyTerminalFence(readStoredJobOrNull(workspaceRoot, job.id) ?? job, winner), claimed }; |
There was a problem hiding this comment.
When the first stop-gate process dies after creating its running .started.json claim but before publishing the job record, the recovery process reaches this branch and exclusively claims a failed admission, but returns without writing or indexing the failed job. Subsequent Stop hooks therefore cannot find the deterministic gate job in state.json; each retry spawns another companion that immediately loses to the existing claims, while the suggested /codex:status <id> reports no such job. Persist the terminal result when claimed is true so this crash-recovery path becomes visible and reusable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e8961e7ac
ℹ️ 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 (!tryCreateStateLock(reapFile, { pid: process.pid, token, createdAt: nowIso() })) { | ||
| return false; |
There was a problem hiding this comment.
If a process dies after creating .state.lock.reap but before releasing it in finally, every later caller returns false here solely because that guard exists. The guard's owner is never read or checked for liveness, so even when both recorded PIDs are dead, all future state mutations time out until somebody manually deletes the file. Detect and safely remove a reap guard owned by a dead process before retrying.
Useful? React with 👍 / 👎.
| const terminal = terminalizeTrackedJob(workspaceRoot, { | ||
| ...existing, | ||
| ...nextJob | ||
| }, { | ||
| ...nextJob, |
There was a problem hiding this comment.
When the broker accepts a connection but never replies to turn/interrupt, or when later process termination throws, this immutable cancelled outcome has already been claimed before interruptAppServerTurn and terminateProcessTree run. The interrupt request has no timeout, so the kill may never be attempted; meanwhile status reports the job as terminal and resolveCancelableJob rejects subsequent cancellation attempts while a write-capable worker can remain alive. Bound the interrupt and attempt process termination in a guaranteed cleanup path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fe3c059de
ℹ️ 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".
| }; | ||
|
|
||
| writeJobFile(workspaceRoot, job.id, { | ||
| const terminal = terminalizeTrackedJob(workspaceRoot, { |
There was a problem hiding this comment.
When persisting the job JSON or state fails after terminalizeTrackedJob has created the immutable cancelled fence, this call throws before execution reaches the try/finally below, so the worker is never terminated. Subsequent reads apply the fence and clear pid, while cancellationPid existed only in the write that failed, so /codex:cancel can no longer select the job and a write-capable worker may continue running. Fresh evidence beyond the prior timeout fix is that the new guaranteed cleanup region begins only after this fallible terminalization call; process termination needs to be guarded independently of terminal-state persistence.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| await client.close().catch(() => {}); |
There was a problem hiding this comment.
When a configured broker endpoint is syntactically invalid, BrokerCodexAppServerClient.initialize() throws in parseBrokerEndpoint before assigning a socket, and this catch then awaits close(). That method waits on exitPromise, but without a socket no event calls handleExit, so connect() remains pending indefinitely instead of reporting the invalid endpoint; this also defeats the cancellation interrupt timeout. Ensure the no-transport close path resolves its exit promise before awaiting it.
Useful? React with 👍 / 👎.
| latePids = fenceSessionJobs(state.jobs); | ||
| state.jobs = state.jobs.filter((job) => job.sessionId !== sessionId); |
There was a problem hiding this comment.
When a same-session worker has created its mutable job file but has not yet called upsertJob, both the lock-free snapshot and this locked state.jobs snapshot omit it. Its upsertJob waits for this update to release the state lock, then inserts the job afterward; because no .removed marker was created for that ID, the worker can claim admission and continue after SessionEnd. A session-level shutdown fence or startup publication check is needed so jobs cannot appear after the final cleanup snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6175b52e7b
ℹ️ 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".
|
|
||
| export function setSessionLifecycle(cwd, sessionId, ended) { | ||
| ensureStateDir(cwd); | ||
| const generation = ended ? readSessionState(cwd, sessionId)?.generation ?? null : randomUUID(); |
There was a problem hiding this comment.
When Claude emits SessionStart after compaction or resuming an already-active session, this unconditionally assigns a new generation even though the session_id is unchanged. Existing background jobs then fail the generation comparison in isJobSessionEnded; the next state mutation prunes and tombstones them while an admitted worker may keep running, making a write-capable task disappear from status and cancellation. Reuse the existing generation while its lifecycle is still active, and rotate it only when starting after an ended lifecycle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e4f37e1ea
ℹ️ 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".
| return applyTerminalFence(storedJob ?? job, initial); | ||
| } | ||
| if (initial?.status === "running") { | ||
| if (job.gateKey && !storedJob && !isProcessAlive(initial.pid)) { |
There was a problem hiding this comment.
Fresh evidence beyond the prior admission-recovery issue is the remaining crash window between writeJobFile and upsertJob: if a stop-gate worker dies there, its mutable job file exists but the job is absent from state.json. This !storedJob guard therefore skips recovery, while reconcileTrackedJobs cannot discover the unindexed job; every subsequent Stop hook sees the dead running claim as a duplicate and blocks indefinitely. Check owner liveness for unindexed gate claims regardless of whether the mutable file was written, and persist/index the terminal recovery outcome.
Useful? React with 👍 / 👎.
| const descriptor = fs.openSync(file, "wx"); | ||
| try { | ||
| fs.writeFileSync(descriptor, `${JSON.stringify(payload)}\n`, "utf8"); |
There was a problem hiding this comment.
With concurrent workers, openSync(..., "wx") makes the claim visible before its JSON is written, so another process can read an empty or partial admission claim. For example, cancellation during this window interprets the claim as a corrupt terminal failure; terminalizeTrackedJob then loses to that apparent outcome and handleCancel deliberately skips process termination, after which the first worker finishes writing admitted and continues running. Publish a fully written temporary file atomically while retaining exclusive-claim semantics, or otherwise make readers retry an in-progress claim.
Useful? React with 👍 / 👎.
1 participant
Summary
Root cause
The detached worker could start before its queued record was durable. Mutable lifecycle writes, SessionEnd cleanup, cancellation, and concurrent Stop hooks could then race, leaving queued zombies, duplicate reviews, or stale
ALLOWoutput.Verification
npm test— 138/138 passingnpm run buildnpm run check-versionnode --checkon every changed.mjsfilegit diff --check origin/main...HEADDeliberate tradeoff
A zero-byte
.removedtombstone is retained per retired job. It is the smallest daemon-free guarantee that a paused late worker cannot publish or run after removal; mutable job files, logs, and lifecycle sidecars are still cleaned up.