Found by the independent review of #159, which proved it with a probe but correctly declined to file it as a defect of that PR. Deliberately NOT milestoned 1.0.0: the pre-change behaviour was strictly worse (no bound at all, so SIGKILL at 10s), and SQLite is crash-safe.
#134 clamped the Sentry flush to the remaining stop budget less TailHookReserve (2s). The HTTP drain is not clamped the same way: cleanShutdown uses context.WithTimeout(ctx, ShutdownTimeout), i.e. min(3s, whatever is left), with no reserve subtracted. So the 2s reserve is only guaranteed when the hooks that run BEFORE the server are fast.
Demonstrated: patching the ArchiveSweeper stop hook to take 2.2s and holding a request open gives exit 1 with the tail hooks, database close included, skipped — while the server hook itself stays inside its own 3s drain budget. The budget gets eaten jointly rather than by any single hook.
That is inside the family the README discloses, but the disclosure is worded as a wedged reaper consuming "the whole budget on its own", and 2.2s out of 5s already suffices. So the README is optimistic about the size of the wedge required.
Definition of done
cleanShutdown derives the drain deadline as min(ShutdownTimeout, remaining - TailHookReserve), the same shape already applied to the flush, so the reserve survives slow preceding hooks.
TestStopTimeout_LeavesHeadroomForTailHooks is extended to model a non-zero preceding-hook consumption, not just a drain against a full budget.
The README caveat is tightened: a slow ArchiveSweeper or RetentionReaper is enough, not only one that consumes the whole budget.
The probe above exits 0 with the database close running.
Worth deciding while you are in there
TailHookReserve = 2s is currently the residual of stopTimeout - ShutdownTimeout, but the code and README treat it as a named constant chosen on its own merits. Measured tail hooks are 55.101us, 395ns, 2.168us and 1.43ms — about a thousandth of the reserve. Either pick the value deliberately and say why, or document that it is derived. A smaller reserve would buy the flush more room, though the 2s nominal cap is the binding constraint for drains under 1s anyway.
Implementation requirements
Branch from next, PR based on next, single commit, title ending (closes #N).
Run make bootstrap (or make assets) in a fresh clone before gating — browser assets are fetched at build time and not committed.
Gate on make check plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.
Found by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/159, which proved it with a probe but correctly declined to file it as a defect of that PR. Deliberately NOT milestoned 1.0.0: the pre-change behaviour was strictly worse (no bound at all, so SIGKILL at 10s), and SQLite is crash-safe.
https://git.eeqj.de/sneak/webhooker/issues/134 clamped the Sentry flush to the remaining stop budget less `TailHookReserve` (2s). The HTTP drain is not clamped the same way: `cleanShutdown` uses `context.WithTimeout(ctx, ShutdownTimeout)`, i.e. `min(3s, whatever is left)`, with no reserve subtracted. So the 2s reserve is only guaranteed when the hooks that run BEFORE the server are fast.
Demonstrated: patching the `ArchiveSweeper` stop hook to take 2.2s and holding a request open gives **exit 1 with the tail hooks, database close included, skipped** — while the server hook itself stays inside its own 3s drain budget. The budget gets eaten jointly rather than by any single hook.
That is inside the family the README discloses, but the disclosure is worded as a wedged reaper consuming "the whole budget on its own", and 2.2s out of 5s already suffices. So the README is optimistic about the size of the wedge required.
## Definition of done
- `cleanShutdown` derives the drain deadline as `min(ShutdownTimeout, remaining - TailHookReserve)`, the same shape already applied to the flush, so the reserve survives slow preceding hooks.
- `TestStopTimeout_LeavesHeadroomForTailHooks` is extended to model a non-zero preceding-hook consumption, not just a drain against a full budget.
- The README caveat is tightened: a slow `ArchiveSweeper` or `RetentionReaper` is enough, not only one that consumes the whole budget.
- The probe above exits 0 with the database close running.
## Worth deciding while you are in there
`TailHookReserve = 2s` is currently the residual of `stopTimeout - ShutdownTimeout`, but the code and README treat it as a named constant chosen on its own merits. Measured tail hooks are 55.101us, 395ns, 2.168us and 1.43ms — about a thousandth of the reserve. Either pick the value deliberately and say why, or document that it is derived. A smaller reserve would buy the flush more room, though the 2s nominal cap is the binding constraint for drains under 1s anyway.
## Implementation requirements
- Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`.
- Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112).
- Run `make bootstrap` (or `make assets`) in a fresh clone before gating — browser assets are fetched at build time and not committed.
- Gate on `make check` plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.
clawbot
self-assigned this 2026-08-18 00:13:14 +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.
Found by the independent review of #159, which proved it with a probe but correctly declined to file it as a defect of that PR. Deliberately NOT milestoned 1.0.0: the pre-change behaviour was strictly worse (no bound at all, so SIGKILL at 10s), and SQLite is crash-safe.
#134 clamped the Sentry flush to the remaining stop budget less
TailHookReserve(2s). The HTTP drain is not clamped the same way:cleanShutdownusescontext.WithTimeout(ctx, ShutdownTimeout), i.e.min(3s, whatever is left), with no reserve subtracted. So the 2s reserve is only guaranteed when the hooks that run BEFORE the server are fast.Demonstrated: patching the
ArchiveSweeperstop hook to take 2.2s and holding a request open gives exit 1 with the tail hooks, database close included, skipped — while the server hook itself stays inside its own 3s drain budget. The budget gets eaten jointly rather than by any single hook.That is inside the family the README discloses, but the disclosure is worded as a wedged reaper consuming "the whole budget on its own", and 2.2s out of 5s already suffices. So the README is optimistic about the size of the wedge required.
Definition of done
cleanShutdownderives the drain deadline asmin(ShutdownTimeout, remaining - TailHookReserve), the same shape already applied to the flush, so the reserve survives slow preceding hooks.TestStopTimeout_LeavesHeadroomForTailHooksis extended to model a non-zero preceding-hook consumption, not just a drain against a full budget.ArchiveSweeperorRetentionReaperis enough, not only one that consumes the whole budget.Worth deciding while you are in there
TailHookReserve = 2sis currently the residual ofstopTimeout - ShutdownTimeout, but the code and README treat it as a named constant chosen on its own merits. Measured tail hooks are 55.101us, 395ns, 2.168us and 1.43ms — about a thousandth of the reserve. Either pick the value deliberately and say why, or document that it is derived. A smaller reserve would buy the flush more room, though the 2s nominal cap is the binding constraint for drains under 1s anyway.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).TODO.md(see #112).make bootstrap(ormake assets) in a fresh clone before gating — browser assets are fetched at build time and not committed.make checkplus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.