fix(dashboard): stop _bounded_turn crashing when GC closes it directly

[kyleseaman]

Summary

PR #5411's CI job "Backend Tests (3.10, 2)" failed on ~264 lines of PytestUnraisableExceptionWarning noise, all attributed to test_signed_out_cli_holds_queue despite that test passing and being unrelated to the change under review. That attribution is misleading: test_signed_out_cli_holds_queue is innocent (verified: zero warnings in isolation) — this is cross-test contamination, an already-documented failure class in this repo (docs/system-specs/common/testing-conventions.md § "3. Leaked async objects").

Root cause: _bounded_turn (dashboard/turn_dispatch.py) wraps a chat turn under a wall-clock ceiling. When a spawn_guarded_turn dispatch is never awaited/cancelled through a live Task — e.g. a test that drives _run_chat and doesn't drain state._background_tasks — its coroutine is eventually reclaimed by the garbage collector, which calls close() on it directly. That throws GeneratorExit at whatever await it's suspended on, resumed synchronously by the collector rather than by a loop callback, so there's no guarantee the owning event loop is even still running.

The cleanup path (added in #6926) answered every teardown unconditionally with task.cancel(); await task. That's correct for a live CancelledError unwind, but:

• Suspending on that second await while already unwinding a GeneratorExit raises RuntimeError: coroutine ignored GeneratorExit — an unraisable exception nobody can catch.
• task.cancel() itself raises RuntimeError: Event loop is closed once the owning loop is gone, unguarded.

Both surface as PytestUnraisableExceptionWarning at whatever later test happens to trigger the GC sweep — which is exactly what CI observed.

Fix

_bounded_turn now catches GeneratorExit explicitly and, in that branch, skips the unsafe re-join — cancelling the inner task best-effort only, and guarding task.cancel() against a closed loop.

Test plan

• Added TestBoundedTurnGeneratorExit in test/test_turn_dispatch.py — two tests that deterministically reproduce the crash (real event loop, closed out from under a suspended _bounded_turn coroutine, then .close()'d directly) and fail against the old code / pass against the fix.
• Ran the full 718-test test_dashboard_chat.py under forced GC + -W error::pytest.PytestUnraisableExceptionWarning: went from 23 unraisable-exception errors to 2 (the 2 remaining are a separate, pre-existing defect in _run_chat's own queue-drain tail, unrelated to this leak — tracked separately).
• Full test_dashboard_chat.py + test_turn_dispatch.py suite passes clean (743 tests, no regressions).
• black / isort / flake8 / mypy clean on touched files.

Pattern harvest

Not generalizable: the vulnerable shape is task.cancel(); await task unconditionally inside a finally of a coroutine that is sometimes dispatched fire-and-forget (spawn_guarded_turn) rather than always driven to completion by an owning Task — that combination is what makes a direct close()/GeneratorExit teardown reachable at all. A grep across src/kiro_crew for the same cancel(); await shape inside a finally turns up ~12 other sites, and every one is an ordinary awaited close()/teardown method that is always driven by a live Task, never orphaned — so none share the precondition this bug needed. A semgrep rule for "await inside finally" would false-positive on all of those legitimate sites without also encoding "and this coroutine may be dispatched without a Task ever guaranteed to join it," which isn't something the pattern-matching layer can see.

🤖 Generated with Claude Code

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

The fix targets a real defect in _bounded_turn itself — the base code at turn_dispatch.py:385-391 awaits inside a finally, which any coroutine close() will break — and the chosen shape (flag the GeneratorExit unwind, skip the re-join, guard cancel() against a dead loop) is the standard idiom for making a coroutine close-safe. The description's claims all have backing code, the tests reproduce the crash deterministically without sleeps, and the alternative (only fixing the leaking tests) would leave the wrapper genuinely un-closeable, so hardening it is the root-cause fix for the crash rather than a symptom patch.

Design-Verdict: PASS

Fixes the wrapper's real defect — awaiting in finally breaks under close() — with the standard GeneratorExit-safe idiom, deterministic repro tests, honest scope.

[DESIGN-REVIEWED] bdcd0f5

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] bdcd0f5

[github-actions]

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

Premise-level review of bdcd0f53498f2070a7b8a943b2ecf14ad0612e20 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.

All twelve other task.cancel(); await task sites (grepped multiline across src/kiro_crew) are ordinary awaited close()/teardown methods — none sit in a finally of a coroutine dispatched fire-and-forget the way spawn_guarded_turn dispatches _bounded_turn, so the fix has no unfixed siblings. The cited failure class exists verbatim in testing-conventions.md § "3. Leaked async objects", and the fix adds no public surface (one local flag, one guard). The framing matches the diff exactly, including its disclosure of two remaining unrelated warnings.

First-Principles-Verdict: PASS

A real interpreter rule — a coroutine may not suspend while unwinding GeneratorExit — is fixed where it was violated, with nothing riding along.

What this change ships

Intent: stop _bounded_turn's cleanup from raising uncatchable exceptions when the garbage collector closes an orphaned turn wrapper directly — a FIX.

1. Test/CI runs stop emitting "coroutine ignored GeneratorExit" from orphaned turn dispatches — justified (documented failure class, testing-conventions.md § Leaked async objects)
2. Cleanup's task.cancel() no longer raises "Event loop is closed" on any exit path — declared, justified
3. On GC teardown the inner task is cancelled but never re-joined — declared, justified (interpreter forbids suspending during GeneratorExit unwind)
4. Two regression tests pinning both crashes — rides with the fix, justified

The fix sits at mechanism level: the misbehaving code is the unconditional cancel(); await task in _bounded_turn's own finally (turn_dispatch.py:385-391), and that is exactly what changed. The deeper trigger — tests leaving dispatches in state._background_tasks — is out of scope, and the description says so. Sibling check: grepped task.cancel() followed by await task across src/kiro_crew — 12 other sites, all awaited teardown methods, none inside a finally of an unjoined coroutine, so no point-patch concern. No new public surface, config key, or concept; the live-cancel join semantics from #6926 are preserved.

[FIRST-PRINCIPLES-REVIEWED] bdcd0f5

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The candidate is the only item to validate. It targets the residual GeneratorExit path at the finally's join (await task), claiming symmetry with the body's suspension point.

Reconstructing the post-diff finally:

• except GeneratorExit sets _generator_exit = True only for a GeneratorExit delivered through the try body, then re-raises → finally skips the join. That path is fully covered.
• The candidate's residual path requires the wrapper to be suspended at the finally's own await task AND then close()-d — which needs an orphaned wrapper never driven by a live Task.

Checking (a) a concrete input in practice: _bounded_turn reaches the finally's await task suspension only after entering cleanup with the task not done (e.g. call_later rejecting), and only an unowned coroutine receives close() rather than a Task-delivered CancelledError. In every production dispatch (spawn_guarded_turn) the wrapper is owned by a registered Task, so GeneratorExit arrives as cancellation at the body await, not as close() at the finally await. The candidate itself concedes it "could not construct a production path (not a test)" and rates itself "low" — (a) resolves to "could/might," and the instructions require dropping such items. It also concedes the swallow "predates this PR (the block was only reindented)," so it is not a defect the changed lines introduce.

No concrete input, no production call path — the candidate fails the Step 1 bar and scores well under 80.

Step 2: the added except GeneratorExit and the try/except RuntimeError around task.cancel() are defensive hardening; on the normal timeout/cancel path cancel() does not raise and the join proceeds as before, and the GeneratorExit path deterministically skips the join. No new grounded defect at 80+.

No findings.

[OPUS-REVIEWED] bdcd0f5

[bolichen97]

Tier 1 auto-approve: fix (2 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: fix with a clear root cause -- stop _bounded_turn crashing when the GC closes it directly (GeneratorExit path skips the join and cancels best-effort on a possibly-dead loop). 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.

[iamwhatever]

Tier 1 auto-approve: fix (2 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: fix -- _bounded_turn raised "coroutine ignored GeneratorExit" when the GC close()-d its coroutine directly instead of a live Task cancelling it, so the wrapper now flags that path and has its finally cancel best-effort (swallowing RuntimeError from an already-closed loop) and skip the await-join that would suspend during a GeneratorExit unwind; one source file plus its test file, pure asyncio teardown with no auth/input-parsing/trust-boundary/sandbox/gate/secret surface 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.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7709 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7709: KEEP. Complementary halves of one investigation; the merged half covers none of this PR's code paths. Files: src/kiro_crew/dashboard/turn_dispatch.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.