Skip to content

fix: hold a closing session's row hidden until the close resolves - #6807

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/close-session-flicker
Open

fix: hold a closing session's row hidden until the close resolves#6807
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/close-session-flicker

Conversation

@rnoack1

@rnoack1 rnoack1 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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 — sseSlots and fetchSlots.fulfilled, both
through applySlots — takes server membership as the truth, and the server still
lists a slot whose DELETE has not finished.

Two producers make such a frame ordinary rather than rare:

  • The backend holds the slot across several awaits. api_chat_slot_delete
    retires the auto-nudge loop, runs the app-teardown hook, cancels the task and
    saves history before popping state._slots. Any push_slots_update() in
    that 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.
  • An HTTP reply can predate the click. An in-flight GET /api/chat/slots
    answers 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:

  • deleteSlot arms a close tombstone (closingSlots, key → dismissal time)
    via slotCloseStarted. Only a close in flight arms one.
  • applySlots withholds a tombstoned key from every list it applies, so no frame
    can resurrect a row the user dismissed.
  • A successful close does not release its tombstone, and an omission does NOT
    retire one: a list omitting the key proves the server popped the slot, but not
    that a fetchSlots reply issued before the close has landed — and that reply
    still carries the key. Success retires when a read ISSUED after the close lands.
  • A failed close releases it explicitly, before dispatching fetchSlots
    reversed, applySlots would withhold the row from the very reply meant to
    restore it.
  • addSlotOptimistic clears any tombstone for the key it creates: a slot being
    created is proof it is not closing. Load-bearing because
    api_chat_slot_resume keys the revived slot by the requested name, so
    resuming a just-closed session reuses its key.
  • Retirement is keyed to request IDENTITY, never elapsed time. "Stale" means a
    reply was ISSUED before the close, which the client knows exactly where a wall-clock
    window only guesses. Each close bumps a monotonic closeSeq and stamps it on the
    tombstone; fetchSlots.pending records the closeSeq current when that read was
    issued. 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.
  • A tombstone is retired by a reply ISSUED after the close once no older read is
    still outstanding, and the close issues that reply itself — so nothing depends on
    a clock and nothing can hide a live session indefinitely.
  • Retirement is swept on READ, so the close ISSUES that read itself — and if the read
    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 retireCloseTombstone
    spelling so the release cannot be dropped at one of them.
  • Per-key storage cleanup on close is REMOVED, not relocated. A helper
    (gcSessionStorage) used to sweep a closed session's own localStorage keys on
    close 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 is
    still 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
    localStorage is shared across tabs — so a close-path sweep could erase a live
    session'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 reusableapi_chat_slot_resume revives a session under its own
key, and restores its created_at from the transcript metadata — so nothing on the
wire 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_delete pops _slots at ONE
    point (chat_handlers.py:3488), and every error it returns is either raised before
    that pop (the two nudge_retire_failed arms at :3410 and :3482, and the app-teardown
    arm 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 _slots is the state being
    asked 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:
    ArtifactDetailPage unbinds an archived slot post-confirmation, where an in-flight
    GET issued before that DELETE still lists it. It pairs slotCloseStarted with the
    removal, 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 ErrorNotice in the
    App shell, not alert() — which errors-use-error-notice bans, and which the earlier
    revision of this branch reached only because that rule globs *.tsx while the helper is
    a .ts util. 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: useKeyboardShortcuts mounts 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 isCloseOutcomeUnknown predicate, so the two cannot drift apart. A
    determinate 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 settleCloseFailureNotice retires it — on an accepted dated
    snapshot 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 deleteSlot carries across
    the thunk boundary with rejectWithValue as switchSlot does — a thrown error is
    reduced by miniSerializeError to string fields only, so a numeric status would
    never 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 definitive flag, not on the
    status: every close failure the handler raises is a literal 500, so a status test
    read a refusal as unknown. SlotCloseError carries the flag, the DELETE error body
    forwards it, and closeDefinitive reads it off the rejection payload. definitive
    means 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's
nudge_retire_failed path deliberately aborts the close and re-pushes so the tab
stays open and driven, and the existing _RECENT_CLOSES tombstones have a one-hour
TTL. 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_frame stamps slotsGeneration on the push and api_chat_slots returns the
same counter in an X-Slots-Generation header — 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, so
a generation is comparable only WITHIN an epoch, and a client holding a high count would
otherwise refuse every snapshot a restarted gateway sent. applySlots records 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 created is restored on resume.

The stamp is drawn BEFORE the rows are read, through DashboardState.stamped_slots, which
both emitting paths take it from rather than calling next_slots_generation themselves.
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, membershipMoved and the wholesale refusal in
fetchSlots.fulfilled). Two parts sit outside that authority and stay load-bearing. The
LOCAL OPTIMISTIC CREATE: addSlotOptimistic bumps closeSeq for 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 closingSlots TOMBSTONE HOLD: the stamp refuses an
out-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 gcSessionStorage removal. Same premise as the race fix: a client cannot prove a
    key 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: the
    close-failure notice this PR introduces carried fixed classes byte-identical to the
    existing 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. null from
    fetchSlotsIfApplied means REFUSED and nothing else. It previously meant refused OR
    failed, 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 ErrorNotice with the
    hand-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. localStorage is shared by every tab on the
    origin 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_SESSIONS the
    COLDEST unlisted sessions lose their DERIVED_PREFIXES caches, ordered by when their
    keys 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 gcOrphanedStorage rather than one this PR introduced. The same call site's hand-rolled .then() fetch became a
    react-query whose queryFn is fetchSlotsIfApplied, because the sweep must run off
    ACCEPTED 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 14
    dispatch sites passes agentSwitchFailureMessage(...), so it is a failure notice that
    was 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 never
