Skip to content

fix: stop session end from tearing down the shared broker under other sessions' jobs - #623

Open
yuanchen8911 wants to merge 1 commit into
openai:mainfrom
yuanchen8911:fix/shared-broker-lifecycle
Open

fix: stop session end from tearing down the shared broker under other sessions' jobs#623
yuanchen8911 wants to merge 1 commit into
openai:mainfrom
yuanchen8911:fix/shared-broker-lifecycle

Conversation

@yuanchen8911

@yuanchen8911 yuanchen8911 commented Aug 10, 2026

Copy link
Copy Markdown

Problem

Any session ending (including /clear) shut down the workspace-shared broker while other sessions' background jobs were mid-turn. The turn was aborted, the detached worker hung forever on the dead socket, and the job eventually vanished with no terminal record (#612).

Changes

Session end respects other sessions' work. SessionEnd skips broker shutdown/teardown while another session has queued or running jobs. Any active job with a recorded worker pid is liveness-checked, so a dead worker (e.g. one that crashed at startup, leaving a permanently queued record) cannot pin the broker. Liveness is authoritative — task runtime is unbounded and updatedAt is not a heartbeat, so a live worker is never expired by record age; only pid-less records, which have no liveness signal, fall back to a one-day staleness bound. SessionEnd also reaps dead workers' active records to failed — interrupting their persisted server-side turns first, since a failed record leaves the active set and nothing else would ever interrupt them (the ending session's own jobs are excluded so its cleanup retains cancelled records for them) — so zombie records cannot accumulate or, after pid reuse, pin the broker indefinitely. Active records are never pruned from state.json by the job cap, so long-lived in-flight work stays visible to the guard — and a floor keeps the newest terminal records retained even when active jobs consume the whole cap, so a job finishing alongside many active peers does not vanish the moment it completes. The last session out still tears everything down.

Broker teardown is atomic with client admission. The broker refuses broker/shutdown while any other client is connected — not merely mid-request, so a worker between requests is protected too — stops listening before it begins tearing the app-server down (so a worker cannot be admitted mid-shutdown), and SessionEnd honors the refusal. Peer shutdown requesters are excluded from the busy check (with one client-side retry for the ordering race), so two session-end hooks racing after all jobs are done cannot refuse each other into leaking the idle broker; killed workers and live finalizers get a short exit grace with SIGKILL escalation for stragglers — whose claims are force-repaired (a just-killed pid can linger as a zombie and read as alive) and whose failed records are retained rather than erased with the session's finished jobs — the same repair and retention applying to a finalizer that dies during the grace wait without recording an outcome, while a finalizer that died between its job-file write and the index update has the index converged to the file's terminal outcome instead of leaving a running record with a dead pid — so a dying or wedged worker's lingering socket cannot veto the final teardown; and the session record is only cleared when it still describes the broker that was torn down, so a racing survivor's freshly spawned broker is never orphaned. Work admitted between the guard check and the shutdown request can therefore not be killed; a later session end retires the broker once the clients disconnect. An ambiguous shutdown outcome (connection lost mid-exchange, where a refusal may have been dropped) fails closed: only a confirmed shutdown or an unreachable endpoint proceeds to teardown.

Session-end cleanup cancels cleanly instead of erasing. The ending session's in-flight jobs are marked cancelled (with a cause) instead of being deleted, so status queries report why the job stopped. Cleanup also interrupts each job's brokered turn (turn/interrupt) before killing the relay worker — killing the worker alone left the turn running, and potentially editing files, inside the surviving shared app-server. The cancelled record is persisted per job before the interrupt (record-first, matching cancel, with the worker's fresher pid and turn identity captured from the job file — both before claiming and again from the post-claim reread, which can hold the only copy when the worker published them while the claim was being taken — before the terminal patch nulls the pid field), so a hook killed mid-interrupt still leaves a terminal record; the interrupts share a deadline well inside the SessionEnd hook timeout that covers the connect as well as the RPC (identity waits are capped per job with a reserve so one id-less job cannot starve every job's interrupt, every interrupt the hook may still send — dead-worker reap interrupts included, which run first on the same deadline — gets an equal share of the remaining budget so one hung interrupt RPC cannot consume the shared deadline and leave later jobs killed (or reaped) without any interrupt attempt, and the hook skips the synchronous codex availability probe entirely), and a timed-out interrupt tears its own connection down — including a client still wedged inside connect/initialize — so a hung broker RPC can neither starve the worker kills nor linger as an active request that blocks broker/shutdown or keeps the caller's process alive. The final state rewrite is a fresh read-modify-write, so records other sessions create while the interrupts are awaited are not clobbered by a stale snapshot, and a job whose worker is concurrently recording its own outcome is left intact rather than having its files deleted under the live worker.

Terminal status is claimed atomically. A first-writer-wins claim file (jobs/<id>.terminal, created with O_EXCL, recording the claimant's intent) decides whether the worker's completed/failed write or a cancellation owns the job's terminal status. This closes the race where a cancellation recorded between the worker's cancelled-check and its terminal write was overwritten — the cancel command could return cancelled while the indexed status ended up completed. The worker refuses to start over a taken claim (repairing an orphaned claim to a terminal record) and re-checks the claim after writing its running record, so a cancel landing during startup cannot be resurrected to running; the repair synchronizes state.json when the job file is already terminal, and repairs a worker-owned orphan claim to failed rather than misreporting a cancellation. The progress updater re-checks after its writes for the same reason. A worker that loses the claim mid-turn repairs a bare claim whose record writes never landed (claimant crashed mid-cancel) instead of leaving running records behind, and the turn id is persisted from the turn/start response itself — landing even on an already-claimed record (identity fields only, merged onto a fresh read so a stale snapshot cannot revert a terminal record), with cancel and session-end polling both stores for it — reading once before the worker-liveness gate so ids persisted by a worker that then died are not missed, and refreshing the worker pid itself so a record-before-spawn snapshot's null pid neither ends the wait early nor sends the kill to nothing — bounded by their budgets, and terminal record writes re-reading at write time so a stale snapshot cannot erase a freshly persisted identity — so cleanup can interrupt a turn even when the cancellation raced the turn's startup. runTrackedJob also preserves a cancellation recorded during the cancel's interrupt window on both its success and error paths.

Workers fail fast on transport death — but keep finished results. captureTurn races turn completion against the connection closing, so a worker whose broker/app-server dies mid-turn writes a terminal failed record instead of hanging forever. A clean close that lands after the final answer but before the inferred-completion timer completes the turn instead of failing it, so an already-finished run is not reported as a transport failure.

Cancel is crash-safe end to end. cancel claims the terminal status and persists the cancelled record before interrupting the turn or killing the worker; log appends and the worker kill are best-effort. The turn interrupt is bounded by the same timeout mechanism as session cleanup, so a hung interrupt cannot hang the command before the worker kill runs. A crash partway through can no longer interrupt a turn while recording nothing, and a failed kill (EPERM, taskkill access denied) no longer crashes the CLI after the cancellation was already recorded. Both the JSON payload (workerTerminated) and the rendered report surface a failed kill, so a surviving worker is distinguishable from a clean cancel; a cancel that loses the terminal claim to a live finalizer reports the benign already-finished outcome instead of failing the command — converging a stale running index to the job file's terminal outcome when the finished worker died before its state.json write, so the job does not stay listed as running with a dead pid until the next session end — while an orphaned claim (owner died before writing its record) is adopted and the cancel proceeds, so a hung worker stays cancellable; the claim's recorded intent decides the terminal record (failed for a dead worker's own claim, cancelled otherwise), a live finalizer is never preempted, the repair cause reaches both the job file and the index, and the cancel command's text output reports the adopted outcome instead of a cancellation stub.

Background enqueue is race-free and handles spawn failures. The queued record is persisted before the worker is spawned, closing the window in which a brand-new job had neither a state record nor a broker socket and a racing session end passed every guard. When the detached worker cannot be spawned (EMFILE/EAGAIN), the job is recorded as failed instead of leaving a pid-less queued record that would count as active forever, and the spawn error event no longer crashes the CLI. Job-file writes go through a temp file renamed into place, so a concurrent reader never observes a torn record: the enqueue's post-spawn pid-merge rewrite lands exactly when the detached worker's startup reads the same file, and a truncate-in-place write handed that reader partial JSON — the worker died before recording anything and the job stayed queued forever with a dead pid.

Progress updates respect terminal records. A still-streaming turn's progress events no longer touch a job that already reached a terminal status or whose terminal claim is already taken, so a cancelled record cannot regain a live phase or a fresher updatedAt.

Testing

npm test: 144/144 pass (50 tests added by this PR); npm run build passes. New coverage includes:

  • session end leaves the shared broker running while another session has an active job; still tears it down when the other session's worker is dead or its pid-less record is stale; never expires a demonstrably live worker by record age
  • the broker refuses shutdown while another client is connected (mid-turn or idle between requests), and the raced work survives the session end; racing shutdown requesters do not deadlock an idle broker; a SIGTERM-ignoring dying worker or wedged finalizer cannot veto the final teardown
  • session end reaps a dead worker's active record to failed (interrupting its persisted turn first when the broker survives), while keeping a terminal record for the ending session's own dead worker; cancel reports a benign already-finished result when a live finalizer owns the terminal claim, repairs an orphaned claim while still stopping the worker (reporting failed for a dead worker's own claim), and session end adopts an orphaned claim on its own job; a cancel that finds the job already finished converges a stale running index left by a finalizer that died before its state.json write
  • a background task records the turn id even when turn/started never arrives, and identity fields still land on an already-claimed record; a bare terminal claim with no record writes behind it is repaired instead of leaving running records
  • session end completes and still tears the broker down when a turn interrupt hangs (never answered by the app server); a hung interrupt for one job cannot starve a later job's interrupt attempt, and a hung dead-worker reap interrupt cannot starve the ending session's own interrupts (asserts the fake broker received every attempt)
  • session end kills a worker whose pid was published only after the hook won the terminal claim (the post-claim reread holds the only copy), and converges the index when a finalizer dies between its job-file write and its state.json update
  • a turn interrupt against an endpoint that accepts but never answers initialize returns within its budget and releases the wedged connection
  • session end interrupts a brokered turn before killing its worker (asserts the fake app-server received turn/interrupt), records a terminal cancelled state for the ending session's running jobs, and leaves a job intact when its worker already claimed the terminal status
  • saveState never prunes active jobs past the job cap, and keeps a newly terminal job even when active jobs fill the cap
  • runTrackedJob cancellation guards: no resurrection before start, refusal to start over an orphaned terminal claim (with record repair, including state.json resynchronization and worker-owned claims repairing to failed), no completed/failed overwrite mid-turn or on the error path, and preservation when the cancellation only won the terminal claim (job-file write not yet landed)
  • progress updates stop for terminal records and once the terminal claim is taken
  • a concurrent reader hammering a job file during repeated writeJobFile rewrites never observes torn JSON (fails with the truncate-in-place write restored)
  • captureTurn unit tests: clean close after the final answer completes the turn; close without a final answer still fails fast; a transport error still surfaces even after the final answer
  • task completes when the app server exits right after the final answer (broker path); the cancel report warns when the worker could not be terminated

Each guard's regression tests were verified to fail with that guard reverted.

Scope boundary and known residuals

This PR fixes the scenarios where a single fault (one crash, one race) breaks the #612 guarantee. The following are deliberate residuals, documented here rather than modeled in code, and intended for follow-up work:

Fixes #612

@yuanchen8911
yuanchen8911 requested a review from a team August 10, 2026 01:42

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 765234cebf

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 765234c to c8642b4 Compare August 10, 2026 02:14

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c8642b455a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from c8642b4 to 46ae223 Compare August 10, 2026 03:01

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 46ae223684

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 46ae223 to 963e01c Compare August 10, 2026 03:24

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 963e01c2c1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/lib/tracked-jobs.mjs
Comment thread plugins/codex/scripts/codex-companion.mjs
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch 2 times, most recently from 85bed6c to 73adfca Compare August 10, 2026 03:51

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 73adfca033

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 73adfca to c28295b Compare August 10, 2026 04:05

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c28295b937

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/codex-companion.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch 2 times, most recently from f8117ae to fed518f Compare August 10, 2026 04:22

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fed518fa04

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/lib/codex.mjs
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from fed518f to 42107b5 Compare August 10, 2026 15:06

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42107b5ccb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/lib/tracked-jobs.mjs Outdated
Comment thread plugins/codex/scripts/lib/app-server.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 42107b5 to a8f45de Compare August 10, 2026 15:24

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8f45de692

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/codex-companion.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch 2 times, most recently from 3e4330f to 34d458b Compare August 10, 2026 15:42

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34d458be7a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 34d458b to 3c594a6 Compare August 10, 2026 16:05

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c594a6ae3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/codex-companion.mjs
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 3c594a6 to 361497c Compare August 10, 2026 16:14

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 361497c4d3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/app-server-broker.mjs
Comment thread plugins/codex/scripts/codex-companion.mjs
@yuanchen8911

Copy link
Copy Markdown
Author

Out-of-scope follow-up tracking from the #612 review:

These are follow-up hardening items, not requirements for the supported-path fix in this PR.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0c447f986

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/app-server-broker.mjs
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from b0c447f to 5e10bb0 Compare August 10, 2026 18:25

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e10bb0f34

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread tests/runtime.test.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 5e10bb0 to 87ec6b6 Compare August 10, 2026 18:50

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87ec6b68f7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/lib/tracked-jobs.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 87ec6b6 to bd5fe9b Compare August 10, 2026 19:16

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd5fe9bae5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from bd5fe9b to 2eb0b10 Compare August 10, 2026 19:43

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2eb0b10045

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch 2 times, most recently from 9a01813 to b21b8d2 Compare August 10, 2026 20:41

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b21b8d2922

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch 2 times, most recently from 1fe4548 to 217f529 Compare August 10, 2026 21:41

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 217f5296ca

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 217f529 to 45bc9e9 Compare August 10, 2026 22:54

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45bc9e90a4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs Outdated
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 45bc9e9 to f52cd62 Compare August 10, 2026 23:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f52cd62801

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/codex-companion.mjs
… sessions' jobs

Any session ending (including /clear) shut down the workspace-shared
broker while other sessions' background jobs were mid-turn: the turn was
aborted, the detached worker hung forever on the dead socket, and the
job eventually vanished with no terminal record (openai#612).

- SessionEnd now skips broker shutdown/teardown while another session
  has queued or running jobs; any active job with a recorded worker pid
  is liveness-checked, so a dead worker (e.g. one that crashed at
  startup, leaving a permanently queued record) cannot pin the broker.
  Liveness is authoritative — a live worker is never expired by record
  age; only pid-less records, which have no liveness signal, fall back
  to a one-day staleness bound. SessionEnd also reaps dead workers'
  active records to failed — interrupting their persisted server-side
  turns first, since a failed record leaves the active set and nothing
  else would ever interrupt them (the ending session's own jobs are
  excluded so its cleanup can retain cancelled records for them) — so zombie records cannot accumulate
  or — after pid reuse — pin the broker indefinitely. The last session out still tears everything
  down.
- Active records are never pruned from state.json by the job cap, so
  long-lived in-flight work stays visible to the guard; a floor keeps
  the newest terminal records retained even when active jobs consume
  the whole cap, so a job finishing alongside many active peers does
  not vanish the moment it completes.
- The broker refuses broker/shutdown while any other client is
  connected (not merely mid-request), stops listening before it
  begins tearing the app-server down (so a worker cannot be admitted
  mid-shutdown), and SessionEnd honors the refusal, so work admitted
  between the guard check and the shutdown request cannot be killed.
  Peer shutdown requesters are excluded from the busy check (with one
  client-side retry for the ordering race), so two session-end hooks
  racing after all jobs are done cannot refuse each other into
  leaking the idle broker, and killed workers and live finalizers get a short exit grace (SIGKILL
  plus forced claim repair for stragglers and for finalizers that die
  during the grace without recording an outcome, whose failed records
  are retained rather than erased with the session's finished jobs; a
  finalizer that died between its job-file write and the index update
  has the index converged to the file's terminal outcome instead of
  leaving a running record with a dead pid) so a
  dying or wedged worker's lingering socket cannot veto the final
  teardown, and the session
  record is only cleared when it still describes the broker that was
  torn down, so a racing survivor's freshly spawned broker is never
  orphaned. An ambiguous shutdown outcome (connection
  lost mid-exchange, where the refusal may have been dropped) fails
  closed: only a confirmed shutdown or an unreachable endpoint
  proceeds to teardown.
- Session-end job cleanup marks the ending session's in-flight jobs
  cancelled instead of erasing them, and interrupts their brokered
  turns before killing the relay workers: killing the worker alone left
  the turn running (and editing files) inside the surviving shared
  app-server. The cancelled record is persisted per job before the
  interrupt (record-first, like cancel, with the worker's fresher pid
  and turn identity captured from the job file — both before claiming
  and again from the post-claim reread, which can hold the only copy
  when the worker published them while the claim was being taken —
  before the terminal patch nulls the pid field); the interrupts share a
  deadline well inside the SessionEnd hook timeout that covers the
  connect as well as the RPC (identity waits are capped per job with
  a reserve so they cannot starve the interrupts, every interrupt the
  hook may still send — dead-worker reaps included — gets an equal
  share of the remaining budget so one hung interrupt RPC cannot
  consume the shared deadline and leave later jobs killed (or reaped)
  without any interrupt attempt, and the hook skips
  the synchronous codex availability probe entirely),
  and a timed-out interrupt tears its connection down — including a
  client still wedged inside connect/initialize — so an abandoned
  request cannot linger on the broker or keep the caller alive.
  The final state rewrite is a fresh read-modify-write, so records
  other sessions create while the interrupts are awaited are not
  clobbered by a stale snapshot. A job whose worker is concurrently
  recording its own outcome is left intact rather than having its
  files deleted under the live worker, while an orphaned claim (owner
  died before writing its record) is adopted so the job still reaches
  a terminal record and its worker is killed; the claim's recorded
  intent decides that record (failed for a dead worker's own claim,
  cancelled otherwise), a live finalizer is never preempted, the
  repair cause is written to both the job file and the index, and the
  cancel command's text output reports the adopted outcome instead of
  a cancellation stub.
- Terminal job status is claimed atomically through a first-writer-wins
  claim file that records the claimant's intent, closing the window
  where a cancellation recorded between the worker's cancelled-check
  and its terminal write was overwritten with completed/failed. The
  worker re-checks the claim after writing its running record (and
  refuses to start over a taken claim, repairing it to a terminal
  record), so a cancel landing during startup cannot be resurrected to
  running; the repair synchronizes state.json when the job file is
  already terminal, and repairs a worker-owned orphan claim to failed
  rather than misreporting a cancellation. The progress updater
  re-checks the claim after its writes, so a still-streaming turn
  cannot revert a cancelled record. A worker that loses the claim
  mid-turn repairs a bare claim whose record writes never landed
  (claimant crashed mid-cancel) instead of leaving running records
  behind, and the turn id is persisted from the turn/start response
  itself — landing even on an already-claimed record (merged onto a fresh
  read so a stale snapshot cannot revert a terminal record), with
  cancel and session-end polling both stores for it (reading once
  before the worker-liveness gate, refreshing the worker pid itself
  so a record-before-spawn snapshot's null pid neither ends the wait
  early nor sends the kill to nothing, and keeping ids persisted by a
  worker that then died) while bounded by their budgets, and terminal record writes
  re-reading at write time so a stale snapshot cannot erase a freshly
  persisted identity — so cleanup can interrupt a turn even
  when the cancellation raced the turn's startup.
- captureTurn races turn completion against transport death, so a
  worker whose broker/app-server dies mid-turn fails fast and writes a
  terminal failed record instead of hanging; a clean close that lands
  after the final answer but before the inferred-completion timer now
  completes the turn instead of failing it.
- cancel claims the terminal status and persists the cancelled record
  before interrupting the turn; log appends and the worker kill are
  best-effort and the turn interrupt is bounded by the same timeout
  mechanism as session cleanup, so a crash partway through can no
  longer interrupt a
  turn while recording nothing, and a failed kill can no longer crash
  the CLI after the cancellation was already recorded. Both the JSON
  payload (workerTerminated) and the rendered report surface a failed
  kill, and a cancel that loses the terminal claim to a live finalizer
  reports the benign already-finished outcome instead of failing the
  command — converging a stale running index to the job file's terminal
  outcome when the finished worker died before its state.json write, so
  the job does not stay listed as running with a dead pid until the
  next session end — while an orphaned claim (owner died before writing its
  record) is repaired and the cancel proceeds, so a hung worker stays
  cancellable.
- Enqueueing a background task persists the queued record before
  spawning the worker, closing the window in which the new job had
  neither a state record nor a broker socket and a racing session end
  passed every guard; a failed spawn records a failed job instead of
  leaving a pid-less queued record that would pin the broker forever,
  and the spawn 'error' event no longer crashes the CLI.
- writeJobFile writes through a temp file and renames it into place,
  so a concurrent reader never observes a torn record. The enqueue's
  post-spawn pid-merge rewrite of the job file lands exactly when the
  detached worker's startup reads it; with a truncate-in-place write
  the worker could read partial JSON, die before recording anything,
  and leave the job queued forever with a dead pid.

Fixes openai#612

Signed-off-by: Yuan Chen <[email protected]>
@yuanchen8911
yuanchen8911 force-pushed the fix/shared-broker-lifecycle branch from 35c70c6 to b2b14fc Compare August 11, 2026 01:58

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2b14fccaa

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +139 to +143
if (!claimTerminalStatus(workspaceRoot, job.id, "reaper")) {
// A claimant died between taking the claim and writing its record (or
// is writing it right now); converge the stores to a terminal state.
reassertTerminalClaim(workspaceRoot, job.id, job);
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Interrupt orphaned reaper claims before repairing them

When a previous cancel/SessionEnd/reaper creates the .terminal claim and dies before sending turn/interrupt or writing the terminal record, and the relay worker is now dead with stored threadId/turnId, this lost-claim branch repairs the job and then continues before the reaper interrupt path below. Fresh evidence in this version is that the orphaned-claim branch still bypasses the newly added dead-worker interrupt block, so if another session keeps the broker alive the server-side Codex turn can continue running after status has already been converged to terminal; interrupt cancel/reaper-owned orphan claims with available IDs before returning.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is real, but the scenario is the documented multi-fault residual, deferred to #628 rather than modeled here. Reaching this branch requires two independent deaths aligning: the relay worker died mid-turn (the workerDead gate above), and the prior claimant died inside its claim-to-interrupt/claim-to-write window — the PR's scope boundary ("Multi-fault interleavings (#628) — two independent crashes/races aligning (e.g. a claimant dying inside the microsecond claim-to-write window while its worker is simultaneously wedged)") describes exactly this pair. Single-fault variants are already covered: a live worker is never reaped, a claimant that died after recording leaves a terminal record the repair honors, and the reaper's own claim path does interrupt before failing the record. The residual impact is bounded as documented — the repaired records read terminal while the orphaned server-side turn, if any, is retired when the last session's teardown stops the shared app-server. Interrupting on the lost-claim path would also mean re-deriving turn identity and budget accounting for a state only reachable through a double crash; that hardening belongs with the rest of the #628 class.

@yuanchen8911

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2b14fccaa

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

continue;
}
removeJobFile(resolveJobFile(cwd, job.id));
removeFileIfExists(resolveJobClaimFile(cwd, job.id));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retain terminal claims until cancelled workers exit

When SessionEnd is cancelling a batch large enough to exceed the terminal retention budget, each upsertJob can prune an earlier just-cancelled record before waitForWorkerExits has confirmed that worker is gone; this line then removes that job's .terminal claim while the process may still be running after SIGTERM. If that worker ignores or outlives the signal, runTrackedJob no longer sees either the cancelled job file or the claim and can take a fresh worker terminal claim, overwriting the cancellation or leaving status inconsistent. Please avoid deleting terminal claim files for cancellation records whose worker has not been reaped yet, or defer pruning these records until after the grace/SIGKILL pass.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the arithmetic and this cannot arise in a supported configuration; declining to change the prune ordering here.

Pruning only engages above MAX_JOBS (50 records), and terminalBudget = max(10, 50 - activeCount). A cancellation refreshes updatedAt, so a just-cancelled record sorts among the newest terminal records — the only records that can displace it are ones cancelled later in the same loop. So the ending session has to cancel more than terminalBudget jobs for the first one to be pruned, i.e. 11+ at the floor. The budget only sits at the floor while activeCount >= 40, and each cancellation in the loop decrements activeCount, raising the budget 1:1 once 50 - active > 10. Keeping it pinned across 11+ cancellations therefore requires ~40 concurrently active jobs owned by other live sessions, on top of the ending session's own 11+, with more than 50 total records — roughly 51 concurrent in-flight background workers in one workspace. Each of those is a detached relay driving a Codex turn; that is not a configuration this plugin supports or that the job cap is sized for.

The premise on the worker side is also the documented residual: the worker must outlive SIGTERM and the grace-plus-SIGKILL escalation added by this PR (waitForWorkerExits), then still finish its turn and take a terminal claim.

Worth noting the consequence is bounded even inside that scenario. The cap policy has already discarded the record — removeJobFile drops the cancelled job file on the same iteration, whether or not the claim file goes with it. The delta is a later-recreated completed record versus no record at all, not the loss of a cancellation that would otherwise have survived.

Retaining the claim file for such records would instead leak .terminal files for every pruned job, which is what this line exists to prevent. If the pruner should become worker-aware, that belongs with the multi-fault interleaving work in #628 rather than in this fix.

@yuanchen8911

Copy link
Copy Markdown
Author

@codex review

Both open threads have replies contesting the findings on reachability grounds rather than mechanism. Please take them into account on this pass:

  • session-lifecycle-hook.mjs:143 (orphaned reaper claims): reaching that branch requires two independent deaths aligning — a dead relay worker plus a claimant that died inside its claim-to-interrupt window — which is the multi-fault residual class this PR documents and defers to Terminal-status repair: residual multi-fault interleavings, turn-identity race window, and broker readiness-probe kills (follow-up to #623) #628.
  • state.mjs:138 (terminal claims vs. pruning): the pruner only engages above 50 records with terminalBudget = max(10, 50 - active), so displacing a just-cancelled record needs ~51 concurrent in-flight background workers in one workspace, with ~40 held by other live sessions; the cancelled job file is also removed on the same iteration regardless.

If either refutation is wrong, the useful reply is the specific step in that reasoning that fails.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: b2b14fccaa

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@yuanchen8911

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: b2b14fccaa

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Session end//clear tears down workspace-shared broker, silently losing other sessions' running jobs

1 participant