test(async): preserve awaitable ownership in test doubles

[bolichen97]

Problem / Motivation

The Windows backend log exposed several tests whose doubles did not preserve
ownership of coroutine arguments or the real sync/async method boundary. Their
assertions usually passed; a later garbage collection then emitted was never awaited, so the warning was attributed to whichever unrelated test happened to
collect next.

Reproduced ownership defects:

• ACP shutdown: a fake wait_for raised before taking the _slow_cancel
  coroutine.
• API model listing: two fake wait_for returns bypassed two deterministic
  _FakeProc.communicate() coroutines.
• Apple Speech: an entire fake subprocess was an AsyncMock, turning the
  synchronous stdin.is_closing() API into a coroutine used as a boolean.
• TaskRunner: two fake create_task calls returned a mock without taking the
  nested _execute coroutine.
• Slack !stop: two session containers were AsyncMock, turning the synchronous
  has_session() accessor into an unawaited coroutine.
• AutoNudge restart: the test stopped a first service, restarted a second one,
  then left the new timer and shielded persistence task alive when its event loop
  closed. That surfaced _update_locked plus two destroyed pending tasks.

Why it matters

These are deterministic ownership bugs in tests, but garbage collection timing
made their warnings appear under unrelated cases and shards. That makes CI look
flaky, obscures the test that actually leaked the coroutine, and can hide a new
resource leak among pre-existing warning noise.

What changed (motivation → approach → change)

Each double now models the relevant real ownership contract:

• accept and close a deliberately unscheduled coroutine before simulating a
  timeout or discarded background task;
• let deterministic fake subprocess communication actually be awaited;
• use a synchronous process/session mock and reserve AsyncMock for genuinely
  awaited methods;
• asynchronously stop and drain the restarted AutoNudge service's owned timer
  and in-flight persistence work before test teardown.

Production behavior is unchanged. There is no retry, sleep, warning filter, or
timeout relaxation.

Open-PR overlap audit:

• No open PR touches the ACP shutdown, Slack activation, or AutoNudge
  approval-stall test files.
