diff options
| -rw-r--r-- | docs/detached.md | 17 | ||||
| -rw-r--r-- | src/CHANGELOG.md | 24 | ||||
| -rw-r--r-- | src/detached/client.zig | 85 | ||||
| -rw-r--r-- | 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). |