Shutdown hooks ignore their context and wg.Wait() unbounded #102

New Issue

clawbot · 2026-08-09T07:27:39+02:00

clawbot commented

2026-08-09 07:27:39 +02:00

Companion defect to #97, surfaced by the independent review of PR #100. Pre-existing on main; #100 fixed the startup half of the lifecycle and this is the shutdown half.

Problem

OnStop in both internal/delivery/engine.go and internal/database/retention.go takes _ context.Context and then calls wg.Wait() with no bound. fx passes a stop context carrying StopTimeout, and both components discard it.

If a worker goroutine wedges — a delivery target that never returns, a SQLite operation blocked on a lock — wg.Wait() blocks forever and the process hangs on shutdown instead of exiting. fx's own stop timeout cannot rescue it, because nothing is watching the context it provides.

This is the exact mirror of #97: there, a long-lived goroutine wrongly inherited the start context; here, shutdown wrongly ignores the stop context. Both come from treating the hook context as decoration.

Why it did not block PR #100

It is pre-existing behaviour, unchanged by that PR, and not reachable in normal operation — Engine.stop cancels the context first, so healthy workers return promptly. It only bites when a goroutine is already stuck, which is precisely when a clean shutdown matters most.

Definition of done

OnStop in both components honours its context: wait for the WaitGroup, but return when the stop context expires rather than blocking indefinitely. The usual shape is a goroutine that closes a channel when wg.Wait() returns, selected against ctx.Done().
On timeout, log at warn or error naming the component and the fact that goroutines were still running, so an operator can see why shutdown was not clean. Do not pretend success.
A test that wedges a worker and asserts OnStop returns within the stop timeout instead of hanging. Mutation-verify it: without the fix the test must hang or fail, not pass.
Confirm no other OnStop hook in the tree has the same pattern (there are seven lifecycle hooks total).

Two adjacent nits, worth folding in

Engine.stop lacks the cancel != nil guard that RetentionReaper.stop has. Currently unreachable — stop only runs after start — but the asymmetry between two components that otherwise mirror each other is a trap for the next reader. Make them consistent.
recordingLifecycle is duplicated across internal/delivery/engine_lifecycle_test.go and internal/database/retention_lifecycle_test.go. Legal, since they are different packages, but if a third component needs it the duplication should move to a shared test helper.

Companion defect to #97, surfaced by the independent review of PR #100. Pre-existing on `main`; #100 fixed the startup half of the lifecycle and this is the shutdown half. ## Problem `OnStop` in both `internal/delivery/engine.go` and `internal/database/retention.go` takes `_ context.Context` and then calls `wg.Wait()` with no bound. fx passes a **stop** context carrying `StopTimeout`, and both components discard it. If a worker goroutine wedges — a delivery target that never returns, a SQLite operation blocked on a lock — `wg.Wait()` blocks forever and the process hangs on shutdown instead of exiting. fx's own stop timeout cannot rescue it, because nothing is watching the context it provides. This is the exact mirror of #97: there, a long-lived goroutine wrongly *inherited* the start context; here, shutdown wrongly *ignores* the stop context. Both come from treating the hook context as decoration. ## Why it did not block PR #100 It is pre-existing behaviour, unchanged by that PR, and not reachable in normal operation — `Engine.stop` cancels the context first, so healthy workers return promptly. It only bites when a goroutine is already stuck, which is precisely when a clean shutdown matters most. ## Definition of done - `OnStop` in both components honours its context: wait for the `WaitGroup`, but return when the stop context expires rather than blocking indefinitely. The usual shape is a goroutine that closes a channel when `wg.Wait()` returns, selected against `ctx.Done()`. - On timeout, log at warn or error naming the component and the fact that goroutines were still running, so an operator can see why shutdown was not clean. Do not pretend success. - A test that wedges a worker and asserts `OnStop` returns within the stop timeout instead of hanging. Mutation-verify it: without the fix the test must hang or fail, not pass. - Confirm no other `OnStop` hook in the tree has the same pattern (there are seven lifecycle hooks total). ## Two adjacent nits, worth folding in - **`Engine.stop` lacks the `cancel != nil` guard** that `RetentionReaper.stop` has. Currently unreachable — `stop` only runs after `start` — but the asymmetry between two components that otherwise mirror each other is a trap for the next reader. Make them consistent. - **`recordingLifecycle` is duplicated** across `internal/delivery/engine_lifecycle_test.go` and `internal/database/retention_lifecycle_test.go`. Legal, since they are different packages, but if a third component needs it the duplication should move to a shared test helper.

clawbot referenced this issue

2026-08-09 07:27:53 +02:00

Root background loops at context.Background() (closes #97) #100

clawbot referenced this issue

2026-08-09 07:44:18 +02:00

Pin the pre-reopen close in archiveWriter.sweepExpired (unprotected connection-leak guard) #103

clawbot referenced this issue from a commit

2026-08-12 11:45:50 +02:00

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

clawbot referenced a pull request that will close this issue

2026-08-12 11:46:15 +02:00

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

clawbot commented

2026-08-12 11:46:33 +02:00

PR: #130

All three affected OnStop hooks now wait through a shared internal/lifecycle.WaitForShutdown, which selects the drained WaitGroup against the stop context, logs at error naming the component on timeout, and returns an error instead of reporting a clean stop. A third component had the same defect and is fixed here: internal/delivery/archive_sweeper.go, added after this issue was filed. The hook audit (nine lc.Append sites now, not seven) and the disposition of both nits are in the PR body.

Verified: one wedged-goroutine test per component, mutation-checked — with the bare wg.Wait() restored, all three fail after their 10s bound and nothing else does. make check green, plus docker build with --no-cache-filter on the lint and test stages, exit 0 with both stages shown executing.

PR: https://git.eeqj.de/sneak/webhooker/pulls/130 All three affected `OnStop` hooks now wait through a shared `internal/lifecycle.WaitForShutdown`, which selects the drained `WaitGroup` against the stop context, logs at error naming the component on timeout, and returns an error instead of reporting a clean stop. A third component had the same defect and is fixed here: `internal/delivery/archive_sweeper.go`, added after this issue was filed. The hook audit (nine `lc.Append` sites now, not seven) and the disposition of both nits are in the PR body. Verified: one wedged-goroutine test per component, mutation-checked — with the bare `wg.Wait()` restored, all three fail after their 10s bound and nothing else does. `make check` green, plus `docker build` with `--no-cache-filter` on the lint and test stages, exit 0 with both stages shown executing.

clawbot closed this issue

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

clawbot referenced this issue from a commit

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

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

~~clawbot referenced this issue 2026-08-17 23:50:11 +02:00~~

Milestone 1.0.0: internet-facing readiness #111

clawbot referenced this issue

2026-08-20 06:42:47 +02:00

SIGTERM during startup panics on a nil httpServer, and cleanShutdown reads two fields with no happens-before edge #226

clawbot referenced this issue

2026-08-24 02:58:35 +02:00