fix(acp): reset the client even when the kill fails on shutdown

[leonlaiyc]

Fixes #4642

Problem / Motivation

async def shutdown(self) -> None:
    """Gracefully stop the ACP process."""
    await self._kill_process(force=True)
    self._reset_state()  # untracks all PIDs (root + children)

Sequential, so any exception out of _kill_process skips _reset_state
entirely.

It can escape. _kill_process awaits four run_in_executor calls — the
child-PID scan, the child-record capture, and two _kill_escaped_children
sweeps — none of them individually guarded; subprocess_executor() refuses new
work once the loop is tearing down; and asyncio.CancelledError is a
BaseException arriving mid-await, which is exactly what a shutdown produces.

Why it matters

Nothing retries. Every caller treats shutdown() as terminal and drops the
client immediately after:

Caller	Shape
knowledge/llm_pool.py:291, :376	try: await self._client.shutdown() → except Exception: logger.debug(...) → self._client = None
connections/mint.py:493 (_shutdown_quietly)	wrapped in except Exception, never called again
providers/acp.py:1189	plain forward

So a skipped reset is permanent, and _reset_state is not bookkeeping-only. It
leaves behind the process's stdin/stdout/stderr pipes, the macOS seatbelt
sandbox temp files, an uncancelled stderr reader task, confirmed-dead PIDs still
recorded in the orphan-tracking files, and — for the claude backend —
.claude/settings.local.json, which the code's own comment describes as
"Remove settings.local.json so bypassPermissions doesn't persist after crash",
surviving the session it was written for.

What changed (motivation → approach → change)

The reset moves into a finally. The exception still propagates — nothing is
swallowed, only the cleanup is made unconditional.

Running the reset after a failed kill is safe by construction, and that is not
an assumption I am making. _reset_state untracks only PIDs it confirms dead
via _pid_gone_or_unmanaged, and deliberately retains tracking for survivors
so the periodic orphan sweep and cleanup_orphaned_sessions() still reap them —
its own comment calls untracking a survivor "the memory-leak this guards
against". A kill that failed halfway therefore leaves the tracking files in
exactly the state those sweeps expect.

This is the same shape as AcpRuntime.terminate_session, which already
unregisters its queue in a finally for the same stated BaseException reason.

Tests

New test/test_acp_client_shutdown_reset.py:

• _kill_process raises CancelledError → settings.local.json is gone,
  _session_id is cleared, CancelledError still propagates
• _kill_process raises RuntimeError("cannot schedule new futures after shutdown") → same
• clean shutdown → control, the path that already worked

They use a real work_dir and a real settings.local.json and assert on the
file, rather than asserting _reset_state was called.

pytest test/test_acp_client_shutdown_reset.py

	result
against origin/main (55ea3ac1d), pristine worktree	2 failed, 1 passed
with this change	3 passed

The one that passes either way is the control.

Wider relevant suite on this branch:

pytest test/ -k "acp or llm_pool or worker_pool or mint"
→ 1472 passed, 23 skipped, 9 failed

All 9 failures are OSError [WinError 1314] — creating a symlink needs a
privilege this box does not hold — in test_acp_liveness.py and
test_connections_mint.py. Running those two files against pristine
origin/main gives the same 9 failed, 106 passed. (A collection error in
test/test_bench_download_fd.py, KeyError: 'file', is likewise pre-existing
and unrelated.)

flake8 clean; the baselined black gate passes with both files in scope.

Manual verification

Not applicable — reproducing this by hand means cancelling a shutdown mid-kill
on a live claude-backend session and then looking for
.claude/settings.local.json. The tests drive the same production method with
that precondition and assert on the same file.

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 2a391f1775c14380650a50477294445e3ac1e3fd via the fork AI-review pipeline; updated in place on each push.

Review details

The diff is a minimal try/finally fix and the discovery pass found no candidates. I've verified _reset_state is safe to run after a failed/cancelled kill (it untracks only confirmed-dead PIDs, retains survivors, and guards each cleanup step). Nothing behaviorally wrong is introduced.

No findings.

[OPUS-REVIEWED] 2a391f1

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 2a391f1775c14380650a50477294445e3ac1e3fd via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2a391f1

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 2a391f1775c14380650a50477294445e3ac1e3fd via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Minimal, well-evidenced fix: cleanup moves into finally, exception still propagates, and _reset_state's survivor-retention makes running it after a failed kill safe.

Suggestions

• The same kill-then-reset sequential pattern exists in start()'s retry path (client.py:3431-3432, 3441-3442); a raise out of _kill_process there skips the reset identically — worth the same finally treatment in a follow-up.

[DESIGN-REVIEWED] 2a391f1

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 2a391f1775c14380650a50477294445e3ac1e3fd via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've read the contract, the intent file, the patch, and the relevant base code (shutdown, _kill_process, _reset_state, the callers in llm_pool.py and mint.py, and the sibling kill-then-reset sites in _ensure_session). Emitting the review now.

First-Principles-Verdict: PASS

One defect, one finally, and a real-file test — the smallest fix that makes a dropped-forever client's cleanup unconditional.

What this change ships

Intent: stop an interrupted shutdown() from permanently leaking pipes, sandbox temp files, and the claude backend's bypassPermissions settings file — a FIX (#4642).

1. Cleanup after a failed/cancelled kill now still runs (settings file deleted, pipes closed, dead PIDs untracked) — justified
2. Regression tests pinning the interrupted-shutdown path on a real file — justified
3. A 21-line comment in shutdown() restating the failure catalogue — rides along, duplicated by the new test module's docstring

The zero option is real harm: callers (knowledge/llm_pool.py:292, connections/mint.py:494) drop the client after shutdown() and nothing else deletes settings.local.json — the orphan sweeps cover PIDs only. The fix sits at cause level for its scope (the sequential composition is the gap), mirrors the recorded AcpRuntime.terminate_session finally pattern, and adds zero public surface.

Watch

Grepping _kill_process(force=True) + _reset_state() finds 2 other paired sites, client.py:3431-3432 and :3441-3442 in _ensure_session's retry path. They are not true siblings: a skipped reset there self-heals on the next _ensure_session call (the dead-process reset at client.py:3413-3414), so the permanent-harm premise is shutdown-specific. No action required; noting so a human can confirm.

Subtractions

Shrink the shutdown() comment to its last paragraph (the safe-by-construction invariant) — the four-exit failure catalogue and caller table already live verbatim in test/test_acp_client_shutdown_reset.py's docstring, and two copies of that prose must now be kept in sync.

[FIRST-PRINCIPLES-REVIEWED] 2a391f1