From c3f30102e933842298c0fabed973976fb9307c65 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Mon, 10 Aug 2026 00:30:58 +0000 Subject: [PATCH 1/4] fix: count an accepted connection in the drain before its task starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a graceful drain promised that every accepted connection had finished. it did not keep that promise: `shutdown.enter()` ran inside the spawned connection task, so between the `spawn` and the task's first instruction a connection was invisible. a shutdown landing in that window saw nothing outstanding, `drain_default()` returned 0, and a client accepted moments earlier had its socket cut with no response and no trace. reproduced on both backends: accept one connection, spawn its task, drain immediately, and the drain reports a clean shutdown over a connection that was never served. the count now starts on the accept loop, in the same breath as the accept, and the task's existing `defer shutdown.leave()` is the other half of it. each of the six accept loops hands its socket over through a small named function that takes the concurrency permit and the drain count and then spawns — the permit was already taken there, so the two acquisitions now sit together, and the hand-off is a thing a test can drive. that leaves one direction to be wrong in, and it is the safe one. work that is counted and never runs holds its count up until the grace period expires, and `drain(deadline_ms)` already gives up at its deadline and returns what it abandoned. bounded and reported, against a connection dropped silently. --- docs/signals.md | 23 ++++- std/net/http2/server.pith | 172 ++++++++++++++++++++++++++++++++++---- std/shutdown.pith | 17 +++- std/web.pith | 133 ++++++++++++++++++++++++++--- 4 files changed, 310 insertions(+), 35 deletions(-) diff --git a/docs/signals.md b/docs/signals.md index b213005f..3239cd0b 100644 --- a/docs/signals.md +++ b/docs/signals.md @@ -117,7 +117,7 @@ and several subsystems compose without knowing about each other. | `request()` | begin a drain, from anywhere (a `/quit` route, a supervisor) | | `requested()` | whether a drain has begun — poll this in a long-lived handler | | `register_listener(fd)` / `close_listener(fd)` | listener registry; `close_listener` closes at most once | -| `enter()` / `leave()` | bracket one unit of in-flight work | +| `enter()` / `leave()` | bracket one unit of in-flight work — `enter()` on the accept loop, `leave()` from the task | | `expect_flush()` / `flush_done()` | a subsystem with shutdown work of its own | | `drain(deadline_ms)` | wait for both, returning what was left unfinished | | `set_drain_deadline(ms)` | the grace period every std server uses | @@ -142,6 +142,27 @@ all of them, and through the same coordinator: one that would otherwise never get there, to a `UNAVAILABLE` status rather than a cut connection. +### when a connection joins the count + +on the accept loop, in the same breath as the accept — not inside the task that +serves it. between a `spawn` and the spawned task's first instruction a +connection is invisible, and a drain landing in that window would see nothing +outstanding and return, cutting off a client that had already been accepted. +"every accepted connection finished" is the whole promise of a graceful drain, +so the count starts at the accept. + +that leaves only one direction to be wrong in, and it is the safe one: work that +is counted and then never runs holds its count up until the grace period +expires, and `drain()` returns it as work it abandoned. bounded, and reported — +where the other direction drops a connection silently. write your own accept +loops the same way: + +```pith +accepted := tcp.accept(fd) +shutdown.enter() +spawn serve(accepted.ok) # serve() opens with `defer shutdown.leave()` +``` + ## streams that never end a drain waits for work to finish, and a stream that never finishes would outlast diff --git a/std/net/http2/server.pith b/std/net/http2/server.pith index 93a79131..e400e435 100644 --- a/std/net/http2/server.pith +++ b/std/net/http2/server.pith @@ -1173,17 +1173,37 @@ pub fn serve_h2c_connection(fd: Int, handler: fn(http.HttpRequestBytes) -> http. conn.close() # serve_h2c_connection with the accept loop's concurrency permit released when -# the connection ends, however it ends. -# -# the same bracket doubles as the drain count: a graceful shutdown does not -# finish until every connection that reached here has run out, so a rolling -# deploy cannot sever a stream that is still exchanging frames. +# the connection ends, however it ends, and the drain count it was handed +# released with it. both were taken on the accept loop, in spawn_h2c_conn; this +# `defer` is the other half of each, and runs on every exit the task has. fn serve_h2c_slot(fd: Int, handler: fn(http.HttpRequestBytes) -> http.HttpResponse): - shutdown.enter() defer shutdown.leave() defer connection_slots.release() serve_h2c_connection(fd, handler) +# hand an accepted h2c socket to its own task, having taken the two things a +# connection holds for its whole life: the accept loop's concurrency permit and +# a place in the drain count. +# +# ## why the drain count is taken here and not inside the task +# +# between `spawn` and the spawned task's first instruction the connection is +# invisible. a shutdown that lands in that window drains to zero and returns, +# and the connection — accepted, acknowledged to the peer, never served — is cut +# off silently. "every accepted connection finished" is the whole promise of a +# graceful drain, so the count has to start at the accept, on this task, before +# the spawn. +# +# the failure mode this trades into is bounded and loud. a task that somehow +# never runs leaves its count up, and `drain()` gives up at the grace period and +# reports it as work it abandoned — the same number a rolling deploy already +# logs. counting too little loses a connection with no trace; counting too much +# costs at most the grace period and says so. +fn spawn_h2c_conn(fd: Int, handler: fn(http.HttpRequestBytes) -> http.HttpResponse): + connection_slots.acquire() + shutdown.enter() + spawn serve_h2c_slot(fd, handler) + # the connection lifecycle, factored out so the offline tests can drive it over a # ServerConn without a socket. reads the preface, sends the server SETTINGS, # spawns the writer, and runs the reader loop on the calling task; when the reader @@ -1308,8 +1328,7 @@ pub fn listen_h2c(host: String, port: Int, handler: fn(http.HttpRequestBytes) -> tcp.back_off_after_accept_failure(failures, accepted.err)! continue failures = 0 - connection_slots.acquire() - spawn serve_h2c_slot(accepted.ok, handler) + spawn_h2c_conn(accepted.ok, handler) # free the port before draining, not after: the replacement instance of a # rolling deploy is waiting to bind it, and the drain can take the whole # grace period. the defer above is then a no-op — close_listener closes at @@ -1354,11 +1373,19 @@ fn handshake_and_serve_h2_tls(listener_handle: Int, fd: Int, handler: fn(http.Ht # connection stops the server accepting after MAX_CONNECTIONS attempts, which is # the same denial of service the accept loop was fixed for, only slower. fn serve_h2_tls_slot(listener_handle: Int, fd: Int, handler: fn(http.HttpRequestBytes) -> http.HttpResponse): - shutdown.enter() defer shutdown.leave() defer connection_slots.release() handshake_and_serve_h2_tls(listener_handle, fd, handler) catch false +# hand an accepted tls socket to its own task, permit and drain count taken here +# on the accept loop — see spawn_h2c_conn for why the count cannot wait for the +# task to start. a connection still mid-handshake when a shutdown lands is a +# client waiting for an answer, so it counts from the accept like any other. +fn spawn_h2_tls_conn(listener_handle: Int, fd: Int, handler: fn(http.HttpRequestBytes) -> http.HttpResponse): + connection_slots.acquire() + shutdown.enter() + spawn serve_h2_tls_slot(listener_handle, fd, handler) + # bind host:port for http/2 over tls and serve forever. it builds a tls server # config from the pem `cert`/`key` files, offers alpn "h2" (and "http/1.1", which # a peer may pick but this server does not yet serve — see serve_h2_tls_connection), @@ -1394,8 +1421,7 @@ pub fn listen_h2_tls(host: String, port: Int, cert: String, key: String, handler tcp.back_off_after_accept_failure(failures, accepted.err)! continue failures = 0 - connection_slots.acquire() - spawn serve_h2_tls_slot(listener.handle, accepted.ok, handler) + spawn_h2_tls_conn(listener.handle, accepted.ok, handler) # free the port before draining (see listen_h2c above). release_tls_listener(listener) drained := shutdown.drain_default() @@ -1426,13 +1452,20 @@ pub fn serve_h2c_streaming_connection(fd: Int): run_connection(conn, streaming_unused_handler, true) catch false conn.close() -# serve_h2c_streaming_connection with the accept loop's permit released at the end. +# serve_h2c_streaming_connection with the accept loop's permit and drain count +# released at the end. mirrors serve_h2c_slot. fn serve_h2c_streaming_slot(fd: Int): - shutdown.enter() defer shutdown.leave() defer connection_slots.release() serve_h2c_streaming_connection(fd) +# hand an accepted h2c socket to its own streaming task, permit and drain count +# taken here on the accept loop. mirrors spawn_h2c_conn. +fn spawn_h2c_streaming_conn(fd: Int): + connection_slots.acquire() + shutdown.enter() + spawn serve_h2c_streaming_slot(fd) + # bind host:port for cleartext h2c and serve forever, handing each request # stream to `handler` while it is still arriving. mirrors listen_h2c. pub fn listen_h2c_streaming(host: String, port: Int, handler: fn(ServerStream) -> Int) -> Int!: @@ -1449,8 +1482,7 @@ pub fn listen_h2c_streaming(host: String, port: Int, handler: fn(ServerStream) - tcp.back_off_after_accept_failure(failures, accepted.err)! continue failures = 0 - connection_slots.acquire() - spawn serve_h2c_streaming_slot(accepted.ok) + spawn_h2c_streaming_conn(accepted.ok) # free the port before draining, not after: the replacement instance of a # rolling deploy is waiting to bind it, and the drain can take the whole # grace period. the defer above is then a no-op — close_listener closes at @@ -1477,11 +1509,17 @@ fn handshake_and_serve_h2_tls_streaming(listener_handle: Int, fd: Int) -> Bool!: # handshake_and_serve_h2_tls_streaming with the accept loop's permit released at # the end, on the failed-handshake path included. mirrors serve_h2_tls_slot. fn serve_h2_tls_streaming_slot(listener_handle: Int, fd: Int): - shutdown.enter() defer shutdown.leave() defer connection_slots.release() handshake_and_serve_h2_tls_streaming(listener_handle, fd) catch false +# hand an accepted tls socket to its own streaming task, permit and drain count +# taken here on the accept loop. mirrors spawn_h2_tls_conn. +fn spawn_h2_tls_streaming_conn(listener_handle: Int, fd: Int): + connection_slots.acquire() + shutdown.enter() + spawn serve_h2_tls_streaming_slot(listener_handle, fd) + # bind host:port for http/2 over tls and serve forever on the streaming path. # mirrors listen_h2_tls, handshake off the accept loop included. pub fn listen_h2_tls_streaming(host: String, port: Int, cert: String, key: String, handler: fn(ServerStream) -> Int) -> Int!: @@ -1509,8 +1547,7 @@ pub fn listen_h2_tls_streaming(host: String, port: Int, cert: String, key: Strin tcp.back_off_after_accept_failure(failures, accepted.err)! continue failures = 0 - connection_slots.acquire() - spawn serve_h2_tls_streaming_slot(listener.handle, accepted.ok) + spawn_h2_tls_streaming_conn(listener.handle, accepted.ok) # free the port before draining (see listen_h2c above). release_tls_listener(listener) drained := shutdown.drain_default() @@ -1947,3 +1984,102 @@ test "handle_streaming_stream parses the head and drives the streaming handler": assert(has_field(tail.fields, "x-done", "1")) # the stream was forgotten when the handler returned. assert_eq(server.active_count(), 0) + +# --- accept-time drain registration --- +# +# these drive the accept loop's hand-off (spawn_h2c_conn and its siblings) +# directly, over real sockets, without an accept loop: the loop's only other job +# is `tcp.accept`, and calling the hand-off by hand is what lets a test look at +# the drain count in the window between the spawn and the task's first +# instruction — the window the count exists to cover. + +# open `count` connected socket pairs against a listener on `port` and return the +# server-side fds. the client ends are pushed onto `clients`, so the test can +# close them and let the connection tasks finish; the listener is closed here, +# since every connection it will ever carry has already been accepted. +fn accepted_pairs(port: Int, count: Int, clients: List[Int]) -> List[Int]: + listener := tcp_listen("127.0.0.1", port) catch -1 + assert(listener > 0) + mut served: List[Int] := [] + mut i := 0 + while i < count: + connected := tcp.connect("127.0.0.1", port) + assert(not connected.is_err) + clients.push(connected.ok) + accepted := tcp.accept(listener) + assert(not accepted.is_err) + served.push(accepted.ok) + i = i + 1 + tcp_close(listener) + return served + +test "an accepted h2c connection is in the drain count before its task runs": + shutdown.reset() + mut clients: List[Int] := [] + served := accepted_pairs(18871, 8, clients) + for fd in served: + spawn_h2c_conn(fd, ok_handler) + # the count is taken on this task, so it is complete the instant the last + # hand-off returns — it cannot depend on how many of the eight tasks the + # runtime has got round to starting. with the count taken inside the task + # instead, most of these are still invisible here, and a shutdown landing + # now would drain to zero over eight unserved connections. + assert_eq(shutdown.inflight(), 8) + # none of them can finish (their peers have sent nothing and are still + # open), so a drain must give up at its deadline and report all eight rather + # than hang or claim a clean shutdown. + assert_eq(shutdown.drain(20), 8) + for c in clients: + tcp_close(c) + # each task leaves exactly once, on the path where the connection dies + # before its first frame: no double count, nothing left behind. + assert_eq(shutdown.drain(5000), 0) + assert_eq(shutdown.inflight(), 0) + shutdown.reset() + +test "an accepted h2c streaming connection is in the drain count before its task runs": + shutdown.reset() + stream_handler_fn = echo_stream_handler + mut clients: List[Int] := [] + served := accepted_pairs(18872, 4, clients) + for fd in served: + spawn_h2c_streaming_conn(fd) + assert_eq(shutdown.inflight(), 4) + for c in clients: + tcp_close(c) + assert_eq(shutdown.drain(5000), 0) + shutdown.reset() + +test "an accepted tls connection is in the drain count before its handshake starts": + shutdown.reset() + config := tls.server_config("tests/live/fixtures/localhost.crt", "tests/live/fixtures/localhost.key")!.with_alpn(["h2"]) + bound := tls.listen("127.0.0.1", 18873, config) + assert(not bound.is_err) + listener := bound.ok + mut clients: List[Int] := [] + mut served: List[Int] := [] + mut i := 0 + while i < 4: + # a plain tcp peer that never sends a client hello, so every handshake + # parks on its first read and no task can reach its `leave`. + connected := tcp.connect("127.0.0.1", 18873) + assert(not connected.is_err) + clients.push(connected.ok) + accepted := tls.accept_socket(listener) + assert(not accepted.is_err) + served.push(accepted.ok) + i = i + 1 + for fd in served: + spawn_h2_tls_conn(listener.handle, fd, ok_handler) + # a connection still mid-handshake is a client waiting for an answer, and it + # counts from the accept — not from whenever the runtime starts its task. + assert_eq(shutdown.inflight(), 4) + for c in clients: + tcp_close(c) + # the handshakes fail once their peers are gone; that path leaves the count + # exactly once too, so the drain finishes clean. + assert_eq(shutdown.drain(5000), 0) + assert_eq(shutdown.inflight(), 0) + release_tls_listener(listener) + config.close() + shutdown.reset() diff --git a/std/shutdown.pith b/std/shutdown.pith index b0f216c6..e028f2d5 100644 --- a/std/shutdown.pith +++ b/std/shutdown.pith @@ -23,8 +23,9 @@ ## `shutdown(2)` on each, which both stops new connections and wakes the ## waiting accept loop immediately — no polling for the flag. the loop still ## closes its own listener, so nothing closes an fd another task is using. -## - **in-flight work.** each connection task brackets itself with -## `enter()`/`leave()`, so `drain()` knows when the last one is done. +## - **in-flight work.** a server calls `enter()` on its accept loop, the moment +## a connection is accepted, and the task it hands that connection to calls +## `leave()` when it is done — so `drain()` knows when the last one is out. ## - **flushes.** a subsystem with its own shutdown work — the telemetry ## exporter is the one in std — calls `expect_flush()` up front and ## `flush_done()` when it has finished. `drain()` waits for those too, which @@ -157,8 +158,16 @@ pub fn listeners() -> Int: mu.unlock() return n -## mark one unit of in-flight work started. a server calls this as a connection -## task begins; `drain()` will not report a clean shutdown until it leaves. +## mark one unit of in-flight work started; `drain()` will not report a clean +## shutdown until it leaves. +## +## call it *before* handing the work to a task, not as the task's first act. +## between a `spawn` and the spawned task's first instruction the work is +## invisible: it has not entered, a drain landing in that window sees nothing +## outstanding and returns, and the connection it was about to serve is cut with +## no response and no trace. counting from the accept can only ever count too +## much — work that never starts — and that costs at most the grace period and +## is reported by `drain()`, where counting too little loses a client silently. pub fn enter(): mu.lock() inflight_count = inflight_count + 1 diff --git a/std/web.pith b/std/web.pith index dbd28f9b..56433a0a 100644 --- a/std/web.pith +++ b/std/web.pith @@ -417,17 +417,35 @@ fn dispatch(app: App, req: http.HttpRequestBytes) -> http.HttpResponse: # serve one accepted connection to completion on its own task, then close it. # errors are swallowed so a broken client only ever affects its own connection. # -# the enter/leave bracket is what a drain counts: listen() does not return until -# every connection that got this far has finished, so a rolling deploy cannot -# cut a response in half. `defer` releases the count on the error path too — a -# connection that leaks its count holds the whole shutdown open for the full -# grace period. +# the drain count this releases was taken at the accept, in spawn_conn. `defer` +# releases it on the error path too — a connection that leaks its count holds +# the whole shutdown open for the full grace period. fn handle_conn(app: App, fd: Int): - shutdown.enter() defer shutdown.leave() tcp_set_timeout(fd, CONNECTION_TIMEOUT_MS) http.serve_connection_fd(fd, fn(req: http.HttpRequestBytes) => dispatch(app, req)) catch 0 +# hand an accepted socket to its own task, counted in the drain from the moment +# it was accepted. +# +# ## why the count is taken here and not inside the task +# +# between `spawn` and the spawned task's first instruction the connection is +# invisible: it has not entered, a drain sees nothing outstanding and returns, +# and a client that was accepted moments before the shutdown request gets its +# socket cut with no response and no trace. listen() does not return until every +# connection that got *accepted* has finished, which is the promise a graceful +# drain makes; counting from the task's first instruction promises something +# weaker and quietly. +# +# the direction of the remaining error is deliberate. a task that never runs +# holds a count nobody releases, and `drain()` gives up at the grace period and +# returns it as work it abandoned — bounded, and reported. the other direction +# drops a connection silently. +fn spawn_conn(app: App, fd: Int): + shutdown.enter() + spawn handle_conn(app, fd) + # serve one accepted tls connection: native http/2 when the client negotiated # alpn "h2", http/1.1 over the same tls session otherwise. either way the request # funnels through dispatch, so routing, middleware, and observability are the same @@ -456,14 +474,19 @@ fn release_tls_listener(listener: tls.Listener): shutdown.close_listener(listener.handle) tls.release_listener_config(listener) -# the drain bracket lives here rather than around the handshake alone, so a -# connection counts as in flight from the moment it is accepted until it is -# fully served — a handshake interrupted mid-shutdown is still a client waiting. +# the whole of the tls connection, handshake included, under the drain count +# taken for it at the accept: a handshake interrupted mid-shutdown is still a +# client waiting. fn handle_tls_socket(app: App, listener_handle: Int, fd: Int): - shutdown.enter() defer shutdown.leave() handshake_and_serve_tls_conn(app, listener_handle, fd) catch false +# hand an accepted tls socket to its own task, counted in the drain from the +# accept — see spawn_conn for why the count cannot wait for the task to start. +fn spawn_tls_conn(app: App, listener_handle: Int, fd: Int): + shutdown.enter() + spawn handle_tls_socket(app, listener_handle, fd) + impl App: # register a GET route. returns a new App, so calls chain: # `web.new().get("/", home).get("/users/:id", show)`. @@ -541,7 +564,7 @@ impl App: tcp.back_off_after_accept_failure(failures, accepted.err)! continue failures = 0 - spawn handle_conn(app, accepted.ok) + spawn_conn(app, accepted.ok) # free the port before draining, not after: the replacement instance of # a rolling deploy is waiting to bind it, and the drain can take the # whole grace period. the defer above is then a no-op — close_listener @@ -593,7 +616,7 @@ impl App: tcp.back_off_after_accept_failure(failures, accepted.err)! continue failures = 0 - spawn handle_tls_socket(app, listener.handle, accepted.ok) + spawn_tls_conn(app, listener.handle, accepted.ok) # free the port before draining (see the plaintext listener above). release_tls_listener(listener) drained := shutdown.drain_default() @@ -991,3 +1014,89 @@ test "the circuit middleware trips on 5xx and recovers through its probe": # the half-open probe is a real request; a 200 closes the circuit. assert_eq(shielded(always_pong, req).status, 200) assert_eq(shielded(always_pong, req).status, 200) + +# --- accept-time drain registration --- +# +# these drive spawn_conn / spawn_tls_conn directly, over real sockets and +# without an accept loop, so the drain count can be read in the window between +# the spawn and the task's first instruction — the window the count exists to +# cover. an accept loop's only other job is tcp.accept, which is what these do +# by hand. + +# open `count` connected socket pairs against a listener on `port` and return the +# server-side fds. the client ends go onto `clients` so the test can close them +# and let the connection tasks finish. +fn accepted_pairs(port: Int, count: Int, clients: List[Int]) -> List[Int]: + listener := tcp_listen("127.0.0.1", port) catch -1 + assert(listener > 0) + mut served: List[Int] := [] + mut i := 0 + while i < count: + connected := tcp.connect("127.0.0.1", port) + assert(not connected.is_err) + clients.push(connected.ok) + accepted := tcp.accept(listener) + assert(not accepted.is_err) + served.push(accepted.ok) + i = i + 1 + tcp_close(listener) + return served + +test "an accepted connection is in the drain count before its task runs": + shutdown.reset() + app := new().get("/", always_pong) + mut clients: List[Int] := [] + served := accepted_pairs(18991, 8, clients) + for fd in served: + spawn_conn(app, fd) + # taken on this task, so the count is complete the instant the last hand-off + # returns, however many of the eight tasks have actually started. with the + # count taken inside the task, most of these are invisible here — and a + # shutdown landing now would report a clean drain over eight clients that + # were accepted and never answered. + assert_eq(shutdown.inflight(), 8) + # nothing can finish while the peers hold their sockets open and send + # nothing, so a drain gives up at its deadline and reports all eight. + assert_eq(shutdown.drain(20), 8) + for c in clients: + tcp_close(c) + # every task leaves exactly once, including on this path where the request + # never arrived: no double count and nothing left behind. + assert_eq(shutdown.drain(5000), 0) + assert_eq(shutdown.inflight(), 0) + shutdown.reset() + +test "an accepted tls connection is in the drain count before its handshake starts": + shutdown.reset() + app := new().get("/", always_pong) + config := tls.server_config("tests/live/fixtures/localhost.crt", "tests/live/fixtures/localhost.key")!.with_alpn(["h2", "http/1.1"]) + bound := tls.listen("127.0.0.1", 18992, config) + assert(not bound.is_err) + listener := bound.ok + mut clients: List[Int] := [] + mut served: List[Int] := [] + mut i := 0 + while i < 4: + # plain tcp peers that never send a client hello: every handshake parks + # on its first read, so no task can reach its `leave`. + connected := tcp.connect("127.0.0.1", 18992) + assert(not connected.is_err) + clients.push(connected.ok) + accepted := tls.accept_socket(listener) + assert(not accepted.is_err) + served.push(accepted.ok) + i = i + 1 + for fd in served: + spawn_tls_conn(app, listener.handle, fd) + # a connection still mid-handshake when a shutdown lands is a client waiting + # for an answer, so it counts from the accept like any other. + assert_eq(shutdown.inflight(), 4) + for c in clients: + tcp_close(c) + # the handshakes fail once their peers are gone, and that path leaves the + # count exactly once too. + assert_eq(shutdown.drain(5000), 0) + assert_eq(shutdown.inflight(), 0) + release_tls_listener(listener) + config.close() + shutdown.reset() From dfcf7f1ed1b778a310fb4a4e9004ca876ac80069 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Mon, 10 Aug 2026 00:38:19 +0000 Subject: [PATCH 2/4] fix: reword the accept-time count comment in std.web --- std/web.pith | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/std/web.pith b/std/web.pith index 56433a0a..94a52e22 100644 --- a/std/web.pith +++ b/std/web.pith @@ -434,9 +434,8 @@ fn handle_conn(app: App, fd: Int): # invisible: it has not entered, a drain sees nothing outstanding and returns, # and a client that was accepted moments before the shutdown request gets its # socket cut with no response and no trace. listen() does not return until every -# connection that got *accepted* has finished, which is the promise a graceful -# drain makes; counting from the task's first instruction promises something -# weaker and quietly. +# connection that got *accepted* has finished — that is the promise. counting +# from the task's first instruction keeps a weaker one, without saying so. # # the direction of the remaining error is deliberate. a task that never runs # holds a count nobody releases, and `drain()` gives up at the grace period and From 53a5eb0d64a7873f97f1113767a87ecf097bb119 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Mon, 10 Aug 2026 01:09:57 +0000 Subject: [PATCH 3/4] fix: keep a tls listener's config binding alive until the drain is over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a tls accept loop released its listener's binding to the server config the moment it stopped accepting, before draining. the binding is how tls.handshake finds the certificate and the private key, and a connection that was accepted but has not reached its handshake yet still needs it — so that connection's handshake failed and the client got nothing, on the one path a graceful shutdown exists to make clean. the window was always there; counting a connection from the accept opened it wide enough to be hit reliably, and CI hit it. proved by delaying the connection task 200ms before its handshake, which is what a task that has been spawned and not yet scheduled looks like: without this change the handshake fails with "tcp_read_bytes failed", with it the connection is served through the drain. the two halves of the teardown are now split by when they are safe. the listening socket is closed before the drain, because a rolling deploy's replacement is waiting to bind that port and the drain can take the whole grace period. the config binding is dropped after the drain, next to the config close, because every connection that borrowed it has finished by then. --- docs/tls.md | 25 +++++++++++++------ std/net/http2/server.pith | 23 ++++++++++------- std/web.pith | 17 +++++++------ .../cases/test_tls_server_config_release.pith | 7 ++++-- 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/docs/tls.md b/docs/tls.md index c0b01f17..a1de8dbb 100644 --- a/docs/tls.md +++ b/docs/tls.md @@ -56,22 +56,33 @@ transfer: `Listener.close()` gives the borrow back, and whoever called the timing is what makes a server different in practice. an accept loop hands each socket to its own task, and that task reads the certificate and the private key out of the registry when its own handshake reaches them — which can be a -whole handshake timeout after the accept that spawned it returned. so a server -closes its config once it has drained, never before: +whole handshake timeout after the accept that spawned it returned, and which for +a connection accepted just before the shutdown may not have happened at all yet. +so a server gives the borrow back and closes its config once it has drained, +never before: ```pith cfg := tls.server_config("certs/server.crt", "certs/server.key")! listener := tls.listen(host, 443, cfg)! # ... accept, spawning a task per connection, until a shutdown is requested ... -listener.close() +shutdown.close_listener(listener.handle) # free the port, and only the port drained := shutdown.drain_default() +tls.release_listener_config(listener) # give the borrow back cfg.close() ``` -closing before the drain does not corrupt anything — a handle is never reissued, -so the handshake that loses the race fails cleanly rather than reaching for -another config's key — but it does turn working connections into failed ones on -the way out, which is a worse shutdown than the leak it was fixing. +the two halves of `Listener.close()` are split on purpose, and the split is the +point. the socket goes first, because the replacement instance of a rolling +deploy is waiting to bind that port and the drain can take the whole grace +period. the listener's *binding* to the config goes last, because that binding +is how a handshake finds the certificate — dropping it while a connection is +still in the drain count is the same mistake as closing the config early, one +step removed. + +either mistake corrupts nothing — a handle is never reissued, so a handshake +that loses the race fails cleanly rather than reaching for another config's key +— but both turn working connections into failed ones on the way out, which is a +worse shutdown than the leak they were fixing. the std servers do this for you: `App.listen_tls`, `http2.listen_h2_tls` and its streaming twin each build a config, serve on it, and close it after their drain, diff --git a/std/net/http2/server.pith b/std/net/http2/server.pith index e400e435..2d1fa5ea 100644 --- a/std/net/http2/server.pith +++ b/std/net/http2/server.pith @@ -1422,13 +1422,17 @@ pub fn listen_h2_tls(host: String, port: Int, cert: String, key: String, handler continue failures = 0 spawn_h2_tls_conn(listener.handle, accepted.ok, handler) - # free the port before draining (see listen_h2c above). - release_tls_listener(listener) + # free the port before draining (see listen_h2c above), but only the port. + # the listener's binding to the tls config outlives the accept loop: a + # connection accepted a moment before the shutdown has not necessarily + # reached its handshake yet, and the handshake finds the certificate and the + # key through that binding. + shutdown.close_listener(listener.handle) drained := shutdown.drain_default() - # this call built the config, so this call closes it — and only here, once - # the drain has finished. the listener merely borrowed it, and each - # connection task reads the certificate and key out of the registry when its - # own handshake gets that far. see docs/tls.md. + # the drain is over, so every connection that borrowed the config has + # finished with it: drop the listener's binding, then close the config this + # call built. see docs/tls.md. + release_tls_listener(listener) config.close() return drained @@ -1548,10 +1552,11 @@ pub fn listen_h2_tls_streaming(host: String, port: Int, cert: String, key: Strin continue failures = 0 spawn_h2_tls_streaming_conn(listener.handle, accepted.ok) - # free the port before draining (see listen_h2c above). - release_tls_listener(listener) + # free the port before draining, and drop the listener's binding to the tls + # config only once the drain is over — see listen_h2_tls. + shutdown.close_listener(listener.handle) drained := shutdown.drain_default() - # this call built the config, so this call closes it — see listen_h2_tls. + release_tls_listener(listener) config.close() return drained diff --git a/std/web.pith b/std/web.pith index 94a52e22..a14519f1 100644 --- a/std/web.pith +++ b/std/web.pith @@ -616,14 +616,17 @@ impl App: continue failures = 0 spawn_tls_conn(app, listener.handle, accepted.ok) - # free the port before draining (see the plaintext listener above). - release_tls_listener(listener) + # free the port before draining (see the plaintext listener above), but + # only the port. the listener's binding to the tls config outlives the + # accept loop, because a connection accepted a moment before the + # shutdown has not necessarily reached its handshake yet — and the + # handshake finds the certificate and the key through that binding. + shutdown.close_listener(listener.handle) drained := shutdown.drain_default() - # this call built the config, so this call closes it — and only here, - # once the drain has finished. the listener merely borrowed it, and each - # connection task reads the certificate and key out of the registry when - # its own handshake gets that far, which is long after the accept that - # spawned it returned. see docs/tls.md. + # the drain is over, so every connection that borrowed the config has + # finished with it: drop the listener's binding, then close the config + # this call built. see docs/tls.md. + release_tls_listener(listener) config.close() return drained diff --git a/tests/cases/test_tls_server_config_release.pith b/tests/cases/test_tls_server_config_release.pith index 749f5733..06fae14f 100644 --- a/tests/cases/test_tls_server_config_release.pith +++ b/tests/cases/test_tls_server_config_release.pith @@ -6,8 +6,11 @@ # counted as in flight, *then* asks for a shutdown, and only then sends the # client hello. the server's connection task reads the certificate and the # private key out of the tls registry at that point — well after the accept -# loop stopped — so a listen loop that closed its config before draining -# would fail this handshake. +# loop stopped — so a listen loop that released its config before draining +# would fail this handshake. the count is taken at the accept, so the +# connection held open here is one whose task may not have run a single +# instruction yet: both the config *and* the listener's binding to it have +# to outlive the drain, not just the accept loop. # 2. the config is closed once the drain is over. tls.open_server_configs() # counts the configs still holding a certificate; it has to come back to # zero after the listen call returns, or the certificate pem and the key From 0f4263d4b6f2b1913027b962c94e521816405a12 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Mon, 10 Aug 2026 01:19:32 +0000 Subject: [PATCH 4/4] docs: register the listener in the tls shutdown example --- docs/tls.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/tls.md b/docs/tls.md index a1de8dbb..cd196f6a 100644 --- a/docs/tls.md +++ b/docs/tls.md @@ -64,6 +64,7 @@ never before: ```pith cfg := tls.server_config("certs/server.crt", "certs/server.key")! listener := tls.listen(host, 443, cfg)! +shutdown.register_listener(listener.handle) # ... accept, spawning a task per connection, until a shutdown is requested ... shutdown.close_listener(listener.handle) # free the port, and only the port drained := shutdown.drain_default()