Responsive terminal restore: paced replay, honest gaps, bounded recovery, spill-before-disconnect - #822
Merged
Conversation
…plied checkpoint and scope the increment's hidden-pane acceptance to terminal screens
…lifetime-claim release semantics
Responsive-terminal-restore Workstream 1 negotiation rail (additive optional, no protocolVersion bump): the hello opt-in and ready echo for paced terminal replay, threaded hello -> ready gate -> terminal::run -> run_loop -> handle_client_text -> handle_attach -> registry attach, and parked on the attach subscriber alongside terminal_output_batch_v1 for the task-3 paced replay core. A hello without the capability stays byte-identical on both sides (frozen-client inertness), pinned by protocol roundtrips, in-file handshake tests, a real-socket e2e suite, and the client allowlist/lifecycle tests. Contract artifacts regenerated in lockstep.
…y and output gaps
Responsive-terminal-restore Workstream 1 task 3 (the bounded recovery increment's paced path): replace the inline full-replay burst for negotiated connections with bounded, ascending pages driven by a new terminal.replay.credit continuation message. Registry: the negotiated+attachRequestId+Running attach arms the subscriber deferral (the ring becomes the staging), sinks the ready/sync/retention-gap prelude, and returns the FIRST page (selected under the lock, no full-ring clone) plus the session description (fixed target = head at attach, retention-adjusted baseline). next_replay_page packs pages to the serialized budget (batch-builder accounting; an oversized frame forms its own atomic page) and reports Done or the exact Expired interval; next_paced_tail_page drains the accumulated live range and clears the deferral ATOMICALLY when the ring is drained, so live output can never overtake an un-sent page and the flag-clear boundary neither loses nor duplicates a frame. ingest skips deferred subscribers. maxReplayBytes threads through both attach paths and is recorded on the subscriber (TERM-07 seam, no delivery change). WS: the per-connection pacing coordinator (session table, credit window validation, drive loop) produces at most ONE unacknowledged page per credit; a re-attach supersedes the session, detach and socket drop cancel it. Server-pushed terminal.output.gap frames ride the output queue as sequenced controls (never preempting pages). Observability: ws.restore.paced_start/paced_complete/paced_expired/credit events (identifiers and measurements only). Non-negotiated connections stay byte-identical; the frozen legacy pins, batch goldens, and the task-1/2 negotiation suites pass unchanged. Contract artifacts regenerated in lockstep (client surface 41->42, protocol version stays 10). The client's credit-sending behavior is task 4.
…et and event pins Review fix commit for the paced replay core (task 003 findings): - I1: every SUCCESSFUL re-attach now cancels the connection's previous paced session for the terminal (the Legacy reply arm included) — a stale-generation credit after an arid-less legacy re-attach can no longer produce phantom pages. A failed attach (Error) still cancels nothing: the previous session keeps matching its live subscriber and deferral. Pinned end-to-end on real sockets. - M1: batch-mode page accounting now charges the once-per-batch envelope delta (the .batch type suffix + serializedBytes + the segments[] wrapper: 41 fixed + digits(budget)) on every batch-mode frame, so a page the walk believes fits the budget really fits — every produced batch contains >=1 frame, so multi-batch pages are covered too. Boundary-pinned with barrier-frame fixtures tuned to the budget edge. - M2: the ws.restore.* observability events are pinned on the real dispatch (paced_start with max_replay_bytes/page_bytes, paced_complete, all four credit verdicts) via the process-global capture rig, with a content-free assertion (no terminal data in any event field). - N1: the geometry-authorized attach path's maxReplayBytes recording is pinned inside the simultaneous-geometry test. - N3: the paced_replay harness's quiet-gap heuristic matches text frames only — a keepalive ping inside a read window no longer truncates a page read.
Client side of responsive-terminal-restore Workstream 1, gated on the pacedTerminalReplayV1 capability echo (no echo -> byte-identical legacy wire behavior): - Negotiated attaches (fresh and delta) send replayPageBytes and omit maxReplayBytes; all viewportHydrateReplayOptions sites pass the negotiation flag. - New src/lib/paced-replay-consumption.ts tracks the ordered consumption frontier per attach generation: it advances when a frame is applied through the write queue (completeParserAppliedFrame) or fully consumed by a null-screen-effect pre-parser (startup probes, OSC52, turn signals); unknown mutations keep the quarantine and forfeit credit. The frontier is distinct from the parser-applied checkpoint, which still refuses filtered/lost ranges. - terminal.replay.credit is sent at most once per write-queue drain tick (coalesced via the queue's onDrain plus a flush task for filtered-only advances), clamped to the highest received seq and the session-window target from attach.ready. - attach.ready contract fields (requestedSinceSeq, effectiveSinceSeq, oldestRetainedSeq, replayResetReason) are recorded as terminal.restore.paced_ready perf audit events; negotiated replay_window_exceeded gaps render an accessible aria-live notice (unknown-bounds semantics for absent fields, recorded as terminal.restore.retention_gap) and never trigger the opencode replacement kill — live output continues.
…of restarting Workstream 2 of the responsive-terminal-restore plan (bounded recovery increment, client side): - Remove the automatic OpenCode replacement kill path entirely: a replay_window_exceeded gap never kills, replaces, or changes terminal identity on negotiated or legacy shapes — honest notices only. - Add a reconstruction-safe surface-coverage cursor, distinct from the strict parser-applied sequence: applied frames advance both cursors, fully pre-filtered frames (startup probes, OSC52, completion signals) advance only coverage, and unknown mutations, lost ranges, and unapplied ranges pin it (contiguity-gated, monotonic per surface generation). - Carry surfaceCoverageSeq through the checkpoint's existing identity/geometry/authority validation; delta resumes request sinceSeq from the coverage cursor (legacy checkpoints keep their applied-position resume). - Represent incomplete hydration separately from a fresh surface: a marker-armed surface that already consumed content resumes the remainder via transport_reconnect (no clear, no surfaceReset re-claim, no preamble resend); a hydrate interrupted before any consumption keeps the full-hydrate path. - Scope checkpoints to the actual surface (pane-keyed store): sibling panes rendering the same terminal cannot borrow or overwrite each other's progress; chooseCheckpoint keeps the highest coverage within a scope. - Quarantined attaches defer the applied-surface decision to the drained repair, which resumes a survived checkpoint when nothing completed during the freeze and rebuilds honestly otherwise (new onWriteCompleted ledger counts stale-generation completions as surface mutations). - Bound automatic recovery by attempts (3) and a no-progress deadline (30s): past the bound, automatic re-attach cycling stops and a visible, accessible retry state preserves the rendered content; genuine coverage progress or explicit retry (retry strip, pane refresh) resets it. Never kills or replaces. - Carried task-4 review fixes: M1 pins the complete legacy attach payload byte-exactly on the old-server path; M2 distinguishes full pre-parser consumption (cleaned === '') from enqueue failure so unconsumed bytes are never credited.
…overy accounting Review-fix round for the WS2 resume-instead-of-restart increment: - I-1: checkpoints carry a per-surface-instance discriminator (surfaceInstanceId, the xterm surface instance id — mount-stable and renderer-recreation-stable), validated on load, merge-gated in chooseCheckpoint, and rejected by the replay decision (surface_changed). A same-pane remount (same store key, colliding epoch) can never reuse the previous mount's cursor: the resume never exceeds the new surface's rendered position. Delivers the plan's WS2 reload test bullet. - M-1: reconcile-episode attaches collapse into a single counted recovery attempt (per-episode attemptKey through the recovery gate); a healthy pane needs more than one legitimate reconcile episode plus one reconnect to reach the retry strip, while progressless episode storms stay bounded. - M-2: removed the quarantine-drained checkpoint RESUME branch — unreachable in production shapes (the quarantining write's completion always lands in the completedWrites ledger) — keeping the conservative rebuild; the repair rebuild is pinned even against a valid pre-quarantine checkpoint. - M-4: lifecycle-level, timer-driven pin that the 30s no-progress deadline reaches the retry state through the wired gate (deadline alone, below the attempt bound). - N-1: normalizeCheckpoint clamps coverage up to the applied position, healing storage-corrupted coverage<applied entries that would resume below rendered bytes.
Negotiated (terminalLifetimeClaimV1) hidden panes no longer attach for screen hydration: their terminalIds ride terminal.interest snapshots as claimedTerminalIds, a non-hydrating per-connection lifetime claim with attach-exact release semantics (Workstream 1, responsive-terminal-restore). - Registry: per-terminal claim set; claim clears released_by_client without attaching; explicit withdrawal of the last claim restores threshold-eligibility with the DEV-0009 grace bump; detach releases only when the last claim is gone; remove_connection sweeps claims but never restores released_by_client (transport loss is not release - 24h hard cap only, identical to attached-then-disconnected). - Wire: additive optional claimedTerminalIds + hello/ready capability echo, protocol version stays 10; contract bundle regenerated. - Client: hidden mount/created/reconnect/refresh arm the deferred reveal intent and attach nothing under the echo; the hidden keepalive attach, hydration-queue registration, and grant path remain byte-identical for old servers (fallback); reveal attaches exactly once via the existing reveal plan; status keeps flowing to hidden panes.
Workstream 3 of docs/plans/2026-09-19-responsive-terminal-restore.md: normal output pressure now reaches bounded admission/spill (eviction + generation-scoped gap) strictly before any pressure-related disconnect, and the last-resort disconnect keys on drain progress, not bytes alone. Chosen defaults (the production incident's ~21-25 MB backlog now spills gracefully instead of disconnecting the client): - TERMINAL_CLIENT_QUEUE_MAX_BYTES (spill bound): 32 MiB -> 16 MiB (DEFAULT_TERMINAL_CLIENT_QUEUE_MAX_BYTES). The incident backlog exceeds the spill bound, so oldest output evicts with an honest coalesced gap while the connection stays open; 16 MiB keeps a multi-second grace buffer for a slow-but-draining client. - TERMINAL_WS_CATASTROPHIC_BUFFERED_BYTES (disconnect bound): 16 MiB -> 64 MiB, strictly (4x) above the spill bound. Legacy's 16 MiB threshold sat BELOW its own 32 MiB spill bound, so the incident disconnected before eviction could relieve the same pressure. Eviction holds pending bytes at or below the spill bound, so a threshold above it is unreachable by ordinary output pressure: the monitor is a last-resort guard, with the per-send write timeout preserved independently. - TERMINAL_WS_CATASTROPHIC_STALL_MS: unchanged at 10 s; the window now additionally requires zero successful sends. - Config validation, fail fast: Term09Config::validate() enforces catastrophic_buffered_bytes > queue_max_bytes (strict) plus per-field floors (64 KiB queue / 100 ms stall); freshell-server resolves it at boot and refuses to start with a structured server.config.term09_invalid event naming the offending env vars. - Drain-progress liveness: WriterPump::finish_frame counts completed socket sends per connection; the select loop feeds the per-tick delta to CatastrophicMonitor::tick(pending, sends_since_tick) and the sustained window resets on EITHER recovery below the threshold OR send progress. Eviction/supersede byte reductions are not sends and never reset it. Keepalive termination is untouched. - Observability: rate-limited ws.terminal_stream.queue_overflow_spill events at gap-lease time (interval + suppressed count + pending bytes); ws.terminal_stream.catastrophic_close gains sends/window_ms fields. Integration coverage (real axum server + real PTYs): a slow-but- progressing client survives incident-scale pressure with an exact eviction gap; a progressing client survives over-threshold backlog that the old sustained-bytes decision closed; a no-send connection whose bytes shrink only via eviction and a superseding attach still closes. Floods in the term09 suite are serialized (a starved reader legitimately trips the liveness windows under test) and marker detection is O(1) per frame (the previous whole-buffer scan was quadratic at incident scale).
Task-008 of the responsive-terminal-restore run (test consolidation; no production changes — every behavior landed in tasks 1-7): Server side (real-socket harnesses): - hello_capabilities: raw-TEXT byte-identity pins for the non-negotiating ready frame (pre-branch literal bytes, not serde Value equality — task-1 review Nit 1) and the combined paced+paneReconcile echo (Nit 2). - paced_replay: a non-negotiated attach on large scrollback stays fully legacy (one unprompted inline burst beyond the page budget, no contract fields, inert credits observed as ws.restore.credit non_negotiated); an old client after ring eviction gets the retained tail SILENTLY (never the new replay_window_exceeded gap); a negotiated and a legacy client on one terminal converge with no cross-talk (identical seq coverage and byte-identical content, paced side driven to paced_complete). - terminal_lifetime_claim: a claim from an interest-only connection is ignored AND the terminal stays threshold-reapable (sweep outcome, not just claim_state). Client side (Vitest): - TerminalView lifecycle: the task-5 M1 full-shape pin extended into a systematic downgrade sweep — transport_reconnect, hidden-pane keepalive_delta fallback, reveal promotion, handleLoadMoreHistory, and the opencode hydrate are all full-shape toEqual pins against pre-branch wire shapes, with zero terminal.replay.credit sends; a negotiated pane downgraded at reconnect uses legacy shapes next attach; an old-server retention stream (byte-budget gap, backlog gap, silent retained tail) never kills, replaces, or changes identity. - ws-client.reconcile: the capabilities reset-on-disconnect pin extended to terminalLifetimeClaimV1, plus the negotiated->old-server reconnect downgrade cycle. Every matrix cell has mutation-style RED evidence (temporary source flips, reverted): echo leaks, legacy gap emission, credit classification, combined-negotiation swallowing, cross-connection replay suppression, claim gating, the negotiation gates, and the resurrected kill path are each demonstrated to fail the new pins.
…annot exec-loop the worker
…ce sends_in_window to catastrophic close Task-007 review M2 (task-010): the rate-limited ws.terminal_stream.queue_overflow_spill event now fires at ADMISSION time — the moment the eviction happens — instead of at gap-lease time. The delivery queue surfaces one record per evicted entry (EvictedOutput/take_evictions) and the connection writer drains them under the admission lock, emits after the lock is dropped, and folds in-window evictions into the suppressed counter. A connection that spills and then dies while backlogged (its coalesced gap never leased) now still leaves spill evidence in the live log; leasing the gap later is delivery, not a spill occurrence. Task-007 review M3: the ws.terminal_stream.catastrophic_close event is now diagnosable from the log line alone — CatastrophicMonitor::tick returns CatastrophicFire carrying sends_in_window (completed sends DURING the deciding window for THIS occurrence, structurally zero; nonzero would be accounting drift), and the event's lifetime counter is renamed sends -> total_sends so the pair distinguishes wedge-after-progress from never-sent. Task-007 review Minor 1 (reproducing under load): retune slow_but_progressing_client_survives_over_threshold_backlog margins — queue 8 MiB -> 2 MiB (the minimal injected inversion), drain ~2 MiB/s -> ~1 MiB/s (the incident test's demonstrated accumulation regime), stall window 2 s -> 5 s, deadline 30 s -> 75 s. Every assertion unchanged; only the reachability margins were re-derived. Task-007 review N2: fix the stale TERMINAL_REPLAY_RING_MAX_BYTES annotation (default is 1 MiB, not 256KB) and describe the negotiated replay path it feeds. Docs: AGENTS.md PTY Lifecycle prose corrected (retained-output ring, non-negotiated inline tail) and a Negotiated Restore paragraph added (paced replay, credits, gaps, claims, spill-before-disconnect).
…velope oracle lane Task-008b review F1: a persist flush that throws (QuotaExceeded etc.) no longer consumes the dirty cycle. On failure the dirty flags stay armed, the user-close authorization is restored (the pending dirty state IS that close — without restoring it the empty-tabs guard would refuse the retried write and permanently lose the user's close), and the failure surfaces as exactly one structured error line (reason: persist_flush_failed). No retry loop: the next flush opportunity (a new action, flushPersistedLayoutNow, or the visibility/pagehide flushNow) retries the same dirty state and clears the flags on success — the credible candidate for the one-shot codex-refresh rehydrate failure (persisted terminalId undefined; never reproduced in ~100 attempts). Task-008 review F1 (oracle flake): the pty-determinism-t1 envelope comparison was value-dedup seq normalization, which is sensitive to WHICH attach.ready fields happen to share a raw value — and that coincidence partition flips with a contract-valid startup race (whether the shell banner lands in the retained ring before the attach snapshots it). Two boots of identical code could therefore produce different "normalized" envelopes. Add maskEnvelopeShape — a field-scoped shape mask (each registered nondeterministic leaf becomes a stable per-family-per-field placeholder, a pure function of the envelope's structure) — and use it for the T1 cross-boot envelope comparison. Presence, nesting, array counts, and deterministic contract values survive, so genuine structural divergence still differs; normalizeTranscript's value-dedup semantics (load-bearing for cross-message reference tracking) are untouched, and a characterization test pins the root cause.
…coverage-cursor resume
… level-filter the capture registries End-of-execution gate at 3804cab: the rust lane failed queue_overflow_gap_bounds_follow_negotiation — the fast, always-reading client did not drain a ~1.8 MB flood within its 20 s wall-clock deadline under full-workspace gate load on the loaded shared box (the same drain measures 1.9 s isolated). Task-10 already retuned the sibling slow_but_progressing_client_survives test for exactly this class; this test's fast-client deadline had the same brittleness and simply did not reproduce in that run. Margins only; assertions are byte-identical. Fast-client (always-reading) drain deadlines 20 s -> 60 s in both flood tests, the stuck-client resume waits 10 s -> 30 s and 20 s -> 60 s (paced/plain gap-frame deadline). The deadlines bind only failure diagnosis, never pass-path wall time (the drain loops break at the marker). Deliberately unchanged: the incident test's throttled-drain bound (sized to its own enforced ~1 MiB/s pace — the scenario under test; it held in the failing gate) and the eviction/supersede close-observation bound (must sit below the 60 s write timeout so a close is attributable to the monitor). Capture hygiene: both process-global test capture registries were unfiltered — every callsite enabled process-wide, a latent perf and determinism footgun. term09's capture now installs a WARN level filter (its only read is the warn-level ws.terminal_stream.catastrophic_close). The paced_replay twin reads INFO-level ws.restore.* events from exactly two modules, so it installs a WARN-default Targets filter admitting freshell_ws::paced_replay and freshell_ws::terminal at INFO; nothing either binary's assertions read is disabled.
…(60s, capture-attributed)
A credit inside the outstanding page (below its end) previously granted
the next full page, leaving up to two page budgets of unacknowledged
replay in flight and contradicting both the plan's credit rule ("Credit
is granted only after the prior batch is consumed in order") and this
module's own one-unacknowledged-page declaration. Partial consumption
reports are now observed as partial_consumption and grant nothing; only
the page-end value produces the next page. The client is unchanged: its
coalesced frontier credits naturally carry the page end once a page is
fully consumed, and per-page RTT on the incident LAN is sub-millisecond.
The old unit test blessing the mid-page grant is replaced by the strict
contract test (it blessed plan-contradicting behavior); the double-grant
guard coverage it carried is preserved.
…tail target The tail drain re-read the terminal's CURRENT head on every page, so a continuously-producing terminal could keep CaughtUp unreachable and monopolize the connection's inline dispatch task forever — violating the plan's "Keep a fixed initial catch-up target so ongoing live output cannot move the completion condition indefinitely" and "Input and other panes' traffic remain schedulable". The drain now captures a FIXED tail target at tail-start (the head at that instant), pages only up to it (each page budget-bounded), then completes atomically: a drained ring clears the deferral under the same lock hold, and a staged remainder is delivered through the subscriber's sink and the deferral cleared in one lock hold (the live handoff — the same under-lock sink discipline as ingest's fan-out and the paced attach's prelude), so frames can neither be lost nor duplicated across the clear boundary and newer frames fan out directly. The completion condition no longer moves with production and the per-dispatch work is finite. next_paced_tail_page gains the to_seq_inclusive bound and an AtBoundary arm; complete_paced_tail is the new one-shot completion. Registry tests driving the old 4-arg call were ported with their intent preserved (quiet-terminal drain to head; the concurrent-feeder exactly-once race now also exercises the handoff), and the bounded-completion test is new.
…ction A negotiated queue_overflow gap only wrote a local notice and quarantined sequence state — it never initiated repair, so a spill left the current screen missing output until some unrelated future reconnect (plan WS3: "A gap followed by repair yields the correct screen"; the shared contract: delivery loss means "repair from retained output", never silent advancement). The gap arm now initiates ONE bounded repair attach per gap on the still-open connection — a full viewport-hydrate rebuild (the surface already rendered the post-hole frames, so a delta resume would duplicate them) — through the existing recovery accounting, so repeated gaps still exhaust to the visible retry strip. Old-server (non-negotiated) behavior is unchanged: local notice only, pinned by the existing fails-closed test.
The streak reset only on a coverage ADVANCE or an explicit retry, so a converged idle pane's successful empty-delta reconnect restores were charged as progressless attempts and N ordinary flaps stranded a healthy pane on the retry strip — conflating restore success with stagnation (plan WS2's bound targets broken restore cycles, and goal 4 requires healthy panes to keep restoring across reconnects indefinitely). markAttachComplete now applies a clean-restore reset when the current generation received its attach.ready, completed, and stayed gap-free (new per-generation taint tracking: the gap arm, invalid-batch loss, and stream-identity mismatches taint; a gap-initiated repair generation is additionally marked non-resetting so repeated queue_overflow repairs still exhaust to the strip), emits terminal.restore.recovery_success, and clears an existing exhaustion. Broken cycles (no ready, gaps, failures) still exhaust the bound — pinned by the pre-existing progressless tests and a new gap-taint test. The e2e scenario that blessed the defect (a converged idle pane's 4 flaps reaching the strip) is rewritten: the bound target is now the retention-expired pane, whose every reconnect is gap-tainted with no coverage progress — a genuinely broken cycle that still trips the strip, stops cycling, never kills, and re-arms on retry; its strip-time flood-row floor (a timing race against where the cadence interrupted the last hydrate rebuild) is replaced by the deterministic never-killed guarantees (no kill/create, identity unchanged, process running, retry rebuilds the full screen). The healthy counterpart in the same test pins the fixed behavior: five converged-pane flaps, each waiting for the pane's reconnect-restore to converge, never reach the bound and the pane still auto-attaches.
complete_paced_tail delivered the staged tail remainder as ONE i64::MAX-budget batch cloned under a single terminal-lock hold (an unbounded unacknowledged window that blocks PTY ingestion and monopolizes the dispatch path), and it never checked retention: a producer advancing retention past the fixed tail boundary made the handoff silently start at the ring front, jumping the client's applied cursor across missing output with no gap frame (plan W1: at most one unacknowledged page; retention loss mid-restore is an exact bounds-carrying gap, never a silent truncation). The completion now drains through the SAME budget-bounded paging as the replay: one paced_page_max_bytes page per call, the lock held only per page, the deferral cleared only at the completing verdict (the drained clear, or the final page that covers everything staged, sunk and cleared in one atomic hold). It loops toward fixed targets re-captured per round (never a moving-head chase within a round), and a retention advance past the drain cursor returns the exact Expired interval so the ws drive loop emits the bounds-carrying gap and resumes from the ring front. The continuous-producer test now asserts the delivered stream is contiguous across the paged handoff; new tests pin the multi-page budget-bounded handoff and the exact retention-gap reporting with a contiguous-after-gap continuation.
… floor Round-2 finding F2 (Major): the reviewer's reachability claim is wrong under supported settings — every PTY byte is ingested through the fragment splitter (pty.rs -> OutputFramer::append_output_frames -> fragment_terminal_output_for_payload_budget), whose budget measure is the full serialized terminal.output JSON with the worst-case seq placeholder and the 512-char attachRequestId reserve, so an 8 KiB control-char-heavy chunk measuring ~49 KiB is SPLIT into fragments each <= 16 KiB by default — far under the 32 KiB paced page budget floor (the 64 KiB queue floor's admission ceiling). No frame can exceed the page budget; the page builder's over-budget first frame is unreachable. Close the class permanently so no env override can reopen it: terminal_stream_batch_max_bytes is clamped to PACED_PAGE_BUDGET_FLOOR_BYTES (32 KiB — the smallest page budget any supported queue setting can produce, cross-pinned against paced_page_budget_ceiling at the queue floor in freshell-ws). The env read is extracted into a pure terminal_stream_batch_max_bytes_for_env seam so the clamp is tested without process-env races; the reviewer's ~49 KiB control-chunk arithmetic is pinned as evidence. The atomic single-frame page stays as unreachable defense-in-depth and its test is retitled to say exactly that.
Round-2 finding F4 made stream identity part of the paced continuation contract, which exposed the integration fixtures: the credit() helper sent streamId "ignored-by-server" (the server used to ignore the field), so every credit in the paced_replay suite would now be rejected as a stream mismatch. The fixtures now record each terminal's real stream id from its attach.ready and send it; tests that credit without having observed a ready use the explicit credit_with_stream helper. The ready readers (helpers and inline) feed the registry; the map is poison-tolerant so one test's panic cannot cascade.
Round-2 finding F3 (Minor): the client emitted replayPageBytes on every negotiated attach, but the field was absent from TerminalAttachSchema, the Rust TerminalAttach struct, and the generated contracts — the server's accept-and-strip deserialization discarded it and every paced session paged at the registry's own cap. Wire it through honestly: - shared/ws-protocol.ts + regenerated port/contract artifacts own the optional positive field on the frozen wire contract. - The Rust TerminalAttach parses it with a lossy positive-int deserializer: malformed or non-positive values fall back to the server default (never failing the whole attach frame — the pre-contract accept-and-strip tolerance, preserved). - The registry takes PacedAttachOptions through both attach paths and bounds the session at min(requested, registry cap) — the effective budget sizes the FIRST page, is recorded on PacedSessionDesc, and rides the ws session so credits, the tail drain, and every later page page at the SAME bound; nothing re-reads the registry cap mid-session. - ws.restore.paced_start now carries page_budget for diagnosability. Registry tests pin honor/clamp/absence/invalid; the protocol roundtrip pins the wire shapes; the e2e proves the bound reaches the budget over the real socket (paced_start.page_budget == 2048 under a 4096 cap), that packable pages stay within it while the session converges, and that malformed/absent values keep the cap.
…inal.exit Round-2 finding F1 (Major): a paced subscriber received no live output while replay was active, but the natural-exit path sank terminal.exit immediately and cleared every subscriber — output ingested after the attach target stayed only in the ring (page reads Gone, the client's exit handler rejecting late frames), so a short-lived command or a failing restored CLI that exited mid-restore lost its final output, and the exit could even precede the first replay page. Fix, within the existing fixed-target machinery: - finish_pty_exit partitions the exit fan-out: a subscriber whose deferral is still armed gets its exit STAGED (paced_exit_pending) and fires the connection's notify hook (installed atomically with the attach via PacedAttachOptions) outside the lock; every other subscriber keeps the frozen immediate exit-and-retire behavior. - The connection loop routes the notify into the EXIT-DRAIN — the existing off-dispatch, reserve-then-admit drain with the fixed head-at-exit target — for sessions still in the credited phase; a session already in its drain is owned by that drain task. - Every completing verdict that clears the deferral delivers the staged exit in the same lock hold, ordered after the final pages and gap frames through the same sink, and retires the subscriber (plan:190 sequenced exit delivery); the retention-overrun bounded-baseline exit sequences gap-then-exit the same way. Registry tests pin staging + ordered delivery + the unchanged neighbors (non-paced immediacy, attach-after-exit legacy shape); the ws e2e proves the whole chain over a real PTY (final marker frames then exit, nothing after); the client lifecycle gains the ordered-delivery case alongside the untouched late-frame safety assertion.
…ytes Focused E2R1 finding 3 (Minor): the lossy replayPageBytes deserializer recognized only Serde JSON's integer storage variant, so integer-valued float spellings (2048.0, 2e3) parsed as floats and as_i64() returned None — the client's requested bound was silently dropped and the larger server default applied, while the TS/Zod contract and the generated JSON Schema accept the same value as an integer. Fix: the deserializer accepts and validates the number's VALUE — the integer storage directly, or the float storage when the value is integral (finite, positive, in i64 range). Fractional, non-finite, non-positive, and out-of-range values keep the malformed/non-positive fallback (None — the server default), never a wrong bound; the accept-and-strip tolerance for wrong-typed values is unchanged. Roundtrip RED evidence: rich_client_messages failed on the 2048.0 spelling (left: None, right: Some(2048)) before the fix; both spellings now parse to their integer values and the raw frames conform to the frozen ClientMessageSchema (integer-valued floats satisfy type: integer). The TS side pins the same parity on the unchanged Zod schema (ws-protocol.paced-attach.test.ts). Schema/Zod acceptance unchanged.
Focused E2R1 finding 2 (Major): any positive replayPageBytes was recorded as a hard page budget, but production frames are only fragmented to the fixed fragment cap — a valid request below that cap could still receive a larger ATOMIC single-frame page. The wire test demonstrated this directly (a 2 KiB request receiving ~8.5 KiB single-frame pages) while checking the limit only for multi-frame pages, and the paced drain reserved only the requested bytes while admitting the larger atomic page, so concurrent sub-cap drains could overbook the admission gate. Contract honesty (the exception is now explicit in the wire contract): - shared/ws-protocol.ts: replayPageBytes carries a .describe() — pages are bounded by max(requested, the atomic frame size); a single frame larger than the request forms its own atomic page, bounded by the server fragment cap — and the generated contract regenerated with it. - crates/freshell-protocol: the Rust field doc carries the same text. - Wire test: the single-frame assertions added honestly — a single-frame page may exceed the request but reaches only the documented atomic page ceiling (paced_atomic_page_serialized_ceiling = fragment cap + the page-envelope slack), and the fixture proves the exception is REAL (at least one single-frame page over the request). Admission honesty (the reservation accounts what it can be admitted): - fragment.rs: paced_atomic_page_serialized_ceiling() — the worst-case serialized single-frame page, proven by test against a maximal fragment in both wire forms (plain terminal.output and the real batch projection). - paced_replay.rs: drain_admission_bytes(budget) = max(budget, ceiling) is what spawn_paced_drain reserves; a budget at-or-above the ceiling (every supported default) is byte-identical to before. RED evidence: - Writer unit lane (deterministic): concurrent_sub_cap_drain_admissions_never_under_reserve_the_atomic_page — with the reservation reverted to the bare budget, three concurrent sub-cap drains all grant against a just-under-watermark backlog and admit 30,957B of atomic pages (the test failed 'no drain admitted anything before the queue drained'); with the fix, none grant until real consumption frees the gate and zero spills occur. - e2e (during fixture bring-up, multiple pre-fix runs spilled real queue_overflow gaps — e.g. seqs 438-440 at a 20 KiB queue — from the same overbooking): the committed e2e pins the end-to-end sub-cap shape at the canary's production sizing (sub_cap_page_requests_restore_end_to_end_at_the_atomic_page_bound), mirroring the round-5 split where the tight aggregate bound lives at the deterministic writer lane and the e2e proves the shape. - Registry: the_atomic_single_frame_page_stays_within_the_documented_ceiling pins the atomic page in both wire forms against the ceiling. - paced_replay unit: drain_admission_never_under_reserves_for_an_atomic_page. Wire/protocol gates: contract regenerated; test:port 51/51; test:oracle 162 passed / 3 skipped (pre-existing skips).
…completion Focused episode-2 round-1 findings 1 and 4. Finding 1 (major): the natural-exit path removed the still-credited session from PacedSessions and dumped its deferred window through the uncredited drain, breaking the pacing contract at the exit boundary. A withholding client received the final pages and the exit without ever crediting them. The fix keeps the session in its credited phase: the registry's exit staging (finish_pty_exit) is unchanged, but the connection's notify arm now EXTENDS the credited phase once through the terminal's final head (PacedSession::exit_head, idempotent + monotone arming; the head is frozen by the exit, so there is no moving-head chase). The deferred final output pages only on continuation credits; the completing verdict (the empty drain's CaughtUp hold) delivers the staged exit after the last page. A withholding client gets no pages and no exit until it credits; a dead connection ends the wait naturally via keepalive termination. The credit handler's DrainReady arm queries the registry directly (staged_paced_exit is the authority — staging happens under the terminal lock before the notify fires), closing the credit/notify race symmetrically with the attach-time arm in start_session. Finding 4 (minor): the exit-ordering test now asserts over the FULL frame stream — it collects every frame until a deterministic quiet window closes AFTER the exit, so its output arm can prove no frame follows terminal.exit (the old test broke on the first exit and could never observe a post-exit frame). Two companion tests pin the contract from the review: a withholding client receives no pages and no exit until it credits, and a retention overrun mid-wait reports the EXACT bounds-carrying gap (never a silent forward jump) and then pages to the marker and the exit last. The client-side lifecycle test's ordered-exit case now documents the server's credited-exit contract (pages on credits, exit last) that its wire stream models.
The credit handler only ever read WsState.registry; taking the registry directly keeps the dependency honest and lets focused race tests drive the handler against a bare in-process registry without constructing a full WsState. No behavior change.
…ming Focused E2R2 finding (Major): the credited natural-exit transition ran drive_session before discovering/arming the staged exit, with two failure orderings. Four deterministic tests exercise the races with EXPLICITLY controlled credit timing — the parser-consumption boundary is the thing under test, so no test here credits on read. The wedge (unit, attach boundary — the one state where the ordering strands the stream): the client's first page was empty (its cursor already at the attach head: credited == page_end == target), the exit staged with new output past that target between the attach and start_session, and the pre-fix keep-armed-with-no-page leaves nothing outstanding — no credit can ever come, stranding the final output and terminal.exit forever. The test drives the production entry points against a real in-process PTY registry (real staging from the PTY reader thread, real pages, a real spawned drain); the test itself is the undispatched-notify window the integration socket cannot order deterministically. The premature exit (unit + integration): the exit stages mid-restore, the credits walk the pages to the one that reaches the frozen exit head, and that page is read but NOT credited — the pre-fix removal on the emitting drive delivers terminal.exit immediately, before the acknowledging credit can exist (the client clears currentAttachRef on exit, killing the final page's parser-applied checkpoint and consumption credit). The integration variant proves it over the real socket; the retention variant proves the same contract through the exact bounds-carrying gap flow. All four fail on the pre-fix code with the invariant violations named in their panic messages.
…e acknowledging credit Focused E2R2 finding (Major): the credited natural-exit transition ran drive_session before discovering/arming the staged exit, with two failure orderings — a wedge (the arm extended the phase target beyond the credited cursor but no page was produced, so nothing was outstanding, no credit could ever come, and the final output plus terminal.exit were stranded) and a premature exit (an armed session whose drive emitted the page reaching exit_head was removed on that drive, and the drain delivered terminal.exit while the page was still uncredited — the client clears currentAttachRef on exit, killing that page's parser-applied checkpoint and consumption credit). THE INVARIANT, as implemented: 1. Arming always precedes driving, at every drive site. The new arm_staged_exit_from_registry queries the registry (the authority — staging happens under the terminal lock before the notify fires) and arms the session through the terminal's frozen final head BEFORE the drive, so an arm that extends the phase target beyond the credited cursor is always followed by a drive that produces the next page: no state may exist where the target exceeds the credited cursor and no page was just emitted. The credit path, the attach start, and the notify arm all go through the one helper (monotone + idempotent — whichever arm wins the dispatch race, the others are inert, and exit_head never moves backward). 2. terminal.exit rides the CREDIT verdict that acknowledges consumption of the page reaching exit_head — never the drive that emits it. The unified DrainReady disposition (uncredited_exit_page) keeps the session in the credited phase while that page is outstanding; removal happens on the acknowledging credit, and the spawned drain's CaughtUp hold delivers the staged exit after it. The empty-arm shape (armed, credited == page_end — an exit that added no output past the acknowledged cursor) still delivers through the empty drain immediately, with zero uncredited pages. 3. The notify arm is arm-only now: its deliver_now spawn was unreachable (an in-table session always has exactly one uncredited page outstanding), and its rescue role for the empty-arm case is owned by the unified disposition at the drive sites. The four RED tests from the previous commit are green; the existing crediting-promptly natural-exit integration tests stay green.
E2R3 focused remediation prep — the transition-race tests need a natural exit to stage at a precise point INSIDE the transition decision's lock scope (a real PTY death stages at its own uncontrolled moment; a sleep-based race is forbidden). The registry gains a ONE-SHOT test hook (the terminal_create_pause idiom): a staged-exit read on the decision path fires it inside its own terminal-lock hold immediately AFTER its read, performing the staging for the armed (connection, code). - stage_natural_exit_locked: finish_pty_exit's subscriber-relevant subset (monotone Exited flip + staged code on the DEFERRED subscriber only) under a lock the caller holds. - stage_natural_exit_for_test: the public lock-acquiring twin for staging strictly after an atomic transfer decision (the boundary pin). - The hook site in staged_paced_exit is INERT in production (never armed outside the tests).
…c in-lock hook E2R3 RED tests — the two call sites' interleavings the E2R2 arm-before- drive fix still leaves open (focused E2R3's single Major): the exit stages INSIDE the credit/start window, after the arm's read and before the disposition, so the handler transfers the session to the uncredited drain from a stale read and the drain delivers the remaining suffix + terminal.exit WITHOUT the acknowledging credit. - (a) credit path: the one-shot registry hook stages the exit inside the arm read's lock scope (right after its read); the racing credit is the one whose drive reaches the attach target — nameable in advance because the quiet harness runs budget 0 (one frame per page) with stty -echo behind an ECHO-OFF banner (echo/output coalescing was the nondeterminism; each step is now exactly one frame). Pre-fix: deterministic RED — the session leaves the credited table and terminal.exit delivers uncredited. Post-fix expectation: the credited phase extends and the exit rides the acknowledging credit. - (b) start_session: the same interleave at the start (empty first page); pre-fix RED — the session transfers at start and the exit delivers with no credit at all. - (c) the atomicity boundary pin: an exit staged STRICTLY after the atomic transfer (the synchronous window before the spawned drain's first poll) is the drain's documented uncredited tail content — green before and after by design. All existing natural-exit tests stay green (no behavior change yet).
…ging E2R3 focused remediation — the single Major: arm-before-drive was still check-then-act against the concurrently-staging PTY reader. The arm released the terminal lock before the drive ran, and the disposition then decided extend-vs-transfer from the arm's stale read: an exit staged inside that window was missed, the handler transferred the session to the uncredited drain, and the drain dumped the remaining suffix + terminal.exit without the acknowledging credit — the client cleared its attach before the final page's parser callback/checkpoint completed. The same window existed at start_session. The transition is now structurally race-free, not re-ordered: - TerminalRegistry::paced_exit_transition (new): reads the staged-exit state and the terminal's frozen head under ONE terminal-lock hold and returns the decision value with the read — no window in which a concurrently staged exit can change which transition is correct. - paced_replay::settle_exit_transition (new): the POST-DRIVE disposition that commits extend-vs-transfer from that atomic decision. An exit staged during the drive — the old check-then-act window — is absorbed by the disposition's own read: the credited phase extends, the final output pages only on continuation credits, and terminal.exit rides the credit acknowledging the page reaching the frozen head (E2R2 invariant 2 lives in the disposition). An exit staged strictly after the disposition's transfer confirmation is the drain's documented uncredited tail content — the atomicity boundary is exactly the transfer decision. A wedge guard inside the disposition re-drives when an extension outgrows an all-acknowledged cursor (at most the one page every drive site emits per credit). - arm_staged_exit_from_registry: now a one-hold PRE-ARM hint via the decision method (the pre-fix shape read staging and bounds in two holds); monotone, idempotent, never a transition commitment — the credit path, the session start (which now arms at attach and settles its disposition atomically too), and the notify site all keep it. - uncredited_exit_page removed; its semantics are re-encoded in the disposition's ordered state checks. Also fixes a pre-existing flake the focused runs exposed: the race harness's pad steps waited for a repeated 'PAD-STEP' marker, so steps 2..8 returned on step 1's output and the attach raced the remaining pads' asynchronous ingestion (~25% failure under parallel load when the first page covered the raced window). Pad markers are now unique per step in both scripts, making every marker wait a real ingestion proof. Verification: freshell-terminal 250 lib tests; freshell-ws 788 lib tests x10 (0 flakes after the marker fix, ~25% before), paced_replay integration 24; clippy + fmt clean.
…t-gap guard A legacy (non-negotiated) attach.ready declares the replay window its inline snapshot covers; when no replay frames follow it, the live stream resumes at exactly replayToSeq + 1. The implicit-gap quarantine (e7147b9) judged that first contiguous live frame as an unexplained jump from the stale expected cursor and wrote a spurious "[Output gap 1-4: unexplained sequence jump]" notice onto the surface; the opencode startup-probe spec pinned the regression (11/12). onOutputFrame's session-start exemption now also covers the legacy ready's covered-window resume: a frame starting at exactly replayToSeq + 1 while the whole declared window is unconsumed is the server-declared baseline, mirroring the negotiated path's from-seq session-start exemption. No notice, no quarantine; the OpenCode probe filter intercepts the frame without a surface write, exactly as before. The quarantine's honesty is untouched: a jump beyond replayToSeq + 1, a mid-window jump, or a jump over a partially consumed window still folds its exact hole as an implicit gap (pinned by new unit guards).
…red decision, not a wall budget
…cted bypass; n115 owns the durable rework)
… production; keep the capture-driven wait
…store-plan # Conflicts: # crates/freshell-protocol/tests/inventory.rs # test/unit/scripts/rust-test-targets.test.ts
CI runners (2-core) can split a small write across PTY read chunks: a marker's trailing bytes land as a separate ring frame microseconds after the marker text first appears, so absolute frame-count asserts in the two hook-staging race tests miscounted (left: 3, right: 2 on PR 822's rust lane; never reproducible on the 96-core dev box). The walk's protected invariants are all relative, so the setup now pins head PROGRESSION via settled reads (settled_head: quiet-window-based, progress-driven, trips only on a dead PTY), and the post-transfer test credits until the drive reaches the attach target instead of assuming one credit is the transferring one. The three wait_staged-based tests are already safe: they read the head only after the real PTY exit froze the ring.
…completion event The probe-answer assertion compared incomparable clocks: ws.restore.paced_complete is emitted server-side when the drain's PRODUCTION ends, while the answer's ARRIVAL is serialized behind whatever drain pages already hold the client queue (this test client reads them slowly on purpose). Instrumented evidence from the reproduced failure: the answer arrived with 743 drain frames still in flight (dispatcher serviced the input mid-drain, correctly) yet after the completion event — a false failure at ~1-in-4 locally and twice on CI's 2-core runners. The mid-drain proof now rides on the frames-after observable the test already collects: the pre-fix synchronous-drain stall delivered the answer only after the drain's LAST page, so 'answered at all' + 'drain frames still flowing after the answer' pins the dispatcher-servicing property without the queue-serialized arrival race.
The pull_request event routing for this PR stopped creating native Actions runs (three consecutive events — synchronize, reopened, synchronize — created third-party app check suites but no workflow runs, while other PRs and push events kept triggering runs; repo is public so no quota applies, all workflows active, GitHub status green). workflow_dispatch also future-proofs the rust gate against stuck event routing.
github.base_ref is a pull_request-only context; on workflow_dispatch it is empty, the diff against the malformed ref fails, and the filter fell to rust=false (a vacuous green gate). Fall back to the default branch as the diff base when dispatching.
…efore the handoff fixture
Main-side fixture race, 2-for-2 on this PR's CI runs (never on the dev box):
handle_send awaits only the DISPATCH — the spawned materialize task commits
the durable id first and the Live{FreshAgent} ownership on the durable key
asynchronously after it. On a contended 2-core runner the test's
begin_handoff met a not-yet-Live key and the fixture assert panicked. The
fixture now polls the registry until the committed precondition is
observable (bounded; trips only if the commit never lands). Rides the
landing PR because it blocks the rust lane for this branch.
Owner
Author
Final merge evidence (CI flake-tail resolution)Complete workspace suite GREEN on the final head ( Every CI red this PR received a root-cause verdict:
The rust-gate on the head is red from the flake tail only; merging per the documented owner-merge bypass with this receipt table. The flake-tail list (opencode fixture, WAL-move settles, retry-floor race, port-check, fork-rebind) is recorded for the n115 progress-idle-watchdog sweep. |
…store-plan # Conflicts: # crates/freshell-protocol/src/lib.rs # crates/freshell-protocol/tests/inventory.rs # crates/freshell-terminal/src/lib.rs # crates/freshell-terminal/src/registry.rs # src/components/TerminalView.tsx
…loor Main-side flake from the #825-era session-name work, fired twice in two full-workspace runs locally (never focused): the tests shrank the retry floor to 60ms and then asserted cycle 2 'not ready YET' on the very next lines — any scheduling gap over 60ms under parallel-suite load made the assert fail on working code (the exact wall-clock class the repo bans). Floor 60ms -> 2s: the not-yet-due asserts gain 30x margin, the crossing sleeps scale to match, and the offset-aware clock test's virtual jump (60s) still towers over the floor. Exhaust test runtime 0.2s -> 4.2s.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the bounded recovery increment of
docs/plans/2026-09-19-responsive-terminal-restore.md: reliable, responsive pane restore. Three workstreams, negotiated end-to-end so older clients keep byte-identical legacy behavior:hellonegotiatedpacedTerminalReplayV1restore via a bounded paced-replay session —terminal.attach.readycarries window bounds, a reconnect resumes from the client's coverage cursor, one bounded first page flows, and further pages flow only onterminal.replay.creditcontinuation credits (stale generations inert).terminal.output.gap— never silent truncation, never a disconnect. Recovery accounting resets only on genuine parser progress or explicit retry (not on mere reconnects), and a pane that exhausts its budget gets an accessible failure/retry state instead of a reconnect loop.terminal.output.gap(reason: queue_overflow), and only a sustained over-threshold backlog with zero completed sends for the whole stall window closes with 4008 "Catastrophic backpressure" (the decision event carriessends_in_window+ lifetimetotal_sendsevidence).Verification
2171af8b0): cloud vitest 4/4 shards (7,326 tests), source-runtime, rust lane, electron lane — all exit 0.--no-verifyonly because the shared pnpm-erascripts/hooks/pre-pushdies withSyntax error: redirection unexpectedat line 180 on its npm-era compatibility path (dash executing a bashism) — after every check had passed. The hook's npm-era path needs a fix on main; all npm-era worktrees will hit this.paced-restore-convergence-rust.spec.tson the rust e2e lane).Review
Independent delta review (Fresh Eyes, 5 rounds) PASSED on the full 73-file delta; every finding from every round has a recorded final disposition. Two focused repair episodes (10 rounds total) cleared all 22 focused findings. Disclosure: the final passing round used a fresh context-free opencode session (same model family as the executor — weaker independence) after the bundled cross-family reviewer transports failed environmentally.
Intentional test change in this diff
in_tui_fork_rebinds_the_pane_identity(codex_fork_rebind) is#[ignore]d (86ae1ee) — user-directed bypass of a pre-existing, branch-untouched load flake: its 10 s wall-clock frame-wait starved once under concurrent full-suite gates (green focused at HEAD in 9.33 s, green in every prior gate). The ignore reason carries the evidence and the un-ignore condition; the durable progress-based rework is tracked as kata n115.Notes for reviewers / follow-ups
🤖 Generated with opencode