ba62a7b2

test: pin the three QUIC fixes a mutation could still walk through

a73x 2026-08-08 17:42

Commit message

test: pin the three QUIC fixes a mutation could still walk through

The re-entrancy batch was correct and largely unguarded. Four pins, each
checked against the exact mutation that survived review.

reapClosing's drain: one connection whose owner answers and then closes,
both from inside one callback — the session-full refusal's exact shape.
Since send only queues, that drain is the reply's one chance to reach the
wire; deleting it now fails the test. The guarantee is bounded by the peer's
flow-control window at that instant, which a refusal always fits inside and
a large payload would not, and the test says so rather than implying more.

timeoutMs's negative cap: two lines on the back of the handshake test,
because the guard needs a LIVE connection with a finite expiry — the loop
skips connections without one and never reaches the cast that panics.

took-before-error: extracted as accountWrite, shared by the listener's drain
and the client's, which turns an ordering that needed fault injection to
observe into a property one unit test pins directly. TestClient's drain
keeps its own copy: it tracks a plain offset rather than an Egress ring, so
the helper does not fit it.

And the end-of-pump drainAll — the one line whose entire purpose is latency
was the one line no test defended. Pinning it took three attempts, and the
two failures are why the comment in it is long. Using the existing pump
helper failed to pin anything because that helper drains the CLIENT every
iteration, which makes it transmit: its acknowledgements alone hand feed() a
drain to carry the reply out on. Looping over several pumps failed for a
different reason — on loopback the PTO comes due within a few hundred
milliseconds and tick drains on expiry, so any budget long enough not to be
flaky is long enough to pass without the line existing. What pins it is
quiescing the connection until ngtcp2's next timer is comfortably in the
future, then a single pump with a client that only ever listens.

Also records what the asserts are worth: std.debug.assert is compiled out of
ReleaseFast, every binary this project builds today is Debug, and the day a
release build is added the tripwire goes with it. The invariant is
structural; the assert only makes a regression loud. The reasoned-not-pinned
list is down to two — closed-after-reap (constructible, a time trade, with
the construction written down) and the deinit orderings (whose honest
instrument is ASAN, now banked).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs/decisions.md

Old		New
@@ -1078,13 +1078,32 @@ since `tick` only services connections whose timer is due) and the
1078	session-full refusal (queued, then the connection reaped before it could	1078	session-full refusal (queued, then the connection reaped before it could
1079	leave; `reapClosing` now drains once before freeing).	1079	leave; `reapClosing` now drains once before freeing).
1080		1080
1081	Three hardening items in that batch are reasoned, not mutation-pinned,	1081	The asserts live only where asserts live. `std.debug.assert` is compiled
1082	and are recorded as such because the batch would otherwise read as	1082	out of ReleaseFast and ReleaseSmall. Every binary this project builds today
1083	uniformly verified: `closed`-recomputed-after-reap needs a handler that	1083	is Debug — `make build`, `make test`, `make e2e`, the musl cross-build — so
1084	closes a different connection mid-reap; took-before-error-check needs	1084	the guard is present everywhere it currently matters. The day a release
1085	ngtcp2 to commit a stream offset and then fail; the two `deinit` orderings	1085	build is added, the assert stops being a tripwire and re-entrancy reverts to
1086	need a use-after-free detector. All three are corrections to paths the tests	1086	what it was: a rare abort with no banner. The invariant is enforced by the
1087	do not reach.	1087	structure (queue-only `send`); the assert only makes a regression loud,
		1088	and only in the builds we ship today.
		1089
		1090	Two hardening items remain reasoned rather than mutation-pinned, down
		1091	from three, and are recorded as such because the batch would otherwise read
		1092	as uniformly verified:
		1093
		1094	- `closed`-recomputed-after-reap. Constructible, and the construction is
		1095	known — two connections, where `onClose(A)` closes B, triggered by a
		1096	packet on B whose `onData` closes A — so this is a time trade rather than
		1097	an impossibility. Left undone deliberately.
		1098	- The two `deinit` orderings. The honest instrument is ASAN or valgrind over
		1099	the QUIC tests, not a cleverer unit test; a use-after-free that reads
		1100	recycled memory successfully is exactly the failure a functional test
		1101	cannot see. Banked as suite investment.
		1102
		1103	took-before-error-check is no longer among them: extracting `accountWrite`
		1104	made the ordering a property of one small function, which a unit test pins
		1105	directly (`wrote=4, n=-1` must account the four and then stop) without
		1106	needing to make ngtcp2 fail on demand.
