Fix test broker leaks, state races, and signal-masked command failures#541
Fix test broker leaks, state races, and signal-masked command failures#541apedroheringer wants to merge 20 commits into
Conversation
- tests/helpers.mjs: track temp workspaces and add a global after() teardown that shuts down any broker a test left running (graceful shutdown, then terminateProcessTree escalation) and removes its session dir; the suite now fails loudly if a broker survives teardown. - tests/runtime.test.mjs: the lazy-broker status test now runs the SessionEnd hook like its neighbors instead of leaking its broker. - lib/locking.mjs (new): minimal mkdir-based advisory lock with stale reclaim, sync and async variants. - lib/state.mjs: serialize saveState/updateState per workspace and write state.json via temp file + fsync + atomic rename, so concurrent read-modify-write cycles no longer lose updates or tear the file. - lib/broker-lifecycle.mjs: serialize ensureBrokerSession's check-then-create sequence per workspace so concurrent callers cannot spawn duplicate brokers. - lib/process.mjs: stop coalescing a null spawnSync status to 0 when the process died from a signal; runCommandChecked now reports it as a failure. - New regression tests: signal-terminated commands, concurrent upsertJob from two processes, concurrent ensureBrokerSession sharing one broker.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 878c86314f
ℹ️ 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".
| if (pidAlive(pid)) { | ||
| throw new Error(`Leaked broker process ${pid} survived suite teardown.`); |
There was a problem hiding this comment.
In environments where the detached broker has exited but remains as a zombie under PID 1, process.kill(pid, 0) still succeeds, so this new teardown path reports a leak that cannot be terminated. I hit this with npm test: all tests passed, then this hook failed with Leaked broker process ..., and ps showed that PID as [MainThread] <defunct>. That makes the suite fail nondeterministically depending on how quickly the container init reaps orphaned detached brokers; the leak check needs to distinguish defunct processes from live brokers before throwing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 96875ff. Teardown no longer relies on a raw kill(pid, 0) liveness check. isProcessRunning() now reads Linux process state and treats Z/X as exited, with regression coverage in tests/process.test.mjs. The full suite at current HEAD passes 112/112.
provencher-oai
left a comment
provencher-oai
left a comment
There was a problem hiding this comment.
Found four additional correctness and safety issues beyond the existing automated review comment.
| import fs from "node:fs"; | ||
|
|
||
| const DEFAULT_TIMEOUT_MS = 5000; | ||
| const DEFAULT_STALE_MS = 30000; |
There was a problem hiding this comment.
Can we record the lock owner's PID and reclaim a dead owner's lock immediately? staleMs is 30 seconds, but state acquisition times out after five seconds and broker startup after ten. Killing a worker while it owns the lock can therefore break cancellation and exhaust the SessionEnd cleanup deadline.
There was a problem hiding this comment.
Addressed in 96875ff. Each lock now records a private owner file with the PID, process start identity, and a UUID token. Acquisition reclaims immediately when that exact owner is no longer running, before staleMs; regression coverage is in lock immediately reclaims an owner process that exited.
| function reclaimIfStale(lockDir, staleMs) { | ||
| try { | ||
| const age = Date.now() - fs.statSync(lockDir).mtimeMs; | ||
| if (age > staleMs) { |
There was a problem hiding this comment.
Can we make stale-lock reclamation conditional on the exact lock instance observed here? Another process can replace the directory between statSync() and rmdirSync(), so removing the replacement lets multiple state writers or broker creators enter the critical section. The unconditional unlock has the same ownership problem.
There was a problem hiding this comment.
Addressed in 96875ff. The owner UUID is part of the ownership filename, and release/reclaim unlinks only the exact observed generation before removing an empty directory. The regression test proves that an old handle cannot release a successor lock.
| function writeStateFileAtomic(stateFile, nextState) { | ||
| const tmpFile = `${stateFile}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; | ||
| try { | ||
| const fd = fs.openSync(tmpFile, "w"); |
There was a problem hiding this comment.
Can we create the replacement with the existing file's permissions, or explicitly use 0o600? This temporary file uses the process umask, so replacing a 0600 state file typically changes it to 0644 even though it contains complete background-task prompts.
There was a problem hiding this comment.
Addressed in 96875ff. Atomic state replacements are created with wx and mode 0o600, fchmoded before writing, and chmoded after rename for defense in depth. state, job, and log artifacts are private verifies the resulting modes.
| after(async () => { | ||
| const leakedPids = []; | ||
| const leakedSessions = []; | ||
| for (const dir of trackedTempDirs) { |
There was a problem hiding this comment.
Can teardown verify that this endpoint belongs to a broker started by the fixture and put a deadline on shutdown? An existing runtime fixture stores unix:/tmp/fake-broker.sock without ownership metadata, so this hook can send broker/shutdown to an unrelated process, unlink its socket, or hang indefinitely waiting for a response.
There was a problem hiding this comment.
Addressed in 96875ff. Global teardown only touches sessions carrying the fixture-created instanceToken and routes them through shutdownBrokerSession(), which verifies the RPC/process identity before kill or unlink. The fixture supplies a 1000 ms timeout, failures are aggregated, and regressions cover both deadline enforcement and preservation of an unowned endpoint.
provencher-oai
left a comment
provencher-oai
left a comment
There was a problem hiding this comment.
Some important changes to fix
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96875ff3ce
ℹ️ 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".
| if (!ownershipVerified && endpointArtifactExists(session.endpoint)) { | ||
| throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); |
There was a problem hiding this comment.
When an owned broker is killed or crashes after binding its Unix socket, the socket pathname is left behind while the persisted PID is no longer running. shutdownBrokerSession() then gets no shutdown ack, observes exited === true, but this check still throws because ownershipVerified is only set by an RPC ack; ensureBrokerSession() calls this path before starting a replacement for a non-ready saved broker, so one stale socket can make every future command in that workspace fail until the user manually deletes the state/socket files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2f6cc6a and hardened in f51a8a4. Stale socket reclaim now requires all independent signals: the endpoint is inside the private session directory, both the recorded PID and process group are gone, and connecting is refused. Negative regressions preserve live, reused, or external endpoints.
A broker that is killed after binding its Unix socket can never send a shutdown ack, so ownershipVerified stayed false while the socket file survived on disk. shutdownBrokerSession() observed exited === true and still threw, and because ensureBrokerSession() shuts the previous session down before spawning a replacement, a single stale socket made every later command in that workspace fail until the state and socket files were deleted by hand. Reclaim the endpoint instead of throwing, but only on two independent signals: the recorded PID is no longer running AND the endpoint refuses connections (ECONNREFUSED/ENOENT). A missing ack alone is ambiguous, since a live-but-hung broker also fails to answer, and a bare PID check is defeated by PID reuse. Anything else — a timeout, an unexpected error, a malformed endpoint — stays conservative and keeps the existing failure, so a socket owned by an unrelated process is never unlinked.
Three gaps in the previous commit, found while reviewing it: Confine reclaiming to the session directory. A refused connection only proves nothing is listening at that instant — a process that has bound but not yet called listen() also refuses — so a refused connect plus a dead PID was not enough to call the socket ours. Reclaiming now also requires the endpoint to sit inside the mkdtemp 0700 directory this plugin created, which an unrelated process cannot write into. Stop treating a reused PID as dead. isProcessTreeRunning() inspects the process *group* on Linux, so a recycled PID belonging to another group read as gone. Pair it with the plain PID check before concluding the owner exited. Normalise the probe timeout. socket.setTimeout(0) disables the timer, so an explicit zero could leave the probe promise pending forever. Adds regression tests for the two evidence combinations that must never reclaim: live owner with a refused endpoint, and a fully stale-looking endpoint outside the session directory.
|
Confirmed — this is a real bug, and it's now fixed in 2f6cc6a + f51a8a4. Thanks for the catch. Tracing it. For an owned session, The fix. That path is now reclaimable instead of fatal, but deliberately not on the absence of an ack alone —
Anything else — timeout, The session-directory containment in (1) is what closes the gap a plain refused-connect leaves open: a process that has Tests. Five regression tests in Two things worth flagging separately, both pre-existing on
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51a8a442a
ℹ️ 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".
| } | ||
|
|
||
| const pid = resolveBrokerPid(session); | ||
| if (session.endpoint && !session.instanceToken) { |
There was a problem hiding this comment.
When upgrading while a broker written by the previous version is still alive, its persisted broker.json has an endpoint/PID/sessionDir but no instanceToken. This branch classifies that live endpoint as unowned before attempting any legacy shutdown path, so SessionEnd throws ownership could not be verified and preserves the state/process until manual cleanup even though it is the plugin's own broker. Please retire or migrate legacy sessions (for example by verifying the saved PID/sessionDir or sending the old unauthenticated shutdown) before rejecting tokenless state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c0b5eb2. A tokenless session is retired only when its endpoint exactly matches the endpoint derived from sessionDir, its PID file is canonical, and the live PID command line matches the complete legacy broker launch sequence. The old unauthenticated shutdown is then allowed and awaited. A positive upgrade regression and a negative test for a new authenticated broker whose persisted token was lost are both included. npm test passes 112/112 and npm run check-version passes.
|
Hi @provencher-oai — I addressed all four requested changes in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b5eb243f
ℹ️ 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".
| processRunning(state.owner.pid, { | ||
| identity: state.owner.processIdentity ?? undefined | ||
| }) | ||
| ) { | ||
| return false; |
There was a problem hiding this comment.
On macOS and Windows, getProcessIdentity() always records null, so if a lock-owning process crashes and its PID is subsequently reused by an unrelated long-lived process, this branch treats that process as the lock owner forever. Because staleMs is only applied to empty locks, every subsequent acquisition of the affected state or broker lock times out indefinitely; persist and verify a platform-specific process identity, or otherwise safely expire abandoned owned locks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 64fdef1. Lock owners now persist a platform-specific process start identity: /proc start time on Linux, ps lstart on macOS/other POSIX systems, and PowerShell StartTime ticks on Windows. isProcessRunning() compares that identity before accepting a live PID as the owner, so a reused PID is reclaimed immediately. The identity lookup is cached once per owner process to avoid adding cost to every state update. Existing macOS/Windows lock files that already contain no identity are reclaimed after staleMs, preventing permanent wedges during upgrade. Added regressions for macOS identity, Windows identity, non-Linux PID reuse, and stale identity-less locks. npm test passes 116/116; npm run check-version passes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Behavioral fixes: - sendBrokerShutdown honors timeoutMs: 0 instead of disabling the timer. - Broker shutdown ACK uses destroySoon and destroys lingering client sockets, so a half-open client no longer leaks the broker process. - Lock owner records fail closed on malformed pid/token/processIdentity. - teardownBrokerSession only removes the canonical broker files inside a session directory this plugin provably created (real cxc-* dir directly under tmpdir, non-symlink), never externally supplied paths. - SessionEnd aggregate error now includes the underlying failure text. Consolidation (no behavior change): - Single isValidPid, ENOENT-tolerant removeFileIfExists, shared ps/PowerShell process-table query, shared /proc cmdline reader, single endpoint connect probe, and one lock acquire step shared by the sync/async paths. - Drop dead writeJsonFile, unused PID/LOG env constants, options.session, and demote broker helpers that no longer have external consumers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e48f8497
ℹ️ 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".
| if (platform === "linux") { | ||
| const readProcessStat = options.readProcessStat ?? readLinuxProcessStat; | ||
| const stat = readProcessStat(pid); | ||
| if (!stat) { |
There was a problem hiding this comment.
On Linux systems where /proc/<pid>/stat is unavailable or temporarily unreadable, kill(pid, 0) may confirm that the process exists but this branch still reports it as stopped. reclaimAbandonedLock consequently removes a live owner's lock immediately, allowing concurrent state updates or broker creation despite the lock; broker exit checks can likewise proceed while the process remains alive. Only an explicit missing-process result should prove exit, while failure to inspect /proc should conservatively remain running and be retried.
Useful? React with 👍 / 👎.
kill(pid, 0) succeeding proves the process exists; a null /proc stat only means inspection failed. Reporting such a process as exited let a stale-lock reclaim remove a live owner's lock. Fail conservative instead: the process stays running until an explicit missing-process signal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
- Register the SIGTERM/SIGINT shutdown handler in one loop. - Reuse removeFileIfExists for broker socket/pid cleanup, closing the exists/unlink race. - loadBrokerSession relies on its catch instead of a pre-existence check. - canDiscardUnownedSession forwards caller options to the process-tree check like every other ownership probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Extract teardownAndClear() for the two teardown+clear call sites in shutdownBrokerSessionLocked, keeping the legacy-discard path pinned to an unverified endpoint so the unverified-endpoint guard in teardownBrokerSession stays load-bearing. Compute the lock owner's process identity lazily on first acquisition instead of at module import, so CLI invocations that never take a lock no longer spawn ps/PowerShell on macOS and Windows. Read the broker pid file in a single syscall with failures treated as an absent pid, removing the exists/read TOCTOU window; an unreadable pid file now degrades to the persisted state pid instead of aborting shutdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S29rWAKA85b1N7zAFC6NtB
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
The verifyProcess escape hatch was threaded through earlier revisions of this branch, but its last caller was refactored away during the helper consolidation, so the ternary can never take the override arm. Collapse processMatchesInstanceToken to the real launch-token probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S29rWAKA85b1N7zAFC6NtB
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc263cbb39
ℹ️ 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".
| pid: child.pid ?? null, | ||
| instanceToken | ||
| }; | ||
| saveBrokerSession(cwd, session); |
There was a problem hiding this comment.
When spawn() fails asynchronously before assigning a PID—for example because the workspace directory disappears—the child emits an unhandled error, but this state has already been written with pid: null. The current invocation crashes, and later invocations cannot recover because shutdownBrokerSessionLocked() rejects tokened sessions without a valid PID while preserving broker.json, permanently wedging broker startup for that workspace. Handle the child’s spawn error and clean up, or defer persistence until the spawn is confirmed.
Useful? React with 👍 / 👎.
child_process.spawn() reports failures like a vanished workspace directory asynchronously, as an "error" event on the next tick. ensureBrokerSession persisted broker.json before that event could fire, so a failed spawn crashed the invocation via the unhandled event and left behind a tokenized session with pid: null — a combination shutdownBrokerSessionLocked() rejects fatally before any reclaim path runs, wedging every later invocation in that workspace. Await the child's spawn/error outcome before writing state: on failure, tear down the just-created session artifacts and surface a descriptive error instead. Regression test covers the rejection, the absence of persisted state, and the cleanup of the temporary session directory. Reported by the Codex GitHub reviewer on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S29rWAKA85b1N7zAFC6NtB
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Final-audit fixes on the broker lifecycle and locking paths: - resolveBrokerPid() swallowed every pid file read error, so an unreadable file (EACCES, EISDIR) silently skipped the PID mismatch check and fell back to session.pid alone. Only ENOENT is treated as an absent file now; any other failure aborts the shutdown path with the state preserved. - ensureBrokerSessionLocked() persisted broker.json without guarding the write. A persist failure (ENOSPC, EACCES) after a confirmed spawn left the detached child running with no state pointing at it, so later invocations could stack a second broker on top. On failure the child tree is killed, the session artifacts are torn down, and the original error is rethrown. - tryAcquire() resolved the process identity between creating the candidate directory and publishing it, so a crash during a slow ps/PowerShell lookup could orphan the candidate. The lookup now happens before mkdir, shrinking the window back to the write-and-rename itself. - Reworded the stale null-identity reclaim comment: this module never shipped without identity records, so null means the lookup failed at acquire time, and reclaiming after staleMs remains the deliberate lesser risk versus wedging every later caller behind a reused PID. - Dropped the dead node:fs import from app-server-broker.mjs. Regression tests cover the fail-closed pid file read and the kill-and-teardown unwind after a failed persist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018uAqG4qqx6YKWzXjd7YMDG
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad3d852593
ℹ️ 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".
| pid: child.pid ?? null, | ||
| instanceToken |
There was a problem hiding this comment.
If a crashed broker's PID is reused before the stale session is reclaimed, the session has no process start identity with which to distinguish the replacement. A replacement that is a process-group leader makes shutdownBrokerSessionLocked() preserve the state as apparently live, while any replacement PID makes canReclaimStaleEndpoint() refuse to remove a leftover socket; ensureBrokerSession() then remains wedged indefinitely even though processHasLaunchToken() proves the process is not this broker. Persist the broker's start identity alongside its PID and use it in the liveness checks.
Useful? React with 👍 / 👎.
| try { | ||
| killProcess(pid); | ||
| shutdownOptions.killProcess(child.pid); | ||
| } catch { | ||
| // Ignore missing or already-exited broker processes. | ||
| // Teardown must still run and the original persist error must still | ||
| // surface; a failure to kill the child is secondary to both. |
There was a problem hiding this comment.
When persisting broker.json fails after the child has spawned, killProcess() only delivers a signal and may also throw, but this path immediately deletes the socket and session directory without waiting for exit. If the broker's signal handler is still closing a hung app server, or termination is denied, the process tree survives with no state or endpoint through which a later invocation can identify and stop it—the exact untracked leak this catch is intended to prevent. Wait for confirmed process-tree exit and surface termination failure before removing the recovery artifacts.
Useful? React with 👍 / 👎.
The Codex GitHub reviewer flagged two P2 gaps on the previous commit: sessions persist only a PID, so a recycled PID wedges the workspace (a group-leader lookalike makes shutdown throw "ownership could not be verified" forever, and any live holder of the PID blocks stale-socket reclaim), and the failed-persist unwind killed and deleted artifacts without confirming the child actually exited. Persist the broker's process identity (its start time) in broker.json at spawn time and pin every liveness and exit check on the persisted pid to it. The identity follows the pid+creation-time convention (psutil, OCSF common-process-id) and fails closed end to end: - On Linux the identity is boot_id:starttime -- starttime alone is in clock ticks since boot and would collide across reboots. When boot_id cannot be read, no identity is recorded and none is compared; a bare tick count is never emitted or matched, and a transient read failure is not cached. - isProcessTreeRunning() only treats the tree as gone on a CONFIRMED replacement (pid alive with a different readable identity). POSIX keeps a group leader's pid reserved while its group has members, so a confirmed replacement also proves the recorded group is gone; an absent or unreadable leader still falls through to the group check, keeping live descendants visible. - ps-based process-table queries run under LC_ALL=C TZ=UTC so the recorded lstart rendering cannot drift with the caller's locale or timezone and falsely mismatch a still-live broker. - A persisted identity is honored only when it is a non-empty string; malformed values fall back to the legacy no-identity behavior instead of reading a live broker as replaced. The failed-persist unwind now mirrors the ownership-before-kill invariant of the forced-shutdown path: it only signals the child when its argv still carries this session's launch token, waits for a confirmed exit pinned to the recorded identity, and preserves the session artifacts with a descriptive error when the exit cannot be confirmed instead of deleting them out from under a live process. Regression tests cover the reused-pid mismatch, the dead-leader with a surviving descendant, the preserved-artifacts branch, and the confirmed teardown, each verified to fail against the unfixed code; test cleanups now assert the processes they spawn actually exited. Reported by the Codex GitHub reviewer on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018uAqG4qqx6YKWzXjd7YMDG
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d99093734e
ℹ️ 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".
| const ownsPersistedProcess = ownsBrokerProcess(session, pid, legacySession, options); | ||
| if (!ownsPersistedProcess) { | ||
| if (isProcessTreeRunning(pid, livenessOptions)) { | ||
| throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); |
There was a problem hiding this comment.
When the broker crashes or exits while its Codex child remains in the detached process group, waitForProcessExit(..., { tree: true }) correctly reports the tree as running, but token verification against the now-dead leader PID necessarily fails here. Shutdown therefore preserves broker.json without terminating the owned descendant group, and every later ensureBrokerSession() fails on the same state until it is removed manually. The persisted process identity and process-group ID should permit recovery when the recorded leader is gone but its group survives.
Useful? React with 👍 / 👎.
The Codex GitHub reviewer flagged the remaining wedge on this branch: when the broker leader dies but its codex app-server child survives in the detached process group, the tree correctly reads as running, yet ownership can only be proven against the dead leader's argv. Shutdown therefore threw "ownership could not be verified" on every invocation until the orphan happened to exit, wedging the workspace. Recognize that state explicitly instead. When the session carries an instance token and the recorded leader is confirmed gone (or provably replaced, per the persisted identity) while its group still has members, shutdown now reclaims the session state without signaling anything: POSIX reserves a leader's pid for as long as its group survives, but a single observation cannot tell the original broker's descendants from a later generation that recycled the same pgid, so the group is deliberately left untouched -- it may not even be idle. Nothing can be accepting on a refused endpoint, so the socket, pid file, log file and broker.json are safe to remove, and the next ensureBrokerSession starts a fresh broker in a fresh session directory. canReclaimStaleEndpoint gains an explicit allowOrphanedTree flag for exactly this caller; legacy sessions keep the full process-group veto and still fail closed. Regression tests cover the reclaim (kill spy proves nothing is ever signaled, the orphan survives, all artifacts are removed), the subsequent ensureBrokerSession with the orphan still alive, and the unchanged legacy behavior; all verified to fail against the previous code. Reported by the Codex GitHub reviewer on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018uAqG4qqx6YKWzXjd7YMDG
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Problem
Running
npm testleaves dozens of orphaned processes behind (brokerapp-server-broker.mjs serveinstances plus fakecodex app-serverchildren spawned from/tmp/codex-plugin-test-*workspaces). On one machine a single suite run left ~30 brokers + ~29 fake app-servers running after the temp dirs had already been cleaned up. Root causes, verified againstmain(db52e28):SessionEndhook (e.g. the lazy-brokerstatustest attests/runtime.test.mjs:2209, while its neighbours at:2153/:2198do clean up). Any test failure or earlyreturnalso skips cleanup.state.jsonwrites are a read-modify-write with no lock and no atomic rename (lib/state.mjs—saveState/updateStateuse a barefs.writeFileSync). Two concurrent commands can lose updates or tear the file.ensureBrokerSessionis check-then-create with no serialization (lib/broker-lifecycle.mjs). Two concurrent callers can both miss the existing broker and spawn duplicates, orphaning one.lib/process.mjscoalescesspawnSync'sstatus: nullto0, sorunCommandCheckedtreats a SIGTERM/SIGKILL death as exit 0.Changes
tests/helpers.mjs:makeTempDirnow tracks created workspaces, and a globalafter()hook shuts down any broker a test left running (gracefulbroker/shutdown, thenterminateProcessTreeescalation), removes its session dir, and fails the suite loudly if a broker survives teardown — so leaks cannot reappear silently.tests/runtime.test.mjs: the lazy-brokerstatustest now runsSessionEndlike its neighbours.lib/locking.mjs(new): minimal mkdir-based advisory lock with stale reclaim; sync and async variants; no new dependencies.lib/state.mjs:saveState/updateStateare serialized per workspace and write via temp file +fsync+ atomicrename.lib/broker-lifecycle.mjs: the check-then-create sequence inensureBrokerSessionis serialized per workspace.lib/process.mjs:status: nullwith a signal is normalized to a non-zero status;runCommandCheckednow reportssignal=SIGTERMfailures.Testing
npm test: 95/95 passing (91 existing + 4 new regression tests: signal-terminated command reported as failure ×2, concurrentupsertJobfrom two processes loses no updates, concurrentensureBrokerSessioncalls share a single broker).npm run check-versionpasses.Related work
lib/process.mjsfix here addresses one path where a signal death is reported as success.🤖 Generated with Claude Code
https://claude.ai/code/session_01K9WP1dAojgJe3xXJMV9DM1