diff --git a/docs/ENGINE.md b/docs/ENGINE.md index 7a39614a..f4423d68 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -623,8 +623,9 @@ token can list sessions and read a scrollback snapshot over The attach sequence is ordered: -1. client sends auth control frame, optionally with its last applied cursor: - `{"type":"auth","token":"...","resume_from":123}` +1. client sends auth control frame, optionally with its last applied cursor + and always with its replay budget: + `{"type":"auth","token":"...","resume_from":123,"snapshot_tail_bytes":1048576}` 2. server sends `snapshot-start` 3. server sends zero or more binary scrollback chunks 4. server sends `snapshot-done` @@ -645,9 +646,25 @@ Server text control frames: {"type":"lag","note":"client too slow; reattach"} ``` -When `resume_from` is still retained, `snapshot-start.reset` is false and the -binary snapshot contains only newer bytes. Otherwise the server returns a full -snapshot with `reset` true. +`snapshot_tail_bytes` is the replay **budget**, and it bounds every reply — a +client's terminal keeps only a fixed scrollback, so the server never ships more +than the client can hold: + +- **Cold attach** (no `resume_from`): a ground-state-aligned tail of at most the + budget, `reset` true. +- **Warm reattach whose cursor is retained and whose delta fits the budget:** + `reset` false and the binary snapshot contains only the newer bytes, + byte-for-byte — an ordinary switch-away/switch-back appends without a clear. +- **Warm reattach whose cursor aged out of the ring, or whose delta exceeds the + budget:** a ground-state-aligned tail of at most the budget, `reset` true. + `reset: true` may therefore follow a `resume_from`. The client discards its + stale cursor and re-anchors to `scrollback_pos - scrollback_bytes` (the start + of the tail), so after replaying the tail its position is `scrollback_pos` + again and live traffic resumes with no gap. + +The returned `scrollback_pos` is always the absolute end position, unaffected by +trimming the front. Omitting `snapshot_tail_bytes` (the in-band lag resync, +which carries its own cursor) leaves a retained delta unbounded and byte-exact. A text frame that does not parse as a control message is treated as raw input, because some tools send keystrokes as text. Snapshot chunks are capped at diff --git a/engine/server/src/pty.rs b/engine/server/src/pty.rs index 8d28cbcf..e0c0133a 100644 --- a/engine/server/src/pty.rs +++ b/engine/server/src/pty.rs @@ -130,13 +130,25 @@ impl Session { /// Snapshot only output newer than a client cursor. The boolean tells the /// client whether its existing terminal state must be reset. /// - /// `tail_bytes` caps a **cold** attach only: a fresh client with no - /// cache sends no `resume_from`, and without a cap the full snapshot is the - /// entire scrollback ring. When present it bounds that full snapshot to at - /// most that many trailing bytes. A warm reattach (`resume_from` present) - /// ignores it entirely, so its `reset: false` delta stays byte-for-byte - /// unchanged; the returned position (`total_written`) is unaffected by - /// trimming the front, so the live stream still resumes with no gap. + /// `tail_bytes` is the replay **budget**, and it now bounds *every* path, + /// not just a cold attach (F1, WI-125). A client's xterm keeps only a fixed + /// scrollback (5000 lines); shipping more than the budget is wasted parse + /// on the client's single message thread, and on a stale reattach it is + /// parsed *above* a screen the client already has out of date. So: + /// + /// - Cold attach (no `resume_from`): bound the full snapshot to the budget. + /// - Warm reattach whose cursor is still retained and whose delta fits the + /// budget: the delta is sent byte-for-byte with `reset: false`, so a + /// switch-away/switch-back that produced little still appends cleanly. + /// - Warm reattach whose cursor aged out of the ring, **or** whose delta + /// exceeds the budget: a ground-state-aligned tail of at most the budget, + /// with `reset: true`. Never the whole untrimmed ring — that was the + /// 4 MiB flood, 4x worse than a cold attach. + /// + /// The returned position is always `total_written`, unaffected by trimming + /// the front, so the live stream resumes with no gap. When `tail_bytes` is + /// `None` (the in-band resync path, which carries its own cursor) nothing + /// is bounded and a retained delta stays byte-exact. pub fn snapshot_for_attach( &self, resume_from: Option, @@ -146,12 +158,25 @@ impl Session { let pos = sb.total_written(); if let Some(cursor) = resume_from { if let Some(delta) = sb.snapshot_since(cursor) { - return (delta, pos, false); + // A retained delta within budget is byte-exact and appends + // (reset:false). An oversized delta is capped to a bounded tail + // and reset:true — the client's xterm cannot keep more than its + // scrollback anyway, so an over-budget delta would only waste + // parse above an already-stale screen. + match tail_bytes { + Some(limit) if delta.len() > limit => { + return (sb.snapshot_tail(limit), pos, true); + } + _ => return (delta, pos, false), + } } - // Cursor aged out of the ring: a full reset snapshot, untrimmed — - // the tail cap is a cold-attach affordance and a warm reattach is - // never quietly narrowed. - return (sb.snapshot(), pos, true); + // Cursor aged out of the ring: a bounded, ground-state-aligned tail + // and a reset — never the whole untrimmed ring. + let snapshot = match tail_bytes { + Some(limit) => sb.snapshot_tail(limit), + None => sb.snapshot(), + }; + return (snapshot, pos, true); } // Cold attach: bound the snapshot to the client's tail hint so first // open never ships the whole ring buffer. diff --git a/engine/server/tests/integration.rs b/engine/server/tests/integration.rs index 9b0dfb15..cc569707 100644 --- a/engine/server/tests/integration.rs +++ b/engine/server/tests/integration.rs @@ -3306,10 +3306,12 @@ async fn cold_attach_tail_hint_caps_the_snapshot() { } #[tokio::test] -async fn warm_attach_is_not_narrowed_by_a_tail_hint() { - // The tail cap is a cold-attach affordance. A warm reattach carries - // `resume_from`; even if a tail hint is also present it must be ignored and - // the resume delta returned in full (`reset: false`). +async fn warm_attach_over_budget_is_a_bounded_reset() { + // F1 (WI-125) inverts the old `warm_attach_is_not_narrowed_by_a_tail_hint`. + // The tail hint is now the replay budget on *every* path. A warm reattach + // whose delta exceeds the budget no longer floods the whole ring: it is + // capped to a ground-state tail and reset:true, because the client's xterm + // cannot keep more than its scrollback anyway. let (base, _h) = boot().await; let client = reqwest::Client::builder() .default_headers(auth()) @@ -3332,8 +3334,8 @@ async fn warm_attach_is_not_narrowed_by_a_tail_hint() { const TAIL: usize = 512; fill_scrollback(&base, &id, 4 * TAIL).await; - // resume_from = 0 resolves to the whole retained ring as a delta. With a - // tail hint also set, the warm path must still return it untrimmed. + // resume_from = 0 resolves to the whole retained ring as a delta — far more + // than the 512-byte budget — so F1 caps it to a bounded reset. let mut warm = ws_attach_with_auth( &base, &id, @@ -3345,11 +3347,96 @@ async fn warm_attach_is_not_narrowed_by_a_tail_hint() { }), ) .await; - let (reset, len, _head) = read_snapshot(&mut warm).await; - assert!(!reset, "a warm resume from 0 is a delta, not a reset"); + let (reset, len, head) = read_snapshot(&mut warm).await; + assert!( + reset, + "an over-budget delta is a bounded reset, not an append" + ); + assert!( + len > 0 && len <= TAIL, + "the over-budget delta must be capped to the tail budget ({TAIL}); got {len}" + ); + assert_ground_state_aligned(&head); + warm.close(None).await.ok(); + kill_session(&client, &base, &id).await; +} + +#[tokio::test] +async fn warm_attach_within_budget_stays_a_byte_exact_delta() { + // The companion to the bounded-reset case: a warm reattach whose delta fits + // the budget is still sent byte-for-byte with reset:false, so an ordinary + // switch-away/switch-back appends without clearing the terminal. + let (base, _h) = boot().await; + let client = reqwest::Client::builder() + .default_headers(auth()) + .build() + .unwrap(); + + let id: String = client + .post(format!("{base}/api/sessions")) + .json(&json!({ + "name": "warm-small", + "command": ["/bin/sh", "-c", "stty -echo; exec /bin/cat"] + })) + .send() + .await + .unwrap() + .json::() + .await + .unwrap()["id"] + .as_str() + .unwrap() + .to_string(); + + // Prime the ring, record the cursor, then produce a small delta. + let mut ws = ws_attach(&base, &id).await; + read_snapshot(&mut ws).await; + ws.send(Message::Binary(b"before-cursor\n".to_vec().into())) + .await + .unwrap(); + let _ = collect_binary_until(&mut ws, b"before-cursor", Duration::from_secs(2)).await; + let cursor = { + let detail: Value = client + .get(format!("{base}/api/sessions/{id}")) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + detail["scrollback_pos"].as_u64().unwrap() + }; + ws.send(Message::Binary(b"after-cursor\n".to_vec().into())) + .await + .unwrap(); + let _ = collect_binary_until(&mut ws, b"after-cursor", Duration::from_secs(2)).await; + ws.close(None).await.ok(); + + // A generous budget the tiny delta fits inside: the resume stays a delta. + let mut warm = ws_attach_with_auth( + &base, + &id, + json!({ + "type": "auth", + "token": TEST_TOKEN, + "resume_from": cursor, + "snapshot_tail_bytes": 1024 * 1024, + }), + ) + .await; + let (reset, delta) = read_snapshot_payload(&mut warm).await; + assert!(!reset, "a delta within budget must not reset the terminal"); + assert!( + delta + .windows(b"after-cursor".len()) + .any(|w| w == b"after-cursor"), + "the delta must carry the post-cursor output" + ); assert!( - len > TAIL, - "warm reattach must ignore the tail hint; got {len} <= {TAIL}" + !delta + .windows(b"before-cursor".len()) + .any(|w| w == b"before-cursor"), + "the delta must exclude pre-cursor output (byte-exact from the cursor)" ); warm.close(None).await.ok(); kill_session(&client, &base, &id).await; @@ -3588,17 +3675,15 @@ fn assert_seq_spine_contiguous(bytes: &[u8]) { } #[tokio::test] -async fn large_stale_warm_reattach_today_floods_the_whole_ring() { - // H1(a). Ring 64 KiB. A load session pushes far past capacity, so a cursor - // recorded early has aged out of the ring by the time we reattach with it. - // Today `snapshot_for_attach` answers that stale warm reattach with the - // FULL untrimmed ring and reset:true — the flood that is 4x worse than a - // cold attach. This test pins that behaviour. - // - // WHEN F1 (WI-125) LANDS: invert the size assertion to `len <= TAIL_BUDGET` - // (the aged-out path returns a bounded tail, still reset:true). +async fn large_stale_warm_reattach_is_a_bounded_reset() { + // H1(a) / F1. Ring 64 KiB. A load session pushes far past capacity, so a + // cursor recorded early has aged out of the ring by the time we reattach + // with it. Before F1 `snapshot_for_attach` answered that stale warm + // reattach with the FULL untrimmed ring (the flood, 4x worse than a cold + // attach). F1 (WI-125) bounds it: a ground-state-aligned tail of at most + // the budget, still reset:true. const CAP: usize = 64 * 1024; - const TAIL_BUDGET: usize = 8 * 1024; // the future F1 budget, well under CAP + const TAIL_BUDGET: usize = 8 * 1024; // the F1 budget, well under CAP let (base, _h) = boot_with(CAP).await; let client = reqwest::Client::builder() .default_headers(auth()) @@ -3625,15 +3710,9 @@ async fn large_stale_warm_reattach_today_floods_the_whole_ring() { .await; let (reset, len, head) = read_snapshot(&mut warm).await; assert!(reset, "a cursor aged out of the ring forces a full reset"); - // BEFORE F1: the whole ring is replayed, ignoring the tail hint. assert!( - len > TAIL_BUDGET, - "today a stale warm reattach floods the whole ring; got {len} <= budget \ - {TAIL_BUDGET}. When F1 lands, invert this to `len <= TAIL_BUDGET`." - ); - assert!( - len <= CAP + CAP / 16, - "the flood is still bounded by the ring capacity; got {len}" + len > 0 && len <= TAIL_BUDGET, + "F1 bounds the aged-out reattach to the tail budget ({TAIL_BUDGET}); got {len}" ); assert_ground_state_aligned(&head); warm.close(None).await.ok(); @@ -3642,10 +3721,9 @@ async fn large_stale_warm_reattach_today_floods_the_whole_ring() { #[tokio::test] async fn four_mib_ring_stale_reattach_is_ground_state_aligned_and_bounded() { - // H1(b). The dimensions prod runs: a 4 MiB ring with ~6.5 MiB pushed. The - // stale warm reattach today returns the full ring (reset:true), aligned to - // the terminal's ground state. F1 flips the size to the tail budget; the - // alignment assertion holds either way. + // H1(b) / F1. The dimensions prod runs: a 4 MiB ring with ~6.5 MiB pushed. + // The stale warm reattach is bounded to the tail budget and reset:true, + // aligned to the terminal's ground state — not the whole 4 MiB ring. const CAP: usize = 4 * 1024 * 1024; const TAIL_BUDGET: usize = 1024 * 1024; let (base, _h) = boot_with(CAP).await; @@ -3672,15 +3750,9 @@ async fn four_mib_ring_stale_reattach_is_ground_state_aligned_and_bounded() { .await; let (reset, len, head) = read_snapshot(&mut warm).await; assert!(reset, "position 1 has long since aged out of a 4 MiB ring"); - // BEFORE F1: the 4 MiB ring is replayed whole. Invert to `<= TAIL_BUDGET`. - assert!( - len > TAIL_BUDGET, - "today the 4 MiB ring is replayed whole; got {len}. Invert to \ - `len <= TAIL_BUDGET` when F1 lands." - ); assert!( - len <= CAP + CAP / 16, - "bounded by the ring capacity; got {len}" + len > 0 && len <= TAIL_BUDGET, + "F1 bounds the aged-out 4 MiB reattach to the tail budget ({TAIL_BUDGET}); got {len}" ); assert_ground_state_aligned(&head); warm.close(None).await.ok(); diff --git a/web/src/Terminal.tsx b/web/src/Terminal.tsx index 533b3e71..dd6b1c14 100644 --- a/web/src/Terminal.tsx +++ b/web/src/Terminal.tsx @@ -25,6 +25,7 @@ import { createReplayQueue, prepareReplayTail, scheduleReplay, + snapshotStartPosition, type ReplayHandle, } from "./terminalReplay"; import { beginForegroundReplay } from "./terminalPrewarm"; @@ -965,9 +966,13 @@ const TerminalView: Component = (props) => { } if (typeof ctrl.scrollback_pos === "number") { snapshotEndPosition = ctrl.scrollback_pos; - outputPosition = Math.max( - 0, - ctrl.scrollback_pos - (ctrl.scrollback_bytes ?? 0), + // Re-anchor to the start of the payload we are about to replay. + // On a reset:true that followed a resume_from (F1's aged-out or + // over-budget path) this discards the now-stale cursor; the live + // stream after snapshot-done resumes at scrollback_pos with no gap. + outputPosition = snapshotStartPosition( + ctrl.scrollback_pos, + ctrl.scrollback_bytes ?? 0, ); } inSnapshot = true; diff --git a/web/src/__tests__/attachAuthFrame.test.ts b/web/src/__tests__/attachAuthFrame.test.ts index 211a2fc9..32a72ef9 100644 --- a/web/src/__tests__/attachAuthFrame.test.ts +++ b/web/src/__tests__/attachAuthFrame.test.ts @@ -71,7 +71,7 @@ describe("attach auth frame", () => { expect(frame.resume_from).toBeUndefined(); }); - it("omits the tail hint and sends the cursor on a warm reattach", () => { + it("carries both the cursor and the tail budget on a warm reattach (F1)", () => { installRuntimeTransport(networkless); setBase("http://engine.test"); setToken("test-token-1234567890abcdef"); @@ -79,7 +79,9 @@ describe("attach auth frame", () => { const frame = firstSent(); expect(frame.type).toBe("auth"); expect(frame.resume_from).toBe(4096); - // The tail cap is a cold-attach affordance; a warm reattach must not narrow. - expect(frame.snapshot_tail_bytes).toBeUndefined(); + // F1: the tail budget is sent on every attach. A retained delta within + // budget is still returned byte-exact (reset:false); an aged-out or + // over-budget delta is capped to a bounded reset instead of the whole ring. + expect(frame.snapshot_tail_bytes).toBe(REPLAY_TAIL_MAX_BYTES); }); }); diff --git a/web/src/__tests__/snapshotStartPosition.test.ts b/web/src/__tests__/snapshotStartPosition.test.ts new file mode 100644 index 00000000..fbafa53d --- /dev/null +++ b/web/src/__tests__/snapshotStartPosition.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; + +import { snapshotStartPosition } from "../terminalReplay"; + +describe("snapshotStartPosition", () => { + it("is the payload start: end position minus payload length", () => { + // A cold or warm snapshot ending at absolute position 1000 that carries + // 400 bytes started at 600. + expect(snapshotStartPosition(1000, 400)).toBe(600); + }); + + it("clamps at zero when the payload spans the whole stream so far", () => { + // The whole ring is the payload (nothing has aged out): the start is 0, not + // negative. + expect(snapshotStartPosition(400, 400)).toBe(0); + expect(snapshotStartPosition(400, 1000)).toBe(0); + }); + + it("re-anchors a reset that followed a stale resume cursor (F1)", () => { + // The client had rendered up to 2_000_000 and reattached with that cursor. + // The cursor aged out of a 4 MiB ring, so the server sent a bounded tail: + // a 1 MiB payload ending at 5_000_000, reset:true. The client must discard + // its stale cursor and re-anchor to the start of the tail it will replay. + const scrollbackPos = 5_000_000; + const scrollbackBytes = 1_048_576; + const start = snapshotStartPosition(scrollbackPos, scrollbackBytes); + expect(start).toBe(scrollbackPos - scrollbackBytes); + expect(start).toBeGreaterThan(2_000_000); // well past the stale cursor + + // After replaying exactly the payload's bytes, the client's position is the + // snapshot end again, so the live stream resumes with no gap or duplicate. + expect(start + scrollbackBytes).toBe(scrollbackPos); + }); + + it("treats a zero-length snapshot as a no-op at the end position", () => { + expect(snapshotStartPosition(1234, 0)).toBe(1234); + }); +}); diff --git a/web/src/api.ts b/web/src/api.ts index 06aebe9e..c0a257db 100644 --- a/web/src/api.ts +++ b/web/src/api.ts @@ -1429,13 +1429,15 @@ export function openAttach(id: string, resumeFrom?: number): RuntimeSocket { ws.send(JSON.stringify({ type: "auth", token: tok, - // Cold attach (no cached cursor): bound the server's full snapshot to - // the same tail budget the cache-restore path already enforces, so a - // first open never ships and replays the whole scrollback ring. - // A warm reattach sends its cursor instead and its delta is untouched. - ...(resumeFrom === undefined - ? { snapshot_tail_bytes: REPLAY_TAIL_MAX_BYTES } - : { resume_from: resumeFrom }), + // The tail budget is sent on EVERY attach, warm or cold (F1). It bounds + // the server's reply on all three paths: a cold snapshot, a warm delta + // whose cursor aged out of the ring, and a warm delta that is simply + // larger than the budget — all capped to a ground-state tail the same + // size the cache-restore path enforces, so no attach ever ships and + // replays the whole scrollback ring. A warm delta that fits the budget + // is still returned byte-exact (reset:false). + snapshot_tail_bytes: REPLAY_TAIL_MAX_BYTES, + ...(resumeFrom === undefined ? {} : { resume_from: resumeFrom }), })); }, { once: true }, diff --git a/web/src/terminalReplay.ts b/web/src/terminalReplay.ts index 388dd01f..31552963 100644 --- a/web/src/terminalReplay.ts +++ b/web/src/terminalReplay.ts @@ -11,6 +11,29 @@ export const REPLAY_TAIL_MAX_BYTES = 1 * 1024 * 1024; /** Keep each xterm parser turn bounded and aligned with the server frame size. */ export const REPLAY_SLICE_BYTES = 64 * 1024; +/** + * The absolute output position at the START of a snapshot payload: the byte + * offset the first snapshot byte sits at. The server reports the position at + * the END of the snapshot (`scrollback_pos`) and the payload's byte length + * (`scrollback_bytes`); the start is their difference, clamped at zero. + * + * This is the cursor a client adopts when a `snapshot-start` arrives, and it is + * what makes F1's bounded reset safe. When a `reset:true` snapshot follows a + * `resume_from` — the cursor aged out of the ring, or the delta exceeded the + * budget — the server sends a ground-state tail whose start is + * `scrollback_pos - scrollback_bytes`, well after the client's now-stale + * cursor. The client discards that stale cursor and re-anchors here, so once it + * has replayed the `scrollback_bytes` of the tail its position is exactly + * `scrollback_pos` again and the live stream resumes with no gap and no + * duplicate. + */ +export function snapshotStartPosition( + scrollbackPos: number, + scrollbackBytes: number, +): number { + return Math.max(0, scrollbackPos - scrollbackBytes); +} + export interface ReplayTail { /** The ground-state-safe tail to send to xterm. */ data: Uint8Array;