[stack 4/8] fix(daemon): fence worker and supervisor lifecycle state - #1161
[stack 4/8] fix(daemon): fence worker and supervisor lifecycle state#1161sethkarten wants to merge 11 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f9f6348. Configure here.
| } catch (error) { | ||
| this.log(`could not verify supervisor launch lock: ${String(error)}`); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Launch lock false release race
High Severity
assertCurrentLockGeneration treats lockSync guard contention as generation loss and returns false, while the refresh path retries the same error. Callers then abort and finally still runs removeSupervisorLaunchLockGeneration, so a live holder can delete its own valid lock while a replacement may already be spawning. Concurrent workers noticing a dead supervisor can then launch a second supervisor or leave recovery abandoned.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f9f6348. Configure here.
f9f6348 to
e669d07
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const now = Date.now(); | ||
| if (forceIdentityCheck || lastStatus === undefined || now >= nextIdentityCheckAt) { | ||
| nextIdentityCheckAt = now + identityPollIntervalMs; | ||
| const observedProcessStartId = getProcessStartId(descriptor.pid); |
There was a problem hiding this comment.
🟠 High daemon/daemon-supervisor.ts:549
createWorkerProcessIdentityProbe can return "match" for a PID that has been reused by an unrelated process. On macOS/BSD, getProcessStartId falls back to ps -o lstart, which has whole-second precision. If a worker exits and another process reuses the same PID within that same second, the observed start-time strings are equal, so status() returns "match" and isAlive() returns true. Callers then treat the unrelated process as the original worker and may send SIGTERM/SIGKILL to it. Consider treating a coarse ps:-based identity as "unknown" rather than "match", or using a higher-resolution identity source so PID reuse within the same second is detected.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/daemon/daemon-supervisor.ts around line 549:
`createWorkerProcessIdentityProbe` can return `"match"` for a PID that has been reused by an unrelated process. On macOS/BSD, `getProcessStartId` falls back to `ps -o lstart`, which has whole-second precision. If a worker exits and another process reuses the same PID within that same second, the observed start-time strings are equal, so `status()` returns `"match"` and `isAlive()` returns `true`. Callers then treat the unrelated process as the original worker and may send `SIGTERM`/`SIGKILL` to it. Consider treating a coarse `ps:`-based identity as `"unknown"` rather than `"match"`, or using a higher-resolution identity source so PID reuse within the same second is detected.
| if ((reclaimError as NodeJS.ErrnoException).code !== "ENOENT") { | ||
| throw reclaimError; | ||
| return current; | ||
| } catch (error) { |
There was a problem hiding this comment.
This catch path gives a registry-guard error the same false result as proven generation loss. A transient contention or I/O error can therefore abort launch, after which finally may remove the still-valid generation.
Could this return distinct current, lost, and unknown/error outcomes, with bounded retry for the last case, and only release after a guarded read proves ownership loss?
| lastStatus = | ||
| observedProcessStartId === undefined | ||
| ? "unknown" | ||
| : observedProcessStartId === descriptor.processStartId |
There was a problem hiding this comment.
The portable ps -o lstart identity has whole-second precision, but equality here is treated as sufficient authorization for later SIGTERM/SIGKILL paths. If a PID is reused within that second, an unrelated process can compare as match.
Could ps: identities be treated as coarse and fail closed for destructive signalling, with a regression covering same-second PID reuse?
2 participants
Stack 4/8 — fix(daemon): fence worker and supervisor lifecycle state
Base:
stack/external-03-persistenceReview order: merge only after the preceding stack layer is accepted. This PR is not intended to merge independently out of order.
Stack navigation
Summary
Validation
npm run check; final focused daemon/ownership/heartbeat/session-lease suite 34/34.Provenance
upstream/mainusing issue reports and PR descriptions/comments only.Linked-item disposition
Fixed on merge
Independently superseded pull requests
Reviewer notes
main, to avoid cumulative duplicate diffs.