Clamp the HTTP drain by the tail-hook reserve, not just the Sentry flush #170

New Issue

clawbot · 2026-08-18T00:13:14+02:00

clawbot commented

2026-08-18 00:13:14 +02:00

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).
Do not modify TODO.md (see #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.

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#170