summarize: agent timeout kills only the direct child, so a surviving grandchild blocks the call indefinitely

[rymalia]

Summary

summarize_with_agent_impl_timeout enforces summarization.agent_timeout_secs by killing the spawned agent when the deadline passes. It kills only the direct child, not the child's process group. A grandchild that outlives it inherits the stdout pipe and holds it open, so the reader thread's join() blocks until that grandchild exits — long past the configured timeout.

The result is that agent_timeout_secs is not a hard bound on how long summarization can block.

Line references are against 7a61dd57 (current main at time of filing).

Where

crates/core/src/summarize.rs, timeout branch of summarize_with_agent_impl_timeout (fn at 1682, branch at 1820):

if start.elapsed() > timeout {
    child.kill().ok();
    let _ = child.wait();
    let _ = stdout_handle.join();   // <-- blocks while a grandchild holds the pipe
    let _ = stderr_handle.join();

stdout_handle / stderr_handle are real reader threads spawned at 1760 / 1766. Killing the direct child does not close the pipe, because the surviving grandchild still holds the write end.

The same file already contains the correct pattern. run_speaker_mapping_via_agent puts its agent in its own process group precisely so a timeout can kill the whole tree:

• 2948 — comment: "Put the agent in its own process group so a timeout can kill the WHOLE tree"
• 2954 — command.process_group(0);
• 3002 — kill_process_group(child_pid); before child.kill()
• 3022 — kill_process_group helper (with a non-Unix no-op stub at 3037)

The summarize path does none of this.

Secondary, milder site

run_title_refinement_via_agent (fn at 2600, timeout branch at 2660) also kills only the direct child. It does not hang — the thread it spawns is a stdin writer and the timeout branch never joins it, so it returns on schedule — but it still leaks orphaned grandchildren. Worth fixing in the same pass, though it is not the blocking bug.

Reproduction

An "agent" that spawns a child surviving the shell being killed:

printf '#!/bin/sh\ncat >/dev/null 2>&1\nsleep 900\n' > /tmp/hang-engine.sh
chmod +x /tmp/hang-engine.sh

~/.config/minutes/config.toml:

[summarization]
engine = "agent"
agent_command = "/tmp/hang-engine.sh"
agent_timeout_secs = 30

Then trigger any summarization.

Observed: at 30s the shell is killed, but ps shows sleep 900 reparented to PID 1 and still running. The summarize call remained blocked 4+ minutes later. Killing the orphan by hand released it immediately.

Control: changing the script's last line to exec sleep 600, so no grandchild survives, makes the timeout behave exactly as documented — the call returns at the deadline. The only difference between the two runs is whether a grandchild outlives the direct child.

Why it matters

Real agent CLIs spawn child processes (MCP servers, tool subprocesses), so this is reachable in normal use rather than only with a synthetic script.

• CLI: minutes process / minutes resummarize can block well past agent_timeout_secs.
• Desktop: any UI that serialises on summarization stays wedged for the duration. I hit this while building a desktop Resummarize button, where a single in-flight guard is held app-wide — every meeting's button stays disabled until the orphan exits, with no recovery short of quitting the app.

A timeout on the caller side is not a fix: whatever holds the guard must outlive any task that could still write to the artifact, or two runs race on the same file. The fix belongs here, in core.

Suggested fix

Implementation note: See the follow-up comment on this issue before implementing. Output collection must be bounded after both a timeout and a successful direct-child exit; the Unix process-group pattern is not sufficient by itself.

Mirror run_speaker_mapping_via_agent: spawn the agent as a process-group leader (command.process_group(0)) and, on timeout, call the existing kill_process_group(child_pid) before child.kill(). The helper and the non-Unix stub already exist, so this is mostly wiring.

Dropping the pipe read handles before join() — or joining with a deadline — would be a complementary guard, so a stuck reader can never outlive the timeout even if a process escapes the group.

[rymalia]

Follow-up: re-verification found an additional completion path not covered by the original reproduction, and it changes the required fix. Line references remain against 7a61dd57, still main; crates/core/src/summarize.rs is unchanged since filing.

1. The clean-exit path blocks too

I originally framed this as "agent_timeout_secs is not a hard bound." That remains true, but the successful direct-child-exit path has the same inherited-pipe problem.

The deadline is consulted only while try_wait() returns Ok(None). Once the direct child exits, summarize_with_agent_impl_timeout unconditionally joins its stdout/stderr reader threads (1786–1793). Those readers use read_to_end(), so a surviving descendant retaining either output FD prevents EOF and blocks completion.

An agent can therefore exit 0 promptly with a complete summary already written to stdout, while the caller remains blocked for the descendant's lifetime.

Verified by calling summarize_with_agent_impl_timeout directly with the existing unit-test harness:

[#592-A] 2s timeout      -> returned after 12.014s   (grandchild = sleep 12)
[#592-B] agent exited 0  -> returned after 12.016s   (timeout was 300s and irrelevant;
                                                       key_points=["summary ok"] was already
                                                       on stdout at ~0.1s)

Variant B uses an otherwise well-behaved agent shape:

#!/bin/sh
cat >/dev/null 2>&1
sleep 12 &
printf 'KEY POINTS:\n- summary ok\n'
exit 0

The originally suggested timeout-only process-group kill does not address this variant, because the timeout path is never entered.

2. The process-group suggestion is Unix-only

process_group(0) is inside #[cfg(unix)] (2951–2955), and
kill_process_group has a #[cfg(not(unix))] no-op stub (3037). Windows is
a shipped desktop target, so process-group wiring alone cannot provide a
cross-platform completion bound.

3. The repo already demonstrates part of the needed guard

engine_process::output_with_timeout_impl contains the right timeout-path
principle:

// Descendants can inherit these descriptors. Do not let an
// unrelated descendant defeat the timeout after the direct
// child has been killed and reaped.
drop(stdout_handle);
drop(stderr_handle);

Those are JoinHandles: dropping them detaches the blocked reader threads; it
does not cancel them. That is appropriate on the timeout/error path, where the
function must return an error rather than wait indefinitely for EOF.

That helper is gated #[cfg(any(test, feature = "parakeet"))], so it is a
pattern to copy rather than a helper to call. Its normal-exit path still joins
unboundedly, so it has the same clean-exit variant described above.

Revised fix priority

1. Primary, cross-platform: bound output collection on both paths.

   • After a timeout, reap the direct child, detach the reader JoinHandles,
     and return the timeout error without waiting for EOF.
   • After a successful direct-child exit, require both readers to complete
     within a bounded grace period. If EOF does not arrive, return an error
     rather than a partial summary. This needs a bounded collection mechanism
     (for example, reader results delivered through channels), not simply
     detaching readers and reporting success.

2. Secondary, Unix: create and terminate the process group.
   process_group(0) plus kill_process_group still matters: it prevents the
   orphan leak on timeout. It can also be used for cleanup after a failed EOF
   grace period, but it is not the cross-platform completion guarantee.

3. Non-Unix: provide equivalent tree termination where available.
   The existing non-Unix no-op stub alone is not a fix.

Both summarize_with_agent_impl_timeout and
run_title_refinement_via_agent need review. The title-refinement timeout
branch returns on schedule, as originally filed; however, its normal-exit
wait_with_output() call (2644) also reads both pipes to EOF and can block
when a descendant survives.

Two wording corrections to my original report:

• “Dropping the pipe read handles before join()” was imprecise. The pipe
  ends live in reader threads; the caller holds JoinHandles. The timeout-path
  action is to detach those handles, or otherwise use bounded collection.
• “No recovery short of quitting the app” was loose. More precisely, there is
  no in-product recovery: the desktop guard remains held until the descendant
  exits or closes the inherited pipes. Killing the descendant externally also
  releases it; quitting resets the in-memory guard but does not terminate the
  reparented descendant.

[silverstein]

Fixed in #593, and thank you for the follow-up comment. That second finding is what made the fix correct rather than half-correct.

I had the process-group half queued from your original report, since run_speaker_mapping_via_agent already had the pattern and it was mostly wiring. The clean-exit path is the one I would have shipped a broken fix for: the deadline is only consulted while try_wait() returns Ok(None), so nothing about a process group helps an agent that exits 0 promptly while a descendant holds the pipe. Your note that output collection must be bounded on both paths is the actual load-bearing insight.

So the fix is:

• process group leader plus kill_process_group() before child.kill() on timeout
• output collected through a shared buffer with a bounded wait on both paths, abandoning a still-blocked reader rather than joining it, and returning the bytes already read so a summary written before the descendant outlived the agent is still delivered
• same process-group treatment for run_title_refinement_via_agent, the secondary site you flagged

Both regression tests are mutation-verified rather than just written: reverting to an unconditional join makes the clean-exit test block the full 30s and fail with took 30.00791745s, which is your 12.016s at a larger sleep.

#588 is next. That one is yours from the #573 review and I asked you to file it, so it is on me to close.

[silverstein]

Shipped in #593, now on main.

Thank you for this one specifically. The original report was already actionable, with line references, a working reproduction, and the observation that run_speaker_mapping_via_agent already had the pattern to copy. But the follow-up comment is what made the fix correct instead of half-correct. I had the process-group change queued and would have shipped it believing the issue closed, and it would not have helped the clean-exit path at all, because the deadline is never consulted once the direct child exits. Finding that by re-verifying your own report, rather than moving on after filing it, is the part that mattered.

Landed:

• process-group leader plus kill_process_group() before child.kill() on timeout
• output collected through a shared buffer with a bounded wait on both completion paths, abandoning a still-blocked reader rather than joining it, and returning bytes already read so a summary written before the descendant outlived the agent still reaches the caller
• same treatment for run_title_refinement_via_agent, the secondary site you flagged

Both regression tests are mutation-verified: reverting to an unconditional join makes the clean-exit test block the full 30s and fail with took 30.00791745s, which is your 12.016s at a larger sleep.