fx.StopTimeout is never set, so the bounded-shutdown fix does not fire under a default docker stop #134

New Issue

clawbot · 2026-08-12T11:55:48+02:00

clawbot commented

2026-08-12 11:55:48 +02:00

Found in the independent review of #130. Not milestoned 1.0.0: it is not reachable by an unauthenticated attacker, so it belongs to the next cycle rather than blocking the tag.

cmd/webhooker/main.go:30 never passes fx.StopTimeout, so shutdown is bounded by fx's 15 s default.

Docker's default docker stop grace is 10 s. So the process is SIGKILLed at 10 s (exit 137) BEFORE the 15 s bound expires, and #130's bounded shutdown — including its operator-facing "goroutines still running" error log, which is the part that actually survives — only materialises where the grace period exceeds 15 s. Under a stock Docker deployment it never fires.

That does not make #130 wrong; it makes it inert in the common deployment.

Definition of done

fx.StopTimeout set explicitly in cmd/webhooker/main.go at a value below the typical deployment grace period, so the bound is reached and the warning is logged before any external SIGKILL.
A test pinning the value, so it cannot silently drift back to the default.
The README documents the chosen timeout and its relationship to the container stop grace, since an operator lowering --stop-timeout below it reintroduces the same silent truncation.

Second, unrelated item from the same review

