Skip to content

fix: count an accepted connection in the drain before its task starts - #691

Merged
kacy merged 4 commits into
mainfrom
fix-drain-accept-registration
Aug 10, 2026
Merged

fix: count an accepted connection in the drain before its task starts#691
kacy merged 4 commits into
mainfrom
fix-drain-accept-registration

Conversation

@kacy

@kacy kacy commented Aug 10, 2026

Copy link
Copy Markdown
Owner

a graceful drain promised that every accepted connection had finished, and did
not keep it. shutdown.enter() ran inside the spawned connection task, so
between the spawn and that task's first instruction a connection was
invisible: it had not entered, drain_default() saw nothing outstanding and
returned, and a client accepted moments before the shutdown request had its
socket cut with no response and no trace.

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 on the loop, so the two acquisitions now sit together, and the
hand-off becomes a thing a test can drive — which is the only way to look at the
count in the window between a spawn and a task's first instruction.

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.

the six loops were the whole surface. nothing else in std pairs enter() with
leave(), and the one other subsystem that registers with the drain,
std.obs's exporter, already calls expect_flush() before its spawn rather
than inside the task.

what was tested

the bug first, on both backends. one connection accepted and spawned, then
drain_default() immediately:

inflight right after the spawn: 0
drain_default immediately: 0

identical under PITH_GREEN=0 and PITH_GREEN=1 — a clean drain reported over
a connection that was never served. with eight connections the count read 2 of 8
on os threads and 1 of 8 under green, so most of a burst is invisible.

five new colocated tests, three in std/net/http2/server.pith (h2c, h2c
streaming, tls) and two in std/web.pith (plaintext, tls). each opens real
socket pairs, hands them to the accept loop's spawn function by hand, and reads
the count in exactly the window the bug lived in. the peers send nothing and
stay open, so no task can reach its leave and the expected count is exact
rather than a race:

  • assert_eq(shutdown.inflight(), 8) right after the hand-offs — the count is
    complete when the last hand-off returns, however many tasks have started.
  • assert_eq(shutdown.drain(20), 8) — with nothing able to finish, the drain
    gives up at its deadline and reports the work rather than hanging or claiming
    a clean shutdown.
  • the peers are then closed, and assert_eq(shutdown.drain(5000), 0) plus
    assert_eq(shutdown.inflight(), 0) — every task leaves exactly once, on the
    path where the connection dies before its first frame or mid-handshake. no
    double count, nothing left behind.

no sleeps anywhere: the listener is bound before anything connects to it, and
every step is ordered off shutdown.inflight().

each test was falsified. with enter() moved back inside the slot functions all
five fail, on both backends:

PITH_GREEN=0  server.pith  25 passed, 3 failed    web.pith  23 passed, 2 failed
PITH_GREEN=1  server.pith  25 passed, 3 failed    web.pith  23 passed, 2 failed

and with the fix restored, 28 passed, 0 failed and 25 passed, 0 failed on
both backends, plus std/shutdown.pith at 17/17 on both.

make run-regressions-only: 332 passed, 0 failed.

docs: docs/signals.md gains a section on when a connection joins the count and
why the accept is the right place, std.shutdown's enter() doc says to call it
before handing work to a task, and the module header describes the two halves
where they now live.

a second bug the first one uncovered

CI caught test_tls_server_config_release failing under os threads, and it was
a real defect rather than a flake. a tls accept loop released its listener's
binding to the server config the moment it stopped accepting, before draining.
that binding is how tls.handshake finds the certificate and the private key,
so a connection that had been accepted but had not reached its handshake yet
lost them: the 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 — the same accept-time count made an existing
test into a real guard for it. the two halves of the teardown are now split by
when each is safe: the listening socket is closed before the drain, because a
rolling deploy's replacement is waiting to bind that port, and the config
binding is dropped after the drain, next to the config close, once every
connection that borrowed it has finished. docs/tls.md showed the old ordering
in its example and now shows this one.

proved by delaying the connection task 200 ms before its handshake — which is
what a task that has been spawned and not yet scheduled looks like:

# before
server took the connection: true
handshake after the shutdown request: false
handshake error: tcp_read_bytes failed

# after
handshake after the shutdown request: true
open server configs during the drain: 1
served during the drain: HTTP/1.1 200 OK
open server configs after: 0

make run-regressions-only re-run on top: 332 passed, 0 failed. the tls, http/2,
websocket, grpc-shutdown and web cases were also run individually under both
backends.

kacy added 4 commits August 10, 2026 00:30
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.
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.
@kacy
kacy merged commit 6713a67 into main Aug 10, 2026
2 checks passed
@kacy
kacy deleted the fix-drain-accept-registration branch August 10, 2026 01:55
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.

1 participant