fix(shell): stop shell_execute hang on background children - #1887
Conversation
Adversarial review — non-author agentVerdict: APPROVE WITH NITS. A non-author agent reviewed this change in an isolated worktree. The agent tried to break the fix. Proof points:
Open nits (minor; deferred to the upstream decision):
This PR is a do-not-merge focus PR for cherry-pick evaluation. |
|
This change fixes the silent-cut finding from the adversarial review. The post-exit grace window can cut the output drain before EOF. A background child can hold the pipe open after the direct process exits. The tool did not report this cut before now.
Tests prove both cases. A normal command that reaches EOF shows no note. A backgrounded child that outlives the direct process shows the note. The full Commit: 11562b9 |
A command that daemonizes leaves shell_execute stuck. The direct process exits at once. A forked or backgrounded child keeps the stdout/stderr pipe open. The tool waited for that pipe to close, with no bound on the wait. The output drain read the pipe with CancellationToken.None. A real token could not stop the read. The tool then hung until the child exited on its own, for example about 715 seconds for a bare nginx call. The fix links a real token into the drain. The token cancels with the caller token and the command timeout, the same as before. Once the direct process exits, the tool arms a short grace period on that token. The grace period lets buffered output flush. Then the drain stops. The tool returns. The fix applies to both the streaming path and the non-streaming path in ShellTool. A foreground command keeps its existing timeout. Only the drain after process exit gets the short grace period. Two new regression tests reproduce a backgrounded child that holds the pipe open. Each test asserts the tool returns within five seconds, not twenty.
The post-exit grace window can cut the output drain before EOF. A background child can hold the pipe write end open after the direct process exits. The tool did not report this cut before now. The result text looked complete, but the capture could be partial. BoundedOutputReader.DrainToWindowAsync now returns a Cancelled flag. The streaming pipe drain in ShellTool now returns the same flag. The flag marks a grace cut. It does not mark a budget cut. ShellTool logs a warning on a grace cut. ShellTool also appends a note to the result text on a grace cut. The note tells the agent that a background process held the pipe open. The note tells the agent that the tool did not capture output after that point. Tests prove both cases. A normal command that reaches EOF shows no note. A backgrounded child that outlives the direct process shows the note.
11562b9 to
20508c5
Compare
The follow-up commit widened DrainToWindowAsync from a two-element tuple to a three-element tuple. It added a Cancelled flag. Two benchmark files still deconstructed the old two-element shape. The build failed with error CS8132 on all three OS test jobs. This commit updates both deconstructions to the three-element shape. The benchmarks discard the new Cancelled flag; they do not need it. ShellTool.cs had a catch block with only a comment and no statement. Slopwatch treats a comment-only catch block as empty and reports error SW003. This commit adds a Debug.WriteLine call that records the cancellation. Other catch blocks in the same file use this pattern. BoundedOutputReaderTests.cs uses an infinite Task.Delay in a test helper. The helper stands in for a pipe that never reaches end of file. Slopwatch reports this delay as warning SW004. This commit adds a SlopwatchSuppress attribute with the reason.
|
CI broke for two reasons. The follow-up commit widened Fix in b16baf3:
|
|
Repair commit f346408 addresses the follow-up review.
Validation:
This change does not add command text to OTLP logs. The tool result remains the operator-visible signal for a grace cut. |
Problem
shell_executehangs when a command starts a background process. Oneexample is a command that starts itself as a daemon. A bare
nginxcallreturns at once in a real shell. Through
shell_execute, the same callhung for about 715 seconds.
The direct process exits fast. A forked child, such as a daemon or a
cmd &background job, inherits the stdout/stderr pipe. The child keepsthe pipe write end open after the direct process exits.
Root cause
The output drain read the pipe with
CancellationToken.None. No tokencould stop that read. The drain waited for the pipe to reach end of file.
End of file came only when the background child also exited. Neither the
caller token nor the command timeout could stop the read.
Fix
src/Netclaw.Actors/Tools/ShellTool.csandsrc/Netclaw.Actors/Tools/BoundedOutputReader.cscarry the fix.CancellationToken. The token links tothe caller token and the command timeout, the same as before.
that token. The grace period lets buffered output flush. The drain then
stops, and the tool returns.
ShellTool: the bufferedpath (
ExecuteCoreAsync) and the path that streams tool output(
ExecuteStreamCoreAsync).background child still holds the pipe open.
BoundedOutputReader.DrainToWindowAsyncnow returns aCancelledflagfor this case.
ShellToollogs a warning on a grace cut and adds a noteto the result text. The note tells the agent that a background process
held the pipe open. The note tells the agent that capture stopped at
that point.
Tests
Two regression tests reproduce the hang with
sleep 20 & exit 0. Thedirect bash process exits at once. The backgrounded
sleepholds the pipeopen for 20 seconds. Each test asserts the tool returns within five
seconds.
ShellToolTests.Direct_process_exit_with_backgrounded_child_holding_pipe_open_returns_promptlyShellToolStreamingTests.Direct_process_exit_with_backgrounded_child_holding_pipe_open_streams_promptlyBoth tests failed before the fix. The tool took about 20 seconds. The
tool missed the five-second bound. Both tests are POSIX-only, because
they use
&background-job syntax.A second pair of tests proves the grace-cut marker:
Validation
dotnet test src/Netclaw.Actors.Tests(full project): 3152 passed, 1skipped (a Windows-only test), 0 failed.
dotnet slopwatch analyze: 0 issues../scripts/Add-FileHeaders.ps1 -Verify: all files have headers.Review record
A non-author agent reviewed this change in an isolated worktree. The agent
tried to break the fix. The agent found no correctness, security, or
data-loss defect. Verdict: APPROVE WITH NITS. The review comment is on
this PR.
The agent found one nit with real impact. A grace cut did not tell the
agent that capture stopped early. A follow-up commit on this PR fixed
that nit. The commit added the
Cancelledflag and the result-text notedescribed above. The remaining nits are minor and stay open by design
(wall-clock grace timer, fixed 500 ms cost on a backgrounded command).