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

CRITICAL: delivery engine and retention reaper both die ~15s after startup (fx OnStart context) #97

New Issue
Open
opened 2026-08-09 04:43:26 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 04:43:26 +02:00
Collaborator
Copy Link

Surfaced by the independent review of PR #95 (#89), which found this pattern in that PR's new sweeper. Checking the rest of the tree shows the same defect is already on main in two places, and one of them is the core of the product.

This should be treated as release-blocking for 1.0.

The defect

The context.Context passed to an fx OnStart hook is not an application-lifetime context. fx derives it with context.WithTimeout(ctx, StartTimeout) where StartTimeout defaults to 15 seconds, and it is cancelled once the start phase ends. Any goroutine that derives its lifetime from it stops shortly after startup.

Both live users of the hook context in this repo do exactly that.

1. internal/delivery/engine.go — the delivery engine (critical)

Engine.start(ctx) at line 213 does ctx, cancel := context.WithCancel(ctx) on the OnStart context and then launches everything from it:

  • every worker goroutine (go e.worker(ctx), line 220)
  • go e.recoverPending(ctx) (line 225)
  • go e.retrySweep(ctx) (line 229)

worker selects on <-ctx.Done() and returns. So roughly 15 seconds after startup, every delivery worker exits and the process silently stops delivering webhooks entirely. Inbound events are still received and persisted to the per-webhook event DBs, and Notify still pushes tasks onto deliveryCh until it fills and starts logging "delivery channel full" — but nothing consumes them. The retry sweep and restart recovery are dead too.

That is the entire purpose of the application. A 1.0 tagged with this ships a webhook proxy that stops forwarding webhooks a quarter of a minute after it boots.

2. internal/database/retention.go — the event retention reaper

RetentionReaper.start(ctx) at line 75 has the same context.WithCancel(ctx) on the hook context. RETENTION_SWEEP_INTERVAL defaults to time.Hour, so in the default configuration the reaper never runs a single sweep — the loop is cancelled 45 minutes before its first tick. The feature delivered in #63 / PR #78 is silently inert, and per-webhook event databases grow without bound exactly as they did before that work landed.

Evidence

The PR #95 reviewer verified the mechanism empirically against a real make build binary, not a test double: with RETENTION_SWEEP_INTERVAL=2s and an archive expiring rows every 10s, sweeps fired at T+4s and T+14s and then stopped for the remaining 46s of a 60s run, while rows that expired at T+24/34/44s survived. The loop context's deadline was T+15s. Reproduced in isolation with a scratch test using a 50ms-deadline context.

Fix

Background loops must own an application-lifetime context, not the start-phase one:

ctx, cancel := context.WithCancel(context.Background())

The existing OnStop hook already calls cancel() and waits on the sync.WaitGroup, so shutdown stays correct — the only thing that changes is that the loops survive past startup. Do not simply lengthen StartTimeout; that mistakes the symptom for the cause.

Apply to both sites. Ignore the hook's ctx parameter (name it _) so the trap cannot be reintroduced by someone "fixing" an unused-parameter warning, and leave a comment at each site explaining why the hook context must not be used.

Definition of done

  • Engine.start and RetentionReaper.start derive their loop context from context.Background(), not the OnStart hook context.
  • A regression test that fails against the current code: start the component with a hook context that is already cancelled (or carries a very short deadline), then assert the loop is still running after that deadline has passed. A test that passes a plain context.Background() in proves nothing, because that is exactly the bug.
  • Graceful shutdown still works — OnStop cancels the loop and wg.Wait() returns without hanging. Cover it, so the fix does not trade one lifecycle bug for another.
  • A sweep of any other long-lived goroutine in the tree for the same pattern. As of 4f5ecb1 the only two hooks that take the context are the two above; the rest use _. Confirm that is still true at implementation time.
  • make check green via the repo's own entrypoints.

Note on PR #95

PR #95 introduces a third instance of this in its new internal/delivery/archive_sweeper.go. That one is being fixed in that PR's rework rather than here, so the two do not collide. This issue covers only the two pre-existing sites on main.

