fix(dashboard): close three residual reload teardown races - #9236
fix(dashboard): close three residual reload teardown races#9236chenmingwei23 wants to merge 1 commit into
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of 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 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All verification is done. The description's claims check out — 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 shippedItem 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 shipsInventory (5 items) — 5 justifiedIntent: 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.
Watch
[FIRST-PRINCIPLES-REVIEWED] 9b4e314 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've read the handler, the two candidates, and traced both against the actual code. Candidate 1 (missing Candidate 2 ( 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 False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
88d120e to
0bff425
Compare
|
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. 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". |
0bff425 to
da10586
Compare
|
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 enumerationReload'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:
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 -- docstringchat_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
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). |
da10586 to
6e00037
Compare
|
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 ( Fix (GPT's prescription): the guard is now SESSION-scoped, adding This also RESOLVES the earlier First Principles CONCERNS about the added 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". |
|
|
6e00037 to
8510470
Compare
bc2ca02 to
4210c52
Compare
|
4210c52 to
c2e36ba
Compare
|
c2e36ba to
bb2fb29
Compare
|
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.
bb2fb29 to
9b4e314
Compare
|
Closing this PR: its original purpose is already on main. Commit For the record, three guards that this PR's review rounds surfaced are NOT on main,
Each had a test that reddened when that guard alone was removed. The work is The two residuals recorded on #9255 stand on their own and are unaffected: the |
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 turnson ONE session. A sibling's cold-starting turn is invisible to this slot's guard:
that slot's
runningis a different object, and itsprovider.start()has notregistered a session yet, so
get_providerreturns nothing either. Reload tore theshared 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:
running_session_keys()folds every slot througheffective_session_key, so itsees the sibling's cold start that neither slot-scoped probe can.
2. A speculative session creation already in flight outlived the teardown.
schedule_eager_spawncancels a slot's prior_eager_spawn_taskas a side effectof 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_sessionruns_unblock_pending_waitsunconditionally BEFORE itsskip_if_busydecline -- so that turn's approval card would be discarded eventhough 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_awaithelper, which carries the identity check on the slotOBJECT, the indistinguishable 404, the SEL
app_isolationdenial record, and are-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 andbake 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 theshared key with no
slot.runningand no registered provider must answer 409 andmust not reset. Removing the session-scoped clause reddens it.
TestReloadCancelsInFlightEagerSpawnBeforeReset-- the prefetch is cancelled andawaited before the reset. Removing the cancel reddens it.
TestReloadRechecksTurnStateAfterEagerCancelAwait-- a turn becoming busy DURINGthe cancel-await is still caught before the reset.
TestReloadRevalidatesSlotIdentityAfterEagerCancel-- a same-name slotreplacement 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 thesuppression 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.
same key is not cancelled, and cancelling every sharer cannot be made correct
from inside this endpoint:
schedule_eager_spawnis also reached from the WSfocus 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.
Its root cause is
close_slotpoppingstate._slotswithout takingslot._lock, so ordering that pop is the real fix rather than adding a re-checkto each handler.
Refs #9019, #9255.