WAIT for it. The emitting paths are on different threads, so one shared lock let a foreign
push_slots_update stall the loop serving every HTTP snapshot, WS frame and heartbeat
behind 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 applySlots
races (chat-2 resurrected on both the SSE and the HTTP surface). Removing the
tombstone 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 mutating
    Object.prototype — four cases that fail on an indexed write.
  • website/src/test/chatSlice.closeRetry.test.ts — exactly one DELETE on the happy
    path 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 silence
    on success. Its deleteSlot stub is thunk-shaped, because close reads the outcome.

563 tests passed across the store suites (chatSlice, dashboardSlice,
useSessionActions, ChatSliceCoverage, chatSlice.slotPrune, plus the three new
files). ESLint clean on every touched file. npm run i18n:check reports
19 checks · PASS with I18N_BASE_REF set, so the diff-scoped gates ran rather than
skipped. npm run typecheck reports the same 22 pre-existing errors as the unmodified
base (missing optional graphology-* / @pierre/* modules) and none in the touched
files.

i18n

Two new keys — hooks.useSessionActions.close_failed_unknown and
close_failed_refused — in en.manual.json and all 12 shipped
locales, following the reload_failed precedent; en-XA.json regenerated with
npm run i18n:pseudo. One message per distinguishable outcome, because neither is true
of 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 unknown
notice claims only what the client can keep, and claims it conditionally: the notice clears
if the session leaves the list, which settleCloseFailureNotice does on the first
accepted 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 and
stamps 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.

Baseline: the sidebar lists three sessions and "Nightly triage sweep" is hovered, exposing its close control

Mid-close: the row disappears the moment DELETE is issued, and the neighbouring subtitle reads "applied push #7" — seven server frames were applied inside the close window and none of them brought the row back

Settled: once the close resolves the row is genuinely gone, with no flicker back into the list

Control: after the tombstone is retired, a frame that still lists the slot DOES restore the row ("still listed by the server (frame 8)"), so the hold is scoped to the close window rather than suppressing the key permanently

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 determinate
refusal, testid session-close-failed) and
temp-screenshots/session-close-failure-notice/2-close-unknown-notice.png (the
indeterminate outcome), alongside the four row-hold frames under
temp-screenshots/session-close-row-hold/. The fork review lane cannot render a branch
file, 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 — a
harness that could not restore the row would report controlRowReturned=false and prove nothing
about 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:

  • The root cause was ordering, not lifetime. A slots reply serialized before a close was
    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.
  • Suppression that is not time-bounded needs an arriving signal, and the signal must be datable.
    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.
  • Once the ordering rule existed, two guards added earlier became dead — and one became harmful.
    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.
  • A mock can encode an impossible server. One pre-existing test answered the post-close read
    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.

@rnoack1
rnoack1 requested a review from a team August 29, 2026 17:11
@rnoack1
rnoack1 requested a review from a team as a code owner August 29, 2026 17:11
@rnoack1
rnoack1 requested a review from patrigao August 29, 2026 17:11
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 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:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

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
@dwu96

dwu96 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

👋 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:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

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.

@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 Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

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

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 ErrorNotice into a stacked layout, and the row-hold behavior itself. Screenshots for the close notices and row-hold are committed but binary-only in this fork lane; the boot notice and restyled agent-switch notice have none at all. Mechanism-wise the labels keep their promises (refused → row restored + retry advice; unknown → row held + settled by a dated read), with one copy claim the client can render false.

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

  • close_failed_unknown — "if it stays gone, the close went through" can be false: when a lost DELETE leaves the same incarnation listed, retireCloseTombstone re-reads forever ("Same instance … again()") and the client's own withholding keeps the row gone while the session lives — the user reads suppression as proof of success. Low frequency (network failure at close) × high impact (live session invisible, believed closed) × tab-lifetime persistence. Fix: key success off the notice clearing ("This notice clears once the close is confirmed"), which is exactly when settleCloseFailureNotice fires.
  • Translated close_failed_refused (de: "Schließen Sie sie erneut über die Sitzungsliste.", fr likewise) drops English's second sentence ("If it stays open, reload the page…"), so non-English users whose retry also fails get no next step. Fix: restore the escalation sentence in the 11 translated locales.

Evidence gaps

  • Refused-close notice (session-close-failed) — committed temp-screenshots/session-close-failure-notice/1-close-refused-notice.png is not renderable in this fork lane; push the branch in-repo for the blind read.
  • Unknown-close notice — same, 2-close-unknown-notice.png.
  • Row-hold states (present → held-hidden → settled → control return) — the four temp-screenshots/session-close-row-hold/*.png frames, same.
  • Boot-slots-failed notice with its "Try again" link (boot-slots-failed) — no screenshot committed anywhere.
  • Restyled agent-switch notice (warn-bordered role="status" banner → red ErrorNotice with Ask-agent button) and the new stacked-notices layout — no screenshot committed.

Suggestions

  • app.boot_slots_failed (en): "If it keeps failing, reload the page." reads as if a retry already happened — align with the fr/de rendering: "Try again, or reload the page if it keeps failing."

[UX-REVIEWED] 2650052

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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

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

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
else localStorage.setItem(OWNER_LEDGER_KEY, JSON.stringify(o))
Quota pressure -> state write succeeds but ledger update fails -> next boot deletes current session state using the retained old stamp.
Anchor: residual/crash-data-loss-corruption
Fix: clear the owner ledger if replacing it fails.

BLOCKING -- website/src/pages/ArtifactDetailPage.tsx:1300 -- Tombstone is armed after deletion
await withSlotClose(dispatch, () => store.getState(), slot.key, async () => {})
Same-key replacement arrives before arming -> tombstone captures and indefinitely hides the replacement -> a duplicate companion session is created.
Anchor: residual/crash-data-loss-corruption
Fix: execute the DELETE inside withSlotClose.

BLOCKING -- website/src/store/chatSlice.ts:3158 -- Resume and fork responses provide no incarnation
incarnation: d.incarnation
Immediate close before a slots snapshot -> undefined closing identity -> a same-key replacement cannot release the tombstone and remains hidden until reload.
Anchor: residual/crash-data-loss-corruption
Fix: include incarnation in the backend resume and fork success responses.

[BLOCK-MERGE] 7d4102d
[GPT-REVIEWED] 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).

F1storageGc.ts writeOwners: the ledger update in recordSessionStorageOwner (diff 8217–8226) reads-modifies-writes OWNER_LEDGER_KEY; on a quota-pressured setItem failure (diff 8144, swallowed by the catch at 8145) the ledger retains the OLD stamp while the same-key state write — an overwrite of an existing key — can still succeed. Next boot, gcOrphanedStorage computes supersedes(newInstanceCreated, staleOldStamp) = true (diff 8292) and deletes the live replacement's NON-derived state, which the code itself documents has no rebuild path (diff 8074). Unrecoverable; conditions (quota near-full — the very reason MAX_ABSENT_SESSIONS exists — plus same-key replacement) are ordinary. Not extreme.

F2ArtifactDetailPage.tsx:1300: the DELETE lands at the loop top (api.deleteChatSlot, base line 1272) and the tombstone is armed only afterward via the empty-op withSlotClose (diff 3109 → beginSlotClose/slotCloseStarted diff 3391/3997). A same-key replacement applied during the DELETE's network flight is what slotCloseStarted then stamps as the "closing" instance, so the differing-incarnation reveal (diff 3363) can never fire and the live replacement is hidden until reload. Recovery: reload. Conditions (concurrent same-key replacement) are exactly what this machinery targets — plausible in multi-tab operation, not contradictory.

F3chatSlice.ts:3158: _live_slot_resume_response (base chat_handlers.py:8340) returns no incarnation, so d.incarnation at the resume/fork optimistic rows (diff 3477/3486) is undefined. An immediate close before any slots snapshot populates the real incarnation makes slotCloseStarted capture instance = undefined (diff 3997), and the reveal guard if (closingInstance && …) (diff 3363) is dead, hiding a same-key replacement until reload. Recovery: reload. Race is narrow but plausible, not impossible.

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.

[ADJUDICATION] 7d4102d1681d4dc20847e2b9372a0ae2c0793eab total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 7d4102d1681d4dc20847e2b9372a0ae2c0793eab

[ADJUDICATION-FENCED] 7d4102d1681d4dc20847e2b9372a0ae2c0793eab fenced=3 flagged=0
UPHOLD-FENCED F1 website/src/utils/storageGc.ts:183 -- A swallowed quota-failed ledger write leaves a stale supersession stamp that deletes the live replacement's unrebuildable state on next boot; quota pressure and key reuse are both ordinary.
UPHOLD-FENCED F2 website/src/pages/ArtifactDetailPage.tsx:1300 -- Arming the tombstone after the DELETE lets a same-key replacement applied during the request flight be stamped as the closing instance and hidden indefinitely; a plausible multi-tab race, not an extreme one.
UPHOLD-FENCED F3 website/src/store/chatSlice.ts:3158 -- The resume/fork response carries no incarnation, so an immediate pre-snapshot close stamps undefined and disables the differing-incarnation reveal, hiding a live replacement; the race is narrow but realistic.
[GPT-ADJUDICATED-FENCED] 7d4102d1681d4dc20847e2b9372a0ae2c0793eab

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

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

  • The client reconstruction (closeSeq, pendingSlotReads, membershipMoved, wholesale refusal) and the server (epoch, generation) stamp now both order the same state. The spec's own words — "the two ordering systems coexisting is a real cost — it wants an owner and a trigger rather than a paragraph, which this spec cannot supply" — mean every future emitter or list consumer must satisfy two clocks, and drift between them reproduces exactly the resurrection class this PR fixes.
    Clears when: a filed, linked follow-up issue owns deleting the server-vs-server half of the client reconstruction once the stamp proves out.
  • The boot-sweep redesign (per-key writer-instance ledger, MAX_ABSENT_SESSIONS budget, deliberate unbounded non-derived residue) fixes what the description itself calls "a PRE-EXISTING cross-tab bug in gcOrphanedStorage rather than one this PR introduced." Only the gcSessionStorage removal is entangled with the flicker fix; the ledger design is independently revertable work now welded to it — a rollback of either drags the other, and the repo's two-commit PR budget is strained.
    Clears when: the gcOrphanedStorage ledger redesign moves to its own PR (keeping only the gcSessionStorage removal here), or a human reviewer accepts the entanglement explicitly.

Suggestions

  • The new wire incarnation already distinguishes the instance a close targeted from a same-key replacement; when the deferred consolidation happens, evaluate whether it can also simplify tombstone retirement before keeping retireReadId matching.

[DESIGN-REVIEWED] 2650052

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 26500526f1f9fed1c080fa3b4fd5572dbfe9b6f2 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All 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

  • Item 4 — rides along, oversized: the client closeSeq/pendingSlotReads reconstruction AND the server generation stamp both ship; dashboardSlice.ts annotates closeSeq "Redundant now the wire stamps a generation", and the spec hunk concedes "the two ordering systems coexisting is a real cost — it wants an owner and a trigger rather than a paragraph".
  • Item 8 — rides along: the declared cost sentence ("keys now survive until the next boot sweep") describes the OLD absence-based sweep, yet the diff replaces that sweep with a persisted writer ledger (mc-storage-gc-owners), supersession proofs, and a 24-session budget. The cross-tab deletion hazard it fixes cites no report, and dead sessions' non-derived keys are now never collected — an accepted unbounded leak the visible description doesn't state.
  • Item 9 — rides along: a boot-list failure notice with retry, when reconnect already re-issues fetchSlots (useWebSocket.ts:887) and the notice's own comment admits the socket fills the sidebar regardless.

What this change ships

Intent: 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.)

  1. Closing a session removes the row instantly and no stale list resurrects it — justified
  2. A failed close shows a notice, split refused vs unknown, all locales — justified
  3. A refused close restores the row; an unknown outcome keeps it hidden until proof — justified
  4. Server stamps every slots snapshot (generation + epoch, WS and header) with new backend lock/handoff machinery — rides along, redundancy self-declared
  5. Close-failure bodies carry a definitive flag the notice reads — justified
  6. Every slot carries a new incarnation wire field, read by tombstone release — justified
  7. Per-key localStorage cleanup on close deleted — justified
  8. Boot storage sweep rewritten onto a persisted writer ledger + supersession + budget — rides along
  9. Boot session-list failure now shows a notice with Retry — rides along
  10. Agent-switch notice moved into a shared stacked ErrorNotice — rides along (overlap harm named: byte-identical fixed classes)

(Screenshots and capture scripts follow the repo's committed-deliverables convention — 1363 files under temp-screenshots/; not items.)

Watch

  • Two ordering baselines must now be maintained together; the PR's own spec says the server stamp subsumes the server-vs-server half but leaves it in. Clears when: the subsumed half is deleted here, or a linked issue owns its retirement.
  • The ledger rewrite makes recordSessionStorageOwner a convention every future session-scoped writer must remember (unstamped keys become permanent), and adds a persisted schema already carrying a migration branch. Clears when: a reported instance of the cross-tab deletion it prevents, or the rewrite lands as its own reviewed change.

Subtractions

  • Delete the server-vs-server half of pendingSlotReads/closeSeq staleness for HTTP replies (dashboardSlice.ts), keeping the local-create bump and tombstone hold the spec names as non-deletable — the wire stamp now answers that ordering.
  • Defer the storageGc.ts ledger rewrite (OWNER_LEDGER_KEY, recordSessionStorageOwner, MAX_ABSENT_SESSIONS, four stamping call sites): ship only the declared gcSessionStorage deletion, keeping the existing absence-based boot sweep.
  • Drop the boot-slots notice + retry (App.tsx, app.boot_slots_failed* in 14 locales); reconnect's existing fetchSlots re-issue is the recovery path.

[FIRST-PRINCIPLES-REVIEWED] 2650052

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The candidate list contained no candidates, and my own pass over every production hunk — the backend generation/epoch stamping (state.py, ws.py, chat_handlers.py, all imports present), the tombstone/ordering logic (dashboardSlice.ts, chatSlice.ts, slotsResidueListener.ts), the storage-GC redesign (storageGc.ts, closeOutcome.ts, sessionCloseFailure.ts), and the four migrated api.chatSlots() call sites — surfaced nothing I can ground to a concrete input, call path, and observable wrong outcome at the required bar. All four chatSlots() consumers are normalised, the staleness rule fails safe on unstamped/refused frames, and the sweep deletes only on writer-stamp supersession.

No findings.

[OPUS-REVIEWED] 2650052

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/close-session-flicker branch from 2a639f8 to f5e2257 Compare August 29, 2026 18:39
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@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 Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/close-session-flicker branch from 1308a25 to 2327f40 Compare August 29, 2026 22:22
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/close-session-flicker branch from 2327f40 to 722625a Compare August 29, 2026 22:57
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/close-session-flicker branch from 722625a to f00cbeb Compare August 29, 2026 23:47
@bolichen97
bolichen97 enabled auto-merge August 29, 2026 23:47
@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 Aug 29, 2026
auto-merge was automatically disabled August 30, 2026 00:23

Head branch was pushed to by a user without write access

@rnoack1
rnoack1 force-pushed the fix/close-session-flicker branch from f00cbeb to 034330e Compare August 30, 2026 00:23
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 30, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • This PR is OVERLAPPING with PR #6823. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6807: MERGE_DISCUSSION. Independent, compatible goals — pre-close gating vs post-close outcome reporting — landing on the same two close call sites, with a verified content conflict in both. The authors should agree an order and a merged shape for close, since the naive resolution silently drops one side's behaviour and breaks 6807's source-text pairing test. Files: website/src/hooks/useSessionActions.ts, website/src/hooks/useKeyboardShortcuts.ts, website/src/hooks/useSessionActions.cov80.test.tsx.
  • This PR is OVERLAPPING with PR #7212. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6807: MERGE_DISCUSSION. Same close path, different layers and different goals (client list ordering vs server same-key-recreate safety), and they merge cleanly at the text level while contradicting each other semantically. If 7212 lands, definitive=True on history_save_failed needs to be conditioned on the restore actually happening, or the refused-close copy will invite a retry against a replacement. Files: src/kiro_crew/dashboard/chat_handlers.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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

@bolichen97

Copy link
Copy Markdown
Collaborator

@rnoack1 Thanks for this one. Audited at d2adc74d; the head has since moved to 8bd76626, and every file overlap below is still present at the new head. Nothing this PR adds exists on main, so the change is worth keeping. Six open PRs touch the same surfaces, and the landing order matters.

One outside that list: #4904 adds a merge_in_progress SlotCloseError arm that inherits definitive=False, so your notice would open with "Don't close it again yet" for a refusal that is explicitly retryable. That arm needs definitive=True.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants