Dispose exec children once on every TUI exit path - #846
Conversation
Stop workers first so a hung session-op cannot delay abort and reap. Log shutdown failures at error while still mapping teardown failure to exit 1.
A leftover after the two-second backstop must reject dispose so the exit 1 path can fire. Abort already SIGKILLs the process group at abort start.
TheGreatAxios
left a comment
There was a problem hiding this comment.
independent critic found no blocking orphans on primary TUI/exec run_shell paths. Once-only dispose holds. Follow-ups already in: quit shuts down before hung tail, reap timeout fails teardown, shutdown errors logged. Remaining should-fix being addressed: subagent dispose currently swallows posixTools.dispose rejection.
TheGreatAxios
left a comment
There was a problem hiding this comment.
re-critic of origin/main..HEAD including ed15287 found no blocking orphans on primary TUI/exec run_shell paths. Subagent posix dispose rejection is no longer swallowed on ephemeral teardown (disposeSubAgentSession awaits with no catch). Remaining file-for-later: persist close_agent still best-effort-catches dispose, so a persist worker leftover after reap can still look like success at the fleet layer.
A leftover child after posix reap must fail persist close and parent toolset dispose instead of looking like a successful shutdown. A wedged close still times out as shutdown.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Critic · Request changes
TUI/exec teardown is once-only and leftover posix dispose can fail the close path, but live children are still SIGKILLed only after agent.close(), so a wedged close or the 2s crash/signal race orphans detached shell-guard processes.
Findings
-
src/subagent/dispose.ts:113/src/subagent/run.ts:1074—posixTools.dispose()(the process-group kill) runs only afterawait agent.close()andawait streamPromise.closeOnAbortno longer disposes posix tools.boundedClosethen times out as success (src/subagent/run.ts:1111,src/subagent/session-store.ts:30). Sequence: persist worker with a liverun_shellchild;agent.close()hangs (the documented close-ordering wedge);close_agent/cancelAllhits the deadline; session isshutdown; the detached child is still alive.src/subagent/session-store.test.ts:572locks in hung-close → shutdown success. -
src/tui/runner/shutdown.ts:44/src/exec/runner.ts:171/src/index.ts:123— crash/signalPromise.racewaits 2s for the whole chain (cancelAll→closeAgent→disposeToolset). Reap is the last leg. Sequence: live parentrun_shell; SIGTERM whileagent.close()takes >2s;awaitActiveDisposeHostlogs the deadline andprocess.exits; detached children survive. The integration fixture usesagent: null, so it never hits this. -
src/index.ts:129— the 2s timer is never cleared when dispose wins. The loser rejects later asunhandledRejection(handler no-ops becauseterminatingis already true). -
src/exec/runner.ts:891— leftover dispose after a successful exec islogger.debugplusexitCode = 1. No stderr. Operator sees a silent 1;run.jsonis alreadydone. -
src/tui/runner/exit.ts:544— shutdown now precedesawaitTail, but the tail is still unbounded. A hung session-op still preventsrun.json, telemetry, and the teardownFailed exit 1.src/tui/runner/exit.test.ts:56documents the hang. -
src/tui/runner/shutdown.ts:18—completionstarts asPromise.resolve(). A signal that re-enters afterstarted = trueand beforecompletion = (async () => …)()(window isdisposeHost()+ startingcancelWorkers) observes a finished dummy and canprocess.exitbefore reap.
Notes
- Once-only plugin dispose, leftover throw from
reapLiveChildren, and TUIteardownFailed→ exit 1 are fine when close actually returns in time. - Permanent tests to keep: hung
agent.close()with a live tracked child must kill or fail close, not report shutdown; signal path with slowcloseAgentmust still reap; successful exec + throwing toolset dispose must be exit 1 and visible.
|
re-critic of origin/main..HEAD at 1f6c66c found no remaining blocking leftover-as-success swallow on TUI quit, disposeExecRuntime, persist close_agent, or parent toolset dispose. cancelAll now awaits retained close; leftover posix dispose rejects. Remaining should-fix, not blocking: exec runExec maps that reject to exitCode 1 with status still "done" and a debug-only log (no stderr); createAgentToolset.dispose stops the fleet walk after the first leftover and skips parent posixTools.dispose. |
|
re-critic of origin/main..HEAD at 7a2ab30 found both remaining should-fixes closed. Leftover exec dispose now fails the ExecResult (exit 1, status failed, error + stderr). Parent toolset dispose finishes remaining closeOne and posix teardown before rethrowing. No blocking leftover-as-success swallow remains on TUI quit, disposeExecRuntime, persist close_agent, or runExec. File-for-later only: on-disk persist("done") / TUI run.json still record a completed send when teardown later fails. |
A hung agent.close used to run before process-group reap, so teardown could report success while detached run_shell children were still live. Dispose first, fail a close deadline instead of succeeding, and clear the two-second host timer when dispose wins.
Summary
Verification
bun run typecheck,bun run build, andbun run testpassbun run checkpassesFixes CL-7534
Changelog: bug