Surfaced by the independent review of PR #95 (#89), which found this pattern in that PR's new sweeper. Checking the rest of the tree shows the same defect is **already on `main`** in two places, and one of them is the core of the product. This should be treated as release-blocking for 1.0. ## The defect The `context.Context` passed to an fx `OnStart` hook is **not** an application-lifetime context. fx derives it with `context.WithTimeout(ctx, StartTimeout)` where `StartTimeout` defaults to 15 seconds, and it is cancelled once the start phase ends. Any goroutine that derives its lifetime from it stops shortly after startup. Both live users of the hook context in this repo do exactly that. ### 1. `internal/delivery/engine.go` — the delivery engine (critical) `Engine.start(ctx)` at line 213 does `ctx, cancel := context.WithCancel(ctx)` on the OnStart context and then launches **everything** from it: - every worker goroutine (`go e.worker(ctx)`, line 220) - `go e.recoverPending(ctx)` (line 225) - `go e.retrySweep(ctx)` (line 229) `worker` selects on `<-ctx.Done()` and returns. So roughly 15 seconds after startup, **every delivery worker exits and the process silently stops delivering webhooks entirely.** Inbound events are still received and persisted to the per-webhook event DBs, and `Notify` still pushes tasks onto `deliveryCh` until it fills and starts logging "delivery channel full" — but nothing consumes them. The retry sweep and restart recovery are dead too. That is the entire purpose of the application. A 1.0 tagged with this ships a webhook proxy that stops forwarding webhooks a quarter of a minute after it boots. ### 2. `internal/database/retention.go` — the event retention reaper `RetentionReaper.start(ctx)` at line 75 has the same `context.WithCancel(ctx)` on the hook context. `RETENTION_SWEEP_INTERVAL` defaults to `time.Hour`, so in the default configuration **the reaper never runs a single sweep** — the loop is cancelled 45 minutes before its first tick. The feature delivered in #63 / PR #78 is silently inert, and per-webhook event databases grow without bound exactly as they did before that work landed. ## Evidence The PR #95 reviewer verified the mechanism empirically against a real `make build` binary, not a test double: with `RETENTION_SWEEP_INTERVAL=2s` and an archive expiring rows every 10s, sweeps fired at T+4s and T+14s and then stopped for the remaining 46s of a 60s run, while rows that expired at T+24/34/44s survived. The loop context's deadline was T+15s. Reproduced in isolation with a scratch test using a 50ms-deadline context. ## Fix Background loops must own an application-lifetime context, not the start-phase one: ```go ctx, cancel := context.WithCancel(context.Background()) ``` The existing `OnStop` hook already calls `cancel()` and waits on the `sync.WaitGroup`, so shutdown stays correct — the only thing that changes is that the loops survive past startup. Do **not** simply lengthen `StartTimeout`; that mistakes the symptom for the cause. Apply to both sites. Ignore the hook's `ctx` parameter (name it `_`) so the trap cannot be reintroduced by someone "fixing" an unused-parameter warning, and leave a comment at each site explaining why the hook context must not be used. ## Definition of done - `Engine.start` and `RetentionReaper.start` derive their loop context from `context.Background()`, not the OnStart hook context. - A regression test that fails against the current code: start the component with a hook context that is **already cancelled** (or carries a very short deadline), then assert the loop is still running after that deadline has passed. A test that passes a plain `context.Background()` in proves nothing, because that is exactly the bug. - Graceful shutdown still works — `OnStop` cancels the loop and `wg.Wait()` returns without hanging. Cover it, so the fix does not trade one lifecycle bug for another. - A sweep of any other long-lived goroutine in the tree for the same pattern. As of `4f5ecb1` the only two hooks that take the context are the two above; the rest use `_`. Confirm that is still true at implementation time. - `make check` green via the repo's own entrypoints. ## Note on PR #95 PR #95 introduces a third instance of this in its new `internal/delivery/archive_sweeper.go`. That one is being fixed in that PR's rework rather than here, so the two do not collide. This issue covers only the two pre-existing sites on `main`.
clawbot commented 2026-08-09 07:04:03 +02:00
Author
Collaborator
Copy Link

Implementation plan for this issue, on branch issue-97-lifecycle-context:

  1. internal/delivery/engine.go and internal/database/retention.go: extract the lc.Append(fx.Hook{...}) call into a registerHooks method, change OnStart to take _ context.Context, drop the context parameter from start(), and root the loop context at context.WithCancel(context.Background()). Leave a doc comment at each start explaining why the hook context must not be used, and a //nolint:contextcheck on the hook. Same shape as the fix in PR #95's ArchiveSweeper, so the three sites read identically.
  2. New internal/delivery/engine_lifecycle_test.go and internal/database/retention_lifecycle_test.go. Each drives the genuine registered hook (through registerHooks, via an Export... shim and a recording fx.Lifecycle) with an already-cancelled OnStart context, then asserts the loop is still doing work — a delivered task for the engine, a reaped event for the reaper. Each also gets a graceful-shutdown test asserting OnStop cancels and wg.Wait() returns inside a bounded timeout, and that the component really is stopped afterwards.
  3. Mutation-verify: revert each fix in turn and confirm the matching tests fail, then restore.
  4. Sweep every fx.Hook in the tree to confirm no other long-lived goroutine roots itself in a hook context.
  5. TODO.md entry in the same commit; make fmt; make check and script/cibuild green.

Out of scope here: internal/delivery/archive_sweeper.go does not exist on main — it belongs to PR #95 and is fixed there.

Implementation plan for this issue, on branch `issue-97-lifecycle-context`: 1. `internal/delivery/engine.go` and `internal/database/retention.go`: extract the `lc.Append(fx.Hook{...})` call into a `registerHooks` method, change `OnStart` to take `_ context.Context`, drop the context parameter from `start()`, and root the loop context at `context.WithCancel(context.Background())`. Leave a doc comment at each `start` explaining why the hook context must not be used, and a `//nolint:contextcheck` on the hook. Same shape as the fix in PR #95's `ArchiveSweeper`, so the three sites read identically. 2. New `internal/delivery/engine_lifecycle_test.go` and `internal/database/retention_lifecycle_test.go`. Each drives the genuine registered hook (through `registerHooks`, via an `Export...` shim and a recording `fx.Lifecycle`) with an already-cancelled `OnStart` context, then asserts the loop is still doing work — a delivered task for the engine, a reaped event for the reaper. Each also gets a graceful-shutdown test asserting `OnStop` cancels and `wg.Wait()` returns inside a bounded timeout, and that the component really is stopped afterwards. 3. Mutation-verify: revert each fix in turn and confirm the matching tests fail, then restore. 4. Sweep every `fx.Hook` in the tree to confirm no other long-lived goroutine roots itself in a hook context. 5. `TODO.md` entry in the same commit; `make fmt`; `make check` and `script/cibuild` green. Out of scope here: `internal/delivery/archive_sweeper.go` does not exist on `main` — it belongs to PR #95 and is fixed there.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#97
Delete Branch "%!s()"

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?