fix: hold a closing session's row hidden until the close resolves - #6807
fix: hold a closing session's row hidden until the close resolves#6807rnoack1 wants to merge 1 commit into
Conversation
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
1 similar comment
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of I have everything I need. The diff's user-facing surface: two new close-failure notices, a boot slots-load-failed notice with a retry link, the agent-switch notice restyled through UX-Verdict: CONCERNS Solid failure-notice design, but no first-time reader has seen any of the four new/restyled notices, and one "unknown" claim can mislead. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 2650052 |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 3 of 3 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- website/src/utils/storageGc.ts:183 -- Failed ledger writes preserve stale deletion proof BLOCKING -- website/src/pages/ArtifactDetailPage.tsx:1300 -- Tombstone is armed after deletion BLOCKING -- website/src/store/chatSlice.ts:3158 -- Resume and fork responses provide no incarnation [BLOCK-MERGE] 7d4102d Adjudication (Opus 4.8) — is blocking on each finding proportionate?I've examined the code each fenced finding names against both the diff (changed lines) and the base tree (called-into code). F1 — F2 — F3 — All three are unbounded per the fence; for each the condition combinations are realistic races (not mutually contradicting, not writer-impossible, not legacy-only), so I cannot complete a confident acceptable-residual-risk record. Torn → UPHOLD-FENCED. |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of The client machinery matches the description's claims, and the spec is updated in the same change with the follow-up scope enumerated. I have what I need for the design review. Design-Verdict: CONCERNS Sound ordering fix, but it ships two overlapping ordering authorities plus a separable storage-GC redesign, with the promised consolidation deferred to prose. Watch
Suggestions
[DESIGN-REVIEWED] 2650052 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All evidence is gathered — patch read in full for source hunks, base-tree conventions and consumer counts checked. Final review: First-Principles-Verdict: CONCERNS Two new ordering systems ship for one race — the diff itself calls half of one "redundant" — plus a storage-GC rewrite riding inside a flicker fix. Not justified as shipped
What this change shipsIntent: stop a closing session's row from flickering in the sidebar and say why when a close fails — a FIX. (Description truncated at 8KB by the workflow; declarations judged on the visible part.)
(Screenshots and capture scripts follow the repo's committed-deliverables convention — 1363 files under Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 2650052 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe candidate list contained no candidates, and my own pass over every production hunk — the backend generation/epoch stamping ( No findings. [OPUS-REVIEWED] 2650052 |
2a639f8 to
f5e2257
Compare
1308a25 to
2327f40
Compare
2327f40 to
722625a
Compare
722625a to
f00cbeb
Compare
Head branch was pushed to by a user without write access
f00cbeb to
034330e
Compare
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
@rnoack1 Thanks for this one. Audited at
One outside that list: #4904 adds a Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
A dismissed row is withheld from every authoritative slot list until the close resolves, so a frame that still names the slot cannot flicker it back.
Problem / Motivation
Closing a session (arrow menu → Close, or the ✕ on the card) selects the next
session and reloads the chat pane, but the card being closed lingers — and often
disappears, comes back, and disappears again.
The row is already removed optimistically. What puts it back is that every
authoritative slot-list writer —
sseSlotsandfetchSlots.fulfilled, boththrough
applySlots— takes server membership as the truth, and the server stilllists a slot whose
DELETEhas not finished.Two producers make such a frame ordinary rather than rare:
api_chat_slot_deleteretires the auto-nudge loop, runs the app-teardown hook, cancels the task and
saves history before popping
state._slots. Anypush_slots_update()inthat window serializes the slot as live — and pushes coalesce on a 200 ms
window whose trailing edge re-serializes at delivery time, so a coalesced
frame is a fresh read of a list that still contains the slot.
GET /api/chat/slotsanswers with a list assembled before the close began.
Each such frame reinstates the row; the next one after the pop removes it again.
That is the flicker.
Why it matters
The session list is how you know what exists, and a card that vanishes, returns,
and vanishes again makes it unreadable at the exact moment you are acting on it —
you cannot tell whether the close took, whether you hit the wrong row, or whether
the session is coming back. The reflex is to click again, on a row that may
already be gone or may be a neighbour that has shifted into its place.
It also hid the one case that genuinely needs attention: a flickering row and a
close the server refused looked identical, because both ended with the card back
on screen.
What changed
Client-side, where both surfaces converge and the network round trip is also
covered — plus one server field the client reads, described under the outcome
split below:
deleteSlotarms a close tombstone (closingSlots, key → dismissal time)via
slotCloseStarted. Only a close in flight arms one.applySlotswithholds a tombstoned key from every list it applies, so no framecan resurrect a row the user dismissed.
retire one: a list omitting the key proves the server popped the slot, but not
that a
fetchSlotsreply issued before the close has landed — and that replystill carries the key. Success retires when a read ISSUED after the close lands.
fetchSlots—reversed,
applySlotswould withhold the row from the very reply meant torestore it.
addSlotOptimisticclears any tombstone for the key it creates: a slot beingcreated is proof it is not closing. Load-bearing because
api_chat_slot_resumekeys the revived slot by the requested name, soresuming a just-closed session reuses its key.
reply was ISSUED before the close, which the client knows exactly where a wall-clock
window only guesses. Each close bumps a monotonic
closeSeqand stamps it on thetombstone;
fetchSlots.pendingrecords thecloseSeqcurrent when that read wasissued. A tombstone retires only when a reply arrives that was issued AFTER the close
AND no read issued before it is still outstanding. A server PUSH carries no issue
generation — a coalesced frame can be serialized before the pop — so a push withholds
but never retires. The close issues that dated read itself, so the row is decided by
evidence as soon as it lands rather than after a fixed wait.
still outstanding, and the close issues that reply itself — so nothing depends on
a clock and nothing can hide a live session indefinitely.
fails it releases the tombstone instead, since a resumed session must never be hidden
by a read that never landed. Both close sites share one
retireCloseTombstonespelling so the release cannot be dropped at one of them.
(
gcSessionStorage) used to sweep a closed session's ownlocalStoragekeys onclose and on history delete. It is gone, and the boot orphan sweep
(
gcOrphanedStorage) is now the only collector. A 2xx does not prove the key isstill ours — teardown can hand it to a replacement resumed under the same name, and
a same-key resume in another tab is invisible to this tab's Redux state while
localStorageis shared across tabs — so a close-path sweep could erase a livesession's scroll and height state. The cost is that a closed session's keys now
survive until the next boot sweep instead of being collected at close.
Normal path: the row goes on the click and stays gone. No pending state is
surfaced, because there is nothing the user needs to do about it.
Retry and failure
Slot keys are reusable —
api_chat_slot_resumerevives a session under its ownkey, and restores its
created_atfrom the transcript metadata — so nothing on thewire distinguishes the instance a close targeted from a replacement resumed under
the same key.
The DELETE is issued once. There is no retry. Any second DELETE risks closing a
stranger, and no client-side guard can exclude that: an omission-based check cannot
see a close-and-resume that completes inside one coalesced slots frame. A failure is
reported instead — which is what the backend's own abort comment calls a state the
user can see and retry.
No verification probe either. A failure needs none, because the row is already
governed by the list that follows it.
api_chat_slot_deletepops_slotsat ONEpoint (
chat_handlers.py:3488), and every error it returns is either raised beforethat pop (the two
nudge_retire_failedarms at :3410 and :3482, and the app-teardownarm at :3455) or restores the slot after it (the save arm re-inserts at :3516 before
answering at :3540). So a failure the server itself reports leaves the key present
and the refetch brings the row back; a close whose success was merely lost in transit
leaves it popped and the refetch omits it. Both land correctly.
404 counts as success — the slot being absent from
_slotsis the state beingasked for. (The app-isolation branches that also answer 404 are unreachable without
an app token.)
The other removal site shares this root cause and now converges here:
ArtifactDetailPageunbinds an archived slot post-confirmation, where an in-flightGET issued before that DELETE still lists it. It pairs
slotCloseStartedwith theremoval, so the key is withheld rather than left resurrectable.
On failure the user is always told why: a row reappearing on its own is
indistinguishable from the flicker this removes. Rendered through
ErrorNoticein theApp shell, not
alert()— whicherrors-use-error-noticebans, and which the earlierrevision of this branch reached only because that rule globs
*.tsxwhile the helper isa
.tsutil. The helper now classifies only; the shell renders. Both close gestures —the session menu and Alt+Shift+W — share that one surface, which is why it lives in the
shell:
useKeyboardShortcutsmounts there, so the shortcut has no component of its own.What the ROW does on failure follows the same split as the message, via one
shared
isCloseOutcomeUnknownpredicate, so the two cannot drift apart. Adeterminate refusal releases the tombstone and re-reads at once, because the slot is
provably still there. An indeterminate failure is treated exactly like a success:
the tombstone is HELD and the dated read establishes the truth — releasing it
early would let a GET issued before the close resurrect a row the server did remove,
which is this PR's own failure mode. It is also what the unknown-outcome copy
states, and it states it CONDITIONALLY: the notice clears if the session leaves the
list, which is exactly when
settleCloseFailureNoticeretires it — on an accepted datedsnapshot that no longer carries that session. It does not promise clearing while the
session is still listed, because there the outcome stays genuinely unresolved: a dated
read that still carries the key cannot separate a failed close from a key the server
reused, so the caution stands and the toast remains dismissible.
That notice branches on the rejection's status, which
deleteSlotcarries acrossthe thunk boundary with
rejectWithValueasswitchSlotdoes — a thrown error isreduced by
miniSerializeErrorto string fields only, so a numeric status wouldnever arrive and both branches would collapse to one message (Thunk rejections lose their status, so slot-gone is classified by a message regex #6199).
Only two outcomes are distinguishable, and the split is about what the user may
safely do next. It is keyed on a server-supplied
definitiveflag, not on thestatus: every close failure the handler raises is a literal 500, so a status test
read a refusal as unknown.
SlotCloseErrorcarries the flag, the DELETE error bodyforwards it, and
closeDefinitivereads it off the rejection payload.definitivemeans the server refused the close and rolled it back, so the session is provably
still there. Everything else — no flag, no status, a timeout, a rate limit, a 5xx
from anything else in the path — leaves the outcome UNKNOWN, and the copy says so and asks the user not to close it again: the DELETE
may have completed, so a manual retry would aim a second close at whatever now holds
the reusable key.
The same ordering rule also protects a newly created session: the generation
advances on every insert, not only when a tombstone is cleared, so a list reply
issued before that session existed can no longer evict it by omission. This rides
along because it is the same root cause — an authoritative reply applied out of
order — and a refused reply is now excluded from the unread reconcile for the same
reason, since its membership is exactly what was refused.
Why not fix it server-side
Two different server-side options exist. The first is rejected; the second ships here.
Broadcast FILTERING — considered and rejected.
api_chat_slot_delete'snudge_retire_failedpath deliberately aborts the close and re-pushes so the tabstays open and driven, and the existing
_RECENT_CLOSEStombstones have a one-hourTTL. Keying a broadcast filter on them would hide that restored tab for an hour. It
also would not cover the in-flight HTTP reply.
A stamped ORDERING token — shipped here. The wire now carries one.
_slots_ws_framestampsslotsGenerationon the push andapi_chat_slotsreturns thesame counter in an
X-Slots-Generationheader — a header rather than an envelope key,because that reply is a bare list with consumers outside the SPA. Each also carries a
per-process
slotsEpoch/X-Slots-Epoch: the counter restarts at 0 in a new gateway, soa generation is comparable only WITHIN an epoch, and a client holding a high count would
otherwise refuse every snapshot a restarted gateway sent.
applySlotsrecords the newest(epoch, generation)applied and refuses a snapshot at or below it within the same epoch,on either transport. That removes the ambiguity the client cannot resolve locally: no
payload field distinguishes a mid-DELETE frame from a resumed one, since the slot carries
no per-instance identity and
createdis restored on resume.The stamp is drawn BEFORE the rows are read, through
DashboardState.stamped_slots, whichboth emitting paths take it from rather than calling
next_slots_generationthemselves.Serializing first and stamping after leaves a window in which a close pops a slot between
the two, so the frame would carry pre-pop rows under a number drawn later than the post-pop
read's — the resurrection restated rather than fixed.
What is genuinely still owed is the DELETION of the client reconstruction — but only of the
half the stamp actually subsumes, which is the ordering of one SERVER emission against
another (
pendingSlotReads,membershipMovedand the wholesale refusal infetchSlots.fulfilled). Two parts sit outside that authority and stay load-bearing. TheLOCAL OPTIMISTIC CREATE:
addSlotOptimisticbumpscloseSeqfor a purely local insertion,and at the moment of the bump the row exists in no server snapshot for a server-drawn number
to be newer or older than. And the
closingSlotsTOMBSTONE HOLD: the stamp refuses anout-of-order snapshot, but a push coalesced mid-close is genuinely the newest emission and
truthfully still lists the slot the DELETE has not yet removed, so ordering cannot refuse it
and the withhold is what suppresses the flicker. Nothing mechanical forces even the
server-half removal: it is ordinary tracked follow-up work, owed once the refusal has proved
itself in production, and the exact scope is enumerated in the session-control module spec.
Also in this PR, and why each is here rather than in its own change
Named because a reviewer should not have to discover them from the diff (First Principles
and Design both asked for exactly this):
The
gcSessionStorageremoval. Same premise as the race fix: a client cannot prove akey is dead across tabs, so a per-key sweep run on one tab can delete another's live
state. It is the same insight applied to storage rather than to membership, which is why
it travels with this change; splitting it would leave the sweep contradicting the spec
paragraph this PR adds.
The notice-stack repositioning in
App.tsx. Not cosmetic and not independent: theclose-failure notice this PR introduces carried
fixedclasses byte-identical to theexisting agent-switch notice, so the two rendered exactly on top of each other and hid a
dismiss control. The stack exists because this PR added the second notice.
A FAILED slots read now rejects, and the boot query renders it.
nullfromfetchSlotsIfAppliedmeans REFUSED and nothing else. It previously meant refused ORfailed, so a failed GET resolved, react-query recorded success, and the user was shown
nothing at all; every retrying caller was also unable to tell a refusal from an outage.
The boot query now binds its error and renders it through
ErrorNoticewith thehand-off, and the two existing callers already treated a rejection as retry-or-fall-back.
The boot storage sweep now needs AGE evidence, and its fetch is a query. Two changes
at one call site, so they are one item.
localStorageis shared by every tab on theorigin while the slot list is one tab's snapshot, so a session another tab creates after
the read serializes but before its coalesced push is absent from that snapshot while
fully live -- deleting its keys destroys the other tab's state. AGE DOES NOT FIX THAT,
because session ids are REUSED: an id unlisted past any grace can be resumed under the
same id after this boot's snapshot serialized, which is precisely when the grace has
already elapsed, so age proves only that the id WAS unlisted. Deletion therefore needs
positive instance-level proof of SUPERSESSION, and THE WRITER SUPPLIES IT: each
session-scoped write records which instance wrote the bytes, PER FULL STORAGE KEY, and a
key is deleted only when the live list presents its id as a different instance than the
one that wrote THAT key. Per key rather than per session id, because a session owns
several independent key families (heights, anchors, panel tabs, activity, web preview)
and a replacement instance typically rewrites only some of them -- a per-session stamp
advanced by any one write would vouch for every other family, so the families the new
instance never touched would keep the OLD instance's state and load into it.
Deriving that stamp from the boot list instead would record what the LIST said rather
than who wrote the state, so a deterministic slot recreated under its reused key -- it
writes as the new instance without touching a list-derived ledger -- would have its
LIVE state deleted on the next boot. The sweep therefore never stamps the ledger
itself. An unlisted id is retained, so is one undated on either side, and so is any key
no stamped writer has written: unproven is always kept.
A capacity budget bounds the RE-DERIVABLE half only. Past
MAX_ABSENT_SESSIONStheCOLDEST unlisted sessions lose their
DERIVED_PREFIXEScaches, ordered by when theirkeys were last written. That is a capacity policy, not an age gate: nothing is deleted
for being old, a LISTED session is never a candidate however cold, and a session another
tab is really using is being written, so it is never the coldest. NON-DERIVED state --
panel tabs, activity, web preview -- sits outside that budget and is deleted only on
monotonic proof of supersession, so it is retained rather than capped. Unbounded growth
there is a deliberate residual: the alternative deletes live state a snapshot merely
omitted, and that state has no rebuild path. That fixes a
PRE-EXISTING cross-tab bug in
gcOrphanedStoragerather than one this PR introduced. The same call site's hand-rolled.then()fetch became areact-query whose
queryFnisfetchSlotsIfApplied, because the sweep must run offACCEPTED data and the query is what makes that the only thing it can read.
The agent-switch notice now renders through
ErrorNotice. Every one of its 14dispatch sites passes
agentSwitchFailureMessage(...), so it is a failure notice thatwas losing the structured report lookup and the agent hand-off, and announcing politely
(
role="status") instead of assertively.The slots stamp runs on the serving loop
The
(generation, membership)pairing needs mutual exclusion, but an event loop must neverWAIT for it. The emitting paths are on different threads, so one shared lock let a foreign
push_slots_updatestall the loop serving every HTTP snapshot, WS frame and heartbeatbehind it. Two changes: the stamp has its own lock, separate from the coalescing lock a
foreign caller takes; and an off-loop stamper hands the stamp to the serving loop and waits
on its own thread, so every stamp executes on one thread and that lock is uncontended by
construction. A wedged loop falls back to stamping locally after a bounded wait rather than
silencing a broadcast. Both halves carry their own failing-first arm.
Tests
Every guard below was written as a failing test first and confirmed to fail against
the unfixed code before the fix went in — including the two original
applySlotsraces (
chat-2resurrected on both the SSE and the HTTP surface). Removing thetombstone withholding still fails 8 of them.
website/src/test/dashboardSlice.closingSlots.test.ts— both resurrection races,scoping (a peer created mid-close is not withheld), an SSE omission not retiring a
tombstone (with the reversed settle order as a negative control), that an optimistic
removal alone does not tombstone, that creating a slot clears one, tolerance of a
partial preloaded state, identity-keyed retirement (a push after 30 minutes still does
NOT retire, while a dated read does), the artifact page's withheld removal, and a slot
key naming a prototype member (
__proto__) being stored as data rather than mutatingObject.prototype— four cases that fail on an indexed write.website/src/test/chatSlice.closeRetry.test.ts— exactly one DELETE on the happypath and on every failure; terminal for a statusless failure and for a 403;
404-as-success; the status surviving the thunk boundary; and the slots read scheduled
for straight after the close.
website/src/hooks/sessionCloseFailure.test.ts— refused and unknown are distinct;every ambiguous status (none, 408, 429, 5xx) reports an unknown outcome and asks the
user not to close again; a refused close carries no retry advice; and both close
gestures are wired to pass the rejection.
website/src/hooks/useSessionActions.cov80.test.tsx— the failure notice and silenceon success. Its
deleteSlotstub is thunk-shaped, becauseclosereads the outcome.563 tests passedacross the store suites (chatSlice,dashboardSlice,useSessionActions,ChatSliceCoverage,chatSlice.slotPrune, plus the three newfiles). ESLint clean on every touched file.
npm run i18n:checkreports19 checks · PASSwithI18N_BASE_REFset, so the diff-scoped gates ran rather thanskipped.
npm run typecheckreports the same 22 pre-existing errors as the unmodifiedbase (missing optional
graphology-*/@pierre/*modules) and none in the touchedfiles.
i18n
Two new keys —
hooks.useSessionActions.close_failed_unknownandclose_failed_refused— inen.manual.jsonand all 12 shippedlocales, following the
reload_failedprecedent;en-XA.jsonregenerated withnpm run i18n:pseudo. One message per distinguishable outcome, because neither is trueof the other: a refused close is provably still there, while an unknown one must not
claim the session survived nor invite a second close. Neither names a sidebar refresh
control that does not exist — the failure path dispatches
fetchSlots(). The unknownnotice claims only what the client can keep, and claims it conditionally: the notice clears
if the session leaves the list, which
settleCloseFailureNoticedoes on the firstaccepted dated snapshot that no longer lists it. While the session is still listed the
notice stays up rather than clearing on a promise the client cannot honour.
Screenshot evidence
Captured with
website/scripts/capture-session-close-row-hold.mjs, which drives the built SPA andstamps an in-band witness into neighbouring row subtitles so the frames themselves show how many
server pushes landed inside the close window. That subtitle text is harness instrumentation, not
product copy.
Both close-failure notices are captured on the branch as well, for the two distinguishable
outcomes the copy splits on. They are committed at
temp-screenshots/session-close-failure-notice/1-close-refused-notice.png(the determinaterefusal, testid
session-close-failed) andtemp-screenshots/session-close-failure-notice/2-close-unknown-notice.png(theindeterminate outcome), alongside the four row-hold frames under
temp-screenshots/session-close-row-hold/. The fork review lane cannot render a branchfile, so those paths are how a human reviewer opens them.
Harness run:
DELETE observed=true pushes=8 framesAppliedInWindow=7 samples=56 resurrections=0 visibleAfterSettle=false controlRowReturned=true. The fourth frame is the negative control — aharness that could not restore the row would report
controlRowReturned=falseand prove nothingabout scoping.
Pattern harvest
Rule candidate: review-prompt
Pattern: an async authoritative snapshot is applied to local state without ordering it against a mutation that happened after the read was issued
This defect generalizes, and the shape is worth naming because the wrong fixes all looked
reasonable:
applied after it, so the closed row came back. Every attempt to fix it by tuning how long the
local suppression lived — retire on success, retire when a list omits the key, retire on a
wall-clock TTL, never retire — failed in a different direction, because none of them asked the
only question that matters: was this reply issued before the change it is about to overwrite?
The fix that held stamps each read with a generation at issue time and refuses a reply that
predates the newest mutation.
A server push cannot be dated by the client, so it can withhold but must never be treated as
proof; only a read the client itself issued can retire the suppression. A prompt asking "what
arriving event clears this, and can the client prove that event postdates the change?" would have
found this in one pass.
A guard that refused a reply whose bookkeeping had aged out fired even when no mutation had
happened, discarding an authoritative list for nothing. Layered mitigations for a race deserve
re-examination after the root ordering is fixed, rather than being kept because they were once
load-bearing.
with a list that both denied the close and omitted the session just created, a state no server
produces; it only passed before because the read used to be deferred past the test's lifetime.
Fixture realism is part of the contract.