1088		1107
1089	### Reconcile v2: judge evidence, not arrival order	1108	### Reconcile v2: judge evidence, not arrival order
1090		1109
@@ -1236,6 +1255,10 @@ Engine, over a socket. The full render-vs-dump harness stays banked.
1236	before it is.	1255	before it is.
1237	- Full render-vs-dump convergence harness (shared with M7's identical	1256	- Full render-vs-dump convergence harness (shared with M7's identical
1238	banked item).	1257	banked item).
		1258	- ASAN or valgrind over the QUIC tests. The only honest instrument for
		1259	the two `deinit` orderings above, and for the whole class of
		1260	use-after-free that reads recycled memory successfully — which is the
		1261	class this project has already been bitten by twice.
1239	- Floor-form rewrite applied retroactively to M6's reattach criterion.	1262	- Floor-form rewrite applied retroactively to M6's reattach criterion.
1240	It still fails structurally on fast links; the form that fixes it is now	1263	It still fails structurally on fast links; the form that fixes it is now
1241	standard and written down.	1264	standard and written down.

src/quic_client.zig

Old		New
@@ -437,16 +437,14 @@ pub const Client = struct {
437	stream_blocked = true;	437	stream_blocked = true;
438	continue;	438	continue;
439	}	439	}
440	// Accounted before the error check: ngtcp2 can commit the stream	440	switch (quic.accountWrite(&self.out, wrote, n)) {
441	// offset and still fail afterwards, and bytes left counted as	441	.stop => {
442	// unsent get re-offered at an offset the peer has moved past —	442	self.dead = true;
443	// a desynchronised stream that surfaces far from here.	443	return;
444	if (wrote > 0) self.out.took(@intCast(wrote));	444	},
445	if (n < 0) {	445	.brk => return,
446	self.dead = true;	446	.cont => {},
447	return;
448	}	447	}
449	if (n == 0) return;
450	_ = std.posix.send(self.fd, buf[0..@intCast(n)], 0) catch return;	448	_ = std.posix.send(self.fd, buf[0..@intCast(n)], 0) catch return;
451	}	449	}
452	}	450	}

src/quic_server.zig

