Skip to content

fix(interactive): preserve the pane on a failed /history refetch (#890)#895

Merged
eous merged 4 commits into
mainfrom
fix/890-clear-ui-guard
Jul 23, 2026
Merged

fix(interactive): preserve the pane on a failed /history refetch (#890)#895
eous merged 4 commits into
mainfrom
fix/890-clear-ui-guard

Conversation

@eous

@eous eous commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Closes #890.

A failed /history refetch blanked interactive's pane on a live stream: the clear_ui route pre-wiped the transcript before the fetch, and the failure path appended showEmptyState. /history failures cluster in exactly the restart windows that emit clear_ui replays and truncated resyncs, so the highest-traffic pane went blank precisely when the cluster was at its most fragile. This ports the coordinator's #882 G3 guard-before-wipe — deliberately not a mechanical copy: interactive's clear_ui route quiesces live events around the rebuild and dispatches the edit-and-resend from its .then, so the pre-wipe/quiesce seam was mapped first and the port earned three additional mechanisms through review.

What changed (commit 1 — turnstone/shared_static/interactive.js)

  • Guard-before-wipe. The wipe + streaming-ref reset live in replayHistory, reached only on a successful fetch. clear_ui no longer pre-wipes; _refetchHistory's failure branch is a DOM/ref/repair-intent no-op except the quiesce release. A failed fetch keeps stale-but-real content, the truncation record stays armed for the connect-chokepoint retry, and the queued edit-resend still fires (the rewind already committed server-side) — coordinator parity throughout.
  • Resumability-gated streaming-ref reset. On reload, refs survive only when an armed truncation cursor lets the reconnect resume into them (the mid-jitter bubble-continuity contract). Every non-resumable flavor resets — including the unarmed onLogin re-auth reload, where surviving refs concatenated the next turn into a stale bubble.
  • Transcript-staleness latch (_historyStale). Set at clear_ui arrival (and defensively on ws reassignment), cleared in exactly one place: replayHistory's successful render. The mutating row affordances (rewind / edit / edit-and-resend) gate on busy || _historyStale, closing the over-rewind hazard across BOTH the fetch window and the failed-fetch aftermath — a turn count computed off a stale transcript can never reach the server. Plain sends stay ungated (count-independent), which is also what guarantees organic heal edges.
  • Transport-free heal. A failed clear_ui refetch schedules one bounded retry (2s, token-guarded, turn-free at fire, yields to any in-flight quiesced fetch); a quiesced same-token refetch at organic idle edges backstops a double failure. Ruled at-site: the heal must never touch the stream — a reload's fresh reconnect draws the server's synthetic state_change: idle back into the backstop's own trigger, which with /history down and /events up is a zero-backoff reconnect storm against a recovering node. A REST refetch emits zero SSE events, so the loop is structurally impossible rather than throttled. destroy() cancels the retry timer (terminal-only; transport reconnects keep the heal intent).
  • Factory connect() pre-seed documented load-bearing: it is now the sole producer of the failed-first-paint placeholder.

Accepted residuals, each ruled at the site: the stale window stays visible (coordinator's cosmetic [data-busy] grey-out remains a deferred parity item); an idle-forever pane keeps rewind/edit latch-closed until the next organic settle (strictly safer than the storm); the armed-resync route's snapshot reuse stays with the tracked both-clients in_progress_snapshot sweep (the handler is line-identical in both clients — patching one side diverges parity).

Sibling found by the symmetry sweep: the coordinator has the same over-rewind gap (pre-existing since #888) — filed as #894 with a campaign-derived port checklist (latch-not-flag, quiesce-free retry variant, transport-free backstop, teardown wiring, storm-assertion validation) and a deferral comment at the coord site (the 2-line coordinator.js diff here).

Tests

  • Static pins (tests/test_interactive_pane_js.py): test_interactive_refetch_failure_preserves_the_pane — the interactive mirror of coord's test_coordinator_refetch_failure_preserves_the_pane — pins the no-pre-wipe shape, the empty failure branch (resolving, never throwing), quiesce-before-fetch ordering, the resumability predicate, the per-method affordance gates, the latch lifecycle, the transport-free backstop (reload call asserted absent), the retry's yield guard, and the destroy() cancel.
  • e2e harness (commit 2 — tests/_sse_recovery_server.py, scripts/recovery_e2e.py): an in-process fault layer (fail_history, delay_history, per-route request counters including per-ws /events connects; production app untouched) and five new browser scenarios beside the existing three:
    • fail-refetch — the issue's acceptance contract end to end: failed resync preserves the pane, armed retry heals, history_requests proves the re-fetch.
    • stale-ref-reload — the onLogin concatenation regression: next turn renders in a fresh bubble, stale bubble byte-unchanged.
    • rewind-window — in-window over-rewind blocked (rewind_requests == 1, server-side).
    • rewind-failed-window — the failed-fetch aftermath: gate stays closed after the failure, the bounded retry heals, the gate reopens.
    • stale-backstop — the storm regression: refetch + retry both fail, a plain send's organic settle heals via the backstop, and the per-ws /events connect counter stays FLAT across the entire episode.
    • Every regression scenario is negative-control validated: run against pre-fix code, each stamps the predicted RECOVERY-FAILED-* shape (fresh0-unchanged0, posts2-rows0, closed2-rows0, and the storm counter), then READY again with the fix restored.

Validation

  • 167 JS-pin tests green (test_interactive_pane_js.py, test_app_js.py, test_coordinator_page.py).
  • All eight harness scenarios RECOVERY-READY (first-hand full sweep on the final tree).
  • Full suite: 9707 passed / 10 skipped on the final tree.

Review

Six unprimed full-surface rounds (1–5 four-finder; round 6 two-finder at the converged tail). Correctness findings per round: 2 → 1 → 1 → 1 → 1 → 0; security and performance returned zero in five consecutive rounds each. Rounds 1–2 surfaced the port's own gaps (the onLogin ref flavor; the window gate) plus one route into the pre-existing snapshot secondary (ruled to the both-clients sweep). Round 3's major forced the seam re-derivation from the staleness invariant (the latch, adversarially design-reviewed before implementation). Round 4 caught the retry-timer teardown. Round 5's critical caught the backstop's manufactured-trigger storm and forced the transport-free re-derivation (adversarially attacked; the retry-yield guard folded from that review). Round 6's residue was a single harness nit, fixed in place. Every declined finding carries its ruling at the site.

Related: #884 / #893 — the server-side /history coalescing half of the restart-window pair (no file overlap; the coalescer absorbs exactly the herds these client paths emit).

eous added 2 commits July 22, 2026 16:46
Port the coordinator's #882 G3 guard-before-wipe: the wipe + streaming-
ref reset live in replayHistory, reached only on a successful fetch.

- clear_ui no longer pre-wipes the transcript; a failed refetch during
  a rewind/retry/resume replay keeps stale-but-real content instead of
  blanking the highest-traffic pane on a live stream (/history
  failures cluster in exactly the restart windows that emit clear_ui).
- _refetchHistory's failure branch is a DOM/ref/repair-intent no-op:
  no empty-state hint below stale content (the old resync-route wart),
  no streaming-ref reset (which orphaned a mid-jitter turn's bubble on
  the resync route); the truncation record stays armed for the
  connect-chokepoint retry; only the quiesce releases.
- _loadHistoryThenConnect resets streaming refs on a ws SWITCH only --
  the old ws's refs otherwise survive a failed fetch into the new ws's
  stream; a same-ws reload keeps them so the reconnect resumes the
  mid-jitter bubble instead of orphaning it.
- The factory connect() empty-state pre-seed is now the sole producer
  of the failed-first-paint placeholder -- documented load-bearing.

The edit-and-resend dispatch, cross-ws supersession, and repair-intent
lifecycle are unchanged; a failed fetch keeps the resend firing (the
rewind already committed server-side), mirroring coord.

Pinned by test_interactive_refetch_failure_preserves_the_pane (the
mirror of coord's test_coordinator_refetch_failure_preserves_the_pane)
plus the re-pointed quiesce/agent-tracking pin.
…istory failure paths

RecoveryServer grows an in-process fault layer (pure-ASGI wrapper; the
production app is untouched): fail_history(count) serves minimal 500s
for the next N GET /history requests, delay_history(ms) holds responses
to widen or hold open a refetch window, and per-route request counters
(history_requests, rewind_requests) let scenarios assert backend state
rather than scripted absence.

Five scenarios on that layer, all stamping RECOVERY-READY/FAILED
titles like their siblings:

- fail-refetch: hide mid-turn -> restart -> failed first resync ->
  the stale transcript survives (no wipe, no empty-state) while the
  truncation record stays armed -> the connect-chokepoint retry heals
  (history_requests proves the re-fetch). The #890 acceptance
  contract, browser-observed end to end.
- stale-ref-reload: mid-segment transport death -> turn completes
  during the outage -> failed unarmed same-ws reload -> the next
  turn renders in a FRESH bubble and the stale bubble's text is
  unchanged (regression test for the resumability-gated ref reset).
- rewind-window: a second rewind clicked during a held clear_ui
  refetch window never reaches the server (rewind_requests == 1) and
  the transcript reflects one rewind (regression test for the
  busy-or-latch affordance gate, in-window arm).
- rewind-failed-window: the failed-fetch AFTERMATH sibling — the
  refetch 500s, the staleness latch keeps the gate closed over the
  stale rows (rewind_requests stuck at 1, proven latch-not-quiesce
  via a settle-poll), the bounded turn-free retry heals (3 -> 1 user
  rows), and only then does the gate reopen (rewind_requests == 2).

Negative-control validated: with the interactive.js fixes reverted,
stale-ref-reload stamps fresh0-unchanged0 (the concatenation bug),
rewind-window stamps posts2-rows0 (the in-window over-rewind), and
rewind-failed-window stamps closed2-rows0 (the failed-exit
over-rewind) — every detector observes its bug, then stamps READY
again with the fixes restored.
Copilot AI review requested due to automatic review settings July 23, 2026 00:08
…ic/interactive.js

Rider from the session queue: the comment cited ui/static/app.js
Pane.replayHistory, which moved to shared_static/interactive.js in the
L-shell step-5a lift — the old path no longer exists.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an interactive-pane failure mode where a /history refetch error during clear_ui would blank the transcript. It ports the coordinator’s “guard-before-wipe” behavior into interactive.js, adds a transcript-staleness latch to prevent mutating actions during/after a failed refetch, and expands the JS pins + browser recovery harness to lock in the new recovery contract.

Changes:

  • Update interactive.js to only wipe/reset on successful history fetches, add _historyStale gating for mutating affordances, and add a bounded retry + transport-free idle-edge backstop.
  • Add/extend static JS-pin tests to assert the new failure-preserves-pane behavior and latch lifecycle.
  • Enhance the recovery e2e harness with in-process fault injection and new browser scenarios covering failed refetch, stale streaming refs, rewind gating, and the transport-free backstop.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
turnstone/shared_static/interactive.js Implements guard-before-wipe, _historyStale latch + affordance gating, bounded retry, and transport-free idle-edge heal behavior.
turnstone/console/static/coordinator/coordinator.js Adds a local breadcrumb comment documenting the known coordinator-side over-rewind gap (#894).
tests/test_interactive_pane_js.py Adds static pins asserting failure-preserves-pane + latch + retry/backstop semantics.
tests/_sse_recovery_server.py Adds ASGI fault-injection wrapper and counters for browser recovery scenarios.
scripts/recovery_e2e.py Adds new browser scenarios and runner plumbing for #890 regressions (fail-refetch, stale-ref, rewind-window, etc.).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/_sse_recovery_server.py Outdated
Comment on lines +414 to +416
"""Hold each ``GET …/history`` ``ms`` ms before forwarding (0
clears). Opens the clear_ui-refetch quiesce window that the row
affordance gate (``busy || _replayQueue``) must close."""
Comment thread scripts/recovery_e2e.py
Comment thread turnstone/shared_static/interactive.js Outdated
Comment on lines 2404 to 2405
// Cross-ws supersession: release the latch + busy so the
// composer recovers; the edit belongs to a pane state that
Copilot AI review requested due to automatic review settings July 23, 2026 00:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

turnstone/shared_static/interactive.js:2400

  • The stale-history retry timer calls _refetchHistory(...) without awaiting or attaching a .catch(). If replayHistory() throws (by design it’s not swallowed inside _refetchHistory), this becomes an unhandled promise rejection from a setTimeout callback, which can surface as a global unhandledrejection and make failures harder to attribute.
              this._staleRetryTimer = setTimeout(() => {
                this._staleRetryTimer = null;
                if (
                  token === this._historyLoadToken &&
                  this._historyStale &&
                  !this._replayQueue &&
                  !this.busy &&
                  !this.currentAssistantEl &&
                  !this.currentReasoningEl
                ) {
                  // !_replayQueue: yield to an in-flight quiesced
                  // fetch (the idle-edge backstop shares this token)
                  // instead of stomping its queue for a same-token
                  // double-render.
                  this._beginReplayQuiesce(token);
                  this._refetchHistory(this.wsId, token);
                }

Comment on lines +2109 to +2111
const staleToken = this._historyLoadToken;
this._beginReplayQuiesce(staleToken);
this._refetchHistory(this.wsId, staleToken);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberate here — and now documented at both the backstop and the bounded retry.

This fire-and-forget _refetchHistory carries no composer/edit state to un-strand: at the idle edge busy is already false and no _pendingEditSend is queued. The primary clear_ui caller has a .catch only to recover a queued edit-and-resend; these two sites have nothing to recover, so they intentionally match the load path (_loadHistoryThenConnect's .finally) and leave a render throw loud/uncaught rather than mask it as an empty pane — the ruled design in _refetchHistory's header ("the render is deliberately OUTSIDE the catch so a render bug surfaces loudly instead of being masked as an empty pane"). _refetchHistory's own finally still releases the quiesce, and the latch self-heals on the next organic settle, so nothing wedges.

On the test-runner point: this PR's recovery harness asserts on document.title verdicts, not unhandledrejection, so there's no exposure here. (scripts/livepass.py does listen — if we ever want to remove that surface entirely, the clean fix is a non-swallowing console.error catch applied consistently, including the load path; I've kept that off this converged branch as scope.)

Added the at-site rationale in 4b56b75.

…forget

Addresses the Copilot review of #895 (docs/comments only, no behavior change):

- recovery_e2e.py / _sse_recovery_server.py: the mutating affordance gate
  is `busy || _historyStale`, not the superseded `busy || _replayQueue`
  quiesce gate the r3 latch replaced — corrected both docstrings (E2 now
  matches E3).
- interactive.js cross-ws supersession: the branch drops the pending edit
  and releases busy but does NOT clear `_historyStale` (its sole clear
  site is replayHistory) — reworded so it no longer implies the latch is
  released.
- interactive.js idle-edge backstop + bounded retry: documented that the
  fire-and-forget `_refetchHistory` (no `.catch`) is deliberate — no
  composer state to un-strand there, unlike the primary clear_ui caller,
  so a render throw stays loud (peer of the load path's `.finally`).
Copilot AI review requested due to automatic review settings July 23, 2026 00:31
@eous
eous merged commit af918c3 into main Jul 23, 2026
18 checks passed
@eous
eous deleted the fix/890-clear-ui-guard branch July 23, 2026 00:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

interactive pane: failed /history refetch blanks the pane (clear_ui route) — port the coordinator's guard-before-wipe

2 participants