Skip to content

fix(dashboard): close three residual reload teardown races - #9236

Closed
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/slot-reload-lock-9019
Closed

fix(dashboard): close three residual reload teardown races#9236
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/slot-reload-lock-9019

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What changed

The reload teardown is already serialized under the switch locks on main, so that
part of this work is done and is no longer in this diff. Three gaps that review of
this work surfaced are still open, and this PR closes those and nothing else.

1. The turn-in-flight guard was slot-scoped on a session that can be shared.
An alias pair carries the same linked_session_key, so two slots run their turns
on ONE session. A sibling's cold-starting turn is invisible to this slot's guard:
that slot's running is a different object, and its provider.start() has not
registered a session yet, so get_provider returns nothing either. Reload tore the
shared session down, answered 200, and the sibling then registered its stale-config
provider -- the exact failure this endpoint exists to prevent.

The guard now asks the session-scoped question at both decision points, the
pre-reset guard and the declined-reset ladder:

if (
    slot.running
    or session_key in state.running_session_keys()
    or (provider is not None and provider.has_active_turn())
):

running_session_keys() folds every slot through effective_session_key, so it
sees the sibling's cold start that neither slot-scoped probe can.

2. A speculative session creation already in flight outlived the teardown.
schedule_eager_spawn cancels a slot's prior _eager_spawn_task as a side effect
of scheduling a new one, but reload suppresses that call for a linked session (see
3), so a focus prefetch already mid-handshake survived the reset and registered a
stale-config session AFTER reload reported success. Reload now cancels and AWAITS
that task before the reset -- awaits, because the prefetch can be mid
get_or_create, and a bare cancel leaves a window in which it still registers.

The cancel is placed BEFORE the turn guard deliberately. It is the only real
suspension point between resolving the session and the reset, so a send could
otherwise start a turn during the await, after the guard had already passed, and
_reset_slot_session runs _unblock_pending_waits unconditionally BEFORE its
skip_if_busy decline -- so that turn's approval card would be discarded even
though the decline then answers 409. Cancelling first, then reading the guard with
no await before the reset, keeps the guard's answer true at the moment of teardown.
Re-authorization across that new await goes through the existing
_reauthorize_after_await helper, which carries the identity check on the slot
OBJECT, the indistinguishable 404, the SEL app_isolation denial record, and a
re-run of the ownership gate.

3. The eager respawn escaped the session lock for shareable sessions.
It is scheduled after the locks release, on purpose -- it debounces and then
handshakes for seconds, and holding the session lock across that would serialize an
unrelated switch behind work that does not race the teardown. But for a LINKED
session another slot can share it, and that slot's per-slot lock is disjoint from
this one's, so the escaped respawn could win get_or_create's same-key race and
bake this slot's bindings into a session an alias slot's switch just committed
different bindings for; the first turn then runs the wrong model and config. Reload
now suppresses the speculative respawn for a linked session. That is safe because
the session is already torn down, so the next real turn cold-starts under whatever
bindings are current at that moment -- the race-free outcome. A plain dashboard:
slot with no link keeps the speculative respawn.

Tests

Each guard is pinned by a test that reddens when that guard alone is removed,
verified by disabling them one at a time:

  • TestReloadRefusesAnAliasColdStartOnTheSharedSession -- a running turn on the
    shared key with no slot.running and no registered provider must answer 409 and
    must not reset. Removing the session-scoped clause reddens it.
  • TestReloadCancelsInFlightEagerSpawnBeforeReset -- the prefetch is cancelled and
    awaited before the reset. Removing the cancel reddens it.
  • TestReloadRechecksTurnStateAfterEagerCancelAwait -- a turn becoming busy DURING
    the cancel-await is still caught before the reset.
  • TestReloadRevalidatesSlotIdentityAfterEagerCancel -- a same-name slot
    replacement during the cancel-await answers 404 and does not reset the
    replacement's session. It asserts a slot IS registered under the name, so it
    tests identity rather than presence.
  • test_reload_suppresses_eager_respawn_for_a_linked_session -- removing the
    suppression reddens it.

120 pass across the seam file, TestSessionReload, and the switch-atomicity suite.

Declared residuals

Neither is chased here, and both are tracked on #9255.

  • The prefetch cancel is scoped to THIS slot. An alias sibling's prefetch on the
    same key is not cancelled, and cancelling every sharer cannot be made correct
    from inside this endpoint: schedule_eager_spawn is also reached from the WS
    focus path, which holds neither of this handler's locks, so a focus frame
    arriving during such a scan re-arms a prefetch on a slot the scan already passed.
    Closing it needs the prefetch mechanism itself to refuse or defer a prefetch for
    a session whose teardown is in flight.
  • The same-name slot replacement window also exists in the four switch handlers.
    Its root cause is close_slot popping state._slots without taking
    slot._lock, so ordering that pop is the real fix rather than adding a re-check
    to each handler.

Refs #9019, #9255.

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 7, 2026 12:55
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Three narrow races closed at the correct scope (session, not slot), root causes of the two declared residuals named and tracked — sound and proportionate.

[DESIGN-REVIEWED] 9b4e314

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 9b4e314a6b4e4d4922f0e232519af22c4fbd7ff4 — 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 verification is done. The description's claims check out — running_session_keys(), _reauthorize_after_await (6 prior call sites), the delete-path cancel, and schedule_eager_spawn's cancel-prior side effect all exist as described, and each added test readably fails on base. The one gap: the alias cold-start guard is added only at reload, while four switch-handler guard sites keep the slot-scoped probe on the same shareable sessions, and that residual is not among the two the PR declares.

First-Principles-Verdict: CONCERNS

The alias cold-start gap is closed only at reload; four switch-handler guards keep the slot-scoped probe on the same shareable sessions, undeclared.

Not justified as shipped

Item 1 is justified for reload itself, but it is a point patch: the root cause — turn state probed per-slot while sessions are shareable — has 4 unfixed sibling guard sites this PR neither fixes nor declares (see Watch).

What this change ships

Inventory (5 items) — 5 justified

Intent: stop a dashboard reload from tearing down a session an alias sibling is using or about to use, and from respawning it with stale bindings — a FIX.

  1. Reload now answers 409 when an alias sibling's turn runs on the shared session — justified
  2. The declined-reset ladder gives the same session-scoped 409 — justified
  3. Reload cancels and awaits this slot's in-flight prefetch before tearing down — justified
  4. A slot replaced during that new await now gets 404 instead of a teardown — justified
  5. A linked-session reload no longer pre-warms; the next turn cold-starts — justified

