Skip to content

Release idle Codex app-server helper trees - #563

Open
ccheng555 wants to merge 17 commits into
openai:mainfrom
ccheng555:cc-5219-idle-app-server-child
Open

Release idle Codex app-server helper trees#563
ccheng555 wants to merge 17 commits into
openai:mainfrom
ccheng555:cc-5219-idle-app-server-child

Conversation

@ccheng555

@ccheng555 ccheng555 commented Jul 27, 2026

Copy link
Copy Markdown

Stops Codex companion app-server helper trees from accumulating after their client or Claude session goes away. Relates to #543 and #526.

Root cause

The shared broker eagerly kept one heavyweight codex app-server alive for its entire session. Some app-server helpers launch in their own process groups, so parent-only cleanup could leave those detached descendants running. On Unix, close() sent a single SIGTERM to one pid; only the Windows path did a tree kill. Socket closure also erased stream ownership, which could make an active turn look idle.

The invariant

Everything below rests on one rule, and it is the rule to check any future change against:

A process identity may be recorded only by the process it describes, or by a holder of a live unreaped handle to it; and every persisted file has exactly one writer at a time.

Both halves are load-bearing. The first is what makes a recorded identity trustworthy later: a process scanning for itself cannot be confused by pid reuse because it is alive while scanning, and a holder of an unreaped child handle cannot have that pid recycled underneath it. An identity recorded by anyone else is a claim that outlives the authority that made it. The second is what keeps two writers from racing over the same record, which no amount of ordering logic can fix when the writes are unlocked whole-record replacements.

Concretely: the enqueuer writes a job record once before spawning and never again; the worker records its own identity and owns the record thereafter; the spawn sites that hold a live child handle capture ownership directly. Cancellation of a not-yet-started worker uses a create-only flag file whose existence is the entire message, so it cannot be contended.

What changed

Lifecycle. The app-server starts lazily and is released after five minutes with no sockets, requests, or active streams, then restarts on demand. Stream lifetime is tracked independently of its client socket, so a disconnect cannot terminate active work. The broker closes its listener before awaiting child cleanup during shutdown, so a late client cannot start a replacement app-server that shutdown would then abandon.

Cleanup. Unix cleanup captures an identity-bearing ownership record while the root is alive, enumerates descendants and process groups, revalidates pid@lstart identity before every signal, waits and rescans after SIGTERM, escalates to SIGKILL, and returns {verified, survivors}. Windows taskkill /T /F is unchanged.

The evidence rule. Absence of evidence is not treated as proof of death. An unreadable process table, an owned descendant that cannot be accounted for, or a missing root all report verified: false. A root positively observed absent from a readable table, with nothing owned unaccounted for, is a clean success — so cancelling an already-exited job does not surface a spurious error. Survivors are tracked as identities rather than bare pids, so pid reuse can neither mask a survivor nor wedge the broker permanently. Unverified cleanup is recorded and warned, never thrown, and it blocks a replacement spawn (RPC -32002) until the survivors are gone; callers no longer discard that outcome and delete the ownership records needed to retry.

Honest scope of the guarantee

verified: true means every process this code recorded owning was observed gone from a readable process table. It does not cover a descendant that calls setsid after the final scan; that case is documented in the source.

Process identity is pid@lstart, and ps reports lstart with whole-second resolution. A pid recycled within the same second as its predecessor therefore produces an identical identity, and the reuse checks throughout this change cannot distinguish the two. Every ownership guarantee here is bounded by that: it holds against pid reuse, except reuse inside the same one-second window.

That is not fixed here, and the obvious remedy does not fit. Reading finer-grained start ticks from /proc/<pid>/stat works on Linux and does not exist on macOS or BSD, so it would deliver the stronger guarantee on one platform and silently leave the other where it is. Adding ppid as a discriminator is worse than useless here: orphaned helpers reparenting is precisely the scenario this code handles, so ppid changes exactly when the identity most needs to stay stable. A cross-platform fix means widening the ps output and reworking the table parser, which is the one piece of this change least worth destabilising for a window this narrow. Documented rather than half-solved.

