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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#102