fix(dashboard): stop _run_chat crashing when GC closes an orphaned turn

[kyleseaman]

Problem / Motivation

PytestUnraisableExceptionWarning noise when a _run_chat chat-turn coroutine is orphaned as a background task and later finalized by the garbage collector after its owning event loop has already closed.

Why it matters

This is the second half of the investigation that produced #7643. That PR's own description flags this exact defect ("_run_chat's own queue-drain tail") as "tracked separately" — a real, reachable bug in the queue-drain / auto-title / synthesis dispatch path that nobody had fixed yet. Left in place, any orphaned _run_chat task (today: test teardown that doesn't join a spawned turn; tomorrow: any code path with the same hygiene bug) crashes with an unraisable RuntimeError misattributed to whatever test the GC happens to run in — exactly the kind of noise that corrupted PR #5411's CI signal in the first place.

What changed (motivation → approach → change)

Symptom: _run_chat's tail, inside its single finally: block, drains the queue via _start_next_queued_turn (spawn_guarded_turn → asyncio.create_task) and falls back to _finish_queue_cycle (four more bare asyncio.create_task calls: synthesis, auto-title, title-refresh, session summary).

Root cause: all of these assume a running event loop. When the coroutine is resumed by close() off-loop — the GC finalizing an orphaned, never-joined task after its owning loop already closed — asyncio.create_task's internal get_running_loop() raises RuntimeError, which replaces the in-flight GeneratorExit mid-unwind and surfaces as an unraisable exception.

Change: added _loop_is_running() to chat_runner.py and used it to make _start_next_queued_turn bail out with False before touching the queue when there's no running loop, and folded the same check into _finish_queue_cycle's will_synthesize condition and its other three create_task sites — so an off-loop unwind degrades to "skip the background work" instead of raising.

Tests

Added TestQueueDrainOffLoopGuard in test/test_dashboard_chat.py: 5 tests pin the guard directly (monkeypatched get_running_loop, no real event loop needed) plus one end-to-end reproduction — a real _run_chat coroutine parked on a genuinely-unresolved await (standing in for a stalled ACP call), its owning loop closed out from under it, then .close()'d directly.

Verified the reproduction is real, not a tautology: reverted the source fix and confirmed all 6 tests fail, with the integration test raising the exact reported RuntimeError: no running event loop at spawn_guarded_turn → asyncio.create_task. Restored the fix; all pass clean under -W error::pytest.PytestUnraisableExceptionWarning, 20/20 repeat runs stable (fully deterministic — no GC-timing dependence).

Also ran the full test_dashboard_chat.py (724 tests), test_turn_dispatch.py, test_chat_runner_coverage.py, test_pending_title.py, test_session_summary_generate.py, and queue-drain test files — 1600+ tests, no regressions. black / isort / flake8 / mypy (--platform linux) clean on touched files.

Manual verification

N/A — unit coverage sufficient. This is an internal async-cleanup path with no UI and no externally observable behavior to click through; the reproduction test above exercises the real failure mechanism directly.

Related Issues

Companion to #7643 — fixes the _run_chat half of the same investigation. Independent of it either way: this PR doesn't touch turn_dispatch.py, and neither PR depends on the other to be correct.

Pattern harvest

Rule candidate: review-prompt
Pattern: a bare asyncio.create_task/ensure_future reachable from a coroutine's finally:/cleanup path assumes a running event loop — a GeneratorExit-driven off-loop unwind (the GC finalizing an orphaned task after its loop already closed) turns the implicit get_running_loop() inside it into an unraisable RuntimeError instead of letting the close finish cleanly. Two independent instances already exist in this codebase: _bounded_turn (#7643) and this PR's _finish_queue_cycle / _start_next_queued_turn. Worth a review-prompt heuristic flagging new create_task/ensure_future calls added inside a finally: block for a running-loop guard.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, no documented API/schema/behavior changed
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

The design checks out: the guard reuses the existing serving_loop/_running_loop machinery in state.py (same identity pattern websocket_hub already uses), all production call sites of _start_next_queued_turn/_finish_queue_cycle run on the serving loop, bind_serving_loop rebinds unconditionally at startup so the lazy latch can't wedge dispatch, and refusing to dispatch on a foreign loop is strictly safer than the prior behavior of scheduling onto a loop that doesn't own the turn. The reproduction test exercises the real mechanism and was falsified against the reverted fix.

