From e81b63106b83bce66641e8dd27d6aa348252baf1 Mon Sep 17 00:00:00 2001 From: Gabriel Schneider Date: Thu, 3 Sep 2026 13:11:14 -0300 Subject: detached: a big screen can attach, and the frame it asks for fits the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects, and either fix alone makes the other one worse. No frontend ever clamped its window to the protocol's grid ceiling — the hello carried it raw — so a 4K display at a small font, already past max_rows 128, had its geometry refused by the session's decoder as BadValue. That path answers with close(.protocol) and no refuse behind it, so the frontend was told only that the session "hung up on the connect": at a session with all 32 slots free. client.zig now asks for the largest grid the wire carries, which is what its own GEOMETRY note already promises a frontend gets — the session is drawn at its own size in the corner of a bigger window, exactly as when another frontend is the smaller one. A ZERO geometry is dropped rather than clamped, because the session grid is the smallest common one and a frontend reporting 1 would collapse everybody else; TIOCGWINSZ answers 0x0 during a teardown and the tty shell forwarded it, which was the same mute hangup by another route. That clamp alone would have replaced one bug with a worse one. max_cols * max_rows is 65536 and a run's length prefix is a u16, so the single grid legal at both bounds is the one grid whose full frame — and an attach always produces a full frame — cannot be described by one run. encodeFrame's @intCast panicked in a safe build and was illegal behaviour in a fast one. The encoder splits the run instead, bounding the CURSOR rather than the run because the gap lookahead runs ahead of it, and frameBound had already paid for the extra header. wire.version 1 -> 2 for the same reason: the geometry a v2 frontend now asks for is one a v1 daemon panics encoding, and `zig build` replacing the binary under a running session is exactly what that field exists for. A v1 daemon answers Refusal.version instead of dying with every pane shell it owns. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016Q4RATpafkwahrovHQLKRf --- docs/detached.md | 17 ++++++--- src/CHANGELOG.md | 24 +++++++++++++ src/detached/client.zig | 85 ++++++++++++++++++++++++++++++++++++++----- src/detached/wire.zig | 95 ++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 203 insertions(+), 18 deletions(-) diff --git a/docs/detached.md b/docs/detached.md index 58a2f9c6..0902f78a 100644 --- a/docs/detached.md +++ b/docs/detached.md @@ -255,6 +255,13 @@ Stated rather than papered over: * **The screen is shared, at the smallest common grid.** Two frontends of different sizes converge on the smaller; the larger window letterboxes. Same semantics as tmux. +* **A frontend asks for at most `max_cols` x `max_rows`** (512x128, above). + A window bigger than that — a 4K display at a small font is already past 128 + rows — attaches at 512x128 and letterboxes the rest, exactly as it does + beside a smaller frontend. `client.zig` clamps the hello and every resize, + because the geometry itself does not fit the wire: an unclamped one was + refused by the session's decoder as `BadValue`, and that refusal reaches the + frontend as a bare hangup with no reason attached. * **No LSP and no selection pipe** in a detached session. The daemon implements **seventeen** of `Host.VTable`'s **twenty-one** methods — fewer than the tty and SDL shells, which install nineteen each, everything but @@ -299,8 +306,8 @@ Stated rather than papered over: ## Tests -`zig build unit-test` runs eleven tests in `src/detached/client.zig`. Ten drive a -real core over a real socket: a frontend is greeted +`zig build unit-test` runs twelve tests in `src/detached/client.zig`. Eleven +drive a real core over a real socket: a frontend is greeted and sent a screen; input comes back as a diff; two frontends share one screen at the smallest common grid; a frontend that dies takes nothing with it; a wrong-version peer is refused, loudly; a peer that sends an undefined tag byte @@ -309,9 +316,11 @@ the grid where the last one left it, and lets the next one take it over; the three surviving effects route as documented above — `set_clipboard` to both frontends, `read_clipboard` and `open_link` to the origin, and to the primary once the origin is gone; the client table refuses rather than queues; and a -frontend that stops reading is dropped. +frontend that stops reading is dropped; and a window bigger than the protocol's +grid attaches at `max_cols` x `max_rows` rather than being refused, which is +also the one test that drives a full frame of the largest grid the wire carries. -The eleventh builds no harness, opens no socket and touches no core, and that is +The twelfth builds no harness, opens no socket and touches no core, and that is the point: it pins the DESIGN rather than the behaviour, and `wire.zig` has its mirror, one test per direction. "A frontend is never asked to fork, write, or watch" walks diff --git a/src/CHANGELOG.md b/src/CHANGELOG.md index 4fef47fc..66c9fb0c 100644 --- a/src/CHANGELOG.md +++ b/src/CHANGELOG.md @@ -26,6 +26,30 @@ own in the process: the macOS build roots at `macos.zig`, so the one in `main.zig` had never run there — in the shell with the least useful stderr of the four. +- A big screen can attach to a detached session. The wire's grid ceiling is + 512x128 and no frontend ever clamped to it — the hello carried the window + raw — so a 4K display at a small font, which is already past 128 rows, had + its geometry refused by the session's decoder as `BadValue`. That path + answers with `close(.protocol)` and no `refuse` behind it, so the frontend + reported the one thing a bare hangup can mean: "that session hung up on the + connect", at a session with all 32 slots free. `client.zig` now asks for the + largest grid the protocol carries, which is what its own GEOMETRY note + already promises a frontend gets — the session is drawn at its own size in + the corner of a bigger window, exactly as when another frontend is the + smaller one. A zero geometry is dropped rather than clamped, because the + session grid is the smallest common one and a frontend reporting 1 would + collapse everybody else: `TIOCGWINSZ` answers 0x0 during a teardown and the + tty shell forwarded it, so that was the same mute hangup by another route. + The clamp alone would have replaced one bug with a worse one: `max_cols * + max_rows` is 65536 and a run's length prefix is a `u16`, so the single grid + legal at both bounds is the one grid whose full frame — and an attach always + produces a full frame — cannot be described by one run. It panicked on the + `@intCast` in a safe build and was illegal behaviour in a fast one. The + encoder splits the run instead, which `frameBound` had already paid for, and + the protocol version is bumped to 2: the geometry a v2 frontend now asks for + is one a v1 daemon panics encoding, so a mixed pair — `zig build` replacing + the binary under a running session — meets a `Refusal.version` instead of + losing every pane shell the daemon owns. - A filtered terminal costs what an unfiltered one does. `Filter`'s second stage asked `RGB.contrast` for every cell it painted, and that call ends in `std.math.pow` six times over — a libm round trip per cell, per frame, to diff --git a/src/detached/client.zig b/src/detached/client.zig index fc39164b..f317b2e2 100644 --- a/src/detached/client.zig +++ b/src/detached/client.zig @@ -156,7 +156,10 @@ pub const Client = struct { server.setNonblock(fd); var c: Client = .{ .gpa = gpa, .fd = fd }; errdefer c.deinit(); - try c.send(.{ .hello = .{ .cols = cols, .rows = rows } }); + try c.send(.{ .hello = .{ + .cols = @min(cols, wire.max_cols), + .rows = @min(rows, wire.max_rows), + } }); return c; } @@ -214,8 +217,33 @@ pub const Client = struct { /// Tell the session this frontend's window changed. Not a promise about the /// next frame: with other frontends attached the session grid is the /// smallest common one. + /// + /// Clamped, like the hello in `open`: a window past `max_cols`/`max_rows` + /// is a geometry the protocol cannot carry, and the decoder on the far end + /// answers one with `BadValue` — which `apply` turns into `close(.protocol)` + /// with no `refuse` behind it, so the frontend was told only that the + /// session "hung up on the connect". A 4K display at a small font is + /// already past `max_rows`, which made an ordinary big screen unable to + /// attach at all. Asking for the largest grid the wire carries is what this + /// file's GEOMETRY note already promises a frontend gets: the session is + /// drawn at ITS size wherever the window is bigger, exactly as it is when + /// another frontend is the smaller one. + /// + /// A ZERO is dropped rather than clamped, and that asymmetry is the whole + /// point of `getCols` refusing zero in the first place: the session grid is + /// the smallest common one, so a frontend that reported 1 would collapse + /// every other frontend to a single cell. `TIOCGWINSZ` answers 0x0 while a + /// terminal is being torn down and tty.zig forwards a `winsize` verbatim + /// (its ATTACH path already refuses a zero one, its resize path did not), + /// so this is reachable without a hostile peer — and it reached the session + /// as the same mute `close(.protocol)` the oversize geometry did. Keeping + /// the last real window is what a momentary zero means. pub fn resize(c: *Client, cols: u16, rows: u16) (Error || wire.Error)!void { - return c.send(.{ .event = .{ .resize = .{ .cols = cols, .rows = rows } } }); + if (cols == 0 or rows == 0) return; + return c.send(.{ .event = .{ .resize = .{ + .cols = @min(cols, wire.max_cols), + .rows = @min(rows, wire.max_rows), + } } }); } /// Wait up to `timeout_ms` for the session to say something, and push @@ -663,10 +691,20 @@ const Harness = struct { host.vtable.push_present.?(host.ctx, surface); } - /// Pump until this client has the message we are waiting for. Bounded, so a - /// broken transport fails a test rather than hanging the suite. + /// The round budget every `pumpUntil*` below shares. A round moves at most + /// one socket buffer, because nothing here is concurrent: the session + /// flushes until `EAGAIN`, and only then does the client read. That buffer + /// is 8 KiB on darwin (`net.local.stream.sendspace`) against linux's 208 + /// KiB, which is the same asymmetry the slow-frontend test at the bottom of + /// this file already had to say out loud — and a full frame of the largest + /// grid the protocol carries is near a megabyte, so 64 rounds is a linux- + /// only number. High enough for that frame on the smaller buffer, and still + /// a bound: a broken transport fails a test rather than hanging the suite. + const rounds = 256; + + /// Pump until this client has the message we are waiting for. fn pumpUntil(h: *Harness, c: *Client, comptime want: std.meta.Tag(wire.ServerMsg)) !wire.ServerMsg { - for (0..64) |_| { + for (0..rounds) |_| { try h.pump(); try c.wait(5); while (try c.next()) |msg| if (std.meta.activeTag(msg) == want) return msg; @@ -679,7 +717,7 @@ const Harness = struct { /// input queued (a `Look` on a directory emits a spawn) lands a frame /// later, and the frame in between legitimately says nothing. fn pumpUntilChange(h: *Harness, c: *Client) !wire.Frame { - for (0..64) |_| { + for (0..rounds) |_| { const msg = try h.pumpUntil(c, .frame); if (msg.frame.nruns > 0) return msg.frame; } @@ -691,7 +729,7 @@ const Harness = struct { /// because a resize is announced when the session settles it, which may be /// one empty frame after the pump that caused it. fn pumpUntilGrid(h: *Harness, c: *Client, cols: u16, rows: u16) !void { - for (0..64) |_| { + for (0..rounds) |_| { try h.pump(); try c.wait(5); while (try c.next()) |_| {} @@ -706,7 +744,7 @@ const Harness = struct { /// what a shared session promises is that they CONVERGE, which is what this /// waits for. fn pumpUntilSameScreen(h: *Harness, a: *Client, b: *Client) !void { - for (0..64) |_| { + for (0..rounds) |_| { try h.pump(); try a.wait(5); try b.wait(5); @@ -733,7 +771,7 @@ const Harness = struct { /// a transport that really does drop one must fail as a wrong screen, not /// as a timeout. fn pumpUntilShowsCore(h: *Harness, c: *Client) !void { - for (0..64) |_| { + for (0..rounds) |_| { _ = h.arena.reset(.retain_capacity); if (sameScreen((try h.core.render(h.arena.allocator())).cells, c.grid.items)) return; try h.pump(); @@ -835,6 +873,35 @@ test "detached session: two frontends share one screen at the smallest common gr try h.pumpUntilSameScreen(&a, &b); } +test "detached session: a window past the protocol attaches at the largest grid it carries" { + var h: Harness = undefined; + try h.init(80, 24); + defer h.deinit(); + + // A 4K display at a small font is already past `max_rows`, and until the + // clamp in `open` that hello was a geometry the session's decoder refused: + // `apply` answered `BadValue` with `close(.protocol)` and no `refuse` + // behind it, so a frontend on a big screen was told the session "hung up on + // the connect" — at a session with every slot free. It attaches now, at the + // biggest grid the wire has. + var c = try h.attach(wire.max_cols + 400, wire.max_rows + 70); + defer c.deinit(); + try testing.expectEqual(wire.max_cols, c.cols); + try testing.expectEqual(wire.max_rows, c.rows); + + // ...and the full frame that follows is the 65536-cell one whose single run + // does not fit a u16 count (wire.zig `run_max`), which is the half of this + // the clamp alone would have made universal rather than fixed. + const frame = (try h.pumpUntil(&c, .frame)).frame; + try testing.expectEqual(wire.FrameKind.full, frame.kind); + try testing.expectEqual(@as(usize, @as(usize, wire.max_cols) * wire.max_rows), c.grid.items.len); + // TWO runs and not one, which is the assertion that fails first if the + // encoder's bound goes: this grid's full frame is 65536 painted cells and + // a run counts to 65535. + try testing.expectEqual(@as(u32, 2), frame.nruns); + try h.pumpUntilShowsCore(&c); +} + test "detached session: a frontend that dies takes nothing with it" { var h: Harness = undefined; try h.init(60, 16); diff --git a/src/detached/wire.zig b/src/detached/wire.zig index 23a85158..8281fc63 100644 --- a/src/detached/wire.zig +++ b/src/detached/wire.zig @@ -84,7 +84,17 @@ const pardes = @import("../pardes.zig"); /// machine — `zig build` replaces the binary under a running session — and a /// frontend decoding another version's frame layout would paint garbage and /// blame the terminal. -pub const version: u16 = 1; +/// +/// 2: the layout did not move, but what a frontend is ALLOWED TO ASK FOR did. +/// A frontend now clamps its window to `max_cols` x `max_rows` instead of +/// sending it raw (client.zig), and a 512x128 grid is a full frame a v1 daemon +/// PANICS encoding — its run length overflowed a u16 by exactly one cell, see +/// `run_max`. A v1 session refused that geometry outright, so nothing was ever +/// lost by refusing the connection instead; a v1 daemon meeting a v2 frontend +/// answers `Refusal.version`, which says so, rather than dying with every pane +/// shell it owns. This is the case the paragraph above is about: `zig build` +/// replaces the binary under a running session. +pub const version: u16 = 2; pub const Error = error{ /// The message ended inside a field. @@ -111,9 +121,17 @@ pub const Error = error{ /// The largest grid this protocol carries. `Surface.cols`/`rows` are u16, so /// these are protocol bounds rather than type bounds, and they exist because /// `max_payload` below is derived from them: a decoder that accepts 65535 -/// columns accepts a 25 GiB frame prefix. A 4K display at a 6-pixel font is -/// about 340 columns and 110 rows, so this is roughly 1.5x the largest grid -/// any real terminal has, and the board's own is 56x14. +/// columns accepts a 25 GiB frame prefix. The board's own grid is 56x14 and a +/// terminal's is usually near 200x50. +/// +/// NOT a ceiling above every real display, which is what this comment used to +/// claim: a 4K window at the SDL shell's minimum 8-pixel font is around 768 +/// columns by 216 rows, and a tty on the same screen passes 128 rows at any +/// ordinary line height. Those windows attach at 512x128 and letterbox the +/// rest (client.zig clamps), rather than being refused as they were. Raising +/// the pair instead would have been a bigger change than it looks: `frameBound` +/// stays well inside `max_payload`, but a themed full frame at 512x128 is +/// already ~0.9 MiB against server.zig's 1 MiB `out_backlog`. pub const max_cols: u16 = 512; pub const max_rows: u16 = 128; @@ -127,6 +145,24 @@ const cell_max = 1 + 1 + 7 + 4 + 4 + 1 + 1 + 1; /// encoder coalesces against (see `encodeFrame`). const run_header = 4 + 2; +/// ...and the longest run that `count:u16` can describe, which is EXACTLY ONE +/// SHORT of the largest grid this protocol carries: `max_cols * max_rows` is +/// 512*128 = 65536, and `maxInt(u16)` is 65535. +/// +/// A full frame of that grid is ONE run over all of it whenever the theme has +/// a background: `render` fills the surface and every pane then repaints its +/// text area, and `Surface.set` clears `default`, so `sendCell`'s `!default` +/// holds for every cell. (Under a theme with `bg = null` — `dark` — untouched +/// body cells stay default and a stretch of six of them breaks the run, so the +/// overflow was theme-dependent as well as geometry-dependent, which is the +/// worst kind of latent.) `encodeFrame`'s `@intCast(run_end - start)` then +/// panicked in a safe build and was illegal behaviour in a fast one — LLVM +/// happens to truncate to zero, which the far side refuses as `BadValue`, but +/// nothing promises that. That grid is what a frontend with a big window now +/// asks for (client.zig clamps to it), so the meeting point went from +/// unreachable to routine, and the encoder splits the run instead. +const run_max = std.math.maxInt(u16); + /// `kind:u8 + cols:u16 + rows:u16 + cursor(6) + nruns:u32`. const frame_head = 1 + 2 + 2 + 6 + 4; @@ -791,7 +827,11 @@ pub fn encodeFrame( const start = i; var run_end = i + 1; i += 1; - while (i < cells.len) { + // `i - start` and not `run_end - start`, because the gap lookahead + // below moves `i` ahead of `run_end` by up to `run_header - 1` before + // the next iteration adopts it: bounding the cursor bounds the run, + // and bounding the run afterwards would not. + while (i < cells.len and i - start < run_max) { if (sendCell(cells, prev, full, i)) { run_end = i + 1; i += 1; @@ -1382,6 +1422,51 @@ test "detached wire: a full frame carries the grid, a diff carries the change" { } } +test "detached wire: the largest grid is one cell past a run, and survives it" { + // `max_cols * max_rows` is 65536 and a run's `count` is a u16, so THE ONE + // grid this protocol calls legal at both bounds is the one grid whose full + // frame cannot be described by a single run. It reached nobody while a + // frontend sent its window raw — a display that big had its hello refused + // before any frame was composed — and it is the ordinary case now that a + // frontend clamps to exactly this. In a safe build the `@intCast` panicked + // and took the session down; in a fast one it wrote a zero-length run the + // frontend answered with `BadValue`. + const cols = max_cols; + const rows = max_rows; + const n = @as(usize, cols) * rows; + const gpa = testing.allocator; + + const cells = try gpa.alloc(pardes.Cell, n); + defer gpa.free(cells); + const mirror = try gpa.alloc(pardes.Cell, n); + defer gpa.free(mirror); + const buf = try gpa.alloc(u8, frameBound(cols, rows)); + defer gpa.free(buf); + + // Every cell painted, which is what `render` produces and therefore what a + // full frame always is: one run, if a run could be that long. + for (cells) |*c| c.* = .{ .text = "a".* ++ @as([6]u8, @splat(0)), .len = 1, .default = false }; + @memset(mirror, .{}); + + const bytes = try encodeFrame(buf, cols, rows, null, cells, &.{}); + const f = (try framed(bytes)).?; + const msg = (try decodeServer(f.tag, f.payload)).frame; + try testing.expectEqual(FrameKind.full, msg.kind); + // Split, and split as late as it can be: 65535 cells then 1. + try testing.expectEqual(@as(u32, 2), msg.nruns); + try msg.apply(mirror); + try expectGridEqual(cells, mirror); + // ...at exactly the two headers the split costs and not one byte more. + // `bytes.len <= frameBound(...)` would prove nothing here — the buffer IS + // `frameBound` bytes, so an over-run is `NoSpace` above, not a false + // assertion here — and what wants pinning is that the second run is the + // whole of the cost. + try testing.expectEqual( + @as(usize, header_len + frame_head + 2 * run_header + n * 8), + bytes.len, + ); +} + test "detached wire: the diff is worth having, in bytes, on the board's grid" { // The numbers quoted in `encodeFrame`'s comment, asserted so the claim // cannot rot. 56x14 is the ESP32-P4 board's default grid (esp32p4.zig). -- cgit v1.3