• fix(models): inject the kiro api key into the /api/models catalog read #6702 touches the API model test file only in the distinct API-key fixture.
• fix(stt): bundle desktop voice dependencies #6746 and fix: launch the streaming speech helper with --fast (#5896) #5907 touch other Apple Speech sections; this change also avoids
  fix: launch the streaming speech helper with --fast (#5896) #5907's import line.
• feat(taskrunner): honor a spec-declared approval: auto mode #2129 touches a later auto-approve API fixture, not the scheduler tests here.

No contributor change from those PRs is replaced.

Tests

• Every defect was first reproduced from main with RuntimeWarning and
  PytestUnraisableExceptionWarning promoted to errors.
• After rebasing onto 645eb77, all six changed test files pass in strict mode:
  132 passed, 5 platform skips.
• Focused strict runs also pass: API models 14/14, ACP shutdown 3/3, TaskRunner
  auto-approve 26/26, Slack stop 2/2, Apple streaming teardown 2/2, and AutoNudge
  approval-stall 11/11.
• Repository Black baseline gate, isort, flake8, and git diff --check pass.

Manual verification

N/A — these changes only correct test-double lifecycle contracts; the strict
automated runs exercise the affected teardown paths directly.

Related Issues

N/A — found while tracing coroutine warnings from backend CI shards.

Checklist

• At most two commits (exactly one), with a Conventional Commits title
• Existing affected tests pass; lifecycle regressions are covered
• Self-review completed; repository style gates pass
• Documentation updated (not applicable: test-only change)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

N/A — the repository template does not yet supply CLA wording.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

The changes verify cleanly against the base: _FakeProc.communicate is a real coroutine so dropping the wait_for patch works, and the ACP _timeout double matches the real call signature at session_provider.py:211 (asyncio.wait_for(self._handle.cancel(), timeout=5.0)). Each fix targets the double that actually leaked the coroutine rather than filtering warnings — the root cause, consistent with the repo's determinism rules.

Design-Verdict: PASS

Root-cause fixes at each leaking test double — ownership modeled, no filters or sleeps — exactly the shape the repo's flake rules prescribe.

[DESIGN-REVIEWED] 3f62c6e

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 3f62c6e

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 3f62c6e

[iamwhatever]

Tier 1 auto-approve: test (6 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-only -- test doubles for asyncio.wait_for and subprocess handles now model awaitable ownership (close the coroutine, keep sync seams sync) instead of leaking un-awaited coroutines; no production file touched. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[buluoray]

Verified every double against the real call contract in production source rather than against the PR description. 0 blocking / 2 non-blocking notes. Approving.

Note that the three blocking AI review lanes (GPT 5.6, Opus 4.8, First Principles) are skipped on this fork PR, so this review is the substantive one rather than a second opinion on top of theirs.

What I verified

test_acp_session_provider_shutdown.py — the hand-written _timeout faithfully models the one reachable call site, session_provider.py:211 await asyncio.wait_for(self._handle.cancel(), timeout=5.0): timeout is a keyword argument, so the keyword-only *, timeout parameter matches and cannot TypeError; the first positional argument is a genuine coroutine object from async def cancel (session_handle.py:1103), passed directly with no create_task/ensure_future, so .close() is valid and cannot AttributeError; and shutdown() reaches it at most once, guarded by if self._handle.is_turn_active. The except Exception at :212 is what makes the TimeoutError branch still fall through to destroy(), which is the invariant under test.

test_api_models_retry.py — removing the wait_for patch is byte-for-byte behavior-preserving. The real asyncio.wait_for(proc.communicate(), timeout=10) (handlers/agents.py:1375) now wraps _FakeProc.communicate(), which is a real async def returning (self._stdout, self._stderr) with no await or sleep inside — so it yields the identical (payload, b"") the mock supplied, and returncode=0 still passes the proc.returncode != 0 check at :1391. A 10s timeout over an immediately-ready coroutine carries no flakiness risk. This raises fidelity: the real wait_for wrapper is now exercised instead of stubbed.

test_apple_speech.py — this one is worth calling out, because the AsyncMock → Mock switch is load-bearing rather than cosmetic, and the PR description undersells it. finish() guards on not self._proc.stdin.is_closing() (apple_speech/__init__.py:895). Under the old whole-process AsyncMock, is_closing() returned a truthy coroutine, so not <coroutine> was always False and the stdin.close() branch at :897 never executed — the old test silently covered nothing at that seam. Mock() plus is_closing.return_value = False is what makes the branch run, and the added proc.stdin.close.assert_called_once_with() is the guard that pins it. The count is also order-independent: finish() closes stdin exactly once and the session's own close() never touches it (it only does kill() + await wait() + pump cancel + launcher unlink). Every awaited member on the exercised path (stdout.readline, wait) remains AsyncMock. proc.stdin.drain() is the one awaited call left as a plain Mock, but feed() is never invoked here, so it is unreachable — see note 2. The enclosing class carries no skip marker (only TestEndToEndMacOS is darwin-gated), so this runs on Linux CI and the green result is real evidence.

test_autonudge_approval_stall.py — _timers (autonudge.py:512, values are asyncio.Task) and _inflight_adds (:544) match the assumed shapes. stop() is synchronous and only cancels, so the follow-up gather is what actually drains; snapshotting timers before stop() is required because stop() both pops each entry and clears the dict (:778). No hang or unguarded raise: _timer swallows its own CancelledError (:1562-1565) and return_exceptions=True captures a child's cancellation. There is a further point the description does not make: the file's autouse teardown only reaches the published singleton, and because this test starts a second service, the singleton after svc2.start() is svc2 — svc1 is structurally invisible to the shared teardown. So the per-test helper is not redundant with it; it drains the one service the fixture cannot reach, and awaiting (rather than the fixture's cancelling) is what lets the persisted stall flag land before svc2 reads it back. This two-service shape is unique to this test, so the fix is complete rather than a one-off patch over a wider leak.

test_channel_activation.py — has_session is genuinely synchronous (session.py:961, def ... -> bool) and consumed as a boolean at slack/events.py:2381, so the old AsyncMock was returning a coroutine that was truthy by accident and never observed return_value=True. The only await orch.sessions.* anywhere in events.py is stop_turn at :2410, explicitly re-stubbed as AsyncMock in both tests; clear_queue (:2384) and the pre-gate has_session/get_session_for_thread calls are all sync, and enqueue/dequeue sit after the stop-path return at :2434 so they are never reached. No awaited member is left as a plain MagicMock child, and no assertion is weakened. This also just brings the two stop tests in line with the pattern the same file already uses at :119-127.

Non-blocking notes

1. assert timeout == 5.0 pins the double to a bare literal. It is accurate today and cannot fire spuriously, since :211 is the only reachable wait_for in that path. But if that timeout ever becomes a named constant or changes value, the assert reds on a change orthogonal to the invariant being tested (destroy-still-runs-after-cancel-timeout). Asserting timeout > 0 — or reading the production constant if one is introduced — would decouple it.

2. _inflight_adds is snapshotted, not drained to empty. Nothing in _stop_and_drain proves the set has quiesced, so a task enqueued after the list() snapshot is missed. It is closed in practice here (the timers are already cancelled, and the only writer is the single in-flight notify_approval_stalled hook), so this is a robustness observation rather than a live leak — a re-check-until-empty loop would make the helper resilient to a producer that can still enqueue during teardown. Related and equally inert here: _cancel_timer returns without cancelling when a timer's loop is already closed (:~1494), so the pre-stop() snapshot could in principle hold a pending un-cancelled task; the loop is live at teardown in this suite, so it does not apply.

Neither note affects correctness of what is merged, and neither is worth another round.