Design-Verdict: PASS

Real, reachable off-loop crash fixed at the right layer, keyed on existing serving-loop identity machinery, with a genuine falsified reproduction.

Suggestions

• Refresh the PR description's "What changed": it describes _loop_is_running() (a bare running-loop presence check), but the shipped guard is _on_serving_loop() (identity against the bound serving loop) — a stricter, silently-skipping behavior worth stating accurately for future archaeology.

[DESIGN-REVIEWED] c2a5448

[github-actions]

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

Premise-level review of c2a544876fc7493dd9ae22b23d7678f35acb26aa 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 checks done. The base tree confirms _running_loop/serving_loop/bind_serving_loop exist (state.py:7767–7799, with first-read latching so unbound states aren't bricked), both server entry points rebind the serving loop at startup (server.py:3405, 4532), all 8 _start_next_queued_turn and 3 _finish_queue_cycle call sites run on the serving loop, no pre-existing safe-spawn helper exists on the dashboard side, and no pins are deleted. The one real gap: the description claims a "no running loop" existence check (_loop_is_running()), but the diff ships a stricter identity check (_on_serving_loop) that also refuses dispatch on a foreign running loop — a behavior change the description never declares.

First-Principles-Verdict: PASS

Shipped guard is stricter than described: it also refuses dispatch on a foreign running loop — undeclared; confirm that refusal is the intended behavior.

Not justified as shipped

3. Foreign-loop refusal — undeclared: the description says the guard bails "when there's no running loop" via _loop_is_running(); the diff ships _on_serving_loop, an identity check that additionally refuses a bound-but-different loop. Derived from asyncio's create-task-binds-to-current-loop rule, but never declared.

What this change ships

Intent: stop an orphaned _run_chat coroutine, GC-finalized after its loop closed, from raising an unraisable RuntimeError — a FIX.

1. Off-loop GC finalization of an orphaned turn now unwinds cleanly instead of raising — justified
2. Off-loop, the queue drain and end-of-cycle background work (synthesis, auto-title, title refresh, session summary) are skipped, queue left untouched — justified
3. Dispatch is also refused when a different loop than the serving loop is running — undeclared (see above)
4. New internal helper _on_serving_loop — justified
5. Six regression tests pin the no-loop and foreign-loop refusals — justified

Verified by reading: provenance is #7643 plus a reproduction test that fails on base by construction; the guard sits at mechanism level inside the two shared functions, covering all 11 call sites (grepped _start_next_queued_turn\(|_finish_queue_cycle\(: 8 + 3); _on_serving_loop has 2 real consumers; no existing dashboard mechanism does this job (push_slots_update's inline loop is self._running_loop() at state.py:7751 is the same idiom, not a reusable helper); both serve entry points rebind the serving loop, so the identity check cannot wrongly starve a production drain. The repo-wide sibling pattern (bare create_task in a finally) is declared and deferred by the description itself — accepted.

[FIRST-PRINCIPLES-REVIEWED] c2a5448

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

Both candidates concern the off-loop / foreign-loop paths that the guard exists to handle, and both were flagged low-confidence by the discovery pass. Working through them against the actual code:

CANDIDATE 1 — The mis-latching is real behavior of the serving_loop getter (state.py:7794-7799): when _serving_loop is None and a foreign loop is running, _on_serving_loop returns True and permanently latches the foreign loop. But this requires _serving_loop to be unbound — which in production never holds: bind_serving_loop runs at the startup hook, and even absent that, the getter latches the real serving loop the first time _run_chat's tail reads it from that loop. The queue-drain tail in production always runs on the bound serving loop, so the guard returns True correctly there. The unbound+foreign-loop combination occurs only across successive event loops in a test process (the PR's own foreign-loop test explicitly calls bind_serving_loop first, so it never exercises the unbound case). No production input reaches (a); the wrong outcome (c) is confined to test-harness hygiene. Dropped.

CANDIDATE 2 — The off-loop branch does emit done/chat_done while _pending_synthesis stays True, but this branch is reached only during GC finalization of an orphaned coroutine after its owning loop has closed. There is no surviving consumer of the mutated slot on a dead loop, and no later legitimate on-loop drain of the same slot object exists (the slot's owning loop is gone). The candidate itself could identify no consumer; (c) does not resolve to an observable wrong outcome on a path that occurs in practice. Dropped.

No new grounded finding surfaced while falsifying these.

No findings.

[OPUS-REVIEWED] c2a5448

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/dashboard/chat_runner.py:5218 -- Lazy binding accepts a foreign loop when no serving loop was pre-bound
serving = state.serving_loop
Unbound state + orphan finalization on a later loop -> _on_serving_loop binds that foreign loop -> queued work is consumed and dispatched on the wrong loop.
Anchor: residual/crash-data-loss-corruption
Fix: Read the already-bound _serving_loop without invoking the lazy-latching property.
[BLOCK-MERGE] c2a5448
[GPT-REVIEWED] c2a5448

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

The adjudicable input block is empty (0 findings). One fenced finding, F1.

F1 analysis. The claim is real as code: _on_serving_loop reads state.serving_loop (the diff's new line at chat_runner.py:5218), and that property lazily latches the running loop whenever _serving_loop is None (state.py:7794–7798). So on an unbound state finalizing on a foreign loop, serving latches to running and running is serving returns True — defeating the guard.

But every condition that reaches harm is test-only and they compound:

• (a) Unbound state required — the latch only fires when _serving_loop is None (state.py:7795). In production bind_serving_loop runs at server startup, on the serving loop, before any request/turn is handled (server.py:3405, server.py:4532), so _serving_loop is non-None by the time any _run_chat coroutine can exist. An unbound state with a live turn coroutine is self-contradictory in production.
• (a) Orphaned-coroutine finalization — declared a test/harness bug, not a production path (chat_runner.py:5199+; spawn_guarded_turn always registers, finish_turn_task always retrieves).
• (a) Foreign loop running during that finalization.
• (b) Recovery / outcome — in the foreign-loop case a loop is running, so create_task succeeds (no RuntimeError crash); the residual is a wrong-loop dispatch. In production this branch is unreachable because the state is always bound.
• (c) Rarity — the unbound precondition cannot arise in real operation; the residual is confined to test/harness scenarios, exactly the class where a human would accept the risk (and the suggested one-line fix stays available).

Evidence record is complete from code opened this run → FLAG.

[ADJUDICATION] c2a544876fc7493dd9ae22b23d7678f35acb26aa total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] c2a544876fc7493dd9ae22b23d7678f35acb26aa
[ADJUDICATION-FENCED] c2a544876fc7493dd9ae22b23d7678f35acb26aa fenced=1 flagged=1
FLAG F1 src/kiro_crew/dashboard/chat_runner.py:5218 -- The lazy-latch defeats the guard only when the state is unbound, but production binds the serving loop at startup (server.py:3405/4532) before any turn coroutine exists, so the unbound precondition — compounded with the already test-only orphan-finalization path and a live foreign loop — is unreachable in real operation.
[GPT-ADJUDICATED-FENCED] c2a544876fc7493dd9ae22b23d7678f35acb26aa

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

• F1 src/kiro_crew/dashboard/chat_runner.py:5218 — The lazy-latch defeats the guard only when the state is unbound, but production binds the serving loop at startup (server.py:3405/4532) before any turn coroutine exists, so the unbound precondition — compounded with the already test-only orphan-finalization path and a live foreign loop — is unreachable in real operation.

[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

• This PR is OVERLAPPING with PR #7161. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7709: KEEP. Same expression, orthogonal conditions — coexist as two conjuncts; only a textual merge-order conflict. Files: src/kiro_crew/dashboard/chat_runner.py.
• This PR is OVERLAPPING with PR #7643. 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.
• This PR is OVERLAPPING with PR #8288. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7709: KEEP. Same function, disjoint hunks and unrelated user goals. Files: src/kiro_crew/dashboard/chat_runner.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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

[bolichen97]

Rebased onto main 02d7a2d0 by a maintainer as part of the 2026-09-08 open-PR audit (this branch was ~475 commits behind).

Conflicts: none, clean rebase. Both commits replayed unchanged; the diff is still 2 files, +278/-9, and state._running_loop() / state.serving_loop both still exist on main, so the new _on_serving_loop helper needed no adjustment.

Gates run locally (changed files only): black --check, isort --check-only, flake8 all clean; pytest test/test_dashboard_chat.py for the 6 tests this PR adds (all pass) plus the 48 existing queue/synthesis/title tests that cover the touched functions (all pass).

Please review the rebased branch. Note that a maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed. Reply here if anything looks wrong.