Watch

  • The session-scoped clause lands only in reload. Grepping slot.running or (isinstance(... has_active_turn()) finds 4 sibling guard sites — chat_handlers.py:5846, 6327, 6461, 7234 (workspace/model/effort switches) — all slot-scoped on keys resolved via effective_session_key, so an alias sibling's cold-starting turn is invisible to them exactly as it was to reload, and skip_if_busy misses it for the same no-registered-provider reason this PR documents. The two declared residuals cover the prefetch scan and the replacement window, not this. Clears when: the four guards consult running_session_keys(), or Slot-switch handlers use a slot-scoped turn guard, missing an alias cold-start on a shared session #9255 (or a sibling issue) records this residual.

[FIRST-PRINCIPLES-REVIEWED] 9b4e314

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 9b4e314a6b4e4d4922f0e232519af22c4fbd7ff4 — this comment is updated in place on each push.

Review details

I've read the handler, the two candidates, and traced both against the actual code.

Candidate 1 (missing effective_session_key re-check after await _eager): The claimed harm is that the reset runs on the captured session_key (key A) after a rebind to key B during the await, tearing down a stale/shared session before the 7612 check catches the mismatch. But this exact property already exists in the pre- and post-PR design: the reset at 7562 runs on the captured session_key, and the rebind is only caught after the reset by the check at 7612 — meaning a rebind landing during the reset-await itself produces the identical "reset A, then return 409" outcome. The new await _eager does not open a new class of harm; it shares the one the existing reset-await already has and the design accepts. Moreover the observable outcome is benign: skip_if_busy=True skips a busy key A, and the session-scoped turn guard covers a sibling's active turn, so only an idle session is torn down — a cold restart with conversation preserved, not data loss/corruption/crash. (a)/(b) hold but (c) is not a wrong outcome that meets the bar. Below 80. Dropped.

Candidate 2 (except (asyncio.CancelledError, Exception) swallowing the handler's own cancellation): The await _eager legitimately raises CancelledError as the awaited task's expected cancellation, which must be swallowed. An externally-cancelled handler at this point would then complete the teardown — but the teardown is the endpoint's entire purpose, conversation is preserved, and there is no credential exposure, data loss, corruption, or crash. The candidate self-rates "low" and cannot establish aiohttp cancels the handler here. (c) is not an observable wrong outcome. Below 80. Dropped.

No Step 2 findings survive the same bar.

No findings.

[OPUS-REVIEWED] 9b4e314

Verdict parsed from the review's SHA-scoped output markers for commit 9b4e314a6b4e4d4922f0e232519af22c4fbd7ff4.

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

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 9b4e314a6b4e4d4922f0e232519af22c4fbd7ff4 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 9b4e314

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from 88d120e to 0bff425 Compare September 7, 2026 13:11
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Head 0bff425 addresses the GPT 5.6 blocking finding (alias eager respawn escaping session serialization) and the First Principles CONCERNS note, both bound to the prior head 88d120e.

GPT finding -- eager respawn escaped serialization for alias slots. The finding is real. The eager respawn is scheduled AFTER the session lock releases and then debounces + handshakes for seconds, so it does not run under the lock. For a session another slot can share (linked_session_key set -- a channel/cron slot, or an alias pair whose per-slot locks are disjoint), that escaped respawn could win get_or_create's same-key race and bake this slot's bindings into a session an alias slot's switch just committed different bindings for; the first turn then runs the wrong model/config. This is the #8442 divergent-bindings harm class reaching through the shared respawn -- distinct from the #9019 teardown-ordering class this PR set out to close, and it pre-exists identically in the four switch handlers, which also schedule_eager_spawn outside their lock.

Fix (GPT's own remedy 1, narrowest in-scope form): reload suppresses the speculative respawn when the session is linked, i.e. if not slot.linked_session_key: schedule_eager_spawn(...). Safe because reload has already torn the session down, so the next real turn cold-starts under whatever bindings are current at that moment -- the correct race-free outcome. The common reload (a plain dashboard: slot with no link) keeps the speculative respawn unchanged, so nothing changes for the case that is not shareable. Test test_reload_suppresses_eager_respawn_for_a_linked_session pins it and reddens (eager called 1 time) if the guard is removed.

The switch handlers' identical outside-the-lock respawn for linked sessions is left unchanged here -- it is their pre-existing behavior and a cross-handler change to the shared eager-respawn contract is out of this PR's teardown-ordering scope. Worth a separate issue.

First Principles CONCERNS -- declaring the slot.running guard. Correct that reload now also 409s when slot.running (a cold-starting first turn, provider not yet registered). It is derived, not new surface: it mirrors the switch handlers' own guard and the rationale at chat_handlers.py:5657-5662 (slot.running is set at dispatch before provider.start registers a session, so a cold-starting turn is invisible to get_provider). Calling it out so "exactly like the switch handlers do" covers the guard as well as the locks. The PR body will be updated to state both once the board has no pending lanes.

Security posture: this remains a concurrency-ordering fix over existing in-process locks and the existing eager-respawn contract; it adds no new external input, credential path, or surface. The organizational security-guidance search is unreachable from this session; the query I would have run is "session teardown eager respawn concurrency shared session bindings".

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from 0bff425 to da10586 Compare September 7, 2026 13:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Head da10586 addresses the GPT 5.6 blocking finding (a concurrent slot rebind leaving reload targeting a stale session), the prescribed fix, plus the docstring nit.

Atomicity enumeration

Reload's operation is four steps: resolve the effective session key -> reset (tear the session down) -> rebuild (eager respawn) -> respond. The pairs that must not interleave, and this PR's coverage of each:

  1. (resolve, reset) vs a concurrent SWITCH handler's commit-then-reset on the SAME session key -- COVERED. Reload now takes slot._lock then _slot_switch_session_lock(session_key), the switch handlers' two locks in their order. Same-slot is ordered by slot._lock; alias slots sharing one session (disjoint slot._lock) are ordered by the session-keyed lock.
  2. (resolve -> reset) vs a REBIND of linked_session_key that lands during the reset await -- COVERED by this commit. linked_session_key is bound outside reload's locks (an unbound cron/workflow slot is linked when its first result is injected), so a rebind can land while reset awaits: reload tore down the resolved OLD key while the slot now runs a different live session. Fix: recheck effective_session_key(slot) after the reset and answer session_rebound 409 on mismatch -- a compare-and-set at the commit point, no new lock, no change to anyone else's contention. Mirrors the switch handlers at chat_handlers.py:5909 and siblings.
  3. (reset, rebuild/eager-respawn) for alias slots -- COVERED (prior commit). The eager respawn is scheduled after the locks release and handshakes for seconds, so it escapes the session lock; for a linked (shareable) session it could bake this slot's bindings into a session an alias switch just re-prepared. Reload suppresses the speculative respawn for a linked session.
  4. (rebuild, respond) -- N/A. Both touch only this slot's own state (feed notice, slots push); no shared-resource race.

Declared residual (out of scope, NOT chased)

A rebind can still land AFTER the step-2 recheck but before the 200 response is written. The switch handlers carry the identical single-recheck residual (chat_handlers.py:5909 is their last read before responding), so this is the repo's accepted commit-point shape, not new to reload. Consequence if it lands: reload returns 200 for a session that was the slot's at recheck time but was re-linked microseconds later; the next real turn cold-starts the now-current session correctly, so the window is a stale 200 with no teardown of a wrong live session (the reset already ran on the key that was correct when it ran). Closing it would need the rebind writer (result injection) to take the session lock -- a cross-cutting change to that path, out of this teardown-ordering item's scope. Declared here rather than chased.

Second (non-blocking) finding -- docstring

chat_handlers.py:7344 said "then eagerly re-arm the resume spawn" while the linked-session branch suppresses it. Qualified to "(for a session this slot alone owns; a linked session suppresses it, see the respawn site)".

Tests

  • test_rebind_landing_during_reset_answers_session_rebound_409 (new): drives a rebind at the seam's reset:post_pop point and asserts reload answers session_rebound 409; reverting the recheck reddens it (assert 200 == 409). Plus a no-rebind control that answers 200.
  • Seam file 10 passed, TestSessionReload 14 passed (-n0). black --target-version py310 / isort / flake8 clean on both changed files.

The security-guidance search is unreachable from this session; the query I would have run is "session rebind teardown concurrency compare-and-set dashboard". This remains a data-integrity concurrency fix over existing in-process locks, adding no permission gate, credential, sandbox, redaction, or injection surface.

The atomicity enumeration and residual above will be folded into the PR body once the board has no pending lanes (a body edit can cancel in-flight runs).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from da10586 to 6e00037 Compare September 7, 2026 14:07
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Head 6e00037 addresses the GPT 5.6 blocking finding on da10586 (linked alias cold starts bypass the reload guard).

Finding, verified: reload's turn-in-flight guard was slot-scoped (slot.running or provider.has_active_turn()). For a SHARED session (an alias pair, both slots carrying the same linked_session_key), an alias slot A's cold-starting first turn is invisible to THIS slot B's guard -- B's slot.running is a different object, and A's provider.start() has not registered a session yet so get_provider returns None. B then tore down the shared session and returned 200, and A registered its pre-reload provider with stale config.

Fix (GPT's prescription): the guard is now SESSION-scoped, adding session_key in state.running_session_keys() at both the pre-reset guard and the declined-reset disambiguation. running_session_keys() folds every slot through effective_session_key, so it sees an alias slot's cold-starting turn on the shared key that neither slot-scoped probe can. No new lock; uses an existing method.

This also RESOLVES the earlier First Principles CONCERNS about the added slot.running guard: the correct property was never "is THIS slot running" but "is any slot running a turn on this session," and the guard now tests that. (The single most common defect class this repo names: prefer the property that determines the outcome over a slot-scoped re-derivation.)

Atomicity enumeration update: this tightens pair 1 (resolve/reset vs a turn on the session) to cover the cold-start-on-a-shared-session case, in addition to the lock ordering. Pairs 2 (rebind-during-reset compare-and-set) and 3 (alias eager-respawn suppression) unchanged. The declared out-of-scope residual (a rebind landing after the final recheck, identical to the switch handlers' single-recheck shape) still stands.

Tests: test_running_turn_on_the_shared_session_refuses_reload (new) -- a running turn on the shared key with no slot.running and no registered provider must 409 turn_in_flight and NOT reset; reverting the session-scoped clause reddens it (assert 200 == 409). Plus an empty-set control that answers 200. Seam file 12 passed, TestSessionReload 14 passed (-n0). black --target-version py310 / isort / flake8 clean.

Note the switch handlers (chat_handlers.py:5665 and siblings) carry the identical slot.running-only guard, so this same alias cold-start gap pre-exists there; a cross-handler tightening is out of this item's scope and worth a separate issue. This remains a data-integrity concurrency fix; no permission gate, credential, sandbox, redaction, or injection surface. Security-guidance search unreachable from this session; the query would have been "session-scoped running turn guard alias cold start dashboard".

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@chenmingwei23

chenmingwei23 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • Framing gap: the respawn comment contradicted the linked-session suppression (fixed; comment corrected in-tree)

Correct catch. The comment saying the post-lock work "touches only this slot's
own state" is true of the feed notice but NOT of the eager respawn, which for a
linked session can register the shared session from this slot's bindings -- that
is the whole reason the respawn is suppressed for a linked session. The comment
was imprecise, not the code: the suppression IS the reconciliation. Corrected
the comment in-tree to say the notice is slot-local while the respawn is not,
which is why only the respawn is guarded; the corrected comment lands on the
next push (held while lanes are pending so a body/code edit does not cancel
in-flight runs). Comment-only, no behaviour change.

@chenmingwei23

chenmingwei23 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

The four guards are the complete set reload's teardown needs to be atomic
against a concurrent switch/rebind/cold-start on the SAME session (issue #9019's
teardown-ordering class): two-lock serialization; post-reset effective_session_key
recheck (session_rebound 409); linked-session respawn suppression; session-scoped
running_session_keys() guard for an alias cold-start. Each was added because a
review round proved that pair reachable for reload.

The identical alias cold-start gap DOES exist in the four switch handlers. Not
fixed here: proportionality -- a five-handler concurrency change is wider than a
single-endpoint teardown-ordering fix, and those handlers are a pre-existing
shape this PR did not introduce. The reload clause is still needed on its own
terms (the data-loss-class finding on that pair cleared only with it present).
Deferred to a tracked issue rather than rebutted, since First Principles agrees
the general fix is larger than this endpoint: filed as #9255 (deferred-finding
label, assignee, Due date), which proposes adding running_session_keys() to each
switch handler's guard with an alias two-slot test mirroring reload's.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from 6e00037 to 8510470 Compare September 7, 2026 14:52
@chenmingwei23

Copy link
Copy Markdown
Contributor Author
  • Revalidate slot identity after eager cancellation (span=78d6e248f252; fixed in 4210c52)

Real, and I verified every condition from code rather than taking the summary.
Reload resolves slot = state._slots.get(name) BEFORE it acquires slot._lock;
close_slot pops that mapping WITHOUT taking slot._lock; and the app-ownership
check runs before the only real suspension point in the handler, the
cancel-and-await of the in-flight eager prefetch. So a same-name delete can
complete while this request holds the old slot's lock, a recreate can bind a new
slot to the same session key, and the teardown then acts on a key that was
authorized against a slot the registry no longer points at -- tearing down a
replacement owner's idle session across the App Kit isolation boundary.

Fix, as prescribed: after the cancel-await and before the reset, require
state._slots.get(name) is slot and otherwise return the same indistinguishable
slot_not_found 404 a missing slot gets, so the response cannot reveal which
foreign slots exist. IDENTITY, not presence: a same-name successor is a different
object, so a truthiness or membership test passes vacuously here -- the same
distinction _eager_spawn already makes on its own slot before it trusts a
session it registered.

This is the same hazard as the slot.running clause I defended earlier in this
review, applied to a different piece of state: a value read before a suspension
stops meaning what it meant once the slot can be rebound or replaced across it.
There it was the session key; here it is the registry entry.

Test test_same_name_replacement_during_cancel_await_answers_404 swaps the
registry entry for a different _ChatSlot under the same name during the
cancel-await, then asserts 404 slot_not_found AND that sessions.reset was
never awaited, so the replacement's session is untouched. It also asserts a slot
IS registered under the name, which is what makes it a test of identity rather
than presence. Disabling the check reddens it (assert 200 == 404). A control
pins that an unreplaced slot still reaches 200. Seam file 16 passed, plus
TestSessionReload and the switch-atomicity suite (118 total), -n0.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from 4210c52 to c2e36ba Compare September 8, 2026 04:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 8, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author
  • Channel cold start defeats all three reload guard probes (span=78d6e248f252; rebutted with the ordering record the review could not open)

The review fenced this rather than flagged it, on an explicit gap: it could not
establish from the code it opened whether the channel path sets slot.running
before the slot is visible to reload. That ordering is establishable, and it
closes the window. Supplying the record.

spawn_guarded_turn (turn_dispatch.py:431-453) only calls
asyncio.create_task(bounded). A created task does not execute until the event
loop next gets control, so the turn coroutine's first step -- and therefore
provider.start(), which is what registers a session -- cannot run before the
creating step finishes.

All three channel dispatch sites assign slot.task SYNCHRONOUSLY after that
call, with no await between:

  • slack/gateway.py:6614 then :6617, carrying the comment "Mirror dashboard
    /api/chat/send path so slot.running == True ... immediately"
  • slack/gateway.py:3663 then :3672 (cron inject)
  • dashboard/handlers/messaging.py:2297 then :2308 (channel inject)

slot.running is task is not None and not task.done() (state.py:4633-4635),
so it is True the instant that assignment lands -- in the same event-loop step as
task creation, strictly before any session registration. The state the finding
needs (session registered, running still False) is therefore unreachable for a
TURN on the channel path.

Both sub-windows are harmless, which is the part that makes this a rebuttal
rather than a rarity argument:

  • BEFORE dispatch, the slot is visible but no session is registered. Reload's
    reset finds nothing (had_live_session=False) and the later turn cold-starts.
    The agent spec and env are read at session-init INSIDE the turn -- the very
    mechanism this endpoint exists to exploit -- so that turn gets POST-reload
    config. Correct, not stale.
  • AT or AFTER dispatch, slot.running is True, so the guard's first probe
    answers 409 turn_in_flight.

The finding's premise about running_session_keys() is accurate --
slot_registry.py:77-88 does fold only slots where slot.running is already
True -- but that clause exists to catch an ALIAS slot's turn on a shared key,
not to substitute for the dispatch ordering above.

A registered session with running False does have one real producer: the
speculative eager PREFETCH, which registers a session with no turn. That is not
the channel path, and it is already handled here -- reload cancels AND awaits
this slot's _eager_spawn_task before the reset, and suppresses the respawn for
a linked session. The residual is the per-slot scope of that cancel (an alias
sibling's prefetch), declared on this PR and tracked with its call-site inventory
in issue #9255, because closing it needs the prefetch mechanism itself to become
session-aware rather than a patch inside this endpoint.

No code change. If the ordering above is wrong at any of the three cited sites I
will fix rather than argue, but the citations are direct and the Slack site
documents the intent.

@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from c2e36ba to bb2fb29 Compare September 8, 2026 05:14
@chenmingwei23

Copy link
Copy Markdown
Contributor Author
  • The slot-identity 404 recheck hand-rolls _reauthorize_after_await and drops its SEL denial log (fixed by subtraction)

Accepted in full, and the counts are right. _reauthorize_after_await
(chat_handlers.py:10027) already makes exactly this decision: identity on the
slot OBJECT via state._slots.get(name) is not slot, the indistinguishable
_slot_not_found() 404, and the SEL app_isolation denial record my inline copy
did not write. Its docstring even carries the reasoning I had restated by hand,
down to the delete-and-recreate case.

The inline copy is replaced by the helper call, so the check now has ONE spelling
in the file. Two things came back for free by deleting my version:

  • the SEL app_isolation denial event, which my copy silently dropped -- an
    audit gap, since this is the branch that refuses a foreign app's teardown;
  • a re-run of the ownership gate, which my copy did NOT do. It re-checked
    identity only, so an app whose claim changed across the await would still have
    been carrying its pre-await authorization. The helper closes both halves.

That makes this a strict subtraction: fewer lines here, more coverage, and the
divergence you flagged cannot reappear because there is no second spelling left.

Item 5 is also now declared rather than a rider. The PR body enumerates it with
the other four pairs, and the description will name it in the same list as the
rest.

On the Watch item -- the four switch handlers awaiting in the same window with no
re-check -- you are right that the root cause is close_slot popping
state._slots without taking slot._lock, so this is a point patch on a shared
defect. Tracked on issue #9255 with the verified inventory (the helper's existing
consumers: autocompact x2, context inject, note post, edit resend; the four
unfixed switch handlers; and both candidate shapes -- add the helper call per
handler, or order the pop against slot._lock, which is the real fix). Not
chased here because each switch handler carries rollback machinery reload does
not, so the placement relative to commit-and-rollback needs its own reasoning per
handler.

Tests unchanged and still pinning: the same-name replacement test asserts 404 and
that sessions.reset was never awaited, and disabling the helper call reddens it
(assert 200 == 404). 30 passed across the seam file and TestSessionReload.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 8, 2026
The reload teardown is already serialized under the switch locks. Three gaps
that review of this work surfaced are still open.

* The turn-in-flight guard is slot-scoped, so an alias sibling's cold-starting
  turn on the SAME session is invisible to it: that slot's `running` is a
  different object and its turn has not registered a provider yet. Reload tears
  the shared session down, returns 200, and the sibling then registers its
  stale-config provider. The guard now reads `slot.running` and the
  session-scoped running set alongside the registered provider, at both the
  pre-reset guard and the declined-reset ladder.

* A speculative session creation already in flight survives the teardown and
  registers a stale-config session after reload reports success. Reload now
  cancels AND awaits the slot's `_eager_spawn_task` before the reset, placed
  before the turn guard so the guard's answer is still true at the moment of the
  teardown, and re-authorizes across that await with the shared
  `_reauthorize_after_await` helper.

* The eager respawn is scheduled after the locks release, so for a LINKED
  (shareable) session it can win `get_or_create`'s same-key race and bake this
  slot's bindings into a session an alias slot's switch just committed different
  bindings for, and the first turn then runs the wrong model and config. Reload
  suppresses the speculative respawn for a linked session; the next real turn
  cold-starts under whatever bindings are current then.

Each guard is pinned by a test that reddens when that guard alone is removed.

Two residuals stay declared rather than chased: the per-slot scope of the
prefetch cancel (an alias sibling's prefetch is not cancelled, and making that
correct needs the prefetch mechanism itself to become session-aware), and the
same-name slot replacement window in the four switch handlers, whose root cause
is `close_slot` popping `state._slots` without taking `slot._lock`.

Refs #9019, #9255.
@chenmingwei23
chenmingwei23 force-pushed the fix/slot-reload-lock-9019 branch from bb2fb29 to 9b4e314 Compare September 9, 2026 03:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 9, 2026
@chenmingwei23 chenmingwei23 changed the title fix(dashboard): serialize slot reload onto the switch session lock fix(dashboard): close three residual reload teardown races Sep 9, 2026
@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Sep 9, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Closing this PR: its original purpose is already on main.

Commit 2629001c2 -- "fix(dashboard): serialize reload teardown under the switch
locks (#9019) (#9337)" -- landed the same fix in the same three files, so #9019 is
resolved without this branch. That overlap is what made all three files conflict:
both sides had added the same thing.

For the record, three guards that this PR's review rounds surfaced are NOT on main,
and close with it:

  1. The turn-in-flight guard is slot-scoped on a session that can be shared. An
    alias pair carries the same linked_session_key, so a sibling's cold-starting
    turn is invisible -- its running is a different object and it has not
    registered a provider yet. Reload tears the shared session down, answers 200,
    and the sibling then registers a stale-config provider. Fix was to read
    slot.running and state.running_session_keys() alongside the provider probe,
    at the pre-reset guard and the declined-reset ladder.
  2. A speculative session creation in flight outlives the teardown. A focus
    prefetch already mid-handshake survives the reset and registers a stale-config
    session after reload reports success. Fix was to cancel AND await the slot's
    _eager_spawn_task before the reset, placed before the turn guard so the
    guard's answer is still true at the moment of teardown, with re-authorization
    across that await via _reauthorize_after_await.
  3. The eager respawn escapes the session lock for shareable sessions. For a
    linked session it can win get_or_create's same-key race and bake this slot's
    bindings into a session an alias slot's switch just committed different bindings
    for, so the first turn runs the wrong model and config. Fix was to suppress the
    speculative respawn for a linked session; the next real turn cold-starts under
    the bindings current then.

Each had a test that reddened when that guard alone was removed. The work is
readable in this PR's final head 9b4e314a6b4e4d4922f0e232519af22c4fbd7ff4 on
branch fix/slot-reload-lock-9019 if anyone wants to pick it up.

The two residuals recorded on #9255 stand on their own and are unaffected: the
per-slot scope of the prefetch cancel, and the same-name slot replacement window in
the four switch handlers whose root cause is close_slot popping state._slots
without taking slot._lock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant