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.
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
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:
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.
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.
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.
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 next2026-08-14 06:18:34 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #102
What changed
OnStopdiscarded the context fx supplies (which carriesStopTimeout) and calledwg.Wait()unbounded, so one wedged goroutine hung the process forever on shutdown.internal/lifecycle.WaitForShutdown(ctx, log, component, wg): closes a channel whenwg.Wait()returns and selects it againstctx.Done(). On timeout it logs at error with the component name and the fact that goroutines are still running, and returns an error wrappingctx.Err()— an unclean shutdown is not reported as success.internal/delivery/engine.go,internal/database/retention.goandinternal/delivery/archive_sweeper.gonow take the stop context, pass it through, and return the error.Engine.stopgained thecancel != nilguard thatRetentionReaper.stopandArchiveSweeper.stopalready had; all three are now identical in shape.Test-only:
ExportStoptakes a context and returns an error; each component gained an export helper that adds a goroutine to itsWaitGroupwhich 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.Appendsites. All nine checked:internal/delivery/engine.gointernal/database/retention.gointernal/delivery/archive_sweeper.gointernal/server/server.gocleanShutdown(ctx)internal/database/database.goWaitGroupinternal/database/webhook_db_manager.goCloseAll, noWaitGroupinternal/healthcheck/healthcheck.gointernal/handlers/handlers.goOnStartonlyinternal/session/session.goOnStartonlyNo other hook has the pattern.
Mutation evidence
All three
stopbodies reverted to barewg.Wait()(fix code left in place, unused),make test: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
OnStopis 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 checkon the rebased branch: exit 0,0 issues., every packageokwith a real duration (no(cached)forinternal/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. NoCACHEDon the lint or test layers:No
docker builder prunewas run; invalidation was scoped to the two stages.Nit 2 (declined as stated in the issue)
recordingLifecyclestill exists ininternal/deliveryandinternal/databasetest files. No third package needs it, and sharing across two_testpackages would mean a new non-test package to hold eight lines of scaffolding. Declined.One thing was folded in:
internal/delivery/archive_sweeper_test.gocarriedcaptureLifecycle, a byte-identical copy ofrecordingLifecyclein the same package (delivery_test). That one is removed and its single use switched over.Note
TODO.mdis untouched per #112. The archive sweeper is touched only for the shutdown defect above; nothing here overlaps #101 or #103.PASS. Independently verified: nine
lc.Appendsites, exactly threesync.WaitGroupfields 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.1runStopHookcallshook.OnStop(ctx)synchronously with no goroutine, so the unboundedwg.Wait()really did hang the process — the premise holds; mutation reproduced in my own clone (threestopbodies reverted to barewg.Wait()) and exactlyTestRetentionReaper_StopHookHonoursStopTimeout,TestArchiveSweeper_StopHookHonoursStopTimeout,TestEngine_StopHookHonoursStopTimeoutfailed, nothing else, noDATA RACE; the wedge helpers genuinely block aWaitGroupmember and the assertions pincontext.DeadlineExceededplus the component name, not a bare "returned"; the leaked waiter goroutine is bounded to shutdown and holds only its channel; noAdd-racing-Wait(allAddis instart);ExportStop/ExportWedgeWorker/ExportWedgeLooplive in_test.goandstringsonbin/webhookerfinds none of them;archive_sweeper.gois touched only inregisterHooks/stop, no overlap with #101 or #103; single commit titled(closes #102), basenext,TODO.mduntouched, mergeable, no attribution trailers or vendor references.Gate evidence:
make checkexit 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.8swith the three wedge tests passing in-container (1.34s / 1.23s / 2.46s); noCACHEDon#20,#21,#30,#31. CIsuccessone83eb29.On the flagged judgement call — returning an error from
OnStop: keep it. It adds almost no new nonzero-exit surface. fx v1.20.1Lifecycle.Stopchecksctx.Err()at the top of every iteration and returns it outright, discarding the collected hook errors, so any over-budget shutdown already makesapp.Stopnon-nil andapp.runreturn 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: theHEALTHCHECKis a liveness probe unaffected by exit codes, no restart policy or orchestration manifest is declared in the repo, and both Dockerrestart: unless-stopped/alwaysand systemdRestart=on-failuresuppress restart after an operator-initiated stop. The operator-facing signal (thelog.Errorininternal/lifecycle/lifecycle.go) is what actually survives, and it is present.Three non-blocking notes, none of which gate the merge:
internal/lifecycle/is the only directory underinternal/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 (noarchive_sweeper.go,retention.go,target_*.go,url_mask.go), but this is the first omitted package.fx.StopTimeoutis never set incmd/webhooker/main.go:30, so the bound is fx's 15s default — longer than Docker's 10s defaultdocker stopgrace. 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 explicitfx.StopTimeoutunder the typical grace would make it effective everywhere.internal/lifecycle/lifecycle_test.goand the new export helpers usewg.Go, while the three production components usewg.Add(1)+defer wg.Done(). Correct and newer, but the two idioms now sit side by side in the same packages.Disclosure:
script/lintrunsgolangci-linton the host, not in a container, somake check's lint leg is host-run; I treated the Dockerfilelintstage 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 — independent re-review in a fresh clone: host
make checkexit 0,0 issues., every packageokwith real durations, zero(cached); branch is 9 commits behindnextbut merges clean (ort, no conflicts) andmake checkon 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 lintDONE 70.4s0 issues.and#33 [builder 8/10] RUN make testDONE 70.5s, neitherCACHED; mutation-verified myself (barewg.Wait()restored) — exactlyTestRetentionReaper_StopHookHonoursStopTimeout,TestArchiveSweeper_StopHookHonoursStopTimeout,TestEngine_StopHookHonoursStopTimeoutfail andinternal/lifecyclehits the 30s package timeout, nothing else, noDATA RACE; ninelc.Appendsites audited, the three withWaitGroups fixed; commit title ends(closes #102),make fmta no-op, no attribution trailers. Three disclosures, none blocking: (1)fx.StopTimeoutis unset so the bound is fx's 15s default (fx@v1.20.1/app.go:417), longer than Docker's 10sdocker stopgrace and the repo ships noSTOPSIGNAL/compose override — under a defaultdocker stopthe 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:80re-checksctx.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 onlyinternal/package missing from the README Package Layout tree (README.md:929), a tree already stale at file granularity.