fix(dashboard): serialize reload teardown under the switch locks (#9019)

[bolichen97]

Summary

Fixes #9019. api_chat_slot_reload tore down the slot's effective session while holding no lock, so a reload could interleave with a switch handler's commit-then-reset span on the same session. Nothing ordered the two: a switch could report success on a session that reload had already replaced, or vice versa.

The contract decision

The issue left an open premise: is skip_if_busy=True alone sufficient, or must reload join the switch's session-keyed lock? The answer is (b) — reload must be serialized. skip_if_busy=True declines atomically, but only while the session is BUSY, and a switch handler's commit-then-reset span is exactly the idle window in which that decline never fires. So the atomic decline cannot cover the span; reload has to take the lock.

Changes

src/kiro_crew/dashboard/chat_handlers.py

• api_chat_slot_reload now wraps its probe-then-teardown in a contextlib.AsyncExitStack acquiring the same two locks the four commit-before-reset switch handlers hold, in the same order: slot._lock, then _slot_switch_session_lock(session_key) with session_key = effective_session_key(slot) resolved inside slot._lock (resolving it earlier would guard the wrong session if a channel/cron rebind lands while the request queues).
• Both lock-acquisition awaits are each a suspension point where the slot's name can be recreated for a different app, so each is immediately followed by state._slots.get(name) is slot and a 404 on mismatch — the same re-authorization the tags/folders/regenerate handlers already do after their own await points (e.g. chat_tags.py). Without the first check, a stale request queued on slot._lock could authorize a teardown against a same-named replacement slot; without the second, the same gap reopens across the session-lock wait (real contention exists there too, when a switch on the same session holds it).
• Also after the session-lock wait: effective_session_key(slot) != session_key is re-checked and answers 409/session_rebound. The slot-identity check alone is insufficient here — effective_session_key reads the MUTABLE slot.linked_session_key, so a cron/channel rebind on the very same slot object (no swap, no recreation) can change what session the captured session_key names while this request sits on the lock. Mirrors the switch handlers' own post-lock effective_session_key(slot) != session_key guard (e.g. the model/agent/effort switch handlers).
• The same effective_session_key(slot) != session_key check runs a THIRD time, after the teardown call (_reset_slot_session, including its one retry) completes, still inside both locks. _bind_cron_slot writes slot.linked_session_key with no lock of its own, so a rebind is just as free to land during that specific await — one await later than the second guard's window — and reporting success there would be the same silent stale-session failure, just later.
• The has_active_turn() 409 fast path, the children 409 (_subagents_attached_response), the app-isolation denial, the reload:pre_reset seam point, and both _reset_slot_session(..., skip_if_busy=True) calls plus the retry-once 409 ladder now run under both locks.
• The slot-not-found 404 stays before the lock (no slot to lock). Success-only work (feed notice, eager respawn, push_slots_update) runs after the locks release, unchanged from before.
• Reload's external 409/200 contract and skip_if_busy=True semantics are unchanged apart from the new 409/session_rebound case above. The only other behavioral change is ordering: a concurrent switch on the same session now serializes behind reload (and vice versa) instead of interleaving.

test/test_slot_reset_interleaving_seam.py

• The two defect-pinning tests in TestReloadRacesSwitchCommitResetSpan were flipped and renamed to assert the serialized (correct) behavior. Each suspends one racer at a seam point while it holds the session lock, launches the other, and asserts (via a bounded _yield_until plus a "has not reached its own seam point" clause) that the second racer is blocked, then releases the first and asserts both finish in serialized order.
• Four more tests in the same class pin the identity/rebind re-checks independently: one holds slot._lock itself (reload queues on it before reaching its first re-check) and swaps in a same-named replacement slot, one holds the session lock directly via chat_handlers._slot_switch_session_lock and does the same swap after reload passes the first re-check, a third holds the session lock and instead mutates slot.linked_session_key on the SAME slot object (no swap) to pin the rebind guard, and a fourth hooks the reset:pre_pop seam INSIDE _reset_slot_session itself to mutate the link mid-teardown, pinning the third guard. Each asserts reload refuses (404 for the two identity cases, 409/session_rebound for the two rebind cases) rather than tearing down the wrong session.

comment-history-baseline.json

• test/test_slot_reset_interleaving_seam.py's entry drops from the pre-existing 8 to 0 (pruned) as a direct consequence of rewriting this PR's own added comments/docstrings to present tense, per the repo's shrink-only comment-history ratchet (scripts/check_comment_history.py --write-baseline, which only lowers or prunes, never raises).

Testing

All green:

• test/test_slot_reset_interleaving_seam.py — 11 passed (including the two flipped tests and the four new identity/rebind tests)
• test/test_chat_slot_switch_atomicity.py — 88 passed (no regression to the switch handlers)
• test/test_dashboard_chat.py::TestSessionReload — 13 passed (reload's external contract unchanged)
• flake8, black --check (py310), mypy, isort — clean on both changed files
• scripts/check_comment_history.py, scripts/check_sync_io_in_async.py, scripts/check_loop_bound_locks.py — clean

Test quality: reverting only the handler fix while keeping the flipped tests makes both tests fail, confirming they exercise the new lock serialization rather than static values. The reload span went from 0 async with (the bug) to 1 guarding both resets. All four identity/rebind tests were mutation-verified: removing any one of the four guards makes exactly its own test fail without affecting the other three.

Refs #8442

Pattern harvest

Not generalizable: reload was the one teardown path outside _slot_switch_session_lock (grepped _slot_switch_session_lock — 5 pre-existing sites already used the ExitStack two-lock template; grepped _reset_slot_session( — all other callers already ran under those locks). The fix joins the existing lock mechanism verbatim rather than introducing a new one. The four additions beyond the original diff — the two state._slots.get(name) is slot re-checks and the two effective_session_key(slot) != session_key rebind guards (one after the session lock, one after the teardown call itself) — are also not a new pattern: all four mirror guards the tags/folders/regenerate and switch handlers already carry elsewhere in this file; reload was simply the one handler in this family that had not been given the full set yet. The 5 pre-existing switch handlers share the identity-check gap (not the rebind gaps, which they already guard) and are not touched by this PR — tracked as #9383 rather than folded in here, since fixing them is a wider, separable change from what #9019 asked for.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 45efb16193d73ae6a31351c6de8de064282cfa42 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Reload joins the existing two-lock switch mechanism verbatim — root-cause fix, no new locking scheme, and the flipped tests were authored to be flipped.

The two rewritten tests were explicit defect pins whose own docstrings said "invert them and drop the 'unguarded' wording" once reload serialized — flipping them is the pre-arranged fix signal, not the erasure of a decision. Lock order matches all five pre-existing sites, the guards awaited inside the locks are synchronous, and the new 409/session_rebound case is additive and mirrors the switch handlers' contract.

Suggestions

• The three identity/rebind re-checks exist only because _bind_cron_slot and slot re-registration write with no lock — for 5 pre-existing slot-switch handlers lack the stale-slot-identity re-check reload just got #9383, consider making those writers take slot._lock/the session lock instead of propagating the per-await guard set to the five switch handlers; otherwise every future await added inside any of these spans silently reopens the hole.

[DESIGN-REVIEWED] 45efb16

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 45efb16193d73ae6a31351c6de8de064282cfa42 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 45efb16

Verdict parsed from the review's SHA-scoped output markers for commit 45efb16193d73ae6a31351c6de8de064282cfa42.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 45efb16193d73ae6a31351c6de8de064282cfa42: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 45efb16193d73ae6a31351c6de8de064282cfa42 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 45efb16

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 45efb16193d73ae6a31351c6de8de064282cfa42: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 45efb16193d73ae6a31351c6de8de064282cfa42 — 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 are complete. session_rebound pre-exists on 10+ switch-handler sites (not new surface), _bind_cron_slot at cron_inject.py:396 confirms the described cause, the 5 pre-existing two-lock sites count out, the flipped tests' deleted pins explicitly instructed the flip, and the pipeline-conductor hunks match main-side commit 53987e756 (#9346) byte-for-byte in the merge result. Final review:

First-Principles-Verdict: CONCERNS

A whole second feature — the fleet probe's age= field (#9346) — ships in this diff undeclared, duplicating main commit 53987e7.

Not justified as shipped

• Item 8 — undeclared, duplicate of main 53987e756 ("fix(pipeline-conductor): put the process age on the banned probe line (fix(pipeline-conductor): put the process age on the banned probe line #9346)"): the description never mentions fleet_probe.py, SKILL.md, or the 221-line test_pipeline_conductor_probe_banned_age.py.

What this change ships

Intent: stop a dashboard slot reload from interleaving with a settings switch's commit-then-reset on the same session (#9019) — a FIX.

1. Reload now waits behind (and blocks) a concurrent switch on the same session — justified
2. Reload answers 404 if the slot was recreated under its name while it waited — justified
3. Reload answers 409/session_rebound if a cron/channel rebind landed mid-request — justified
4. Both re-checks run a third time after the teardown itself — justified
5. The two api_chat_slot_reload tears down the effective session under no lock, so a reload can race a switch's commit-then-reset #9019 defect-pin tests flip to assert serialized order — justified
6. Five new tests pin the identity/rebind guards — justified
7. Comment-history baseline entry pruned — justified
8. BANNED probe lines gain age=, and mid-scan pid recycling downgrades cwd to unknown — undeclared, rides along, duplicate of 53987e756 on main

Watch

• The rider (item 8): the pins hold — the fix items alone remove the api_chat_slot_reload tears down the effective session under no lock, so a reload can race a switch's commit-then-reset #9019 defect, and main already carries the identical age change as fix(pipeline-conductor): put the process age on the banned probe line #9346, so this content's zero option costs nobody. It stops short of BLOCK only because the hunks may be base-drift of the diff computation (a stacked base), in which case they merge as nothing. Clears when: the branch is rebased so the PR diff shows only the dashboard fix, or the author confirms the pipeline-conductor hunks are byte-identical to merged fix(pipeline-conductor): put the process age on the banned probe line #9346.

Subtractions

• Drop the pipeline-conductor hunks (SKILL.md, fleet_probe.py, test_pipeline_conductor_probe_banned_age.py, the test_pipeline_conductor_agent.py _proc edit) from this branch — that change already exists on main as 53987e756.

[FIRST-PRINCIPLES-REVIEWED] 45efb16

[bolichen97]

• span=27afc892be32 F1 (src/kiro_crew/dashboard/chat_handlers.py:7387 — stale slot can authorize teardown of its replacement) — fixed in 49ef790b0.

Confirmed against source: slot = state._slots.get(name) is read before any await, and the original diff acquired slot._lock without re-validating that name still maps to that same object. The sibling teardown handlers (chat_tags.py's state._slots.get(name) is not slot guard, and the analogous checks in chat_folders.py / chat_regenerate.py) already close this exact window after their own await points, so reload was simply the one handler in this family missing it.

Fix: immediately after await _stack.enter_async_context(slot._lock), re-check state._slots.get(name) is slot and return 404 on a mismatch, before resolving session_key or running any teardown. Added test_reload_refuses_a_slot_recreated_under_the_same_name_while_it_queued, which holds slot._lock itself, launches reload so it queues on the lock, swaps in a same-named replacement slot, releases the lock, and asserts reload returns 404 without touching the replacement's session. Mutation-verified: removing the re-check makes the new test fail (200 instead of 404).

self-added: no
mechanism: post-lock-acquisition slot-identity re-check, reusing the existing is not slot pattern from sibling handlers

[bolichen97]

• span=27afc892be32 F1 (src/kiro_crew/dashboard/chat_handlers.py:7411 — session-lock await reopens the cross-slot-identity gap the slot-lock re-check closed) — fixed in e427b0ae1.

Confirmed against source: the first is not slot re-check guards only the slot._lock await; await _stack.enter_async_context(_slot_switch_session_lock(session_key)) is a second real suspension point (a concurrent switch on the same session holds that lock), and nothing re-validated slot identity between resuming from it and the _app_cancel_denied call that follows.

Fix: added a second state._slots.get(name) is not slot re-check immediately after the session-lock acquisition, before any authorization call reads from slot. This fix covered only the identity half — a later round found session_key could still go stale via an in-place rebind on the SAME slot object (no swap), tracked under this same span and fixed separately in 2ba8ee1ef (see the later disposition on this thread). Added test_reload_refuses_a_slot_recreated_while_queued_on_the_session_lock, which acquires the same session lock externally via chat_handlers._slot_switch_session_lock, lets reload pass the first re-check and queue on the session lock, swaps in a same-named replacement, releases, and asserts reload refuses with 404. Mutation-verified: removing this second re-check (while keeping the first) makes the new test fail (200 instead of 404) without affecting the first test.

self-added: yes
mechanism: second post-await slot-identity re-check, same shape as the first, after the session-lock acquisition

[bolichen97]

Watch (the identity re-check lands only in reload; the 5 pre-existing _slot_switch_session_lock sites share the exact queueing window) — accepted-and-deferred, tracked as #9383.

Legitimate: confirmed against source that api_chat_slot_agent (5643), api_chat_slot_model (6253), and the bulk/effort/workspace switch handlers (6908/7101/7526) all queue on slot._lock (some also _model_pick_lock) with no state._slots.get(name) is slot re-check anywhere in their lock spans — the same shape this PR just closed for reload.

Not proportional to fold into this PR: #9019's stated scope is "reload joins the switch lock so the two serialize," not "audit and harden every _slot_switch_session_lock acquisition site in the codebase." Widening this PR to touch 5 more handlers untouched by its own diff, each needing its own mutation-verified regression test, is a materially larger and separable change from what was asked. Filed #9383 (label deferred-finding, assignee bolichen97, Due: 2026-09-22) naming all 5 sites, the exact re-check shape to mirror, and the rarity conditions carried over from GPT's adjudication on this PR's own finding.

self-added: no

[bolichen97]

• span=27afc892be32 F1 (src/kiro_crew/dashboard/chat_handlers.py:7461 — teardown await reopens the rebind race) — fixed in e14ad6d6a.

Confirmed against source and against the adjudication's own citation: _bind_cron_slot (cron_inject.py:396) writes slot.linked_session_key with no lock of its own, so it can run while reload is suspended inside _reset_slot_session's await (or its one retry) — one await later than the session-lock rebind guard's window, and past it. Reporting success there would leave the now-current session untouched while claiming otherwise.

Fix: after the teardown call (and its retry) completes, still inside both locks, re-run the same effective_session_key(slot) != session_key check and answer 409/session_rebound on a mismatch — the third instance of the guard the switch handlers already carry. Added test_reload_refuses_a_rebind_that_lands_during_the_reset_await, which hooks the reset:pre_pop seam point INSIDE _reset_slot_session (reached only after both earlier guards already passed) to mutate slot.linked_session_key there, then asserts reload still refuses with 409/session_rebound even though the reset itself already ran against the original key. Mutation-verified: removing this third guard alone (keeping the other three) makes the new test fail (200 instead of 409) without affecting the other three guard tests.

self-added: yes
mechanism: third instance of the effective_session_key(slot) != session_key rebind guard, placed after the teardown call instead of before it