Three further gaps are known and deliberately not addressed here, to keep the diff reviewable: zombie-aware descendant ordering for environments where PID 1 does not reap, refreshing the process table before each individual signal rather than once per poll round, and buffering terminal notifications for detached streaming responses.

One test, shared broker clears a disconnected stream after its request rejects, has been observed failing roughly once in four full-suite runs while passing 6/6 in isolation. That points at cross-test interference under load rather than a defect in the behaviour it covers, but it is disclosed rather than left for someone else to trip over.

Review history

Eight rounds of adversarial review ran against this branch — seven automated passes plus one deep independent audit. A recurring pattern is worth naming for reviewers: several rounds fixed the reported defect and introduced a new one, twice reintroducing an earlier defect by a different route. That is what motivated the invariant above. Rather than continue patching symptoms, the final round removed the shared-writer condition that made the races possible, which shrank the surface: ownership capture now happens at two call sites instead of three, and task records no longer carry an ownership snapshot at all.

Two findings from that history were corrected publicly in the thread rather than quietly: a review thread resolved on faulty reasoning and reopened, and a fix that reintroduced pid-reuse unsafety through a persisted flag.

Behavior notes for reviewers

  • On Unix the app-server child is spawned detached so its group can be terminated as a unit. Tradeoff: if the broker is SIGKILLed, the detached child survives until its stdin closes. The graceful-path leak this fixes was the more common case.
  • Lazy start means the first request after an idle release pays app-server cold-start latency. CODEX_COMPANION_BROKER_CHILD_IDLE_MS tunes the window; 0 releases immediately, invalid values fall back to the five-minute default.
  • Fail-closed cleanup can refuse a replacement spawn while unverified survivors are alive. It recovers automatically once they exit.
  • Cancelling a queued job whose worker has not started yet now prevents that work from running. Previously it reported success while the worker went on to complete the job.

Checks

  • npm test: 128 tests, 124 pass. The 4 failures are pre-existing on main (state-directory and environment tests unrelated to this change) and fail identically there.
  • Red-armed procedurally, not by assertion: reverting only the cancel-flag check fails exactly the two flag-protocol tests and nothing else; reverting source while keeping tests fails the new tests; restoring makes them pass. Each round's numbers were measured on a full unsandboxed run rather than reported by the tool that wrote the change.
  • New tests cover regrouped-helper reclamation after a root exits, identity-capture failure, unverified-cleanup record preservation, pid-reuse survivor clearing, shutdown not starting a second app-server, client disconnect during a live turn, process-table failure, root and descendant pid reuse, resistant descendants, PATH isolation, worker self-identity capture, queued-job cancellation preventing work, flag convergence to cancelled, and helper reclamation without an ownership snapshot.
  • git diff --check clean. Pre-push secret and PII sweep clean.

ccheng555 and others added 7 commits July 26, 2026 23:12
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
… non-fatal cleanup verification, async non-blocking process cleanup, ps portability with degraded fallback, de-flaked idle-release tests

Attributed-To: Codex executor lane (implementation), Claude interactive (verification, test-anchor fix)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
@ccheng555
ccheng555 marked this pull request as ready for review July 27, 2026 18:45
@ccheng555
ccheng555 requested a review from a team July 27, 2026 18:45

@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: 38529f436e

ℹ️ 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".

