Repository navigation
[AI-2857] Spawn detached children without inheriting the agent's handles - #978
Conversation
CreateProcess inherits every handle marked inheritable, and an agent invokes its hook holding more of them than GetStdHandle reports, so clearing the three std handles left a detached child holding the agent's stdout pipe for its whole lifetime. Only bInheritHandles: false cuts it, and Process cannot ask for that. The session-end hand-off keeps the old spawn: it passes its payload through the child's stdin, and a pipe reaches a child only by being inherited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoPrevent detached Windows children from inheriting agent handles
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /// <c>CreateProcess</c> inherits every one of them. Measured against Codex: a watcher | ||
| /// spawned from a hook held the agent's stdout pipe open for the watcher's whole | ||
| /// lifetime, so the agent's read of the hook's stdout never reached EOF and it abandoned | ||
| /// SessionStart and Stop at its hook timeout, ~33s apiece, on every turn. The std-handle |
There was a problem hiding this comment.
1. Hook comments preserve stale run timing 📘 Rule violation ⚙ Maintainability
StartDetachedWindows's remarks cite a Codex measurement and a roughly 33-second timeout on every turn instead of limiting the explanation to the handle-inheritance invariant. A change to Codex's timeout or hook behavior leaves this empirical narrative inaccurate even though the implementation rationale remains valid.
Agent Prompt
## Issue description
The `StartDetachedWindows` remarks embed a vendor-specific measurement and approximate timeout that can become stale independently of the handle-inheritance invariant.
## Fix Focus Areas
- src/Capacitor.Cli/ProcessHelpers.WindowsSpawn.cs[21-26]
## Recommended Fix
Remove the Codex run narrative and timing. Retain a concise explanation that inheritable pipe handles can prevent hook output from reaching EOF and that `bInheritHandles: false` prevents this.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| namespace Capacitor.Cli; | ||
|
|
||
| static partial class ProcessHelpers { |
There was a problem hiding this comment.
3. Windows spawn file breaks type naming 📘 Rule violation ⚙ Maintainability
ProcessHelpers.WindowsSpawn.cs declares ProcessHelpers as its sole top-level primary type, so the file base name does not exactly match the type name. Future additions can follow either ProcessHelpers.cs or the suffixed file pattern, leaving ownership of the partial type ambiguous.
Agent Prompt
## Issue description
The new file's base name does not match its sole top-level primary type, contrary to the repository's one-primary-type naming convention.
## Fix Focus Areas
- src/Capacitor.Cli/ProcessHelpers.WindowsSpawn.cs[7-7]
- src/Capacitor.Cli/SystemProcessStarter.cs[19-20]
## Recommended Fix
Move the Windows implementation into a dedicated type such as `WindowsProcessStarter` in `WindowsProcessStarter.cs`, retain the native declarations as its nested implementation details, and update the call and documentation references in `SystemProcessStarter`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The payload reaches the continuation over a pipe, which a child can only receive by inheritance, so this spawn names that one handle in the child's inherit list instead of refusing all of them. Two smaller cuts ride along: the Unix detached start disposes its Process wrapper, and a refused CreateProcessW carries the OS reason out to the caller's log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The caller is handed a stream and no other reference, so the stream closes the Process wrapper on Unix and owns the pipe handle on Windows. Ownership moves to the SafeFileHandle before the FileStream is built: a throwing constructor would otherwise leave the handle value with two owners, and the second close could land on whatever Windows had reused it for. An attribute list is deleted only once initialization has actually succeeded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pipe close can fail with buffered bytes and a child that already exited, so the owner is disposed from a finally and a gate keeps either disposal form from releasing twice. The lifetime test drives the child's real stdin, because a wrapper built over an unrelated stream leaves Process.Close to shut stdin itself and the child then races the assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A real FileStream reports nothing about how it was closed and tolerates repeated closes, so tests built on one stay green with the release gate removed. The double counts sync and async disposal and can fail its close, which is what distinguishes released-once from released-twice and proves the owner is freed when the pipe is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each disposal form has its own finally, and the sync test leaves the async one free to drop the owner when an async pipe close throws. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@realtonyyoung ready for review — out of draft. An independent Codex review flow ran this to sign-off over five rounds. It returned What it found, in order
Findings 4–8 are all defects in fixes for earlier findings, which is the part worth knowing: the first round's fix for the P1 introduced its own leak, and two rounds of tests didn't pin what they claimed to. Verification
Two things to know before you read it
|
|
Code review by qodo was updated up to the latest commit 9c0aaf3 |
|
I reviewed the latest diff by inspection. Two production concerns remain: on Unix, |
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
Requesting changes for the detached auto-sync stderr race and detached-child PID ownership on failure; the fake-starter fallback test also needs cleanup. Details are in my PR comment and existing inline threads. Builds and tests were not run.
The hook that spawns it closes the pipe read ends as it exits, so a write after that lands on a dead fd and can take the sync down. Config and profile resolution both sit between startup and dispatch, and both can warn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pid is reusable the moment the child exits, so failure cleanup that re-resolved one could kill an unrelated process. The child now carries the handle it was created with — the process wrapper on Unix, the CreateProcess handle on Windows — and every kill goes through that. Terminating a child the starter cannot hand back is the same rule seen from the other side: the caller never receives it, so nothing else could stop it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@realtonyyoung all three addressed, pushed as 1. Auto-sync dying on warnings — fixed, and you're right that this PR caused it. The regression is visible in the diff: the old code kept the child's stdout/stderr open and drained them with The child now nulls its writers at startup, before config and profile resolution can emit anything. The dispatch-site silencing is gone rather than duplicated. 2. PID-reuse kill — fixed by removing the pid re-acquisition entirely. New This also let 3. Fake starter leaving a child alive — already fixed before your review, and it was a real orphan. Linux caught it: On LinuxYou noted you didn't build or run. I did, in a .NET 10 container, because this PR's Unix paths have two tests that Windows always skips.
One thing worth flagging since it is not caused by this PR but is close to the edge: that integration test asserts the hook returns in under 1s, against Claude's real 1.5s kill. In a loaded container the baseline Still open, unchangedThe branch does not build without |
|
NO FINDINGS on the updated head. Auto-sync now silences output before profile resolution can warn; the detached child retains its original process handle for failure cleanup; and the fake starter cleans up a child it cannot hand back. I reviewed the changed production and test code by inspection and did not build or run tests. |
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
The detached-child and early-output findings from my previous review are addressed. No further actionable findings by inspection; no build or tests run.
|
CI is green — and a correction to something I said earlier. Correction: the
|
2 participants
AI-2857 — no GitHub issue; this repo's issues are tracked in Linear only.
What & why
On Windows a child spawned from a coding-agent hook inherits the agent's stdout pipe and holds it for its whole lifetime, so the agent's read of the hook's stdout never reaches EOF and it abandons the hook at its timeout.
CreateProcessinherits every handle marked inheritable, and an agent invokes its hook holding more of them thanGetStdHandlereports, so clearing those three cannot close it.IProcessStartergainsStartDetached, which on Windows goes throughCreateProcesswithbInheritHandles: false— the only cut that holds, and oneSystem.Diagnostics.Processoffers no way to ask for. Unix keeps the fd >= 3 CLOEXEC sweep, which closes its own differently-shaped leak.Where to look
ClaudeSessionEndHandoffdeliberately keeps the old spawn: it hands its payload to the child through stdin, and on Windows a pipe only reaches a child by being inherited. Closing that one needs the spawn to pass an explicitPROC_THREAD_ATTRIBUTE_HANDLE_LISTnaming just that pipe.Verification
Live Codex (0.154.0) against a pristine
origin/maincontrol worktree — same machine, same harness, each hook wrapped in a shim timestamping entry and exit, so hook runtime is separated from what the agent does after it exits.origin/mainCompleted; 15.4 s totalA watcher spawned by the patched binary was confirmed alive afterwards, so capture still works rather than the spawn silently failing.
Unit suite: 4273 passed, 3 failed — the three
IsExcluded_*symlink tests,A required privilege is not held by the client(Windows symlink privilege), which fail the same way onorigin/main.🤖 Generated with Claude Code