fix(process-manager): don't attribute a killed process's exit to its replacement - #1302
Open
pedramamini wants to merge 1 commit into
Open
fix(process-manager): don't attribute a killed process's exit to its replacement#1302pedramamini wants to merge 1 commit into
pedramamini wants to merge 1 commit into
Conversation
…replacement ProcessManager.spawn() kills whatever process holds a sessionId before the spawner registers the replacement under that same key. The killed process keeps draining stdio and fires `close` (or PTY `onExit`) afterwards, and every downstream handler is keyed by sessionId alone - so those late events landed on the live successor: - its exit code (143 after SIGTERM) was reported through the successor's parser as `Agent exited with code 143`, flagging a healthy turn as crashed - `ExitHandler` deleted the successor's tracking entry, orphaning a running process the user could no longer stop or interrupt - late stdout/stderr from the dead turn was appended to the live turn's buffers The renderer's existing "is the process still running?" guard couldn't catch this, because the entry had already been deleted by the time it looked. Each spawner now captures its own ManagedProcess and drops stdio/exit/error events once a different generation owns the key (a *missing* entry is not superseded - that's the ordinary post-kill path whose exit event the renderer needs to settle the tab). ExitHandler re-checks identity before its final emit and delete, covering the window that opens while it awaits Copilot's on-disk shutdown reconciliation. Mirrors the identity check OpencodeServerSpawner already does.
📝 WalkthroughWalkthroughChangesSuperseded process generation handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds process-generation identity checks to prevent replaced child and PTY processes from affecting their successors.
Confidence Score: 2/5This PR should not merge until predecessor side effects are blocked immediately after asynchronous reconciliation and supersession remains detectable after a replacement exits. The new checks prevent the originally reported ordering while a replacement remains tracked, but exit handling can still mutate replacement state before its final check, and old callbacks become eligible again after the replacement removes the shared map entry. Files Needing Attention: src/main/process-manager/handlers/ExitHandler.ts, src/main/process-manager/spawners/ChildProcessSpawner.ts, src/main/process-manager/spawners/PtySpawner.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Old as Predecessor
participant PM as Process Manager
participant New as Successor
participant EH as Exit Handler
participant UI as Event Consumers
Old->>EH: close begins exit handling
EH->>EH: await shutdown reconciliation
PM->>PM: delete old map entry
PM->>New: spawn replacement
PM->>PM: map sessionId to successor
EH->>UI: post-exit data/error/usage
EH->>EH: check generation identity
EH-->>UI: suppress final exit
Reviews (1): Last reviewed commit: "fix(process-manager): don't attribute a ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // and the delete would orphan a running process. Callers already screen | ||
| // out replaced generations before entering; this re-checks the window | ||
| // that opened while we awaited. | ||
| if (this.processes.get(sessionId) !== managedProcess) { |
There was a problem hiding this comment.
When a replacement claims the same session ID during Copilot shutdown reconciliation, post-exit processing resumes before this identity check and writes the predecessor's final output, errors, usage, and completion events through the shared session ID. It can also flush the successor's data buffer, causing the live replacement to receive stale state from the exited process.
Knowledge Base Used: Process Manager
Comment on lines
+478
to
+481
| const isSuperseded = (): boolean => { | ||
| const current = this.processes.get(sessionId); | ||
| return current !== undefined && current !== managedProcess; | ||
| }; |
There was a problem hiding this comment.
When the replacement finishes and removes its map entry before the killed predecessor finishes draining, this check starts treating the predecessor as current again. Its late output is forwarded and its SIGTERM close emits a second exit, causing stale output and duplicate renderer or Cue completion effects. The same missing-entry behavior is present in PtySpawner.
Knowledge Base Used: Process Manager
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/process-manager/spawners/ChildProcessSpawner.ts (1)
567-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuperseded predecessors now skip temp-image cleanup.
cleanupTempFiles(managedProcess.tempImageFiles)only runs insidehandleExit/handleError. Returning early here leaves the predecessor's temp image files on disk for the rest of the app session. Consider cleaning them in the superseded branch.♻️ Cleanup on the superseded path
childProcess.on('close', (code) => { if (isSuperseded()) { logger.warn('[ProcessManager] Ignoring exit from superseded process', 'ProcessManager', { sessionId, pid: childProcess.pid, exitCode: code, }); + if (managedProcess.tempImageFiles?.length) { + cleanupTempFiles(managedProcess.tempImageFiles); + } return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/process-manager/spawners/ChildProcessSpawner.ts` around lines 567 - 593, Ensure superseded processes still clean up their temporary image files before returning from the close and error handlers. In the isSuperseded() branches within the childProcess close/error listeners, invoke cleanupTempFiles with the managed process’s tempImageFiles, while preserving the existing warning logs and early-return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/process-manager/handlers/ExitHandler.ts`:
- Around line 258-271: Update the post-await generation check in the
exit-handling flow around managedProcess so it suppresses the exit event only
when sessionId maps to a different existing process; treat a missing
this.processes entry as non-superseded and continue final exit emission.
Preserve the warning and early return for an actual replacement.
---
Nitpick comments:
In `@src/main/process-manager/spawners/ChildProcessSpawner.ts`:
- Around line 567-593: Ensure superseded processes still clean up their
temporary image files before returning from the close and error handlers. In the
isSuperseded() branches within the childProcess close/error listeners, invoke
cleanupTempFiles with the managed process’s tempImageFiles, while preserving the
existing warning logs and early-return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a52bd33-dd8a-499f-af8f-76feed824799
📒 Files selected for processing (6)
src/__tests__/main/process-manager/handlers/ExitHandler.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/__tests__/main/process-manager/spawners/PtySpawner.test.tssrc/main/process-manager/handlers/ExitHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/spawners/PtySpawner.ts
Comment on lines
+258
to
+271
| // `awaitCopilotShutdown` above can park this handler for seconds, long | ||
| // enough for the session to be re-spawned under the same key. Emitting | ||
| // then would settle the successor's turn with this process's exit code, | ||
| // and the delete would orphan a running process. Callers already screen | ||
| // out replaced generations before entering; this re-checks the window | ||
| // that opened while we awaited. | ||
| if (this.processes.get(sessionId) !== managedProcess) { | ||
| logger.warn( | ||
| '[ProcessManager] Session re-spawned during exit handling, suppressing exit event', | ||
| 'ProcessManager', | ||
| { sessionId, code } | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every site that deletes a ProcessManager tracking entry, to see whether any
# delete happens without emitting 'exit' (which the new guard would then swallow).
rg -nP -C4 "processes\.delete\(" --type=ts -g '!**/__tests__/**'Repository: RunMaestro/Maestro
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching ExitHandler.ts =="
fd -a 'ExitHandler\.ts$' . || true
echo "== git diff stat =="
git diff --stat || true
echo "== git diff relevant excerpt =="
git diff -- src/main/process-manager/handlers/ExitHandler.ts | sed -n '1,220p' || true
echo "== outline ExitHandler =="
ast-grep outline src/main/process-manager/handlers/ExitHandler.ts 2>/dev/null || true
echo "== ExitHandler lines 200-320 =="
sed -n '200,320p' src/main/process-manager/handlers/ExitHandler.ts | nl -ba -v200Repository: RunMaestro/Maestro
Length of output: 743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ExitHandler lines 1-360 =="
sed -n '1,360p' src/main/process-manager/handlers/ExitHandler.ts
echo "== processes.delete occurrences (all ts files) =="
rg -n "processes\.delete\(|\.delete\(" src/main/process-manager -g '*.ts' || true
echo "== process manager handlers =="
find src/main/process-manager -maxdepth 3 -type f -name '*.ts' | sortRepository: RunMaestro/Maestro
Length of output: 15737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProcessManager.ts around processes.delete at 300-340 =="
sed -n '280,340p' src/main/process-manager/ProcessManager.ts | nl -ba -v280
echo "== ExitHandler.ts around final delete at 420-475 =="
sed -n '420,475p' src/main/process-manager/handlers/ExitHandler.ts | nl -ba -v420
echo "== PtySpawner.ts around processes.delete at 190-235 =="
sed -n '190,235p' src/main/process-manager/spawners/PtySpawner.ts | nl -ba -v190
echo "== Behavioral probe for the identity guard =="
python3 - <<'PY'
class ManagedProcess:
name = 'prev'
processes = {'session': ManagedProcess()}
managed_process = processes['session']
def current_process():
p = processes.get('session')
return p is managed_process
processes['session'] = None
print('current after removed =', current_process())
processes.clear()
print('current after deleted =', current_process())
processes['session'] = ManagedProcess()
print('current after replaced =', current_process())
PYRepository: RunMaestro/Maestro
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProcessManager.ts around processes.delete at 300-340 =="
sed -n '280,340p' src/main/process-manager/ProcessManager.ts
echo "== ExitHandler.ts around final delete at 420-475 =="
sed -n '420,475p' src/main/process-manager/handlers/ExitHandler.ts
echo "== PtySpawner.ts around processes.delete at 190-235 =="
sed -n '190,235p' src/main/process-manager/spawners/PtySpawner.ts
echo "== Behavioral probe for the identity guard =="
python3 - <<'PY'
class ManagedProcess:
name = 'prev'
processes = {'session': ManagedProcess()}
managed_process = processes['session']
def current_process():
p = processes.get('session')
return p is managed_process
processes['session'] = None
print('current after removed =', current_process()),
processes.clear()
print('current after deleted =', current_process()),
processes['session'] = ManagedProcess()
print('current after replaced =', current_process())
PYRepository: RunMaestro/Maestro
Length of output: 4818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProcessManager.ts top/middle and isSuperseded references =="
sed -n '1,120p' src/main/process-manager/ProcessManager.ts
echo "== isSuperseded and set/delete references =="
rg -n "isSuperseded|processes\.set\(|processes\.delete\(|processes\.clear\(" src/main/process-manager/ProcessManager.ts -g '*.ts'
echo "== ProcessManager kill/remove context =="
sed -n '220,350p' src/main/process-manager/ProcessManager.ts
echo "== ChildProcessSpawner onExit semantics =="
sed -n '1,280p' src/main/process-manager/spawners/ChildProcessSpawner.tsRepository: RunMaestro/Maestro
Length of output: 19941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProcessManager.ts lines 120-250 =="
sed -n '120,250p' src/main/process-manager/ProcessManager.ts
echo "== PtySpawner.ts lines 1-140 =="
sed -n '1,140p' src/main/process-manager/spawners/PtySpawner.tsRepository: RunMaestro/Maestro
Length of output: 8867
Don’t suppress exit events for removed tracking entries.
If kill() removes the entry while awaitCopilotShutdown is already parked, this.processes.get(sessionId) !== managedProcess also returns true and skips the final exit emission, leaving the tab’s process state unsettled. Treat a missing map entry as non-superseded the same way the PTY spawner does.
🔧 Align with the spawner semantics
- if (this.processes.get(sessionId) !== managedProcess) {
+ const current = this.processes.get(sessionId);
+ if (current !== undefined && current !== managedProcess) {
logger.warn(
'[ProcessManager] Session re-spawned during exit handling, suppressing exit event",
{ sessionId, code }
);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // `awaitCopilotShutdown` above can park this handler for seconds, long | |
| // enough for the session to be re-spawned under the same key. Emitting | |
| // then would settle the successor's turn with this process's exit code, | |
| // and the delete would orphan a running process. Callers already screen | |
| // out replaced generations before entering; this re-checks the window | |
| // that opened while we awaited. | |
| if (this.processes.get(sessionId) !== managedProcess) { | |
| logger.warn( | |
| '[ProcessManager] Session re-spawned during exit handling, suppressing exit event', | |
| 'ProcessManager', | |
| { sessionId, code } | |
| ); | |
| return; | |
| } | |
| // `awaitCopilotShutdown` above can park this handler for seconds, long | |
| // enough for the session to be re-spawned under the same key. Emitting | |
| // then would settle the successor's turn with this process's exit code, | |
| // and the delete would orphan a running process. Callers already screen | |
| // out replaced generations before entering; this re-checks the window | |
| // that opened while we awaited. | |
| const current = this.processes.get(sessionId); | |
| if (current !== undefined && current !== managedProcess) { | |
| logger.warn( | |
| '[ProcessManager] Session re-spawned during exit handling, suppressing exit event', | |
| 'ProcessManager', | |
| { sessionId, code } | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/process-manager/handlers/ExitHandler.ts` around lines 258 - 271,
Update the post-await generation check in the exit-handling flow around
managedProcess so it suppresses the exit event only when sessionId maps to a
different existing process; treat a missing this.processes entry as
non-superseded and continue final exit emission. Preserve the warning and early
return for an actual replacement.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a process generation race in ProcessManager where late stdout/stderr/exit events from a killed predecessor could be mis-attributed to a newly spawned successor that reuses the same sessionId, leading to incorrect exit reporting and orphaning a live process.
Changes:
- Add per-spawn identity checks in
ChildProcessSpawnerandPtySpawnerto ignore late events once a differentManagedProcessowns thesessionId. - Add a final identity re-check in
ExitHandler.handleExit()to avoid emittingexitand deleting the process entry if a re-spawn occurred while exit handling awaited reconciliation work. - Add regression tests covering superseded-generation stdout/stderr/exit behavior across both spawner types and within
ExitHandler.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/process-manager/spawners/PtySpawner.ts | Ignores PTY onData and onExit events from superseded generations to prevent mis-attribution and accidental deletion of a successor entry. |
| src/main/process-manager/spawners/ChildProcessSpawner.ts | Ignores stdout/stderr/close/error events from superseded generations to avoid attributing predecessor output and exit to a successor. |
| src/main/process-manager/handlers/ExitHandler.ts | Re-checks process identity before emitting exit and deleting the map entry to cover re-spawn during async exit reconciliation. |
| src/tests/main/process-manager/spawners/PtySpawner.test.ts | Adds regression tests verifying late PTY events from a superseded generation are ignored and do not delete the successor entry. |
| src/tests/main/process-manager/spawners/ChildProcessSpawner.test.ts | Adds regression tests verifying late stdio/close/error from a superseded generation are ignored and do not orphan the successor. |
| src/tests/main/process-manager/handlers/ExitHandler.test.ts | Adds a regression test verifying handleExit suppresses the exit event and leaves the successor tracked if re-spawn occurs mid-handler. |
Comments suppressed due to low confidence (1)
src/tests/main/process-manager/spawners/ChildProcessSpawner.test.ts:685
- This newly added comment uses an em-dash (—), which this repo explicitly disallows in any text. Replace it with a plain hyphen or rephrase.
// handleExit is async (post-exit reconciliation) — let it settle.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Regression (issue #1044): ProcessManager.spawn() kills the process holding a | ||
| // sessionId before registering the replacement under the same key. The killed | ||
| // process drains stdio and fires `close` afterwards, and every downstream | ||
| // handler is keyed by sessionId alone — so those late events used to be |
Comment on lines
+628
to
+644
| it('ignores a late close from the killed predecessor', async () => { | ||
| const { emitter, processes, config, firstHandlers, second } = spawnTwoGenerations(); | ||
| const onExit = vi.fn(); | ||
| const onAgentError = vi.fn(); | ||
| emitter.on('exit', onExit); | ||
| emitter.on('agent-error', onAgentError); | ||
|
|
||
| // Predecessor finally reports its SIGTERM death (128 + 15). | ||
| firstHandlers.get('close')?.(143); | ||
| // handleExit is async — give the (suppressed) exit path room to run. | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
|
|
||
| expect(onExit).not.toHaveBeenCalled(); | ||
| expect(onAgentError).not.toHaveBeenCalled(); | ||
| // The live process stays tracked, so the user can still stop it. | ||
| expect(processes.get(config.sessionId)).toBe(second); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #1044
The report
A session died with exit code 143 (SIGTERM) right after the agent ran a harmless single-file
grepwhose pattern contained credential-ish keywords. The reporter's hypothesis was a secret-detection guardrail firing SIGTERM. There is no such guardrail in Maestro - the real cause is a process-generation race, which also explains why it fired once and never reproduced.Root cause
ProcessManager.spawn()kills whatever process currently holds asessionIdbefore the spawner registers the replacement under that same key:The killed process does not disappear instantly. It drains stdio and fires
close(PTY:onExit) a few hundred ms later - after the successor has claimed the key. Every downstream handler is keyed bysessionIdalone, so those late events were attributed to the live successor:ExitHandler.handleExit()looked up the successor's managed process and randetectErrorFromExit(143, ...)against it, emittingagent-error-> "Agent exited with code 143" on a turn that was running fine.emit('exit')andprocesses.delete(sessionId), orphaning the live process - untracked, so the user can no longer stop or interrupt it.The renderer's existing "verify the process is actually gone" guard in
useAgentExitListenercan't catch this: the entry was already deleted byhandleExit, sogetActiveProcesses()reports it gone and the exit is applied.The reporter's attached debug package shows the trigger firing twice in the captured window:
Timing-dependent on a sub-second window, which matches "happened once, never recurred". The credential keywords are a coincidence.
Fix
Each spawner captures its own
ManagedProcessand ignores stdio/close/erroronce a different generation owns the key. A missing entry is deliberately not treated as superseded - that's the ordinary post-kill path, and the renderer needs that exit event to settle the tab.ChildProcessSpawner- guards stdout, stderr,close,error.PtySpawner- guardsonData,onExit(and no longer deletes a successor's entry).ExitHandler.handleExit()- re-checks identity before its finalemit('exit')+delete, covering the window that opens while it awaits Copilot's on-disk shutdown reconciliation.This mirrors the identity check
OpencodeServerSpawneralready performs (if (this.processes.get(sessionId) === managedProcess)).Scope note: this fixes the mis-attribution, not the re-spawn itself. A spawn landing on a busy key is still logged as a warning, as before.
Tests
7 new regression tests, each verified red before the fix and green after:
ChildProcessSpawner.test.ts- lateclose/error/stdio from a superseded generation emit nothing and leave the successor tracked; the current generation still reports exit normally.PtySpawner.test.ts- same foronExit/onData.ExitHandler.test.ts- a re-spawn during exit handling suppresses the exit event and leaves the successor tracked.npx vitest run src/__tests__/main- 266 files, 8043 passed.npm run lintand eslint clean.Summary by CodeRabbit