Comment thread plugins/codex/scripts/lib/process.mjs
Comment thread plugins/codex/scripts/app-server-broker.mjs Outdated
Comment on lines +275 to +276
for (const unit of units) {
delivered = signalVerifiedUnit(unit, signal, processes, options.killImpl) || delivered;

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 Reap helpers before terminating their app-server parent

In environments where PID 1 does not reap orphaned children, signaling every depth-sorted unit back-to-back kills detached helpers and their app-server parent without giving the parent an opportunity to reap them. The subsequent polling treats Z records as gone, returns verified: true, and leaves permanent zombie PID entries; this is reproducible in the repository's container, where both new helper-cleanup runtime tests time out and the helper PIDs remain as <defunct>. Terminate and wait for descendant units before killing the parent, or otherwise avoid claiming successful cleanup while zombies persist.

Useful? React with 👍 / 👎.

…s, keep stream ownership on socket close, reap descendants before their parent

Attributed-To: Claude interactive
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co

@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: 16547a0cc8

ℹ️ 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".


function signalTracked(tracked, signal, options) {
const processes = readUnixProcessTable(options.runCommandImpl, options);
mergeTrackedDescendants(tracked, processes, options.rootPid, options.rootIdentity);

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 Keep the root out of the descendant phase

When PID 1 does not reap orphans, the new two-phase cleanup still kills the parent in the first phase: signalTracked(descendants, ...) calls mergeTrackedDescendants, which always seeds rootPid and then adds that root to the supplied descendants map. buildSignalUnits consequently signals the root process group alongside its helpers, so the parent cannot reap them and the subsequent polling treats their zombie records as gone. Fresh evidence in this revision is that the attempted descendant/root phase split is neutralized by this merge; both new helper-cleanup runtime tests time out with defunct helper PIDs in the container.

Useful? React with 👍 / 👎.

Comment thread plugins/codex/scripts/app-server-broker.mjs
…kill phase, reclaim orphaned process group on unexpected child exit

Attributed-To: Claude interactive
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co

@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: 58d84b1801

ℹ️ 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".

Comment thread plugins/codex/scripts/lib/process.mjs
Comment thread plugins/codex/scripts/app-server-broker.mjs
ccheng555 and others added 2 commits July 27, 2026 13:03
A TERM-resistant helper whose parent already exited is reparented out of
the root's tree, so it was signaled by the group kill but never tracked:
polling saw no live records, skipped SIGKILL, and reported verified while
the helper was still alive. Group members observed at scan time now enter
the shared tracked map, so they escalate and count as survivors.

Attributed-To: Codex executor lane
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
connect() rejected before returning the client, so a detached app-server
that answered initialize with an RPC error but stayed alive was never
closed, and every later request could spawn another live helper tree.
connect() now closes its own client on a failed initialize and relays an
unverified cleanup outcome to the broker's existing refusal gate.

Attributed-To: Codex executor lane
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@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: 386411c99f

ℹ️ 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".

Comment thread plugins/codex/scripts/app-server-broker.mjs Outdated
An adversarial audit found this branch's cleanup layer repeatedly claiming
success it could not prove: an empty process-group selection, a missing
root, and a dead root all produced verified: true while a detached helper
could still be alive. This replaces those claims with one contract.

Ownership is captured while the root is alive, so cleanup consults what it
recorded owning rather than rediscovering a tree whose root has since
exited. Absence of evidence is no longer proof of death: an unreadable
process table, an unaccounted-for owned descendant, or a missing root all
report verified: false. A root positively observed absent from a readable
table, with nothing owned unaccounted for, is still a clean success, so
cancelling an already-exited job does not surface a spurious error.
Survivors are tracked as pid@lstart identities rather than bare pids, so
pid reuse can neither mask a survivor nor wedge the broker permanently.

Also: identity-capture failure no longer pre-kills the child in a way that
made close() skip tree cleanup and suppress the unverified-cleanup gate;
the broker closes its listener before awaiting child cleanup during
shutdown, so a late client cannot start a replacement app-server it would
then abandon; and callers no longer discard an unverified cleanup outcome
and delete the ownership records needed to retry it.

Attributed-To: Codex executor lane
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ccheng555

Copy link
Copy Markdown
Author

@codex review

@ccheng555

Copy link
Copy Markdown
Author

Status note for anyone reading the review threads: the automated passes above all ran against commits that predate the current head (c663c57). GitHub re-anchors older comments onto current line numbers, so several read as live when the code they describe no longer exists.

Resolving the ones the current head addresses:

  • Await the asynchronous process-tree teardown — all three call sites now await it (codex-companion.mjs, session-lifecycle-hook.mjs, broker-lifecycle.mjs via teardownBrokerSession).
  • Keep stream state when its owner socket closesclearSocketOwnership now detaches only the notification socket and leaves the turn marked running, so a second client cannot start one mid-turn.
  • Clean up the process group after an unexpected child exit — the exit handler reclaims the group before any replacement can spawn, and blocks spawning if that fails.
  • Reap detached children when initialization failsconnect() closes the client on a failed initialize and propagates an unverified cleanup outcome.
  • Track all signaled process-group members — group members observed during signalling are merged into the tracked set.
  • Reclaim independently grouped helpers after a child crash — group termination seeds from the ownership snapshot before PGID selection, so a helper that created its own group is still covered.

Left open deliberately: the two findings about reaping helpers before their app-server parent and keeping the root out of the descendant phase. Both are the same gap on systems where nothing reaps orphans, and both are named as out of scope in the PR description rather than silently dropped.

@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: c663c57f41

ℹ️ 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".

Comment thread plugins/codex/scripts/lib/process.mjs Outdated
Comment thread plugins/codex/scripts/lib/process.mjs
@ccheng555

Copy link
Copy Markdown
Author

Correction to my previous note: I resolved the "Reclaim independently grouped helpers after a child crash" thread on faulty reasoning and have reopened it.

I had read terminateProcessGroup as covering independently-grouped helpers because it seeds the tracked set from the ownership snapshot before selecting by process-group id. That is true but insufficient — the selection loop then skips any live group member that is absent from the snapshot, which is exactly the helper in question. The finding stands.

The two findings from the review of c663c57 are both real, and I have reproduced both against the source rather than only reading the reports:

  • Refuse to adopt an unverified current PIDexpectedRootIdentity falls back to the identity of whatever process currently holds the pid. Background task records persist only pid, so that fallback is the live path for cancel and session-end cleanup, and a reused pid means signalling an unrelated process tree. This contradicts the identity-safety claim in the description, so the description is wrong as written until this is fixed.
  • Reclaim helpers created after the ownership snapshot — as above; the practical effect is worse than a leak, since the resulting unverified outcome makes the broker refuse replacement children.

Working on both.

Cleanup could adopt the identity of whatever process currently held a
recorded pid, which makes identity verification a no-op and can signal an
unrelated process tree after pid reuse. Background task records persisted
only a pid, so that was the live path for cancel and session-end cleanup.

- Remove the current-identity fallback; a record with no ownership evidence
  refuses to signal and reports unverified rather than killing.
- Capture ownership when spawning background workers and brokers so the
  normal path stays fully verified.
- Allow best-effort cleanup after an identity-capture failure only when the
  caller still holds a live child handle, which is what makes pid reuse
  impossible; persisted records never qualify and are left alone with a
  message saying so. Leaking a process is preferred over killing a stranger.
- Include a live member of an owned process group even when it postdates the
  ownership snapshot, so a helper spawned after initialize is reclaimed
  instead of wedging the broker against ever spawning a replacement.

Attributed-To: Codex executor lane (implementation), Claude interactive (review, verification)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ccheng555

Copy link
Copy Markdown
Author

Pushed ce3a15e addressing both findings from the review of c663c57.

Refuse to adopt an unverified current PID. The fallback that took the current holder of a recorded pid as the expected identity is gone. A record with no ownership evidence now refuses to signal and reports unverified, rather than killing whatever inherited that pid. Background workers and broker sessions capture ownership at spawn time so the normal path stays fully verified instead of merely permitted.

The one case that still needs best-effort cleanup is an identity-capture failure. That is now allowed only when the caller still holds a live, unreaped child handle, since that is the specific condition that makes pid reuse impossible. Persisted records never qualify: they are left alone with a message saying the job could not be verified as owned. Leaking a process is the intended outcome there.

Worth stating plainly, because I got it wrong first: an earlier version of this fix persisted the capture-failure flag into the job record and honoured it on cancel and session end. That reintroduced the same defect by a different route, since a record on disk outlives its process. Caught on review, not by the tests, and there is now a test that fails specifically on it.

Reclaim helpers created after the ownership snapshot. A live member of the owned process group is included even when it postdates the snapshot, so a helper spawned after initialize is reclaimed. A snapshot pid whose identity no longer matches stays excluded, so pid-reuse protection is unchanged. This also clears the wedge where the resulting unverified outcome made the broker permanently refuse replacement children.

Verification. Full suite: 118 tests, 114 pass, 4 fail — the same 4 environmental failures present on the base commit, by name. Four new tests. Reverting the source while keeping the tests fails exactly those four; reverting only the live-handle gating fails exactly the one test written for it.

Still deliberately out of scope, as described in the PR body: reaping helpers before their app-server parent, and keeping the root out of the descendant phase, both on systems where nothing reaps orphans.

@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: ce3a15e9ae

ℹ️ 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".

Comment thread plugins/codex/scripts/codex-companion.mjs Outdated
@ccheng555

Copy link
Copy Markdown
Author

Acknowledging the startup-race finding: it is real and it is mine. Ownership capture is a synchronous process-table scan sitting between the worker spawn and the job record being written, so a worker that starts quickly can read a record that does not exist yet, exit with No stored job found, and leave the parent reporting a job as queued that will never run. The window existed before, but inserting a ps call into it widened it by orders of magnitude.

The obvious fix — persist before spawning, then patch ownership onto the stored record — is not sufficient, so I am not pushing it. It moves the race rather than removing it: the worker loads the job record at startup and writes back a record derived from that load, so the parent's ownership patch and the worker's first write contend, and whichever lands second wins. Measured on a full unsandboxed run, that reordering takes the suite from 118 tests / 114 pass / 4 fail to 120 / 115 / 5, with cancel sends turn interrupt to the shared app-server before killing a brokered task failing reproducibly on Unable to verify cleanup ... ownership records were preserved for retry. Stashing the change makes that test pass again, which isolates it.

The shape that should work is keeping ownership out of the record the worker rewrites — a sidecar the parent owns and the worker never touches — so there is no contention to order. I would rather land that than a reordering that trades one race for another.

Head stays at ce3a15e, which is green apart from four pre-existing environmental failures.

Ownership data had no single author. The parent wrote a job record at
enqueue time and the worker rewrote the same file as it ran, both through
unlocked whole-record writes, so correctness depended on an ordering that
neither side controlled. Successive fixes kept trading one race for another.

The invariant now enforced by data ownership rather than by conditionals:
a process identity may be recorded only by the process it describes, or by
a holder of a live unreaped handle to it, and every persisted file has
exactly one writer at a time.

- The enqueuer writes the queued record before spawning and never writes it
  again. It no longer scans the process table, which removes the window
  where a fast worker read a record that did not exist yet and exited.
- The worker records its own identity. A process scanning for itself cannot
  be confused by pid reuse, because it is alive while scanning.
- Task records no longer carry an ownership snapshot. Descendants and
  process-group members are still discovered at termination time, and the
  worker leads its own group, so late-spawned helpers are still reclaimed.
- Cancellation of a job whose worker has not started yet uses a create-only
  flag file whose existence is the whole message, so it cannot be contended.
  This closes a case where cancelling a queued job reported success while
  the worker went on to run it to completion.
- Spawn sites that hold a live child handle keep capturing ownership
  directly; that is the other half of the invariant, and it is unchanged.

Attributed-To: Codex executor lane (implementation), Claude interactive (design review, verification)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ccheng555

Copy link
Copy Markdown
Author

Pushed 5f27c1a, which addresses the startup-race finding by removing the condition that made it possible rather than reordering it, and closes a related case nobody had reported.

The startup race. Ownership capture is a process-table scan, and it sat between spawning the background worker and writing its job record. The enqueuer no longer captures ownership at all; it writes the record before spawning and never writes it again. The worker records its own identity instead. A process scanning for itself cannot be confused by pid reuse, because it is alive while scanning — which is the property every earlier version of this code was missing.

I tried the obvious fix first (persist, spawn, then patch ownership onto the record) and did not push it, because it moves the race rather than removing it: the worker loads that record at startup and writes back a record derived from it, so the parent's patch and the worker's first write contend. Measured, that reordering took the suite from 4 failures to 5, with cancel sends turn interrupt to the shared app-server before killing a brokered task failing reproducibly.

A case that was not reported and is worth flagging. On ce3a15e, cancelling a job whose worker had not started yet reported success and marked the job cancelled while the worker went on to run it to completion. terminateProcessTree(NaN) returns early with verified: !ownershipEstablished, and a queued record has no ownership, so it returned verified: true. Cancellation of a not-yet-started worker now uses a create-only flag file whose existence is the whole message, checked after the worker persists its running record — so there is nothing to order and nothing to contend.

The change shrinks the surface. Ownership capture drops from three call sites to two, both of which hold a live unreaped child handle. Task records no longer carry an ownership snapshot; descendants and process-group members are still discovered at termination time, and the worker leads its own group, so late-spawned helpers are still reclaimed — there is a test for that specifically.

The invariant this all rests on is now stated in the PR description, so future changes have something to be checked against rather than being re-derived per call site.

Verification. 124 tests, 120 pass, 4 fail — the same 4 pre-existing environmental failures, by name. Reverting only the cancel-flag check fails exactly the two flag-protocol tests and nothing else, which isolates them to the behaviour rather than to the feature's general absence. One flaky test is disclosed in the description rather than left to be discovered.

@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: 5f27c1a2f0

ℹ️ 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".

Comment thread plugins/codex/scripts/session-lifecycle-hook.mjs
Comment thread plugins/codex/scripts/lib/process.mjs Outdated
… roots converge

Two defects in the previous commit, both found by review and both confirmed
against the source before changing anything.

A cancel tombstone for a queued job was written and then deleted in the same
session-end pass, because the job was pruned and pruning sweeps its flag. A
worker caught between reading its queued record and publishing its pid would
find no flag and run the job after the session had ended. The job is now
retained until the worker acknowledges the cancellation with its own terminal
write; pruning still sweeps the flag once the job is genuinely finished.

Cleanup could not converge for a worker that exited abnormally. Since task
records stopped carrying an ownership snapshot, cancellation supplies a
persisted identity instead, and the branch handling a root that is absent from
a readable process table still keyed on the snapshot. It reported unverified
forever, leaving the job permanently cleanup-pending and failing session end.
The decision now keys on the process table having been read successfully: a
root absent from a readable table, with nothing owned unaccounted for, is a
verified success whichever form the ownership evidence took.

The failure cases are unchanged and covered: an unreadable table stays
unverified, a live tracked descendant stays unverified, a present root whose
identity does not match stays a refusal, and an absent pid with no ownership
evidence at all is still not a success.

Attributed-To: Codex executor lane (implementation), Claude interactive (triage, verification)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ccheng555

Copy link
Copy Markdown
Author

Pushed 03eac6e. Both findings against 5f27c1a were real, both were defects in that commit, and both were confirmed against the source before I touched anything.

The cancel tombstone did not survive the pass that wrote it. Session end wrote a flag for a queued job and then pruned that job, and pruning sweeps the flag. A worker caught between reading its queued record and publishing its pid would find nothing and run the job after the session ended — which is the exact case the flag exists to prevent. The job is now retained until the worker acknowledges the cancellation with its own terminal write. Pruning still sweeps flags for genuinely finished jobs; the ordering changed, not the sweep.

Cleanup could not converge for a worker that exited abnormally. Dropping the ownership snapshot from task records in the previous commit left the "root absent from a readable table" branch still keyed on that snapshot, so it reported unverified forever and the job sat permanently cleanup-pending, failing session end on every retry. The decision now keys on the process table having been read successfully rather than on which form the ownership evidence took. A root absent from a readable table with nothing owned unaccounted for is a verified success either way.

The failure cases are unchanged and now have their own guard: an unreadable table stays unverified, a live tracked descendant stays unverified, a present root whose identity does not match stays a refusal, and an absent pid with no ownership evidence at all is still not a success.

Both are consequences of the previous commit's scope reduction rather than new territory, so they were in scope under the invariant rather than requests to widen it.

Verification. 127 tests, 123 pass, 4 fail — the same 4 pre-existing environmental failures, by name. Isolating red arm run per fix: reverting only the retention change fails only the tombstone test, reverting only the convergence change fails only the abnormal-exit test. Neither disarm disturbs the other.

@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: 03eac6e0da

ℹ️ 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".

Comment thread plugins/codex/scripts/lib/process.mjs
processGroupId,
state,
startedAt,
identity: `${pid}@${startedAt}`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Use a higher-resolution process identity

On Unix systems where PID churn allows a PID to be reused within the same second, ps's lstart value is identical for both process incarnations because it has only whole-second precision. The resulting ${pid}@${startedAt} identity therefore treats the unrelated replacement as the owned process, allowing the cleanup paths to signal it and its descendants despite their PID-reuse checks. Use a kernel start-time value with finer resolution, such as Linux /proc/<pid>/stat start ticks, for ownership verification.

Useful? React with 👍 / 👎.

Admitting live group members that postdate the ownership snapshot made
reclaiming a late-spawned helper possible, but it also admitted the members of
an unrelated group when the group id itself had been reused. Those members are
absent from the snapshot, so the per-record identity check could not exclude
them, and the leader identity check ran only after signals had already been
delivered. The result was signalling a stranger's process tree and reporting it
as unverified afterwards.

Check the leader before admitting anything: if a live process holds the group
id and is not the root we recorded, nothing in that group is ours, so refuse
without signalling. A missing leader is still not a mismatch, which keeps the
crashed-root case working where surviving helpers do need reclaiming.

Attributed-To: Claude interactive
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ccheng555

Copy link
Copy Markdown
Author

Pushed 3f95424 for the group-leader finding. Declining the identity-resolution one, with reasoning below rather than silence.

Group leader identity — fixed, and it was the more serious of the two. Admitting live group members that postdate the ownership snapshot is what makes reclaiming a late-spawned helper possible, but it also admitted members of an unrelated group once the group id itself had been reused. Those members are absent from the snapshot, so the per-record check could not exclude them, and the leader check ran only after signals had gone out. That is signalling a stranger's process tree and then reporting it unverified afterwards, which is the exact failure class this PR exists to prevent. The leader is now checked before anything is admitted. A missing leader is still not a mismatch, so the crashed-root case where surviving helpers do need reclaiming keeps working. Red-armed: removing the guard makes the new test fail by signalling the stranger.

Identity resolution — real, not fixed, now documented. You are right that ps reports lstart at whole-second resolution and that a pid recycled inside the same second yields an identical pid@lstart. Every reuse check in this change is bounded by that window, and the description now says so explicitly instead of implying the guarantee is unconditional.

I am not fixing it here, for three reasons. /proc/<pid>/stat start ticks do not exist on macOS or BSD, so that remedy strengthens one platform and silently leaves the other unchanged. Adding ppid as a discriminator is actively wrong for this code, because orphaned helpers reparenting is the scenario it handles, so ppid changes precisely when identity most needs to stay stable. A genuinely cross-platform fix means widening the ps output and reworking the table parser — and that parser is the piece of this change least worth destabilising for a window this narrow, in a PR that has already had several rounds where a fix introduced the next defect.

It is on the disclosed limits list in the description alongside the other known gaps, so whoever picks this area up inherits the problem stated rather than hidden.

Verification. 128 tests, 124 pass, 4 fail — the same 4 pre-existing environmental failures, by name.

@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: 3f95424ce1

ℹ️ 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".

Comment thread plugins/codex/scripts/app-server-broker.mjs Outdated
Retaining stream lifetime after a client disconnects left one piece still
keyed on the socket. A client that disconnected before a detached review/start
response arrived cleared the active stream socket, which suppressed recording
the review thread id from that result. The completion notification then carried
a thread the broker had never recorded, so it never matched, the stream stayed
marked running, and the broker remained busy indefinitely.

Each streaming request now takes a turn number and the result-derived update is
gated on that turn still being the active one. The owning socket may come and
go; the turn is what the stream actually belongs to.

Known gap, stated rather than implied: unlike the other fixes on this branch,
this one has no dedicated regression test. Covering it needs a broker
integration test around detached review streams, which is the least stable part
of the suite. The full suite passes unchanged.

Attributed-To: Claude interactive
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ccheng555

Copy link
Copy Markdown
Author

Pushed 19ac644. This is where I stop iterating on automated review for this PR, so this comment states the final position rather than only the change.

This finding was real and worth fixing. Retaining stream lifetime after a client disconnects left one piece still keyed on the socket, so a client that dropped before a detached review/start response arrived suppressed recording that review thread id. The completion notification then carried a thread the broker had never recorded, so it never matched, and the broker stayed busy indefinitely — a permanent wedge introduced by an earlier fix on this branch. Stream ownership is now tied to the turn rather than to its client socket.

Stated plainly: unlike every other fix here, this one has no dedicated regression test. Covering it needs a broker integration test around detached review streams, which is the least stable part of this suite, and I would rather ship a small reasoned fix with the gap named than either skip it or destabilise the suite chasing it. Full suite passes unchanged: 128 tests, 124 pass, 4 pre-existing environmental failures.

Closing position. Every finding in this review series has been real, and several were introduced by earlier fixes in the series — that history is in the description rather than tidied away. The branch is now at the state I intend to defend: the invariant governing process ownership is written into the description, the known limits are listed there including the one-second identity resolution window and the zombie-ordering gaps, and each remaining open thread is deliberate rather than overlooked.

Further automated findings will not be triaged into this branch unless they identify a violation of the stated invariant. Anything that requires widening the guarantee belongs in follow-up work against #543 and #526, where the surrounding design questions can be settled by someone who owns this area. Continuing to expand scope in an unreviewed branch trades a defensible change for a larger one nobody has read.

Happy to rebase, split, or revise this if a maintainer picks it up.

@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: 19ac644d82

ℹ️ 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".

}
send(target, message);
if (message.method === "turn/completed" && activeStreamSocket === target) {
if (message.method === "turn/completed" && activeStreamRunning) {

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 Clear inferred-complete streams in the broker

When Codex omits the parent turn/completed after subagents drain, runAppServerTurn intentionally infers completion in lib/codex.mjs, so the client returns and closes its socket without a matching notification. This condition is now the only successful path that clears activeStreamRunning, while socket close deliberately retains it; the broker therefore remains busy indefinitely, forcing later tasks onto fresh direct app-servers and causing callers without the busy fallback to fail. Add an explicit client-to-broker completion acknowledgement or mirror the inference logic here.

Useful? React with 👍 / 👎.

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.

1 participant