fix(transport): acknowledge down-poll chunks so a slow push cannot lose bytes

[Daily-AC]

Fixes #57.

Root cause

The peer-to-peer TLS stream rides on the relay's HTTP long-poll transport. /h/down dequeued bytes from the session's side queue and then wrote them into the response body, so anything that stopped the reader from receiving that response whole destroyed those bytes for good. The stream got a hole in the middle, and the end-to-end TLS layer reported the hole as tls: bad record MAC.

internal/httpconn made the loss silent. In Read:

body, _ := io.ReadAll(resp.Body)

A body cut short returns the partial data and an error. The error was discarded and the partial body was handed straight to TLS.

Two things made that happen on a slow link and only after a few minutes:

• Unbounded coalescing. sideQueue.drain appended every queued chunk into one response. A reader that fell behind while the writer kept going got handed megabytes in a single response body.
• A whole-exchange client timeout. http.Client{Timeout: 60 * time.Second} bounded the entire request including the body download. On a slow link a coalesced multi-megabyte response exceeds that bound — at the ~60 KB/s measured in the issue, 8 MB needs about 140 s — so the timeout fired mid-body, which is exactly the truncation above. Smaller responses on the same link were fine, which is why 2 MB succeeded where 18.9 MB did not.

That matches the report: 18.9 MB failed at 2 m 50 s on the slow link, while 2 MB on the same link and 18.9 MB on a fast link both succeeded.

The fix

The down direction is acknowledged, at the transport layer:

• A data-bearing 200 carries X-Wanctl-Down-Seq. The next poll reports the last fully received sequence in ack=.
• The relay holds a delivered chunk in sideQueue.unacked until an ack covers it, and re-sends it byte for byte under its original sequence otherwise. Nothing is dropped on the strength of having written it to a socket.
• Checking the ack, draining, numbering the chunk and storing it as unacked is one operation per direction, admitted one poll at a time. Two polls that overlap — a reader whose request was abandoned while parked on an empty queue, plus the retry it sent next — used to each take a chunk, and the second overwrote the first's. A poll abandoned after it drained still records its chunk, so the bytes wait for the next poll rather than vanishing with the response nobody received.
• httpconn no longer swallows a truncated body. It leaves the ack where it is and polls again, so the relay re-sends; the same bounded retry covers a poll request that fails outright. It also discards a re-send of a sequence it already consumed, so a duplicate cannot be injected either.
• Retrying is gated on a negotiated capability. The relay marks every authenticated /h/down answer with X-Wanctl-Down-Ack — the data-bearing 200, the empty 204, and the 400/404/410 refusals, though not the 401, which is answered before the relay knows who is asking. A reader that has not seen that header (or a sequence header) on this session fails the read loudly instead of retrying, because a relay from before this protocol dequeues before it answers and a retry there would resume after the lost bytes and skip them silently. Failing is what main does today, so this is never worse than before the PR; it only ever retries more.
• Coalescing is capped at maxDrainBytes, and the cap now bounds the append instead of being tested before it: a chunk that would overshoot is split and its tail is served first on the next drain, preserving order.
• A graceful close keeps the session drainable. Deleting it on /h/close used to throw away everything queued behind the first capped response — the far side read one cap's worth and the rest 404ed. The queues now stop accepting bytes and report EOF once dry, while the session stays reachable until both directions have been taken: closed to new bytes, no poll part way through taking any out, and none left queued, held as a split tail, or waiting to be acknowledged. The in-flight part matters — a chunk that has left the channel but has not yet been recorded is in no field at all, and reading only the fields made an occupied direction look finished. One direction reaching EOF says nothing about the other, and retiring on the first would 404 away a chunk the peer had not yet acknowledged. Retirement is attempted by every site that can complete the last of those conditions — each poll, each read on the in-process bridge, and the close itself, which is what shuts the second queue. A single site is not enough: whoever looks first may look while another reader still holds a direction, and a check that came too early is only harmless if someone checks again. When the far side never comes back for its half, none of them can succeed and the idle sweeper retires it instead: reapHTTP scans every 40 s and drops a session nobody has polled for 60 s, so an abandoned one is gone within roughly 60–100 s. Credential revocation still tears the session down immediately (410).
• A closed session is readable, not writable. Keeping it registered left /h/up open on it, so an upload after the close could still queue bytes nobody would read. /h/up now refuses a closed session with 410, including an empty body, and push and close take the same lock, so once the close has returned no later push can succeed.
• The client bounds the wait for response headers at 45 s and the whole request at 5 minutes, rather than the old 60 s bound on the entire exchange, so a body that is merely slow is no longer killed.

