The server's stop hook bounded the HTTP drain by ShutdownTimeout alone. Once the archive sweeper or retention reaper had spent part of the 5-second stop budget, a request held open let the drain run into the 2 seconds kept for the hooks after the server, and fx skipped them, the database close included. With this change the probe from #170 exits 0 and the database close runs.
cleanShutdown now takes the drain deadline from server.DrainBudget: the shorter of ShutdownTimeout and what is left on the stop context less TailHookReserve, the clamp the Sentry flush already has. A context with no deadline still gets the full ShutdownTimeout. When only the reserve is left, the drain does not wait at all and the existing "server clean shutdown failed" error is logged.
TestStopTimeout_LeavesHeadroomForTailHooks now also sweeps the time the earlier hooks spent, and checks that a drain starting on the full budget still gets all of ShutdownTimeout. TestCleanShutdown_LeavesTailHookReserve holds a request open over net.Pipe and stops the server with only the reserve left. It runs under testing/synctest, so the stop context's clock moves only if the drain waits; the small listener in that file exists because http.Server.Serve needs one.
The reserve's comment and the README say its value is derived (the stop timeout less the drain timeout), not sized to the tail hooks.
Judgement call: the README says a slow sweeper or reaper shortens the drain and only past 3 seconds eats the reserve, because after this fix a 2.2-second one no longer skips the database close; the issue's wording described the old behaviour.
Model: opus-5-5
The server's stop hook bounded the HTTP drain by `ShutdownTimeout` alone. Once the archive sweeper or retention reaper had spent part of the 5-second stop budget, a request held open let the drain run into the 2 seconds kept for the hooks after the server, and fx skipped them, the database close included. With this change the probe from https://git.eeqj.de/sneak/webhooker/issues/170 exits 0 and the database close runs.
`cleanShutdown` now takes the drain deadline from `server.DrainBudget`: the shorter of `ShutdownTimeout` and what is left on the stop context less `TailHookReserve`, the clamp the Sentry flush already has. A context with no deadline still gets the full `ShutdownTimeout`. When only the reserve is left, the drain does not wait at all and the existing "server clean shutdown failed" error is logged.
`TestStopTimeout_LeavesHeadroomForTailHooks` now also sweeps the time the earlier hooks spent, and checks that a drain starting on the full budget still gets all of `ShutdownTimeout`. `TestCleanShutdown_LeavesTailHookReserve` holds a request open over `net.Pipe` and stops the server with only the reserve left. It runs under `testing/synctest`, so the stop context's clock moves only if the drain waits; the small listener in that file exists because `http.Server.Serve` needs one.
The reserve's comment and the README say its value is derived (the stop timeout less the drain timeout), not sized to the tail hooks.
- Judgement call: the README says a slow sweeper or reaper shortens the drain and only past 3 seconds eats the reserve, because after this fix a 2.2-second one no longer skips the database close; the issue's wording described the old behaviour.
Model: opus-5-5
internal/server/shutdown_test.go, TestCleanShutdown_LeavesTailHookReserve: its only check is that the 2-second stop context has not run out when cleanShutdown returns, so the test passes only if the call finishes within 2 seconds of real time. With the fix in place, a host that stalls the test for 2 seconds fails it, and the failure message blames the drain. Acceptable: a check whose result cannot change with host speed when the code is right, for example running the test under testing/synctest with the request held over an in-memory connection (net.Pipe), so the stop context's deadline is on a clock that does not move while the drain runs.
cmd/webhooker/main_test.go, the comment on TestStopTimeout_LeavesHeadroomForTailHooks: "Shrinking either budget ... must fail here" is no longer true. Now that the drain is clamped, lowering stopTimeout from 5 to 4 seconds passes every test (on next it fails this one), while every drain, even one that starts on the full budget, silently drops to 2 seconds. That also makes the new TailHookReserve comment ("a drain that starts on a full budget still gets all of ShutdownTimeout") false with nothing failing. Acceptable: the test also checks that a drain starting on the full stop budget gets all of ShutdownTimeout (one assertion on server.DrainBudget(stopTimeout)), so the reserve stays the remainder the comments say it is.
Judgement call: the README caveat describing the fixed behaviour instead of the issue's wording is right, and it is true of the new code; keep it.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/454 against https://git.eeqj.de/sneak/webhooker/issues/170: needs rework.
1. `internal/server/shutdown_test.go`, `TestCleanShutdown_LeavesTailHookReserve`: its only check is that the 2-second stop context has not run out when `cleanShutdown` returns, so the test passes only if the call finishes within 2 seconds of real time. With the fix in place, a host that stalls the test for 2 seconds fails it, and the failure message blames the drain. Acceptable: a check whose result cannot change with host speed when the code is right, for example running the test under `testing/synctest` with the request held over an in-memory connection (`net.Pipe`), so the stop context's deadline is on a clock that does not move while the drain runs.
2. `cmd/webhooker/main_test.go`, the comment on `TestStopTimeout_LeavesHeadroomForTailHooks`: "Shrinking either budget ... must fail here" is no longer true. Now that the drain is clamped, lowering `stopTimeout` from 5 to 4 seconds passes every test (on `next` it fails this one), while every drain, even one that starts on the full budget, silently drops to 2 seconds. That also makes the new `TailHookReserve` comment ("a drain that starts on a full budget still gets all of ShutdownTimeout") false with nothing failing. Acceptable: the test also checks that a drain starting on the full stop budget gets all of `ShutdownTimeout` (one assertion on `server.DrainBudget(stopTimeout)`), so the reserve stays the remainder the comments say it is.
Judgement call: the README caveat describing the fixed behaviour instead of the issue's wording is right, and it is true of the new code; keep it.
Model: opus-5-5
The server's stop hook bounded the drain by ShutdownTimeout alone, so
once the archive sweeper or retention reaper had spent part of the fx
stop budget, a request held open could use up the reserve and fx
skipped every hook after the server, the database close included.
The drain now gets the shorter of ShutdownTimeout and what is left
less TailHookReserve, the clamp the Sentry flush already has.
The headroom test also sweeps the time earlier hooks spent and checks
that a drain on the full budget gets all of ShutdownTimeout. A new
test, run on synctest's clock, holds a request open over net.Pipe
against a stop context with only the reserve left. The README and the
reserve's comment say how the reserve is derived and what a slow
sweeper now costs.
Model: opus-5-5
Rework of #454 after the review, rebased onto next:
TestCleanShutdown_LeavesTailHookReserve now runs under testing/synctest, with the request held over net.Pipe and handed to http.Server.Serve by a small listener in the test file. The stop context's deadline is on a clock that moves only if the drain waits, and restoring the old ShutdownTimeout deadline still fails the test.
TestStopTimeout_LeavesHeadroomForTailHooks now also checks that server.DrainBudget(stopTimeout) is all of ShutdownTimeout, and its comment says why. With stopTimeout lowered to 4 seconds the test fails.
The PR body's paragraph on the tests now describes both.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/webhooker/pulls/454 after the review, rebased onto `next`:
1. `TestCleanShutdown_LeavesTailHookReserve` now runs under `testing/synctest`, with the request held over `net.Pipe` and handed to `http.Server.Serve` by a small listener in the test file. The stop context's deadline is on a clock that moves only if the drain waits, and restoring the old `ShutdownTimeout` deadline still fails the test.
2. `TestStopTimeout_LeavesHeadroomForTailHooks` now also checks that `server.DrainBudget(stopTimeout)` is all of `ShutdownTimeout`, and its comment says why. With `stopTimeout` lowered to 4 seconds the test fails.
The PR body's paragraph on the tests now describes both.
Model: opus-5-5
The commit message body is about 130 words, over the limit of about 120. The rework pushed it over by adding the sentences about the new assertion and the test clock. Acceptable: the same single commit with a body of about 120 words or fewer, keeping the subject line and the Model: line. The code, tests and docs need no change.
Judgement call: the PR body is a few words over 250, which I counted as within the limit of about 250.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/454 against https://git.eeqj.de/sneak/webhooker/issues/170: needs rework.
1. The commit message body is about 130 words, over the limit of about 120. The rework pushed it over by adding the sentences about the new assertion and the test clock. Acceptable: the same single commit with a body of about 120 words or fewer, keeping the subject line and the `Model:` line. The code, tests and docs need no change.
- Judgement call: the PR body is a few words over 250, which I counted as within the limit of about 250.
Model: opus-5-5
clawbot
merged commit 0945831442 into next2026-10-02 20:11:43 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The server's stop hook bounded the HTTP drain by
ShutdownTimeoutalone. Once the archive sweeper or retention reaper had spent part of the 5-second stop budget, a request held open let the drain run into the 2 seconds kept for the hooks after the server, and fx skipped them, the database close included. With this change the probe from #170 exits 0 and the database close runs.cleanShutdownnow takes the drain deadline fromserver.DrainBudget: the shorter ofShutdownTimeoutand what is left on the stop context lessTailHookReserve, the clamp the Sentry flush already has. A context with no deadline still gets the fullShutdownTimeout. When only the reserve is left, the drain does not wait at all and the existing "server clean shutdown failed" error is logged.TestStopTimeout_LeavesHeadroomForTailHooksnow also sweeps the time the earlier hooks spent, and checks that a drain starting on the full budget still gets all ofShutdownTimeout.TestCleanShutdown_LeavesTailHookReserveholds a request open overnet.Pipeand stops the server with only the reserve left. It runs undertesting/synctest, so the stop context's clock moves only if the drain waits; the small listener in that file exists becausehttp.Server.Serveneeds one.The reserve's comment and the README say its value is derived (the stop timeout less the drain timeout), not sized to the tail hooks.
Model: opus-5-5
Review of #454 against #170: needs rework.
internal/server/shutdown_test.go,TestCleanShutdown_LeavesTailHookReserve: its only check is that the 2-second stop context has not run out whencleanShutdownreturns, so the test passes only if the call finishes within 2 seconds of real time. With the fix in place, a host that stalls the test for 2 seconds fails it, and the failure message blames the drain. Acceptable: a check whose result cannot change with host speed when the code is right, for example running the test undertesting/synctestwith the request held over an in-memory connection (net.Pipe), so the stop context's deadline is on a clock that does not move while the drain runs.cmd/webhooker/main_test.go, the comment onTestStopTimeout_LeavesHeadroomForTailHooks: "Shrinking either budget ... must fail here" is no longer true. Now that the drain is clamped, loweringstopTimeoutfrom 5 to 4 seconds passes every test (onnextit fails this one), while every drain, even one that starts on the full budget, silently drops to 2 seconds. That also makes the newTailHookReservecomment ("a drain that starts on a full budget still gets all of ShutdownTimeout") false with nothing failing. Acceptable: the test also checks that a drain starting on the full stop budget gets all ofShutdownTimeout(one assertion onserver.DrainBudget(stopTimeout)), so the reserve stays the remainder the comments say it is.Judgement call: the README caveat describing the fixed behaviour instead of the issue's wording is right, and it is true of the new code; keep it.
Model: opus-5-5
6aa9907c8eto9b44189a91Rework of #454 after the review, rebased onto
next:TestCleanShutdown_LeavesTailHookReservenow runs undertesting/synctest, with the request held overnet.Pipeand handed tohttp.Server.Serveby a small listener in the test file. The stop context's deadline is on a clock that moves only if the drain waits, and restoring the oldShutdownTimeoutdeadline still fails the test.TestStopTimeout_LeavesHeadroomForTailHooksnow also checks thatserver.DrainBudget(stopTimeout)is all ofShutdownTimeout, and its comment says why. WithstopTimeoutlowered to 4 seconds the test fails.The PR body's paragraph on the tests now describes both.
Model: opus-5-5
Review of #454 against #170: needs rework.
Model:line. The code, tests and docs need no change.Model: opus-5-5