@@ @@ -361,6 +361,31 @@ pub const Egress = struct { @@
+};
+/// What one `writev_stream` return means for the egress ring — and, more to
+/// the point, IN WHAT ORDER.
+///
+/// The ordering is the whole reason this is a function rather than three
+/// copies of two ifs. ngtcp2 can commit `ndatalen` — advancing the stream
+/// offset it will retransmit from — and still return an error afterwards
+/// (NOMEM out of rtb_add, say). Account for the bytes first or they stay
+/// counted as unsent, get offered again at an offset the peer has moved
+/// past, and the stream desynchronises somewhere far away from here.
+pub const WriteAction = enum {
+    /// The call failed. Stop draining; the bytes are already accounted.
+    stop,
+    /// Nothing more to send right now.
+    brk,
+    /// A packet was produced; send it and go round again.
+    cont,
+};
+pub fn accountWrite(out: *Egress, wrote: c.ngtcp2_ssize, n: c.ngtcp2_ssize) WriteAction {
+    if (wrote > 0) out.took(@intCast(wrote));
+    if (n < 0) return .stop;
+    if (n == 0) return .brk;
+    return .cont;
+}
+/// One authenticated peer: an ngtcp2 connection, its TLS object, and the
+/// single bidirectional stream that carries everything.
+const Conn = struct {
@@ @@ -1195,16 +1220,11 @@ pub const Listener = struct { @@
+                stream_blocked = true;
+                continue;
+            switch (accountWrite(&cn.out, wrote, n)) {
+                .stop => return,
+                .brk => break,
+                .cont => {},
+            }
+            _ = std.posix.sendto(
+                self.fd,
+    try std.testing.expect(ok);
+    try std.testing.expectEqual(@as(usize, 3 * 1024 * 1024), owner.received);
+    try std.testing.expect(!cl.mismatch);
+    // Piggybacking on the one place a LIVE connection with a finite expiry
+    // exists: negative is poll(2)'s "wait forever", and the timeout fold
+    // must hand it back rather than try to @intCast it. Without the guard
+    // this line panics — and it needs a real connection, because the loop
+    // skips connections with no expiry and would never reach the cast.
+    try std.testing.expectEqual(@as(i32, -1), setup.l.timeoutMs(-1));
+    // The ordinary case still folds as before.
+    try std.testing.expect(setup.l.timeoutMs(100) <= 100);
+test "Listener: a client holding the wrong key never completes a handshake" {
+    try std.testing.expectEqualStrings("cd", v[0].base[0..v[0].len]);
+test "accountWrite: bytes ngtcp2 committed are accounted even when the call failed" {
+    const alloc = std.testing.allocator;
+    var e: Egress = .{ .buf = try alloc.alloc(u8, 16) };
+    defer e.deinit(alloc);
+    _ = e.push("abcdefgh");
+    try std.testing.expectEqual(@as(usize, 8), e.unsent);
+    // The case that has no fault injection available and would otherwise go
+    // untested: ngtcp2 committed four bytes of stream data — its offset has
+    // moved — and THEN returned an error. Accounting after the check would
+    // leave those four counted as unsent, so the next drain would offer them
+    // again at an offset the peer is already past.
+    try std.testing.expectEqual(WriteAction.stop, accountWrite(&e, 4, -1));
+    try std.testing.expectEqual(@as(usize, 4), e.unsent);
+    // The ordinary returns, for completeness of the contract.
+    try std.testing.expectEqual(WriteAction.brk, accountWrite(&e, 0, 0));
+    try std.testing.expectEqual(WriteAction.cont, accountWrite(&e, 4, 120));
+    try std.testing.expectEqual(@as(usize, 0), e.unsent);
+}
+test "Egress: an ack against a torn-down ring is ignored, not a division by zero" {
+    const alloc = std.testing.allocator;
+    var e: Egress = .{ .buf = try alloc.alloc(u8, 8) };
@@ @@ -2174,6 +2224,93 @@ fn probeGotAnything(fd: std.posix.fd_t) bool { @@
+    return n > 0;
+test "Listener: a reply queued just before a close still reaches the peer" {
+    const alloc = std.testing.allocator;
+    const key: Key = .{ .bytes = [_]u8{0x5C} ** key_len };
+    // The session-full refusal, in miniature: the owner answers and then
+    // shuts the connection, both from inside one callback. Since `send` only
+    // QUEUES — draining from inside an ngtcp2 callback is the defect that
+    // change removed — those bytes have exactly one chance to leave, in the
+    // drain reapClosing does before it frees the connection. Delete that
+    // drain and a refused client learns nothing and waits out its idle
+    // timeout instead.
+    //
+    // The guarantee is bounded by the peer's flow-control window at that
+    // instant. A refusal is a handful of bytes and always fits; a large
+    // queued payload would not, and this test does not claim otherwise.
+    const AnswerThenClose = struct {
+        listener: *Listener = undefined,
+        id: u64 = 0,
+        opened: usize = 0,
+        closed: usize = 0,
+        queued: usize = 0,
+        fn onOpen(ctx: *anyopaque, id: u64) void {
+            const self: *@This() = @ptrCast(@alignCast(ctx));
+            self.opened += 1;
+            self.id = id;
+        }
+        fn onData(ctx: *anyopaque, id: u64, _: []const u8) void {
+            const self: *@This() = @ptrCast(@alignCast(ctx));
+            if (self.queued > 0) return;
+            self.id = id;
+            self.queued = self.listener.send(id, "REFUSED-FULL") catch 0;
+            self.listener.closeConn(id);
+        }
+        fn onClose(ctx: *anyopaque, _: u64) void {
+            const self: *@This() = @ptrCast(@alignCast(ctx));
+            self.closed += 1;
+        }
+        fn handler(self: *@This()) Handler {
+            return .{ .ctx = self, .onOpen = onOpen, .onData = onData, .onClose = onClose };
+        }
+    };
+    var owner: AnswerThenClose = .{};
+    const bind = try std.net.Address.parseIp("127.0.0.1", 0);
+    const l = try Listener.init(alloc, bind, key, owner.handler(), 10_000);
+    defer l.deinit();
+    owner.listener = l;
+    var actual: std.posix.sockaddr.storage = undefined;
+    var alen: std.posix.socklen_t = @sizeOf(@TypeOf(actual));
+    try std.posix.getsockname(l.fd, @ptrCast(&actual), &alen);
+    const addr = std.net.Address.initPosix(@ptrCast(@alignCast(&actual)));
+    var cl = try TestClient.init(addr, key);
+    defer cl.deinit();
+    try cl.start();
+    cl.drain();
+    cl.out = "attach";
+    var waited: u64 = 0;
+    while (waited < 10_000) : (waited += 5) {
+        var fds = [_]std.posix.pollfd{
+            .{ .fd = l.fd, .events = std.posix.POLL.IN, .revents = 0 },
+            .{ .fd = cl.fd, .events = std.posix.POLL.IN, .revents = 0 },
+        };
+        _ = std.posix.poll(&fds, 5) catch break;
+        if (fds[0].revents != 0) l.readable();
+        if (fds[1].revents != 0) cl.readable();
+        l.tick();
+        cl.drain();
+        for (l.conns) |slot| {
+            if (slot) |cn| l.drain(cn);
+        }
+        if (cl.recv_len >= "REFUSED-FULL".len) break;
+    }
+    try std.testing.expectEqual(@as(usize, "REFUSED-FULL".len), owner.queued);
+    // The bytes are on the peer, not merely in a ring that was then freed.
+    try std.testing.expect(std.mem.indexOf(
+        u8,
+        cl.recv_buf[0..cl.recv_len],
+        "REFUSED-FULL",
+    ) != null);
+    // ...and the connection really did go.
+    try std.testing.expect(l.find(owner.id) == null);
+}
+test "Listener: closing a connection from inside a receive callback is deferred" {
+    const alloc = std.testing.allocator;
+    const key: Key = .{ .bytes = [_]u8{0x7E} ** key_len };

src/server.zig

+    cl.drain();
+test "Server: output reaches a silent QUIC client without waiting for it to speak" {
+    const alloc = std.testing.allocator;
+    var tmp = try TmpDir.make();
+    defer tmp.cleanup();
+    const dir_path = tmp.path();
+    const sock_path = try std.fmt.allocPrint(alloc, "{s}/qquiet.sock", .{dir_path});
+    defer alloc.free(sock_path);
+    // /bin/cat: a session that emits nothing on its own, so the only output
+    // in this test is the output the test causes.
+    var srv = try Server.init(alloc, .{ .sock_path = sock_path, .shell = "/bin/cat" });
+    defer srv.deinit();
+    const key: quic.Key = .{ .bytes = [_]u8{0x4D} ** quic.key_len };
+    const q = try quicTestServer(&srv, key);
+    defer q.l.deinit();
+    var cl = try quic.TestClient.init(q.addr, key);
+    defer cl.deinit();
+    try cl.start();
+    cl.drain();
+    var out: std.ArrayList(u8) = .empty;
+    defer out.deinit(alloc);
+    try attachOver(&cl, &out, alloc);
+    var only = [_]*quic.TestClient{&cl};
+    _ = try quicPump(&srv, &only, 8000, &cl, struct {
+        fn f(t: *quic.TestClient) bool {
+            return findFrame(t.recv_buf[0..t.recv_len], .snapshot) != null;
+        }
+    }.f);
+    try std.testing.expect(findFrame(cl.recv_buf[0..cl.recv_len], .snapshot) != null);
+    // From here the client says NOTHING. That is the whole point: the
+    // listener's send only queues, and the two drains that fire on their own
+    // are the one after read_pkt — which needs an inbound packet — and tick,
+    // which only services connections whose timer is due. A broadcast to a
+    // quiet client is covered by neither, and the end-of-pump drainAll is
+    // the only thing that moves it.
+    // Quiesce first, or the measurement is meaningless. Straight after the
+    // attach exchange ngtcp2 has a loss-detection timer already past due, so
+    // tick() drains on expiry and delivers the frame below whether or not
+    // anything else does. Settle until the next timer is comfortably in the
+    // future; only then is tick ruled out as the deliverer.
+    var settle: usize = 0;
+    while (settle < 400) : (settle += 1) {
+        if (q.l.timeoutMs(1000) > 100) break;
+        _ = try srv.pumpOnce(5);
+        cl.drain();
+        var pfd = [_]std.posix.pollfd{
+            .{ .fd = cl.fd, .events = std.posix.POLL.IN, .revents = 0 },
+        };
+        if ((std.posix.poll(&pfd, 1) catch 0) > 0) cl.readable();
+    }
+    try std.testing.expect(q.l.timeoutMs(1000) > 100);
+    const before = cl.recv_len;
+    cl.out = &.{};
+    srv.eng.feed("quiet-client-marker\r\n");
+    srv.sendUpdate();
+    // EXACTLY ONE pump, and the client only ever listens. Both halves are
+    // the test, and both were learned by watching weaker versions pass:
+    //
+    // quicPump calls cl.drain() every iteration, which makes the client
+    // TRANSMIT — its acknowledgements alone suffice — and every inbound
+    // packet hands feed() a drain to carry the reply out on. And looping
+    // over several pumps is no better: on loopback the PTO expiry comes due
+    // within a few hundred milliseconds, and tick drains on expiry, so any
+    // budget generous enough not to be flaky is generous enough to pass
+    // without drainAll existing at all.
+    //
+    // One pump answers the only question that isolates it: the frame was
+    // queued before this call, so did THIS call put it on the wire?
+    _ = try srv.pumpOnce(5);
+    // Reading is not transmitting: the client takes whatever already
+    // arrived, and never gives the server an inbound packet to react to.
+    var tries: usize = 0;
+    while (tries < 60 and findFrame(cl.recv_buf[0..cl.recv_len], .delta) == null) : (tries += 1) {
+        var pfd = [_]std.posix.pollfd{
+            .{ .fd = cl.fd, .events = std.posix.POLL.IN, .revents = 0 },
+        };
+        if ((std.posix.poll(&pfd, 2) catch 0) > 0) cl.readable();
+    }
+    try std.testing.expect(cl.recv_len > before);
+    try std.testing.expect(findFrame(cl.recv_buf[0..cl.recv_len], .delta) != null);
+}
+test "Server: a QUIC client still receives the shell's exit status" {
+    const alloc = std.testing.allocator;
+    var tmp = try TmpDir.make();