internal/lifecycle/ is the only internal/* package missing from the README "Package Layout" tree (README.md:930-982) — it was added by #130 and the tree was not updated. Add it. The tree is already stale at file granularity, which is out of scope here; only the missing package needs fixing.

Implementation requirements

Branch from next, PR based on next, single commit, title ending (closes #N).
Do not modify TODO.md (see #112).
If markdown changes, run make fmt and include the result.
Gate on make check plus the Docker lint path with the cache defeated (#119).

Found in the independent review of https://git.eeqj.de/sneak/webhooker/pulls/130. Not milestoned 1.0.0: it is not reachable by an unauthenticated attacker, so it belongs to the next cycle rather than blocking the tag. `cmd/webhooker/main.go:30` never passes `fx.StopTimeout`, so shutdown is bounded by fx's 15 s default. Docker's default `docker stop` grace is 10 s. So the process is SIGKILLed at 10 s (exit 137) BEFORE the 15 s bound expires, and https://git.eeqj.de/sneak/webhooker/pulls/130's bounded shutdown — including its operator-facing "goroutines still running" error log, which is the part that actually survives — only materialises where the grace period exceeds 15 s. Under a stock Docker deployment it never fires. That does not make https://git.eeqj.de/sneak/webhooker/pulls/130 wrong; it makes it inert in the common deployment. ## Definition of done - `fx.StopTimeout` set explicitly in `cmd/webhooker/main.go` at a value below the typical deployment grace period, so the bound is reached and the warning is logged before any external SIGKILL. - A test pinning the value, so it cannot silently drift back to the default. - The README documents the chosen timeout and its relationship to the container stop grace, since an operator lowering `--stop-timeout` below it reintroduces the same silent truncation. ## Second, unrelated item from the same review `internal/lifecycle/` is the only `internal/*` package missing from the README "Package Layout" tree (`README.md:930`-`982`) — it was added by https://git.eeqj.de/sneak/webhooker/pulls/130 and the tree was not updated. Add it. The tree is already stale at file granularity, which is out of scope here; only the missing package needs fixing. ## 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). - If markdown changes, run `make fmt` and include the result. - Gate on `make check` plus the Docker lint path with the cache defeated (https://git.eeqj.de/sneak/webhooker/issues/119).

clawbot self-assigned this 2026-08-12 11:55:48 +02:00

clawbot referenced this issue

2026-08-14 06:17:35 +02:00

Bound shutdown hooks by their stop context (closes #102) #130

clawbot commented

2026-08-14 06:18:46 +02:00

#130 has landed on next, so the bound it built is now shipped but inert under the deployment the repo actually produces. That makes this release-relevant rather than next-cycle.

Two additions to scope, both from the review of #130:

The chosen fx.StopTimeout must sit inside Docker's default 10s grace. fx's DefaultTimeout is 15s and the Dockerfile sets no STOPSIGNAL or grace override, so Docker SIGKILLs 5s before the current bound can fire.
WaitForShutdown in internal/lifecycle/lifecycle.go selects on done against ctx.Done() with no preamble, so a component that drained cleanly against an already-expired context returns shutdown timed out about half the time. Unreachable through fx today — fx re-checks ctx.Err() before each remaining hook — so it is latent only for direct callers of this new shared helper. Fix with a select { case <-done: return nil; default: } preamble and a test that pins it.

Also worth writing down in the same change: bounding shutdown means a wedge in the first-stopped component aborts the rest of the sequence, because fx returns outright on an expired stop context and skips the remaining hooks. That is fx's behaviour, not a defect introduced here, but nothing in the repo records it.

https://git.eeqj.de/sneak/webhooker/pulls/130 has landed on `next`, so the bound it built is now shipped but inert under the deployment the repo actually produces. That makes this release-relevant rather than next-cycle. Two additions to scope, both from the review of https://git.eeqj.de/sneak/webhooker/pulls/130: 1. The chosen `fx.StopTimeout` must sit inside Docker's default 10s grace. fx's `DefaultTimeout` is 15s and the Dockerfile sets no `STOPSIGNAL` or grace override, so Docker SIGKILLs 5s before the current bound can fire. 2. `WaitForShutdown` in `internal/lifecycle/lifecycle.go` selects on `done` against `ctx.Done()` with no preamble, so a component that drained cleanly against an already-expired context returns `shutdown timed out` about half the time. Unreachable through fx today — fx re-checks `ctx.Err()` before each remaining hook — so it is latent only for direct callers of this new shared helper. Fix with a `select { case <-done: return nil; default: }` preamble and a test that pins it. Also worth writing down in the same change: bounding shutdown means a wedge in the first-stopped component aborts the rest of the sequence, because fx returns outright on an expired stop context and skips the remaining hooks. That is fx's behaviour, not a defect introduced here, but nothing in the repo records it.

clawbot commented

2026-08-14 06:23:30 +02:00

Plan, one commit on issue-134-fx-stop-timeout:

cmd/webhooker/main.go: extract the option set into newApp() *fx.App and add fx.StopTimeout(stopTimeout) with stopTimeout = 5s. 5s sits inside Docker's 10s default grace with headroom for signal delivery and process exit, and matches the HTTP server's own 5s drain budget, so the first hook can use its full budget without the bound truncating it.
Test in cmd/webhooker/main_test.go: fx exposes (*fx.App).StopTimeout(), and fx.New applies options before running invokes, so the test asserts newApp().StopTimeout() == stopTimeout and that it is below the 10s grace. Dropping the option makes it read fx's 15s DefaultTimeout and the test fails — will mutation-verify and report the observed failure.
internal/lifecycle/lifecycle.go: split the channel wait into an unexported waitDone(ctx, log, component, done) carrying the select { case <-done: return nil; default: } preamble, exposed to the external test package via export_test.go. The deterministic test drives waitDone with an already-closed done and an already-cancelled ctx in a loop — select picks uniformly among ready cases, so a single pass proves nothing, and no wall-clock sleeps are involved. Testing this through the public WaitForShutdown cannot be deterministic: whether the internal waiter goroutine has closed done by the time the preamble runs is scheduler-dependent.
README: record the chosen timeout and its relationship to the container stop grace, note that an expired stop context makes fx return outright and skip the remaining hooks (so a wedge in the first-stopped component means the database close never runs), and add the missing internal/lifecycle/ entry to the Package Layout tree.

Gate: docker build --no-cache-filter=lint --no-cache-filter=builder . as the authoritative run, make check on the host as a secondary signal, plus an end-to-end docker run / docker stop showing a clean exit inside the grace period. TODO.md untouched.

Plan, one commit on `issue-134-fx-stop-timeout`: 1. `cmd/webhooker/main.go`: extract the option set into `newApp() *fx.App` and add `fx.StopTimeout(stopTimeout)` with `stopTimeout = 5s`. 5s sits inside Docker's 10s default grace with headroom for signal delivery and process exit, and matches the HTTP server's own 5s drain budget, so the first hook can use its full budget without the bound truncating it. 2. Test in `cmd/webhooker/main_test.go`: `fx` exposes `(*fx.App).StopTimeout()`, and `fx.New` applies options before running invokes, so the test asserts `newApp().StopTimeout() == stopTimeout` and that it is below the 10s grace. Dropping the option makes it read fx's 15s `DefaultTimeout` and the test fails — will mutation-verify and report the observed failure. 3. `internal/lifecycle/lifecycle.go`: split the channel wait into an unexported `waitDone(ctx, log, component, done)` carrying the `select { case <-done: return nil; default: }` preamble, exposed to the external test package via `export_test.go`. The deterministic test drives `waitDone` with an already-closed `done` and an already-cancelled `ctx` in a loop — `select` picks uniformly among ready cases, so a single pass proves nothing, and no wall-clock sleeps are involved. Testing this through the public `WaitForShutdown` cannot be deterministic: whether the internal waiter goroutine has closed `done` by the time the preamble runs is scheduler-dependent. 4. README: record the chosen timeout and its relationship to the container stop grace, note that an expired stop context makes fx return outright and skip the remaining hooks (so a wedge in the first-stopped component means the database close never runs), and add the missing `internal/lifecycle/` entry to the Package Layout tree. Gate: `docker build --no-cache-filter=lint --no-cache-filter=builder .` as the authoritative run, `make check` on the host as a secondary signal, plus an end-to-end `docker run` / `docker stop` showing a clean exit inside the grace period. `TODO.md` untouched.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#134