fix(coding-agent): verify worker process identity in stopWorker; skip failed workers in global heartbeats_list - #1046
Conversation
…nd keep one failed worker from failing global heartbeats_list Two defects combined to make a single stale descriptor break every global heartbeat listing with "Cannot list heartbeats while session worker is failed" (PrimeIntellect-ai#1045): - stopWorker trusted kill(pid, 0) (EPERM included) as proof the worker was still running and never checked the descriptor's recorded processStartId, unlike the recovery path. After a worker exited, a pid-reusing (or EPERM-answering) unrelated process made the stop path signal the imposter's process group, wait out both deadlines, and persist lifecycle "failed" with "did not stop after SIGKILL" -- across supervisor restarts. - The global heartbeats_list fan-out included failed workers (isVisibleWorker only excludes client-owned ones) and returned the first failure snapshot as the whole command's result, so one zombie descriptor poisoned the entire listing while session-scoped listings kept working. stopWorker now verifies the pid's process start id against the descriptor before signalling or waiting on it, treating an identity mismatch as an already-stopped worker, and heartbeats_list skips failed workers, which cannot host firing heartbeats. Closes PrimeIntellect-ai#1045
![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: b06797e321
ℹ️ 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".
…ntity at each stop-path signal Address both review findings on the initial identity fix: the tombstoned descriptor adoption path in adoptOrRecoverWorker sent an unconditional SIGKILL to the descriptor's pid before stopWorker ran, so a recycled pid could still be signalled during supervisor restart -- the exact incident path. It now checks workerDescriptorProcessIdentityMatches first. stopWorker also no longer caches the identity verdict in a boolean computed before the shutdown request: the worker can exit during the graceful wait and its pid be reassigned, in which case the cached "verified" flag would have followed the replacement process into SIGKILL. isWorkerProcessAlive now re-reads the process start id on every liveness check, immediately before each signal, narrowing the check-to-signal race to the kill(2) call itself. The stop-identity regression test now also covers the adoption path with a tombstoned descriptor and asserts the bystander process survives and the worker does not end up lifecycle "failed".
|
@codex review Summary of addressed findingsBoth P1 findings from the previous round have been addressed in commit fed4fd4: 1. Guard the adoption pre-kill with the identity check
2. Revalidate process identity before subsequent signalsThe cached Verification
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fed4fd4c76
ℹ️ 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".
… to suite/regressions Move the process identity and heartbeat aggregation regression tests to the prescribed location under test/suite/regressions/ with the required <issue-number>-<slug>.test.ts naming convention. This consolidates all PrimeIntellect-ai#1045 regressions into a single file: - stopWorker identity verification for recycled pids - adoptOrRecoverWorker pre-kill identity guard - heartbeats_list skipping of failed workers
Tests moved to test/suite/regressions/1045-failed-worker-poison.test.ts
Test moved to test/suite/regressions/1045-failed-worker-poison.test.ts
|
@codex review Round 2 - Addressed FindingMove issue regressions to suite/regressionsFixed in 50ffa44. Moved all three regression tests to
The standalone Verification
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ffa44c24
ℹ️ 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".
…able When a descriptor has a recorded processStartId but getProcessStartId() returns undefined (e.g., /proc unavailable, ps failures), we now refuse to signal rather than assuming the pid belongs to our worker. This prevents accidentally signalling an unrelated process when the identity verification system itself cannot function. Old workers without processStartId remain trusted for backward compatibility. The three conditions in workerDescriptorProcessIdentityMatches now: 1. No recorded ID → trust (backward compat with old descriptors) 2. Process dead → harmless to proceed (signal will fail with ESRCH) 3. Have recorded ID but can't observe → REFUSE (can't verify = don't signal) 4. IDs match → signal 5. IDs don't match → don't signal
|
@codex review Round 3 - Findings Addressed/Declined1. Refuse to signal when process identity is unverifiable (FIXED)Fixed in 7dc2817. 2. Use the required suite harness (DECLINED)The suite harness ( Verification
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dc2817dc1
ℹ️ 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".
Split process identity checks into two functions with different semantics: 1. workerDescriptorProcessIdentityMatches() - for signaling decisions Returns true only for verified 'match', false for 'mismatch' or 'unverifiable'. Used before SIGTERM/SIGKILL to avoid signalling wrong processes. 2. workerDescriptorProcessMightBeAlive() - for cleanup decisions Returns true for 'match' or 'unverifiable', false only for verified 'mismatch'. Used in wait loops and final liveness check to avoid deleting descriptors of possibly-live workers. This fixes the issue where returning false for unverifiable identity caused stopWorker to skip the wait loop and delete the worker as though it had exited, leaving live workers untracked. The underlying workerDescriptorProcessIdentityCheck() returns a tri-state: - 'match': verified, process is ours - 'mismatch': verified, process is NOT ours (recycled pid) - 'unverifiable': can't check (old descriptor or platform limitation)
|
@codex review Round-3 findings addressed: aa3ae8f splits process-identity checking into tri-state (match/mismatch/unverifiable) with separate signaling and cleanup predicates, and the suite-harness relocation was declined with reasoning (OS-level process signaling tests, matching the custom-harness precedent of the 4600-series daemon tests). Please re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa3ae8fc06
ℹ️ 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".
…rendering The portable start-time listing (ps -o lstart=) renders local time, so the same live worker produced a different identity when the supervisor restarted under a different TZ or locale - the mismatch was read as PID reuse, adoption skipped signalling, and the descriptor of a live worker was deleted, leaving it untracked. The process query now pins TZ=UTC and LC_ALL=C, and the token format is versioned (ps2:) so a legacy ps: token recorded by an older build degrades to the unverifiable tri-state instead of a false mismatch: the worker stays tracked and unsignalled until same-format evidence exists. Same-format inequality remains a trusted mismatch. Red-green verified: without the cross-format degradation, the legacy-token test observes the live worker's descriptor being reaped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43556ee4f9
ℹ️ 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".
# Conflicts: # packages/coding-agent/CHANGELOG.md
|
Thank you for the report and proposed work. This root cause is now covered by maintainer-owned stacked PR #1161, authored independently from We did not inspect or reuse this PR's diff, branch, commits, implementation code, or tests; its public description/comments were used only as a bug report. To keep one review surface, this PR is superseded by #1161 and is being closed. The complete review stack is #1158–#1165. It is being left unmerged for human review after CI and review-bot findings are cleared. |
2 participants
Closes #1045.
What happened
A single worker descriptor stuck at
lifecycle: "failed"made every globalheartbeats_listfail withCannot list heartbeats while session worker is failed, while session-scoped listings kept working. Observed live on v0.7.1 (macOS); full causal chain in #1045.Two defects combined:
stopWorkerfalse-alive on pid reuse. The stop path's liveness probe iskill(pid, 0)withEPERMtreated as alive, and it never consults the descriptor's recordedprocessStartId(the recovery path already does, viaprocessIdentityMatches). After a worker had genuinely exited (shutting down (exit 0)in the supervisor log), an unrelated process holding its old pid answeredEPERM, so the supervisor signalled the imposter's process group, waited out both the graceful and SIGKILL deadlines, and persistedlifecycle: "failed"+lastError: "did not stop after SIGKILL"— surviving supervisor restarts.heartbeats_listfan-out filters withisVisibleWorker(which only excludes client-owned workers) and returns the first failure snapshot as the entire command's response. A failed worker with no client and no cached snapshot therefore failed the whole listing indefinitely.Fix
stopWorkercomputesworkerProcessIdentityVerifiedup front (before asking the worker to shut down, while the pid should still be the worker's) using a newworkerDescriptorProcessIdentityMatches()helper that comparesgetProcessStartId(pid)against the descriptor. An identity mismatch means the worker process is already gone: nothing is signalled and the stop completes instead of throwing. When the descriptor has noprocessStartId, or the observed start id cannot be read, behavior is unchanged (conservative). This mirrorsisLeaseOwnerAlive()insession-lease.tsand is the same false-alive pattern as Session lease liveness check fails on Android/Termux: dead pid returns EPERM → permanent SessionAlreadyActiveError #868, at a second call site.heartbeats_listskipslifecycle === "failed"workers in the global fan-out — they cannot host firing heartbeats, and their absence should not fail the listing for every healthy worker.Tests
daemon-supervisor-heartbeats.test.ts: new case — a failed worker alongside a healthy one yields a successful listing with the healthy worker's heartbeats, and the failed worker is never forwarded to.daemon-supervisor-stop-identity.test.ts(new): spawns a detached bystander process to stand in for a recycled pid, gives the descriptor a mismatchedprocessStartId, and assertsstopWorker(force)completes, removes the worker, and leaves the bystander unsignalled.Both new tests fail on unfixed
main(the stop test previously SIGTERM/SIGKILLed the innocent bystander) and pass with the fix.Verified locally: the targeted files plus all 25
daemon-*.test.ts(minusdaemon-supervisor-process) +session-lease.test.ts— 501 tests passing; repocheck(biome, tsgo, installer render, browser smoke) passes via the pre-commit hook.daemon-supervisor-process.test.tsfails identically on my machine on cleanmain(its spawned daemons collide with my live daemon in~/.prime/agent), so I'm relying on CI for that job.Field validation of the underlying diagnosis
On the affected machine, sending
retry_workerfor the stuck descriptor's root session over the daemon socket recovered the worker (lifecycle: ready, fresh pid) and globalheartbeats_listimmediately returnedsuccess: true— confirming the failed descriptor was the only thing breaking the listing.Note
Fix process identity comparison in coding-agent daemon to prevent false stale detection and unsafe signaling
compareProcessStartIdsinsession-lease.tsreturning'match' | 'mismatch' | 'unverifiable'; callers now treat only'mismatch'as a confirmed identity change, and'unverifiable'(e.g. cross-format comparisons) as safe to continue.ps-based process start ID prefix fromps:tops2:and pinsTZ=UTC/LC_ALL=Cwhen queryingps, making tokens stable across locale and timezone changes.compareProcessStartIdsacrossstopWorker,adoptOrRecoverWorker,isLeaseOwnerAlive, and related helpers so signals are only sent to verified-matching PIDs.failed-lifecycle workers from theheartbeats_listaggregation in the daemon supervisor to prevent command failures caused by dead workers.ps:-format tokens will compare as'unverifiable'against newps2:tokens, so live daemons and leases recorded before upgrading will not be killed or reclaimed on the first restart.Macroscope summarized b24bd42.