A poll that arrives without ack= is served the old fire-and-forget way, so a mixed-version fleet keeps working. That path is covered by a test.

Choosing the cap

maxDrainBytes is 2 MiB, sized against the link in the issue rather than picked round. At 60 KB/s a full response downloads in about 34 s — roughly a ninth of the client's 5-minute per-request bound, with room to spare for the 20 s the poll may have parked on the relay first. Larger buys nothing on that link and makes the re-send after a truncated body more expensive. Smaller costs throughput everywhere else, because serial polling cannot carry more than one cap per round trip.

Measured on the in-repo probe (32 MiB, simulated 100 ms per-poll RTT):

cap	polls	throughput
256 KiB	128	2.47 MiB/s
2 MiB	16	19.24 MiB/s

TestDownPollThroughputAtRTT keeps that measurable; it is skipped unless WANCTL_TRANSPORT_THROUGHPUT=1 is set.

transport=ws

No bug of this class. A WebSocket session is one TCP connection piped byte for byte by relay.pipe, so a carrier failure tears the session down with an error instead of leaving a gap. The hybrid ws/http path reads the same side queues in process through httpSessionConn, where the hand-off is a function return, not a response that can half arrive — but its HTTP leg shares the drain cap, so ending that leg now uses the same drainable close.

Verification

internal/relay/slowlink_test.go builds an in-process relay, controller and agent and runs a real TLS stream over the HTTP transport. It carries real net/http on both ends with only the TCP listen and dial replaced by net.Pipe, so it needs no listening socket.

On main, TestPushSurvivesTruncatedDownPoll (20 MB, two responses cut short) fails with the issue's exact error:

push failed after 4 data-bearing down polls (1 truncated, 0 dropped):
agent read: local error: tls: bad record MAC
(controller saw: controller write: up chunk: relay returned 404)

and TestPushSurvivesDroppedDownPoll (8 MB, two polls killed before the response) fails with a connection reset. Both pass on this branch, with the agent's SHA-256 matching the payload.

Each review finding has its own test, each verified to fail with only that fix reverted:

test	what it pins
TestOverlappingDownPollsOnTheWireDoNotLoseAChunk	a poll failed at the client but left running on the relay, overlapped by its retry, over real HTTP
TestOverlappingPollsCannotTakeDifferentChunks	polls that acked nothing are all served the same chunk
TestAbandonedPollKeepsItsChunkForTheNextPoll	a poll abandoned after draining leaves its chunk, under the same sequence
TestDownPollRetriesOnlyAgainstAnAcknowledgingRelay	new client against an old relay: dropped and truncated responses, paired against the current relay. The older relay has to prove it advertises neither the capability nor a sequence header, so the case cannot pass by mis-wiring
TestGracefulCloseStaysDrainable	4 chunks over the cap, peer closes mid-response, reader still gets all of it then EOF
TestGracefulCloseWaitsForBothDirections	one direction drained while the other holds an unacknowledged chunk and a split tail, for both close paths and for a poll already parked when the close landed
TestUploadsAfterGracefulCloseAreRejected	64 uploads after a close that left a backlog, where the session is deliberately kept, all refused, nothing queued, empty body included
TestUploadRacingGracefulCloseIsAllOrNothing	an upload concurrent with the close: the status and the queue agree
TestRetirementWaitsForAPollHoldingAChunk	the empty direction polled in a loop while the other works through two megabytes, for both close paths; the chunk stays re-sendable
TestGracefulCloseRetiresASessionAReaderWasParkedOn	a reader already parked when the close arrives, for both close paths; the session is still retired promptly rather than left to the sweeper
TestRevokedCredentialTearsDownImmediately	revocation is not a graceful close
TestDrainCapSplitsRatherThanOvershoots	one over-cap chunk, two straddling it, irregular sizes; order and memory bound

Also covered: TestSideQueueResendsUntilAcked, TestDrainCoalescingIsBounded, and TestDownPollWithoutAckIsFireAndForget for the other direction of the version mix, an old client against the current relay.

The existing fault carrier injects one fault per data-bearing poll and never leaves two polls running at once, so the overlap needed a second carrier: it fails a poll at the client while letting that same request keep running on the relay, detached from the client's context, so the retry arrives with the abandoned poll still parked on the queue.

go build ./... && go vet ./... && go test ./...        # clean
go test -race ./internal/relay/ ./internal/httpconn/   # clean
GOOS=windows GOARCH=amd64 go build ./...               # clean
gofmt -l .                                             # empty

Not verified here

The real slow link. This reproduces the mechanism deterministically rather than by throttling, so the production 60 KB/s path to the Windows agent still wants one real 18.9 MB push after deploy.

🤖 Generated with Claude Code