fix(shell): stop blocking until timeout when a detached child holds the pipes - #2530
fix(shell): stop blocking until timeout when a detached child holds the pipes#2530ayaangazali wants to merge 1 commit into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
Quick status on this one since it has been open a couple of weeks. What it fixes: the foreground shell path waited for stdout/stderr EOF before checking the exit code, so a command that leaves a detached child holding the pipes ( Branch is still current with main (main has not moved since 4a550ef, so there is nothing to rebase onto) and it still merges clean. One thing worth flagging: the CI checks here have never actually run, they are all sitting at Happy to adjust the approach or drop it if it is not the direction you want. |
…he pipes The foreground shell path waited for stdout/stderr EOF before checking the exit code, so a detached child that inherited the pipes kept the tool blocked for the full command timeout, after which the run was wrongly reported as killed by timeout. Process exit is now observed by polling returncode (asyncio gates its exit waiters on pipe disconnection), and remaining output is drained for a bounded grace period after the shell exits.
c2ac766 to
0fb4911
Compare
Community PRs (ported with kimi_cli->cran_code adaptation): - MoonshotAI#2507 ACP AskUserQuestion signals QuestionNotSupported (no phantom dismissal) - MoonshotAI#2520 fork/undo context truncation aligns to wire turns by text matching - MoonshotAI#2572 double-encoded tool args; MoonshotAI#2530 shell pipe hang - MoonshotAI#2592 media results degrade gracefully when model lacks image_in - MoonshotAI#2539 MCP tool-name normalization with collision guard; MoonshotAI#2535 cache-key scoping kimi-code Tier-1 (compaction quality): - compact.md rewritten as first-person handoff note - summary prefix teaches 'notes, not proof — verify before relying' - post-compaction token estimate adds system-prompt + tool-schema overhead
Related Issue
Resolve #2468
Description
In the foreground shell path,
_run_shell_commandwaits for stdout/stderr EOF (asyncio.wait_for(gather(read stdout, read stderr), timeout)) before ever checking the exit code. A command likesome_daemon & echo doneleaves a detached child holding the write ends of the pipes, so EOF never arrives: the tool blocks for the entire command timeout even though the shell exited immediately, then reports "Command killed by timeout" for a command that actually succeeded. That is the hang in #2468.Two things I learned while fixing it, which shaped the implementation:
process.wait()instead does not work: asyncio gates its subprocess exit waiters on pipe disconnection, sowait()blocks exactly the same way (verified empirically on macOS and Linux Python 3.14:wait()returned only when the detached child died, whilereturncodewas set within 50ms of the shell exiting). So exit is observed by pollingreturncodeat 50ms intervals, which theKaosProcessprotocol guarantees on every backend.gatherafter cancel triggers asyncio's "_GatheringFuture exception was never retrieved" loop-level error, which prompt_toolkit surfaces as a full-screen "Unhandled exception in event loop" banner that freezes the TUI. The existing PTY e2e tests (test_ctrl_c_during_running_turn_interrupts,test_shell_cancel_running_command_kills_process_and_recovers) caught this in my first attempt, hence the_consume_exceptiondone callback.Behavior after the fix:
PIPE_DRAIN_GRACE(2s), then the tool returns the real exit code. Output produced by a detached child after that grace is dropped, which I think is the right semantic: the shell finishing is what "the command finished" means.Note this intentionally does not implement full child-process tracking/reaping that the issue mentions as the ideal. The detached child keeps running (same as before the fix); the change just stops it from stalling the agent loop. Figured the bigger cleanup story needs maintainer direction first.
Tests: new
test_detached_child_holding_pipes_does_not_block_until_timeout(fails at 25s timeout before the fix, returns in ~2s after). The fake processes in existing tests grew areturncodeattribute since the code now polls it, and the cancel-test fake now models "running until killed" to keep testing the same scenario it did before.Checklist
make gen-changelogto update the changelog. (hand-edited CHANGELOG.md in the same style, i don't have the Kimi API setup the skill needs)make gen-docsto update the user documentation. (no user-facing docs describe the shell tool's pipe/timeout behavior, nothing to regenerate)being upfront: freshman here, i dug through this one with claude code as a rubber duck for the asyncio semantics (the wait() thing surprised both of us). tested everything i could locally but if the approach is off i'm all ears, learning as i go :)