a73x

85152686

fix: the review round's two should-fixes and four nits

a73x   2026-08-21 08:33

Commit message
fix: the review round's two should-fixes and four nits

Counters nothing reads are half a feature: agent_chans, refusals with no
offerer, and refusals against a full table now print in stats, where the
field question gets asked. Holding-8-refusing and holding-0-refusing are
different problems with one symptom, so the live count goes beside them.

wallview declared testtmp in .imports, so the shipped wall viewer's graph
claimed a dependency on a test fixture; the table has a column for that.

deliverAgentData turned a malformed frame from 'abandon the batch' into
'skip one frame' when it came out of the pump — its sibling agent_open and
agent_close arms never stopped abandoning. Restored, and pinned.

Also: the full-table test now asserts the 'once' its name claims, the
preflight test exercises a killed agent's leftover socket rather than a
path that never existed, and two comments that overstated what they
guarded are corrected.
README.md
Old New
@@ -185,6 +185,14 @@ died. Without that check the flag was kept silently — the offer is a
185 declaration, not a capability — and the first sign was `permission denied 185 declaration, not a capability — and the first sign was `permission denied
186 (publickey)` from a git remote inside the session. 186 (publickey)` from a git remote inside the session.
187 187
188 One case the preflight cannot catch: run `mux -A` from *inside* a mux
189 session and `SSH_AUTH_SOCK` is the daemon's own socket, which accepts every
190 connection and only refuses afterwards — so the dial succeeds and the inner
191 client attaches. If an `-A` client is attached to the outer session the
192 chain works end to end; if not, the inner one is the silent agentless
193 offerer this check exists to catch. Nesting is the one place to check by
194 hand, with `ssh-add -l`.
195
188 `-A` goes on the attach form (`mux -A HOST`, `mux -A quic://HOST`, 196 `-A` goes on the attach form (`mux -A HOST`, `mux -A quic://HOST`,
189 `mux -A --sock PATH`), not on `mux wall`, which refuses a flag where a 197 `mux -A --sock PATH`), not on `mux wall`, which refuses a flag where a
190 target belongs. Sibling tiles grown from an `-A` attach by chord (`Ctrl-\ c`, 198 target belongs. Sibling tiles grown from an `-A` attach by chord (`Ctrl-\ c`,
build.zig
Old New
@@ -273,7 +273,7 @@ const mod_table = [_]ModSpec{
273 // typed-at session; the overlay machinery itself is interact's, shared — 273 // typed-at session; the overlay machinery itself is interact's, shared —
274 // and phase 3 promotes the tile into that core rather than growing a 274 // and phase 3 promotes the tile into that core rather than growing a
275 // second copy of it. 275 // second copy of it.
276 .{ .name = "wallview", .path = "src/wallview.zig", .layer = 4, .link_libc = true, .imports = &.{ "protocol", "client", "interact", "wall", "handoff", "xdg", "sockpath", "proxy", "engine", "paint", "testtmp" }, .quic_tests = true }, 276 .{ .name = "wallview", .path = "src/wallview.zig", .layer = 4, .link_libc = true, .imports = &.{ "protocol", "client", "interact", "wall", "handoff", "xdg", "sockpath", "proxy", "engine", "paint" }, .test_imports = &.{"testtmp"}, .quic_tests = true },
277 // ---- layer 5 ---- 277 // ---- layer 5 ----
278 // wall owns the spelling grammar and the state file, so argv is parsed 278 // wall owns the spelling grammar and the state file, so argv is parsed
279 // by the SAME rules the page's POST /tiles and the restored file are — 279 // by the SAME rules the page's POST /tiles and the restored file are —
src/mux_main.zig
Old New
@@ -868,10 +868,17 @@ test "agentReachable: a live socket answers, a stale or unset path does not" {
868 868
869 try std.testing.expect(agentReachable(sock)); 869 try std.testing.expect(agentReachable(sock));
870 870
871 // The stale case, which is why this dials rather than reading the 871 // The stale case, and the reason this dials rather than reading the
872 // variable: the path is still spelled, the socket file may even still 872 // variable: the agent is gone but its socket FILE is still there, so the
873 // be there, and nothing is listening. 873 // path stats fine and the connect is refused. `deinit` closes the
874 // listener without unlinking, which is exactly what a killed agent
875 // leaves behind.
874 listener.deinit(); 876 listener.deinit();
877 try std.fs.accessAbsolute(sock, .{});
878 try std.testing.expect(!agentReachable(sock));
879
880 // And the two cheaper absences, so all three of a user's states are
881 // covered by the one probe.
875 std.fs.deleteFileAbsolute(sock) catch {}; 882 std.fs.deleteFileAbsolute(sock) catch {};
876 try std.testing.expect(!agentReachable(sock)); 883 try std.testing.expect(!agentReachable(sock));
877 try std.testing.expect(!agentReachable("")); 884 try std.testing.expect(!agentReachable(""));
src/server.zig
Old New
@@ -1397,6 +1397,18 @@ pub const Server = struct {
1397 return best; 1397 return best;
1398 } 1398 }
1399 1399
1400 /// Channels open right now, for `stats`. The refusal counters only mean
1401 /// something next to it: "refused 40, holding 8" is a full table, while
1402 /// "refused 40, holding 0" is nobody offering, and those are different
1403 /// problems with the same symptom.
1404 fn liveAgentChans(self: *const Server) usize {
1405 var n: usize = 0;
1406 for (self.agent_chans) |slot| {
1407 if (slot != null) n += 1;
1408 }
1409 return n;
1410 }
1411
1400 fn freeAgentChan(self: *const Server) ?usize { 1412 fn freeAgentChan(self: *const Server) ?usize {
1401 for (self.agent_chans, 0..) |slot, s| { 1413 for (self.agent_chans, 0..) |slot, s| {
1402 if (slot == null) return s; 1414 if (slot == null) return s;
@@ -3479,12 +3491,15 @@ pub const Server = struct {
3479 var w: std.Io.Writer = .fixed(buf); 3491 var w: std.Io.Writer = .fixed(buf);
3480 try w.print( 3492 try w.print(
3481 "snapshots={d} snapshot_bytes={d} deltas={d} delta_bytes={d}" ++ 3493 "snapshots={d} snapshot_bytes={d} deltas={d} delta_bytes={d}" ++
3482 " snapshot_equiv_bytes={d} clients={d} attaches={d} sessions={d}", 3494 " snapshot_equiv_bytes={d} clients={d} attaches={d} sessions={d}" ++
3495 " agent_chans={d} agent_refused_no_offer={d} agent_refused_full={d}",
3483 .{ 3496 .{
3484 self.stats.snapshots, self.stats.snapshot_bytes, 3497 self.stats.snapshots, self.stats.snapshot_bytes,
3485 self.stats.deltas, self.stats.delta_bytes, 3498 self.stats.deltas, self.stats.delta_bytes,
3486 self.stats.snapshot_equiv_bytes, self.liveClients(), 3499 self.stats.snapshot_equiv_bytes, self.liveClients(),
3487 self.stats.attaches, self.liveSessions(), 3500 self.stats.attaches, self.liveSessions(),
3501 self.liveAgentChans(), self.agent_refused_no_offer,
3502 self.agent_refused_full,
3488 }, 3503 },
3489 ); 3504 );
3490 for (self.sessions, 0..) |slot, si| { 3505 for (self.sessions, 0..) |slot, si| {
@@ -10498,6 +10513,20 @@ test "Server: a full channel table refuses the newest dial and says so once" {
10498 for (seen, 0..) |id, i| for (seen[i + 1 ..]) |other| try std.testing.expect(id != other); 10513 for (seen, 0..) |id, i| for (seen[i + 1 ..]) |other| try std.testing.expect(id != other);
10499 srv.next_agent_id = seen[3]; 10514 srv.next_agent_id = seen[3];
10500 try std.testing.expect(srv.nextAgentId() != seen[3]); 10515 try std.testing.expect(srv.nextAgentId() != seen[3]);
10516
10517 // "Once", which is the half the name claims and nothing asserted: a
10518 // second dial against the same full table counts again and says nothing,
10519 // and only a freed slot re-arms the line. ssh retries, so a log that
10520 // scrolled would be its own outage.
10521 try std.testing.expect(srv.agent_full_said);
10522 const again = try std.net.connectUnixSocket(path);
10523 defer again.close();
10524 spun = 0;
10525 while (spun < 400 and srv.agent_refused_full < 2) : (spun += 1) _ = try srv.pumpOnce(5);
10526 try std.testing.expectEqual(@as(u32, 2), srv.agent_refused_full);
10527 try std.testing.expect(srv.agent_full_said);
10528 srv.closeAgentChan(0, .notify);
10529 try std.testing.expect(!srv.agent_full_said);
10501 } 10530 }
10502 10531
10503 test "Server: an agent connection with nobody offering is refused fast" { 10532 test "Server: an agent connection with nobody offering is refused fast" {
src/wall.zig
Old New
@@ -97,8 +97,9 @@ pub fn spellingFromArgv(
97 if (i + 1 >= args.len) return error.MissingSockPath; 97 if (i + 1 >= args.len) return error.MissingSockPath;
98 return .{ .spelling = try std.fmt.allocPrint(alloc, "--sock {s}", .{args[i + 1]}), .consumed = 2 }; 98 return .{ .spelling = try std.fmt.allocPrint(alloc, "--sock {s}", .{args[i + 1]}), .consumed = 2 };
99 } 99 }
100 // A wall takes targets, and no target starts with a dash: the two 100 // A wall takes targets, and no target starts with a dash: bare `--sock`
101 // spellings that do are handled above. Left to fall through, `mux wall 101 // took its path and returned above, and the one-piece `--sock PATH`
102 // dialect is exempted on the next line. Left to fall through, `mux wall
102 // -A host` became a tile for a host named `-A` and failed to resolve 103 // -A host` became a tile for a host named `-A` and failed to resolve
103 // somewhere far from the typo — and a wall has no per-tile agent flag 104 // somewhere far from the typo — and a wall has no per-tile agent flag
104 // to have meant, `-A` belonging to a single attach. 105 // to have meant, `-A` belonging to a single attach.
src/wallview.zig
Old New
@@ -949,20 +949,29 @@ fn openAgentChan(
949 /// stance for the same frame, and here it is simply a channel we closed a 949 /// stance for the same frame, and here it is simply a channel we closed a
950 /// moment before this arrived. Oversize hangs the channel up instead: the 950 /// moment before this arrived. Oversize hangs the channel up instead: the
951 /// write below is blocking, and a truncated agent reply is worse than none. 951 /// write below is blocking, and a truncated agent reply is worse than none.
952 ///
953 /// False means the payload was too short to name a channel at all, and the
954 /// caller abandons the batch on it — the stance its sibling `agent_open`
955 /// and `agent_close` arms take for the same malformation, and the one this
956 /// arm took before it was lifted out of the pump.
952 fn deliverAgentData( 957 fn deliverAgentData(
953 locals: []?AgentLocal, 958 locals: []?AgentLocal,
954 payload: []const u8, 959 payload: []const u8,
955 transport: *client.Transport, 960 transport: *client.Transport,
956 ) void { 961 ) bool {
957 const id = proto.decodeAgentId(payload) catch return; 962 const id = proto.decodeAgentId(payload) catch return false;
958 const s = findLocal(locals, id) orelse return; 963 const s = findLocal(locals, id) orelse return true;
959 if (proto.agentDataOversize(payload)) return closeLocal(locals, s, transport); 964 if (proto.agentDataOversize(payload)) {
965 closeLocal(locals, s, transport);
966 return true;
967 }
960 // Bytes onto the fd in order, never parsed and never reassembled: the 968 // Bytes onto the fd in order, never parsed and never reassembled: the
961 // daemon reads the far end in `agent_data_max` bites, so one agent 969 // daemon reads the far end in `agent_data_max` bites, so one agent
962 // message can arrive as several frames and several messages as one. The 970 // message can arrive as several frames and several messages as one. The
963 // agent protocol delimits itself over a stream, and this end is a pipe. 971 // agent protocol delimits itself over a stream, and this end is a pipe.
964 proto.writeAllFd(locals[s].?.fd, payload[proto.agent_id_len..]) catch 972 proto.writeAllFd(locals[s].?.fd, payload[proto.agent_id_len..]) catch
965 closeLocal(locals, s, transport); 973 closeLocal(locals, s, transport);
974 return true;
966 } 975 }
967 976
968 /// Every channel, silently. For the redial only: these channels belonged to 977 /// Every channel, silently. For the redial only: these channels belonged to
@@ -1432,11 +1441,11 @@ fn pumpTile(t: *Tile) void {
1432 &proto.encodeAgentId(id), 1441 &proto.encodeAgentId(id),
1433 ) catch {}; 1442 ) catch {};
1434 }, 1443 },
1435 .agent_data => deliverAgentData( 1444 .agent_data => if (!deliverAgentData(
1436 &agent_locals, 1445 &agent_locals,
1437 frame.payload, 1446 frame.payload,
1438 &transport, 1447 &transport,
1439 ), 1448 )) break :frames,
1440 .agent_close => { 1449 .agent_close => {
1441 const id = proto.decodeAgentId(frame.payload) catch break :frames; 1450 const id = proto.decodeAgentId(frame.payload) catch break :frames;
1442 // Silent, mirroring the daemon: it has already 1451 // Silent, mirroring the daemon: it has already
@@ -3356,7 +3365,7 @@ test "agent channels: an oversize frame hangs the channel up, a full one lands"
3356 var small: [proto.agent_id_len + 2]u8 = undefined; 3365 var small: [proto.agent_id_len + 2]u8 = undefined;
3357 small[0..proto.agent_id_len].* = proto.encodeAgentId(5); 3366 small[0..proto.agent_id_len].* = proto.encodeAgentId(5);
3358 @memcpy(small[proto.agent_id_len..], "hi"); 3367 @memcpy(small[proto.agent_id_len..], "hi");
3359 deliverAgentData(&locals, &small, &transport); 3368 try std.testing.expect(deliverAgentData(&locals, &small, &transport));
3360 var got: [8]u8 = undefined; 3369 var got: [8]u8 = undefined;
3361 try std.testing.expectEqual(@as(usize, 2), try std.posix.read(chan[0], &got)); 3370 try std.testing.expectEqual(@as(usize, 2), try std.posix.read(chan[0], &got));
3362 try std.testing.expectEqualSlices(u8, "hi", got[0..2]); 3371 try std.testing.expectEqualSlices(u8, "hi", got[0..2]);
@@ -3367,12 +3376,17 @@ test "agent channels: an oversize frame hangs the channel up, a full one lands"
3367 defer std.testing.allocator.free(over); 3376 defer std.testing.allocator.free(over);
3368 @memset(over, 'x'); 3377 @memset(over, 'x');
3369 over[0..proto.agent_id_len].* = proto.encodeAgentId(5); 3378 over[0..proto.agent_id_len].* = proto.encodeAgentId(5);
3370 deliverAgentData(&locals, over, &transport); 3379 try std.testing.expect(deliverAgentData(&locals, over, &transport));
3371 try std.testing.expectEqual(@as(?AgentLocal, null), locals[0]); 3380 try std.testing.expectEqual(@as(?AgentLocal, null), locals[0]);
3372 try std.testing.expect(peerClosed(chan[0])); 3381 try std.testing.expect(peerClosed(chan[0]));
3373 3382
3374 // And the daemon is told, because it is holding the far socket open for 3383 // And the daemon is told, because it is holding the far socket open for
3375 // an answer this end will never write. 3384 // an answer this end will never write.
3385 // A payload too short to name a channel: false, and the pump abandons the
3386 // batch on it rather than skipping one frame, because a stream that has
3387 // lost frame alignment is not one to keep reading.
3388 try std.testing.expect(!deliverAgentData(&locals, "ab", &transport));
3389
3376 const frame = (try proto.readFrame(std.testing.allocator, link[0])) orelse 3390 const frame = (try proto.readFrame(std.testing.allocator, link[0])) orelse
3377 return error.NoAgentCloseOnOversize; 3391 return error.NoAgentCloseOnOversize;
3378 defer frame.deinit(std.testing.allocator); 3392 defer frame.deinit(std.testing.allocator);
test/e2e.sh
Old New
@@ -7050,8 +7050,10 @@ ok "agent forwarding: ssh-add -l in the session lists the client's key"
7050 # transport is touched. 7050 # transport is touched.
7051 # 7051 #
7052 # Both spellings of "no agent", because reading the variable would only 7052 # Both spellings of "no agent", because reading the variable would only
7053 # catch the first: unset, and set to a path nothing is listening on — the 7053 # catch the first: unset, and set to a path nothing answers on. The unit
7054 # ordinary leftover of an agent that has died. 7054 # test covers the subtler third — an agent killed leaving its socket file
7055 # behind, which stats fine and refuses the connect — since arranging a
7056 # listener that dies mid-leg buys nothing here.
7055 set +e 7057 set +e
7056 env -u SSH_AUTH_SOCK "$MUX" -A --sock "$SOCK48" > "$OUT.anoag" 2>&1 7058 env -u SSH_AUTH_SOCK "$MUX" -A --sock "$SOCK48" > "$OUT.anoag" 2>&1
7057 RC=$? 7059 RC=$?