Skip to content

Drain guest responses before scale-to-zero - #369

Open
tnsardesai wants to merge 2 commits into
mainfrom
hypeship/guest-drain-hold
Open

Drain guest responses before scale-to-zero#369
tnsardesai wants to merge 2 commits into
mainfrom
hypeship/guest-drain-hold

Conversation

@tnsardesai

@tnsardesai tnsardesai commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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.

  • track every non-loopback HTTP response on the API, DevTools, and ChromeDriver listeners
  • retain the request's scale-to-zero hold until the handler returns and the response reaches a safe terminal state
  • poll Linux TIOCOUTQ after net/http finalizes explicit response framing
  • use adaptive asynchronous polling so keep-alive requests do not wait on a fixed polling interval
  • cancel an older idle drain when the next request starts, carrying its hold forward until the connection next drains
  • transfer older request holds only after the next request acquires its hold, bounding retained keep-alive state to one request without creating a scale-to-zero gap
  • apply rolling five-minute write deadlines to both ordinary writes and io.ReaderFrom/sendfile transfers by advancing them once per 1 MiB chunk
  • distinguish source read failures from socket write failures so truncated sources close gracefully instead of resetting queued bytes
  • flatten existing io.LimitedReader bounds so http.ServeContent and http.FileServer retain sendfile
  • release hijacked HTTP connections from response-tail tracking while retaining the handler-lifetime hold; subsequent WebSocket writes bypass response-drain recovery
  • preserve close-delimited HTTP semantics by duplicating the socket, sending FIN with shutdown(SHUT_WR), closing the Go connection promptly, and retaining the hold until TCP acknowledges the close
  • abort stalled or uninspectable sockets with SO_LINGER=0
  • recognize reset or TCP_USER_TIMEOUT sockets as terminal even when Linux retains a nonzero historical output-queue count
  • never reuse a raw Linux descriptor after attempting close(2), including when close reports EINTR
  • cap duplicated-socket close monitors at 256; reject excess monitors with an abortive close rather than allowing unbounded file-descriptor and goroutine growth
  • recover terminalization failures through bounded abort retries or duplicated-socket monitoring with TCP_USER_TIMEOUT; terminal duplication failures close the original Go connection before releasing its hold
  • if no safe terminal state can be established, terminate the whole guest without releasing the hold
  • terminate failed-disable requests with http.ErrAbortHandler, preventing net/http from synthesizing a successful response for an operation that never ran
  • limit guest-termination recovery to failures that own a response hold; untracked protocol-error and hijacked writes use best-effort connection cleanup
  • attribute buffered and handler-visible write failures to one write_error outcome
  • expose drain outcome counters, active and fail-closed hold gauges, active close monitors, and monitor-limit rejections

The 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/api
  • go test -race $(go list ./... | grep -v '/e2e$')
  • 50 repeated race-enabled lifecycle, WebSocket, shutdown, framing, keep-alive, slow-reader, timeout, and terminal-recovery tests
  • progressive ordinary and sendfile transfers run longer than their configured timeout while refreshing the deadline for every chunk
  • peer reset with queued bytes exits closed-socket monitoring immediately
  • a raw close reporting EINTR is terminal and the descriptor is never inspected or closed again
  • terminal duplication failure closes the original TCP descriptor, releases its monitor slot, and restores hold gauges
  • post-hijack and pre-registration write failures cannot acquire a close monitor or terminate the guest
  • source read failures preserve graceful close semantics while socket write failures remain abortive
  • nested read limits retain the Linux sendfile path
  • monitor saturation keeps active holds and duplicated sockets within the configured cap
  • buffered finishRequest and handler-visible write failures each emit exactly one write_error outcome
  • 1,000 pipelined requests on one connection retain one pending hold, never physically re-enable between requests, complete every response, and return all gauges to baseline after the final drain
  • persistent abort, socket duplication, linger, inspection, and TCP_USER_TIMEOUT failures reach bounded guest termination without releasing the hold
  • Darwin amd64 and Linux arm64 compile checks for the scale-to-zero package
  • production-runtime A/B with a 16 MiB response and approximately 200 KiB/s reader: the unchanged image stalled 106,496 bytes short, while patched fresh guests repeatedly delivered the complete checksum without a caller-side keepalive or wake fallback
  • a patched guest resumed from standby, completed the same slow transfer, and returned to standby afterward

Note

High Risk
Changes core HTTP serving and scale-to-zero lifecycle for all external traffic, with Linux-specific socket recovery that can abort connections or terminate the entire guest on persistent failures.

Overview
Fixes premature scale-to-zero by keeping the guest awake until each external HTTP response is safely finished on the wire, not merely when the handler returns.

Scale-to-zero middleware now acquires a per-request hold at Disable, releases it only after both the handler finishes and the connection reaches a terminal drain outcome, then calls Enable. It tracks TCP via scaletozero.Serve on the API, DevTools, and ChromeDriver listeners (metrics stay on plain ListenAndServe). Untracked connections or failed Disable abort with http.ErrAbortHandler instead of returning 500.

Response draining polls Linux TIOCOUTQ after net/http goes idle, with adaptive polling, rolling write deadlines per 1 MiB chunk (including ReadFrom/sendfile), keep-alive hold handoff (one pending drain per connection), and hijacked/WebSocket paths that skip tail tracking. Close-delimited responses duplicate the socket, SHUT_WR, and monitor TCP_INFO until close is acknowledged; stalled paths use abortive close, capped (256) close monitors, bounded recovery, and guest termination (SIGTERM/SIGKILL to PID 1) if the hold cannot be released safely.

Observability: new Prometheus metrics for drain outcomes, active/fail-closed holds, close monitors, and monitor rejections via ResponseDrainCollector.

Reviewed by Cursor Bugbot for commit a76f270. Bugbot is set up for automated code reviews on this repo. Configure here.

@tnsardesai
tnsardesai force-pushed the hypeship/guest-drain-hold branch 2 times, most recently from 831acb0 to 16df82d Compare September 6, 2026 03:37
@tnsardesai tnsardesai changed the title Wait for streaming responses to drain Drain guest responses before scale-to-zero Sep 6, 2026
@tnsardesai
tnsardesai force-pushed the hypeship/guest-drain-hold branch 4 times, most recently from 4a3f7d2 to 616817c Compare September 8, 2026 08:16

@Sayan- Sayan- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Close waits 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 unsupported Expect) 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).

@tnsardesai
tnsardesai force-pushed the hypeship/guest-drain-hold branch 3 times, most recently from 91886a1 to ee50103 Compare September 10, 2026 18:37
@tnsardesai
tnsardesai force-pushed the hypeship/guest-drain-hold branch from ee50103 to a1d8298 Compare September 10, 2026 18:58
@tnsardesai
tnsardesai marked this pull request as ready for review September 10, 2026 19:32
@tnsardesai
tnsardesai requested a review from Sayan- September 10, 2026 19:32

@Sayan- Sayan- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for iterating! overall looks good. wanted to confirm if you want to fix these before merging

  • p2, the new response-drain metrics do not retain their intended dimensions through production collection. The browser-VM telemetry pipeline drops the outcome label before aggregation, and guest_termination is incremented immediately before shutdown closes the metrics listener. SigNoz therefore cannot distinguish drain outcomes or reliably observe guest termination, although guest and VMM logs retain separate evidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants