sneak/webhooker
1
0
Fork 0
Code Issues 29 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

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

Merged
clawbot merged 1 commits from issue-102-stop-hook-context into next 2026-08-14 06:18:34 +02:00
Conversation 2 Commits 1 Files Changed 11 +392 -51
clawbot commented 2026-08-12 11:46:15 +02:00
Collaborator
Copy Link

Closes #102

What changed

OnStop discarded the context fx supplies (which carries StopTimeout) and called wg.Wait() unbounded, so one wedged goroutine hung the process forever on shutdown.

  • New internal/lifecycle.WaitForShutdown(ctx, log, component, wg): closes a channel when wg.Wait() returns and selects it against ctx.Done(). On timeout it logs at error with the component name and the fact that goroutines are still running, and returns an error wrapping ctx.Err() — an unclean shutdown is not reported as success.
  • internal/delivery/engine.go, internal/database/retention.go and internal/delivery/archive_sweeper.go now take the stop context, pass it through, and return the error.
  • Nit 1: Engine.stop gained the cancel != nil guard that RetentionReaper.stop and ArchiveSweeper.stop already had; all three are now identical in shape.

Test-only: ExportStop takes a context and returns an error; each component gained an export helper that adds a goroutine to its WaitGroup which never observes cancellation, standing in for a delivery target that never returns or a sweep blocked on a locked SQLite database.

Hook audit

The issue says seven lifecycle hooks; the tree now has nine lc.Append sites. All nine checked:

Hook Status
internal/delivery/engine.go affected, fixed
internal/database/retention.go affected, fixed
internal/delivery/archive_sweeper.go affected, fixed (same defect, added after the issue was filed)
internal/server/server.go already honours the stop context via cleanShutdown(ctx)
internal/database/database.go closes the handle, no WaitGroup
internal/database/webhook_db_manager.go CloseAll, no WaitGroup
internal/healthcheck/healthcheck.go no-op stop
internal/handlers/handlers.go OnStart only
internal/session/session.go OnStart only

No other hook has the pattern.

Mutation evidence

All three stop bodies reverted to bare wg.Wait() (fix code left in place, unused), make test:

    retention_lifecycle_test.go:263: OnStop did not return: it discarded the stop context and is waiting on a wedged goroutine that will never observe cancellation
--- FAIL: TestRetentionReaper_StopHookHonoursStopTimeout (11.12s)
FAIL	sneak.berlin/go/webhooker/internal/database	11.589s
    archive_sweeper_test.go:946: OnStop did not return: it discarded the stop context and is waiting on a wedged goroutine that will never observe cancellation
--- FAIL: TestArchiveSweeper_StopHookHonoursStopTimeout (10.95s)
    engine_lifecycle_test.go:270: OnStop did not return: it discarded the stop context and is waiting on a wedged goroutine that will never observe cancellation
--- FAIL: TestEngine_StopHookHonoursStopTimeout (10.78s)
FAIL	sneak.berlin/go/webhooker/internal/delivery	12.097s

Only those three failed; the mutation was then reverted.

Not flaky by construction: the repo has no injected clock, so the margin is the mechanism. The stop budget handed to OnStop is 250ms and the test allows the hook 10s to return — 40x. A slow machine cannot make it pass or fail spuriously; without the fix the hook never returns at all, and with the fix it returns after its own deadline, not after a wall-clock race.

Gate evidence

make check on the rebased branch: exit 0, 0 issues., every package ok with a real duration (no (cached) for internal/database, internal/delivery).

Containerized path with the cache defeated on the stages under test — docker build --no-cache-filter=lint --no-cache-filter=builder ., exit 0. No CACHED on the lint or test layers:

#18 [lint 8/8] RUN make lint
#18 92.07 0 issues.
#28 [builder  8/10] RUN make test
#28 94.31 --- PASS: TestRetentionReaper_StopHookHonoursStopTimeout (1.88s)
#28 99.12 --- PASS: TestArchiveSweeper_StopHookHonoursStopTimeout (2.99s)
#28 99.12 --- PASS: TestEngine_StopHookHonoursStopTimeout (4.75s)
#28 99.12 ok  	sneak.berlin/go/webhooker/internal/delivery	7.382s

No docker builder prune was run; invalidation was scoped to the two stages.

Nit 2 (declined as stated in the issue)

recordingLifecycle still exists in internal/delivery and internal/database test files. No third package needs it, and sharing across two _test packages would mean a new non-test package to hold eight lines of scaffolding. Declined.

One thing was folded in: internal/delivery/archive_sweeper_test.go carried captureLifecycle, a byte-identical copy of recordingLifecycle in the same package (delivery_test). That one is removed and its single use switched over.

Note

TODO.md is untouched per #112. The archive sweeper is touched only for the shutdown defect above; nothing here overlaps #101 or #103.

Closes https://git.eeqj.de/sneak/webhooker/issues/102 ## What changed `OnStop` discarded the context fx supplies (which carries `StopTimeout`) and called `wg.Wait()` unbounded, so one wedged goroutine hung the process forever on shutdown. - New `internal/lifecycle.WaitForShutdown(ctx, log, component, wg)`: closes a channel when `wg.Wait()` returns and selects it against `ctx.Done()`. On timeout it logs at error with the component name and the fact that goroutines are still running, and returns an error wrapping `ctx.Err()` — an unclean shutdown is not reported as success. - `internal/delivery/engine.go`, `internal/database/retention.go` and `internal/delivery/archive_sweeper.go` now take the stop context, pass it through, and return the error. - Nit 1: `Engine.stop` gained the `cancel != nil` guard that `RetentionReaper.stop` and `ArchiveSweeper.stop` already had; all three are now identical in shape. Test-only: `ExportStop` takes a context and returns an error; each component gained an export helper that adds a goroutine to its `WaitGroup` which never observes cancellation, standing in for a delivery target that never returns or a sweep blocked on a locked SQLite database. ## Hook audit The issue says seven lifecycle hooks; the tree now has nine `lc.Append` sites. All nine checked: | Hook | Status | | --- | --- | | `internal/delivery/engine.go` | affected, fixed | | `internal/database/retention.go` | affected, fixed | | `internal/delivery/archive_sweeper.go` | affected, fixed (same defect, added after the issue was filed) | | `internal/server/server.go` | already honours the stop context via `cleanShutdown(ctx)` | | `internal/database/database.go` | closes the handle, no `WaitGroup` | | `internal/database/webhook_db_manager.go` | `CloseAll`, no `WaitGroup` | | `internal/healthcheck/healthcheck.go` | no-op stop | | `internal/handlers/handlers.go` | `OnStart` only | | `internal/session/session.go` | `OnStart` only | No other hook has the pattern. ## Mutation evidence All three `stop` bodies reverted to bare `wg.Wait()` (fix code left in place, unused), `make test`: ``` retention_lifecycle_test.go:263: OnStop did not return: it discarded the stop context and is waiting on a wedged goroutine that will never observe cancellation --- FAIL: TestRetentionReaper_StopHookHonoursStopTimeout (11.12s) FAIL sneak.berlin/go/webhooker/internal/database 11.589s archive_sweeper_test.go:946: OnStop did not return: it discarded the stop context and is waiting on a wedged goroutine that will never observe cancellation --- FAIL: TestArchiveSweeper_StopHookHonoursStopTimeout (10.95s) engine_lifecycle_test.go:270: OnStop did not return: it discarded the stop context and is waiting on a wedged goroutine that will never observe cancellation --- FAIL: TestEngine_StopHookHonoursStopTimeout (10.78s) FAIL sneak.berlin/go/webhooker/internal/delivery 12.097s ``` Only those three failed; the mutation was then reverted. Not flaky by construction: the repo has no injected clock, so the margin is the mechanism. The stop budget handed to `OnStop` is 250ms and the test allows the hook 10s to return — 40x. A slow machine cannot make it pass or fail spuriously; without the fix the hook never returns at all, and with the fix it returns after its own deadline, not after a wall-clock race. ## Gate evidence `make check` on the rebased branch: exit 0, `0 issues.`, every package `ok` with a real duration (no `(cached)` for `internal/database`, `internal/delivery`). Containerized path with the cache defeated on the stages under test — `docker build --no-cache-filter=lint --no-cache-filter=builder .`, exit 0. No `CACHED` on the lint or test layers: ``` #18 [lint 8/8] RUN make lint #18 92.07 0 issues. #28 [builder 8/10] RUN make test #28 94.31 --- PASS: TestRetentionReaper_StopHookHonoursStopTimeout (1.88s) #28 99.12 --- PASS: TestArchiveSweeper_StopHookHonoursStopTimeout (2.99s) #28 99.12 --- PASS: TestEngine_StopHookHonoursStopTimeout (4.75s) #28 99.12 ok sneak.berlin/go/webhooker/internal/delivery 7.382s ``` No `docker builder prune` was run; invalidation was scoped to the two stages. ## Nit 2 (declined as stated in the issue) `recordingLifecycle` still exists in `internal/delivery` and `internal/database` test files. No third package needs it, and sharing across two `_test` packages would mean a new non-test package to hold eight lines of scaffolding. Declined. One thing was folded in: `internal/delivery/archive_sweeper_test.go` carried `captureLifecycle`, a byte-identical copy of `recordingLifecycle` in the *same* package (`delivery_test`). That one is removed and its single use switched over. ## Note `TODO.md` is untouched per https://git.eeqj.de/sneak/webhooker/issues/112. The archive sweeper is touched only for the shutdown defect above; nothing here overlaps https://git.eeqj.de/sneak/webhooker/issues/101 or https://git.eeqj.de/sneak/webhooker/issues/103.
clawbot added 1 commit 2026-08-12 11:46:15 +02:00
Bound shutdown hooks by their stop context (closes #102)
All checks were successful
check / check (push) Successful in 3m45s
Details
e83eb2977e 
fx hands OnStop a context carrying the application's stop timeout,
and the delivery engine, the retention reaper, and the archive
sweeper all discarded it and called wg.Wait() bare. A worker wedged
inside a delivery target that never returns, or a sweep blocked on a
locked SQLite database, hung the process forever instead of letting
it exit when the timeout expired.

All three now wait through internal/lifecycle.WaitForShutdown, which
selects the drained WaitGroup against the stop context and, on
timeout, logs at error naming the component and returns an error
rather than reporting a clean stop.

Engine.stop also gains the cancel != nil guard its two mirrored
components already had.
clawbot self-assigned this 2026-08-12 11:46:23 +02:00
clawbot added the needs-review label 2026-08-12 11:46:23 +02:00
clawbot commented 2026-08-12 11:54:42 +02:00
Author
Collaborator
Copy Link

PASS. Independently verified: nine lc.Append sites, exactly three sync.WaitGroup fields in non-test code (engine.go:137, retention.go:43, archive_sweeper.go:48), all three fixed, no fourth site with the pattern; fx v1.20.1 runStopHook calls hook.OnStop(ctx) synchronously with no goroutine, so the unbounded wg.Wait() really did hang the process — the premise holds; mutation reproduced in my own clone (three stop bodies reverted to bare wg.Wait()) and exactly TestRetentionReaper_StopHookHonoursStopTimeout, TestArchiveSweeper_StopHookHonoursStopTimeout, TestEngine_StopHookHonoursStopTimeout failed, nothing else, no DATA RACE; the wedge helpers genuinely block a WaitGroup member and the assertions pin context.DeadlineExceeded plus the component name, not a bare "returned"; the leaked waiter goroutine is bounded to shutdown and holds only its channel; no Add-racing-Wait (all Add is in start); ExportStop/ExportWedgeWorker/ExportWedgeLoop live in _test.go and strings on bin/webhooker finds none of them; archive_sweeper.go is touched only in registerHooks/stop, no overlap with #101 or #103; single commit titled (closes #102), base next, TODO.md untouched, mergeable, no attribution trailers or vendor references.

Gate evidence: make check exit 0, zero (cached) markers, 0 issues.. docker build --no-cache-filter=lint,builder --progress=plain . exit 0 — #20 [lint 7/8] RUN make fmt-check DONE 5.3s, #21 [lint 8/8] RUN make lint → #21 68.05 0 issues. DONE 70.5s, #31 [builder 8/10] RUN make test DONE 85.8s with the three wedge tests passing in-container (1.34s / 1.23s / 2.46s); no CACHED on #20, #21, #30, #31. CI success on e83eb29.

On the flagged judgement call — returning an error from OnStop: keep it. It adds almost no new nonzero-exit surface. fx v1.20.1 Lifecycle.Stop checks ctx.Err() at the top of every iteration and returns it outright, discarding the collected hook errors, so any over-budget shutdown already makes app.Stop non-nil and app.run return 1 regardless of what a hook returns. The error return only changes the exit code in the one case where the wedged component is the last hook stopped — and that case previously hung forever rather than exiting 0. Nothing in-repo makes it harmful: the HEALTHCHECK is a liveness probe unaffected by exit codes, no restart policy or orchestration manifest is declared in the repo, and both Docker restart: unless-stopped/always and systemd Restart=on-failure suppress restart after an operator-initiated stop. The operator-facing signal (the log.Error in internal/lifecycle/lifecycle.go) is what actually survives, and it is present.

Three non-blocking notes, none of which gate the merge:

  1. internal/lifecycle/ is the only directory under internal/ missing from the README "Package Layout" tree (README.md:930-982); every other package directory is listed. The tree is already stale at file granularity (no archive_sweeper.go, retention.go, target_*.go, url_mask.go), but this is the first omitted package.
  2. Follow-up worth an issue, not a defect here: fx.StopTimeout is never set in cmd/webhooker/main.go:30, so the bound is fx's 15s default — longer than Docker's 10s default docker stop grace. Under plain Docker the process is SIGKILLed at 10s (exit 137) before the newly-bounded shutdown fires, so the fix's benefit only materialises where the grace period exceeds 15s. An explicit fx.StopTimeout under the typical grace would make it effective everywhere.
  3. internal/lifecycle/lifecycle_test.go and the new export helpers use wg.Go, while the three production components use wg.Add(1) + defer wg.Done(). Correct and newer, but the two idioms now sit side by side in the same packages.

Disclosure: script/lint runs golangci-lint on the host, not in a container, so make check's lint leg is host-run; I treated the Dockerfile lint stage above as the authoritative lint evidence. Review was performed in a fresh clone at /home/user/agentwork/review-130-clawbot/repo; nothing was changed or committed on the branch.

PASS. Independently verified: nine `lc.Append` sites, exactly three `sync.WaitGroup` fields in non-test code (`engine.go:137`, `retention.go:43`, `archive_sweeper.go:48`), all three fixed, no fourth site with the pattern; fx v1.20.1 `runStopHook` calls `hook.OnStop(ctx)` synchronously with no goroutine, so the unbounded `wg.Wait()` really did hang the process — the premise holds; mutation reproduced in my own clone (three `stop` bodies reverted to bare `wg.Wait()`) and exactly `TestRetentionReaper_StopHookHonoursStopTimeout`, `TestArchiveSweeper_StopHookHonoursStopTimeout`, `TestEngine_StopHookHonoursStopTimeout` failed, nothing else, no `DATA RACE`; the wedge helpers genuinely block a `WaitGroup` member and the assertions pin `context.DeadlineExceeded` plus the component name, not a bare "returned"; the leaked waiter goroutine is bounded to shutdown and holds only its channel; no `Add`-racing-`Wait` (all `Add` is in `start`); `ExportStop`/`ExportWedgeWorker`/`ExportWedgeLoop` live in `_test.go` and `strings` on `bin/webhooker` finds none of them; `archive_sweeper.go` is touched only in `registerHooks`/`stop`, no overlap with https://git.eeqj.de/sneak/webhooker/issues/101 or https://git.eeqj.de/sneak/webhooker/issues/103; single commit titled ` (closes #102)`, base `next`, `TODO.md` untouched, mergeable, no attribution trailers or vendor references. Gate evidence: `make check` exit 0, zero `(cached)` markers, `0 issues.`. `docker build --no-cache-filter=lint,builder --progress=plain .` exit 0 — `#20 [lint 7/8] RUN make fmt-check DONE 5.3s`, `#21 [lint 8/8] RUN make lint` → `#21 68.05 0 issues.` `DONE 70.5s`, `#31 [builder 8/10] RUN make test DONE 85.8s` with the three wedge tests passing in-container (1.34s / 1.23s / 2.46s); no `CACHED` on `#20`, `#21`, `#30`, `#31`. CI `success` on `e83eb29`. On the flagged judgement call — returning an error from `OnStop`: keep it. It adds almost no new nonzero-exit surface. fx v1.20.1 `Lifecycle.Stop` checks `ctx.Err()` at the top of every iteration and returns it outright, discarding the collected hook errors, so *any* over-budget shutdown already makes `app.Stop` non-nil and `app.run` return 1 regardless of what a hook returns. The error return only changes the exit code in the one case where the wedged component is the last hook stopped — and that case previously hung forever rather than exiting 0. Nothing in-repo makes it harmful: the `HEALTHCHECK` is a liveness probe unaffected by exit codes, no restart policy or orchestration manifest is declared in the repo, and both Docker `restart: unless-stopped`/`always` and systemd `Restart=on-failure` suppress restart after an operator-initiated stop. The operator-facing signal (the `log.Error` in `internal/lifecycle/lifecycle.go`) is what actually survives, and it is present. Three non-blocking notes, none of which gate the merge: 1. `internal/lifecycle/` is the only directory under `internal/` missing from the README "Package Layout" tree (`README.md:930`-`982`); every other package directory is listed. The tree is already stale at file granularity (no `archive_sweeper.go`, `retention.go`, `target_*.go`, `url_mask.go`), but this is the first omitted *package*. 2. Follow-up worth an issue, not a defect here: `fx.StopTimeout` is never set in `cmd/webhooker/main.go:30`, so the bound is fx's 15s default — longer than Docker's 10s default `docker stop` grace. Under plain Docker the process is SIGKILLed at 10s (exit 137) before the newly-bounded shutdown fires, so the fix's benefit only materialises where the grace period exceeds 15s. An explicit `fx.StopTimeout` under the typical grace would make it effective everywhere. 3. `internal/lifecycle/lifecycle_test.go` and the new export helpers use `wg.Go`, while the three production components use `wg.Add(1)` + `defer wg.Done()`. Correct and newer, but the two idioms now sit side by side in the same packages. Disclosure: `script/lint` runs `golangci-lint` on the host, not in a container, so `make check`'s lint leg is host-run; I treated the Dockerfile `lint` stage above as the authoritative lint evidence. Review was performed in a fresh clone at `/home/user/agentwork/review-130-clawbot/repo`; nothing was changed or committed on the branch.
clawbot commented 2026-08-14 06:17:35 +02:00
Author
Collaborator
Copy Link

PASS — independent re-review in a fresh clone: host make check exit 0, 0 issues., every package ok with real durations, zero (cached); branch is 9 commits behind next but merges clean (ort, no conflicts) and make check on the merged tree is likewise exit 0 / 0 issues. / zero (cached); docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0 with #21 [lint 8/8] RUN make lint DONE 70.4s 0 issues. and #33 [builder 8/10] RUN make test DONE 70.5s, neither CACHED; mutation-verified myself (bare wg.Wait() restored) — exactly TestRetentionReaper_StopHookHonoursStopTimeout, TestArchiveSweeper_StopHookHonoursStopTimeout, TestEngine_StopHookHonoursStopTimeout fail and internal/lifecycle hits the 30s package timeout, nothing else, no DATA RACE; nine lc.Append sites audited, the three with WaitGroups fixed; commit title ends (closes #102), make fmt a no-op, no attribution trailers. Three disclosures, none blocking: (1) fx.StopTimeout is unset so the bound is fx's 15s default (fx@v1.20.1/app.go:417), longer than Docker's 10s docker stop grace and the repo ships no STOPSIGNAL/compose override — under a default docker stop the process is SIGKILLed before the new bound can fire, so the fix is real but inert in that deployment until #134 lands; (2) no closed-DB-under-live-workers regression — fx@v1.20.1/internal/lifecycle/lifecycle.go:80 re-checks ctx.Err() before each remaining hook and returns, so an expired stop context skips the DB close entirely rather than closing it under running goroutines, and hook order (DB appended first, stopped last; server stopped before engine) is correct; the trade is a truncated shutdown, which still beats the previous infinite hang; (3) internal/lifecycle/ is the only internal/ package missing from the README Package Layout tree (README.md:929), a tree already stale at file granularity.

PASS — independent re-review in a fresh clone: host `make check` exit 0, `0 issues.`, every package `ok` with real durations, zero `(cached)`; branch is 9 commits behind `next` but merges clean (ort, no conflicts) and `make check` on the merged tree is likewise exit 0 / `0 issues.` / zero `(cached)`; `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0 with `#21 [lint 8/8] RUN make lint` DONE 70.4s `0 issues.` and `#33 [builder 8/10] RUN make test` DONE 70.5s, neither `CACHED`; mutation-verified myself (bare `wg.Wait()` restored) — exactly `TestRetentionReaper_StopHookHonoursStopTimeout`, `TestArchiveSweeper_StopHookHonoursStopTimeout`, `TestEngine_StopHookHonoursStopTimeout` fail and `internal/lifecycle` hits the 30s package timeout, nothing else, no `DATA RACE`; nine `lc.Append` sites audited, the three with `WaitGroup`s fixed; commit title ends ` (closes #102)`, `make fmt` a no-op, no attribution trailers. Three disclosures, none blocking: (1) `fx.StopTimeout` is unset so the bound is fx's 15s default (`fx@v1.20.1/app.go:417`), longer than Docker's 10s `docker stop` grace and the repo ships no `STOPSIGNAL`/compose override — under a default `docker stop` the process is SIGKILLed before the new bound can fire, so the fix is real but inert in that deployment until https://git.eeqj.de/sneak/webhooker/issues/134 lands; (2) no closed-DB-under-live-workers regression — `fx@v1.20.1/internal/lifecycle/lifecycle.go:80` re-checks `ctx.Err()` before each remaining hook and returns, so an expired stop context skips the DB close entirely rather than closing it under running goroutines, and hook order (DB appended first, stopped last; server stopped before engine) is correct; the trade is a truncated shutdown, which still beats the previous infinite hang; (3) `internal/lifecycle/` is the only `internal/` package missing from the README Package Layout tree (`README.md:929`), a tree already stale at file granularity.
clawbot merged commit 2ee720a9af into next 2026-08-14 06:18:34 +02:00
clawbot deleted branch issue-102-stop-hook-context 2026-08-14 06:18:34 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#130
Delete Branch "issue-102-stop-hook-context"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?