Drain guest responses before scale-to-zero - #369
Conversation
831acb0 to
16df82d
Compare
8c3356d to
4a3f7d2
Compare
4a3f7d2 to
616817c
Compare
Sayan-
left a comment
There was a problem hiding this comment.
Static review of the response drain at 616817c. Comment only, no verdict. Items are ranked by severity. The last two entries are scope notes rather than defects. No fixes proposed here; this is intended as input for follow-up work.
| # | Severity | Area | Concern | Where |
|---|---|---|---|---|
| 1 | Medium | Protocol / resource | A peer RST (or TCP_USER_TIMEOUT expiry) on the duplicated, half-closed socket leaves the kernel socket in TCP_CLOSE with TIOCOUTQ still nonzero. waitForClosedResponse requires queued == 0 && acked, so it never releases early and retains the hold, the dup fd and the goroutine for the full 5 minute timeout. The drain is then recorded as timeout although the socket was definitively dead. |
connection.go:511-534, socket_linux.go:16-18 and 43-59 |
| 2 | Medium | Correctness | drainConn.Write sets one absolute deadline per Write call. A WriteTo-backed body (bytes.Reader wrapped in io.NopCloser) reaches the connection as a single Write of the whole body, so a slow but progressing transfer that runs longer than 5 minutes is reset. DownloadDirZip is the affected endpoint. The per 1 MiB refresh exists only in ReadFrom, so the PR description's "rolling" deadline does not hold on this path. |
connection.go:68-93 vs 95-131, fs.go:859 |
| 3 | Medium-low | Observability | The deferred writeError check runs when the middleware returns, before net/http finishRequest, which is where unflushed responses (roughly anything under 2 KiB) first touch the socket. Write failures there surface as connection_closed at debug level and are never counted as write_error. Conversely, a write failure inside the handler is counted twice: connection_closed via abortNow and write_error via the deferred check. |
middleware.go:225-230 and 156-158; go server.go:2109 and 2116 |
| 4 | Low-medium | Correctness | Every non-EOF error from TCPConn.ReadFrom is treated as a write error, including source-side read errors (pipe CloseWithError, sendfile EIO). The abortive linger-0 close purges already-queued response bytes and sends RST where the pre-PR behavior was a graceful FIN with a truncated body. Affects DownloadDirZstd, TakeScreenshot, ReadFile. SSE responses are unaffected because the generated visit method takes the Flusher path. |
connection.go:117-126 |
| 5 | Low | Resource | There is no cap on concurrent close-delimited monitors. Each stalled close holds one fd and one goroutine for up to the timeout. When F_DUPFD_CLOEXEC fails with EMFILE, Close takes the abortive path and RSTs the response tail. This is the aggregate consequence of item 1. |
connection.go:330-349 |
| 6 | Low | Efficiency | drainConn.ReadFrom wraps a second io.LimitedReader around the one io.CopyN already created. sendFile and spliceFrom unwrap a single layer, so http.ServeContent and http.FileServer now fall back to a 32 KiB userspace copy. Only /extensions/* is affected; *os.File bodies still reach sendfile. |
connection.go:116; go net/sendfile.go:27-39 |
Scope notes (intentional or pre-existing, listed for completeness):
- Hijack completes every pending drain immediately, including a previous request's carried-forward drain. Once a WebSocket handler returns, nothing covers an unacked close frame or FIN. This matches the PR description and connection_test.go:24-83. Residual exposure is small: coder/websocket
Closewaits for the peer's close frame before returning, and the cooldown follows release. Same as pre-PR behavior. - Responses that net/http produces before entering the handler (
OPTIONS *, 400/431/501 parse errors, 417 for unsupportedExpect) never acquire a hold. Pre-existing and inherent to middleware placement. chi routes 404/405 through the chain, so those do get holds.
Evidence for item 1 (Linux v6.12): tcp_reset calls tcp_done_with_error, which runs tcp_write_queue_purge and tcp_done; neither touches snd_una or write_seq (tcp_input.c:4509-4550, tcp.c:3254-3268 and 4847-4871). tcp_ioctl SIOCOUTQ zeroes only SYN_SENT/SYN_RECV and otherwise returns write_seq - snd_una (tcp.c:636-644). tcp_write_err takes the same path (tcp_timer.c:75-79), and the TCP_USER_TIMEOUT value equals the poll timeout, so it cannot shorten detection. The keep-alive idle path is fine: an RST arriving before Close makes shutdown return ENOTCONN, which isTerminalConnectionError accepts.
Evidence for item 2 (Go 1.25): io.NopCloser returns nopCloserWriterTo when the reader implements WriterTo (io.go:682-687); bytes.Reader.WriteTo issues one Write for the remaining slice (bytes/reader.go:137-153); both bufio layers pass a large slice straight through once the buffer is empty (bufio.go:679-686); FD.Write loops over the whole slice under the absolute deadline (internal/poll/fd_unix.go:360-401). middleware_test.go:332-365 exercises this shape and asserts that the handler errors.
Checked and found not to be issues: long-lived SSE streams under the 1 MiB chunk deadline (the Flusher path refreshes the deadline per write), and hold leaks from a stalled partial next request (StateActive fires only after a complete header block is read).
problem
An HTTP handler can return while response bytes remain unacknowledged in the guest TCP send queue. Re-enabling scale-to-zero at that point can suspend the guest before the peer receives the response tail.
change
This implements the fix entirely inside the guest image. It requires no caller or control-plane protocol changes.
TIOCOUTQafter net/http finalizes explicit response framingio.ReaderFrom/sendfile transfers by advancing them once per 1 MiB chunkshutdown(SHUT_WR), closing the Go connection promptly, and retaining the hold until TCP acknowledges the closeSO_LINGER=0TCP_USER_TIMEOUT; if no safe terminal state can be established, terminate the whole guest without releasing the holdhttp.ErrAbortHandler, preventing net/http from synthesizing a successful response for an operation that never ranThe release invariant is: response data and required close framing were acknowledged outside the guest, or the connection was abortively terminated. A timeout, inspection failure, or cleanup failure alone never releases the hold.
tests
go build ./...go vet ./...go test -race ./lib/scaletozero ./lib/metrics ./cmd/api/apigo test -race $(go list ./... | grep -v '/e2e$')TCP_USER_TIMEOUTfailures reach bounded guest termination without releasing the hold