fix: put the prometheus scrape endpoint under the drain - #692
Merged
Conversation
`serve()` ran an accept loop that std.shutdown could not see. it never registered its listener, so a shutdown request left the port accepting; it looped on `while true`, so it never returned and its `defer tcp_close(fd)` only ever ran on the back-off give-up path; and it took no drain count, so a scrape already in flight was invisible and `drain_default()` reported a clean shutdown over a response being written. the loop now follows the shape the six http/2 and web loops settled on: register the listener at the bind, test the shutdown flag at the top of the loop and on a failed accept, take the drain count on the accept loop rather than inside the spawned task, free the port before draining, and return the drain result. `serve_scrape` releases that count through `defer`, alongside the permit it already released. nothing is released after the drain: unlike a tls listener, a scrape borrows nothing from the listener — only its own socket and the process-wide metric registry, neither of which this call owns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
the
/metricsendpoint was the one accept loop in std that a graceful shutdowncould not see.
serve()bound its listener without registering it, looped onwhile true, and spawned each scrape without taking a drain count — so ashutdown request left the port accepting, and a scrape already mid-response was
invisible to
drain_default(), which reported a clean shutdown over it. becausethe loop never exited, its
defer tcp_close(fd)was reachable only on theback-off give-up path: the listener was never closed on shutdown at all.
the loop now follows the shape #691 settled on for the six http/2 and web loops.
the listener is registered at the bind, so
request()can stop it accepting andclose_listenerowns the close exactly once. the loop tests the shutdown flag atthe top and again on a failed accept.
spawn_scrapetakes the concurrency permitand the drain count together, on the accept loop, and
serve_scrapereleasesboth through
defer— including on the path where the scrape dies before itsrequest head arrives. the port is freed before the drain rather than after, and
serve()returns the drain result instead of an unreachable 0.nothing is released after the drain, because a scrape has no equivalent of the
tls config binding that #691 tripped over. it borrows only its own socket, which
it owns, and the process-wide metric registry, which
serve()does not own anddoes not tear down. the
Semaphorebound the loop already had is unchanged.testing the flag at the top of the loop turned out to be load-bearing rather than
cosmetic: a listener bound after
request()has run is not in the registryrequest()walked, so nothing will ever shut it down, and a loop that wentstraight into
accept()would park there for the rest of the process's life.that is a real race for
spawn prometheus.serve(...)against a SIGTERM atstartup, and it has its own test.
what was tested
the bug first, on both backends. a scrape parked mid-request (its head sent, its
blank line never), then a shutdown request and a drain:
byte-identical under
PITH_GREEN=0andPITH_GREEN=1: nothing registered,nothing counted, a clean drain reported instantly over a scrape still holding a
socket, and the port still answering. after the fix, the same program:
four new colocated tests in
std/prometheus.pith, all of them ordered offobservable state rather than off a sleep:
socket pairs are opened against a listener bound first, handed to
spawn_scrapeby hand, and the count read in exactly the window the bug livedin:
inflight() == 8the instant the last hand-off returns. the peers stayopen and silent, so no task can reach its
leaveand the number is exactrather than racy —
drain(20) == 8then shows the drain giving up at itsdeadline and reporting the work rather than claiming a clean shutdown. closing
the peers gives
drain(5000) == 0andinflight() == 0: every task leavesexactly once, on the path where the scrape dies before its request head.
request()endsthe accept loop on its own,
serve()returns 0,listeners()is back to 0 anda fresh connect to the port is refused.
before the bind, and
serve()returns without ever parking inaccept().answered in full and one that dies before sending a byte both settle back to
inflight() == 0. the drain rather than a bare read, because the count isreleased in a
deferthat need not have run when the client holds the body;and
inflight()checked separately afterwards, because a doubleleavedrivesthe count negative and
drain()reports that as 0 too.each was falsified, on both backends. with
enter()moved back insideserve_scrape, only the accept-time test fails, and identically either way:with
register_listenerdropped,3 passed, 4 failedon both. and withwhile not shutdown.requested()reverted towhile true, the file hangs until theharness kills it (exit 143 under
PITH_GREEN=0and=1) — the "never accepts"test catching exactly the park it exists for.
with the fix in place,
7 passed, 0 failedon both backends, and 15 consecutiveruns of the file on each with no failures.
make run-regressions-only: 332 passed, 0 failed.two test-only flakes fixed on the way past
test "serve answers several concurrent scrapes"failed once in about a dozenruns under
PITH_GREEN=1before any of this. it waitedtime.delay(200)for thelistener to bind, which on a loaded two-core box is not always enough — the
client then fails to connect and the assertion fails on an empty body. it now
waits on
shutdown.listeners()instead, which is only possible becauseserve()registers its listener now. the same helper is what the new tests use, so none of
them sleeps.
raw_scrapealso judged a response by a singleread, so a response splitacross two segments would have been assessed on its first. it now reads to the
end of the connection, which the server closes when it is done.
unrelated, and left alone
under
PITH_GREEN=1a socket read timeout does not bound a parked read. thescrape in the reproduction above holds its
tcp_set_timeout(fd, 2000)and stillsits there: on os threads the drain finished in 1709ms, under green the same
scrape held the count for a 4000ms grace period and then a 12000ms one, both to
the millisecond.
fdio.rs's green read path callswait_ready(fd, false, -1)—an unbounded reactor wait — where the os-thread path relies on
SO_RCVTIMEO. soevery
tcp_set_timeoutguard in std is inert under the green runtime. it ispre-existing, it is the safe direction here (the work is reported by the drain
rather than dropped), and it is not this change's to fix.