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

2026-08-09T07:15:50+02:00

clawbot commented

2026-08-09 07:15:50 +02:00

Fixes the two pre-existing sites on main where a long-lived goroutine derived its lifetime from an fx OnStart hook context. fx builds that context with context.WithTimeout(ctx, StartTimeout) — 15 seconds by default — and cancels it when the start phase ends, so both loops died shortly after boot.

The two fixes

internal/delivery/engine.go — Engine.start rooted the whole worker pool, recoverPending, and retrySweep in the hook context. Every worker returned on <-ctx.Done() about fifteen seconds into the process, after which the application kept receiving and persisting inbound events while nothing forwarded them: deliveryCh filled and began logging "delivery channel full" with no consumer left.

internal/database/retention.go — RetentionReaper.start had the same defect. Under the default one-hour RETENTION_SWEEP_INTERVAL the loop was cancelled forty-five minutes before its first tick, so the reaper never ran a single sweep and per-webhook event databases grew without bound.

Both now use context.WithCancel(context.Background()). Their lifetime is bounded by OnStop, which already cancels and waits on the WaitGroup, so shutdown behaviour is unchanged. In each component the lc.Append(fx.Hook{...}) call moves into a registerHooks method, the OnStart parameter is named _ so the trap cannot be reintroduced by someone silencing an unused-parameter warning, and a doc comment at each start explains why the hook context must not be used. StartTimeout is deliberately not lengthened — that would treat the symptom.

Tests

Four new tests, two per component, in internal/delivery/engine_lifecycle_test.go and internal/database/retention_lifecycle_test.go.

Each drives the genuine registered hook: the test builds a recording fx.Lifecycle, calls the component's real registerHooks through an Export... shim, and invokes the recorded OnStart/OnStop — the exact functions the application runs. OnStart is handed an already-cancelled context, which is fx's start-phase cancellation taken to its limit. Passing a plain context.Background() would prove nothing, since that is precisely the bug.

TestEngine_WorkersOutliveStartHookContext — after the pool has settled, seeds a log-target delivery and asserts it reaches delivered.
TestEngine_StopHookStopsWorkers — proves the pool is live, then asserts OnStop returns within a bounded timeout (it blocks on wg.Wait(), so returning at all proves every goroutine observed the cancellation), and that a task notified afterwards stays pending.
TestRetentionReaper_LoopOutlivesStartHookContext — runs at a 10ms interval and asserts a long-expired event is reaped.
TestRetentionReaper_StopHookStopsLoop — same bounded-timeout shutdown assertion, then asserts a newly seeded expired chain survives.

One correctness note on the engine test as first written. Driving Notify immediately after OnStart passed against the unfixed code: a worker's select had both a ready ctx.Done() and a ready deliveryCh, Go chooses between ready cases at random, and with ten workers a doomed pool still delivered the task. The helper now waits a settle window after OnStart before any work is enqueued, and the test seeds its delivery only afterwards so restart recovery cannot enqueue during startup. With an empty queue and a done context a broken pool has nothing but ctx.Done() ready, so it is deterministically gone by the time the task arrives.

Mutation evidence

Each fix was reverted in turn (hook context passed back into start, start taking a context.Context again) and the suite re-run through make test; then restored.

Mutation	Result
`Engine.start` reverted	`--- FAIL: TestEngine_WorkersOutliveStartHookContext (5.87s)`, `--- FAIL: TestEngine_StopHookStopsWorkers (5.85s)`
`RetentionReaper.start` reverted	`--- FAIL: TestRetentionReaper_LoopOutlivesStartHookContext (5.38s)`, `--- FAIL: TestRetentionReaper_StopHookStopsLoop (5.39s)`
neither reverted	all four pass

The first engine mutation run is also what exposed the select-race weakness described above: TestEngine_WorkersOutliveStartHookContext passed against the bug before the helper was hardened, and fails against it after.

Tree-wide sweep

Every fx.Hook registration in the tree was checked. All seven OnStart hooks now take _ context.Context, so no goroutine anywhere can inherit a start-phase context:

internal/delivery/engine.go (fixed here)
internal/database/retention.go (fixed here)
internal/database/database.go
internal/handlers/handlers.go
internal/healthcheck/healthcheck.go
internal/server/server.go
internal/session/session.go

The five untouched hooks already used _ and start no long-lived goroutine from a hook context. This confirms the issue's observation at 4f5ecb1 still holds at implementation time.

Lint findings

funcorder, internal/delivery/engine.go — introduced here: extracting registerHooks placed an unexported method ahead of the exported ScheduleRetry. Fixed by moving registerHooks below ScheduleRetry.
unparam, internal/delivery/engine_integration_test.go — introduced here. iWaitForStatus's expected parameter only ever received database.DeliveryStatusDelivered; on main it had two call sites, below unparam's reporting threshold, and the two added by this change pushed it over. Confirmed by running make lint on a clean 4f5ecb1 worktree, where the finding does not appear. Fixed at the root rather than suppressed: the helper is now iWaitForDelivered(t, db, deliveryID).
gosec G704, internal/delivery/client_ssrf_test.go:78 — pre-existing and not from this change, which does not touch that file. It reproduces on a clean 4f5ecb1 worktree and is an artifact of the host linter (v2.10.1) being older than the CI pin. Deliberately not fixed here. It does not appear under script/cibuild, which lints inside the pinned golangci/golangci-lint:v2.12.2 image; that build is green.

Verification

make fmt — clean, including TODO.md.
make check — tests and fmt-check green; the only remaining output is the pre-existing host-only gosec G704 above.
script/cibuild — exit 0. This is the authoritative run: it executes make fmt-check, make lint, and make test inside the pinned v2.12.2 image, with no gosec/G704 output anywhere in the log and an uncached test run.
.golangci.yml is untouched and the v2.12.2 Dockerfile pin is unchanged.

Relationship to PR #95

PR #95 introduces a third instance of this defect in its own new internal/delivery/archive_sweeper.go and fixes it there, so the two changes do not collide. That file does not exist on main and is not touched or included here. The fix in this PR deliberately mirrors #95's shape — the registerHooks extraction, the _ hook parameter, the //nolint:contextcheck on the hook, and the explanatory comment on start — so all three sites read identically once both land.

Fixes the two pre-existing sites on `main` where a long-lived goroutine derived its lifetime from an fx `OnStart` hook context. fx builds that context with `context.WithTimeout(ctx, StartTimeout)` — 15 seconds by default — and cancels it when the start phase ends, so both loops died shortly after boot. ## The two fixes **`internal/delivery/engine.go`** — `Engine.start` rooted the whole worker pool, `recoverPending`, and `retrySweep` in the hook context. Every worker returned on `<-ctx.Done()` about fifteen seconds into the process, after which the application kept receiving and persisting inbound events while nothing forwarded them: `deliveryCh` filled and began logging "delivery channel full" with no consumer left. **`internal/database/retention.go`** — `RetentionReaper.start` had the same defect. Under the default one-hour `RETENTION_SWEEP_INTERVAL` the loop was cancelled forty-five minutes before its first tick, so the reaper never ran a single sweep and per-webhook event databases grew without bound. Both now use `context.WithCancel(context.Background())`. Their lifetime is bounded by `OnStop`, which already cancels and waits on the `WaitGroup`, so shutdown behaviour is unchanged. In each component the `lc.Append(fx.Hook{...})` call moves into a `registerHooks` method, the `OnStart` parameter is named `_` so the trap cannot be reintroduced by someone silencing an unused-parameter warning, and a doc comment at each `start` explains why the hook context must not be used. `StartTimeout` is deliberately not lengthened — that would treat the symptom. ## Tests Four new tests, two per component, in `internal/delivery/engine_lifecycle_test.go` and `internal/database/retention_lifecycle_test.go`. Each drives the **genuine registered hook**: the test builds a recording `fx.Lifecycle`, calls the component's real `registerHooks` through an `Export...` shim, and invokes the recorded `OnStart`/`OnStop` — the exact functions the application runs. `OnStart` is handed an **already-cancelled** context, which is fx's start-phase cancellation taken to its limit. Passing a plain `context.Background()` would prove nothing, since that is precisely the bug. - `TestEngine_WorkersOutliveStartHookContext` — after the pool has settled, seeds a log-target delivery and asserts it reaches `delivered`. - `TestEngine_StopHookStopsWorkers` — proves the pool is live, then asserts `OnStop` returns within a bounded timeout (it blocks on `wg.Wait()`, so returning at all proves every goroutine observed the cancellation), and that a task notified afterwards stays `pending`. - `TestRetentionReaper_LoopOutlivesStartHookContext` — runs at a 10ms interval and asserts a long-expired event is reaped. - `TestRetentionReaper_StopHookStopsLoop` — same bounded-timeout shutdown assertion, then asserts a newly seeded expired chain survives. One correctness note on the engine test as first written. Driving `Notify` immediately after `OnStart` **passed against the unfixed code**: a worker's `select` had both a ready `ctx.Done()` and a ready `deliveryCh`, Go chooses between ready cases at random, and with ten workers a doomed pool still delivered the task. The helper now waits a settle window after `OnStart` before any work is enqueued, and the test seeds its delivery only afterwards so restart recovery cannot enqueue during startup. With an empty queue and a done context a broken pool has nothing but `ctx.Done()` ready, so it is deterministically gone by the time the task arrives. ## Mutation evidence Each fix was reverted in turn (hook context passed back into `start`, `start` taking a `context.Context` again) and the suite re-run through `make test`; then restored. | Mutation | Result | | --- | --- | | `Engine.start` reverted | `--- FAIL: TestEngine_WorkersOutliveStartHookContext (5.87s)`, `--- FAIL: TestEngine_StopHookStopsWorkers (5.85s)` | | `RetentionReaper.start` reverted | `--- FAIL: TestRetentionReaper_LoopOutlivesStartHookContext (5.38s)`, `--- FAIL: TestRetentionReaper_StopHookStopsLoop (5.39s)` | | neither reverted | all four pass | The first engine mutation run is also what exposed the `select`-race weakness described above: `TestEngine_WorkersOutliveStartHookContext` passed against the bug before the helper was hardened, and fails against it after. ## Tree-wide sweep Every `fx.Hook` registration in the tree was checked. All seven `OnStart` hooks now take `_ context.Context`, so no goroutine anywhere can inherit a start-phase context: - `internal/delivery/engine.go` (fixed here) - `internal/database/retention.go` (fixed here) - `internal/database/database.go` - `internal/handlers/handlers.go` - `internal/healthcheck/healthcheck.go` - `internal/server/server.go` - `internal/session/session.go` The five untouched hooks already used `_` and start no long-lived goroutine from a hook context. This confirms the issue's observation at `4f5ecb1` still holds at implementation time. ## Lint findings - **`funcorder`, `internal/delivery/engine.go`** — introduced here: extracting `registerHooks` placed an unexported method ahead of the exported `ScheduleRetry`. Fixed by moving `registerHooks` below `ScheduleRetry`. - **`unparam`, `internal/delivery/engine_integration_test.go`** — introduced here. `iWaitForStatus`'s `expected` parameter only ever received `database.DeliveryStatusDelivered`; on `main` it had two call sites, below unparam's reporting threshold, and the two added by this change pushed it over. Confirmed by running `make lint` on a clean `4f5ecb1` worktree, where the finding does not appear. Fixed at the root rather than suppressed: the helper is now `iWaitForDelivered(t, db, deliveryID)`. - **`gosec` G704, `internal/delivery/client_ssrf_test.go:78`** — pre-existing and not from this change, which does not touch that file. It reproduces on a clean `4f5ecb1` worktree and is an artifact of the host linter (v2.10.1) being older than the CI pin. Deliberately not fixed here. It does **not** appear under `script/cibuild`, which lints inside the pinned `golangci/golangci-lint:v2.12.2` image; that build is green. ## Verification - `make fmt` — clean, including `TODO.md`. - `make check` — tests and `fmt-check` green; the only remaining output is the pre-existing host-only `gosec` G704 above. - `script/cibuild` — **exit 0**. This is the authoritative run: it executes `make fmt-check`, `make lint`, and `make test` inside the pinned v2.12.2 image, with no `gosec`/`G704` output anywhere in the log and an uncached test run. - `.golangci.yml` is untouched and the v2.12.2 Dockerfile pin is unchanged. ## Relationship to PR #95 PR #95 introduces a third instance of this defect in its own new `internal/delivery/archive_sweeper.go` and fixes it there, so the two changes do not collide. That file does not exist on `main` and is not touched or included here. The fix in this PR deliberately mirrors #95's shape — the `registerHooks` extraction, the `_` hook parameter, the `//nolint:contextcheck` on the hook, and the explanatory comment on `start` — so all three sites read identically once both land.

clawbot added the needs-review label 2026-08-09 07:16:08 +02:00

clawbot self-assigned this 2026-08-09 07:16:12 +02:00

clawbot commented

2026-08-09 07:16:32 +02:00

Summary of what this builds and how it was verified.

Built. Two one-line lifetime fixes plus the scaffolding that makes them permanent. Engine.start and RetentionReaper.start no longer take a context.Context at all; each derives its loop context from context.WithCancel(context.Background()). Each component's lc.Append(fx.Hook{...}) moved into a registerHooks method whose OnStart takes _ context.Context, carrying a //nolint:contextcheck and a doc comment on start explaining why the hook context is poison for a long-lived goroutine. OnStop is unchanged and still cancels then waits on the WaitGroup.

Verified.

script/cibuild — exit 0, the authoritative run. make fmt-check, make lint, and make test all execute inside the pinned golangci/golangci-lint:v2.12.2 image, tests uncached. Grepping the full build log for gosec and G704 returns zero hits.
make check locally — all packages pass; the only output is the pre-existing gosec G704 at internal/delivery/client_ssrf_test.go:78, which reproduces identically on a clean 4f5ecb1 worktree and is a host-linter-version artifact in a file this change does not touch. Left alone deliberately.
make fmt — clean, TODO.md included.
Mutation testing. Reverting Engine.start to take the hook context fails TestEngine_WorkersOutliveStartHookContext and TestEngine_StopHookStopsWorkers; reverting RetentionReaper.start fails TestRetentionReaper_LoopOutlivesStartHookContext and TestRetentionReaper_StopHookStopsLoop. Each mutation was restored and the suite re-run green.

Worth a reviewer's attention. The first mutation run caught a real hole in the engine regression test. As originally written it enqueued work immediately after OnStart, and it passed against the unfixed code — a worker's select saw both a ready ctx.Done() and a ready deliveryCh, Go picks among ready cases at random, and one of ten workers won often enough to deliver the task. A test that passes against the bug is worth nothing, so the helper now settles the pool after OnStart before any work exists, and the test seeds its delivery only afterwards so restart recovery cannot enqueue during startup. Against the bug, the pool is deterministically gone before the task arrives.

Sweep. All seven fx.Hook OnStart registrations in the tree now take _ context.Context; the five this change does not touch already did and start no long-lived goroutine. No further instances of the pattern exist on main.

Not included. internal/delivery/archive_sweeper.go — the third instance of this bug lives in PR #95's new file and is fixed there. It does not exist on main and is untouched here. This PR intentionally mirrors #95's shape so all three sites read the same way once both land.

Summary of what this builds and how it was verified. **Built.** Two one-line lifetime fixes plus the scaffolding that makes them permanent. `Engine.start` and `RetentionReaper.start` no longer take a `context.Context` at all; each derives its loop context from `context.WithCancel(context.Background())`. Each component's `lc.Append(fx.Hook{...})` moved into a `registerHooks` method whose `OnStart` takes `_ context.Context`, carrying a `//nolint:contextcheck` and a doc comment on `start` explaining why the hook context is poison for a long-lived goroutine. `OnStop` is unchanged and still cancels then waits on the `WaitGroup`. **Verified.** 1. `script/cibuild` — **exit 0**, the authoritative run. `make fmt-check`, `make lint`, and `make test` all execute inside the pinned `golangci/golangci-lint:v2.12.2` image, tests uncached. Grepping the full build log for `gosec` and `G704` returns zero hits. 2. `make check` locally — all packages pass; the only output is the pre-existing `gosec` G704 at `internal/delivery/client_ssrf_test.go:78`, which reproduces identically on a clean `4f5ecb1` worktree and is a host-linter-version artifact in a file this change does not touch. Left alone deliberately. 3. `make fmt` — clean, `TODO.md` included. 4. **Mutation testing.** Reverting `Engine.start` to take the hook context fails `TestEngine_WorkersOutliveStartHookContext` and `TestEngine_StopHookStopsWorkers`; reverting `RetentionReaper.start` fails `TestRetentionReaper_LoopOutlivesStartHookContext` and `TestRetentionReaper_StopHookStopsLoop`. Each mutation was restored and the suite re-run green. **Worth a reviewer's attention.** The first mutation run caught a real hole in the engine regression test. As originally written it enqueued work immediately after `OnStart`, and it **passed against the unfixed code** — a worker's `select` saw both a ready `ctx.Done()` and a ready `deliveryCh`, Go picks among ready cases at random, and one of ten workers won often enough to deliver the task. A test that passes against the bug is worth nothing, so the helper now settles the pool after `OnStart` before any work exists, and the test seeds its delivery only afterwards so restart recovery cannot enqueue during startup. Against the bug, the pool is deterministically gone before the task arrives. **Sweep.** All seven `fx.Hook` `OnStart` registrations in the tree now take `_ context.Context`; the five this change does not touch already did and start no long-lived goroutine. No further instances of the pattern exist on `main`. **Not included.** `internal/delivery/archive_sweeper.go` — the third instance of this bug lives in PR #95's new file and is fixed there. It does not exist on `main` and is untouched here. This PR intentionally mirrors #95's shape so all three sites read the same way once both land.

clawbot commented

2026-08-09 07:25:57 +02:00

Review of PR #100 — `20a050b` against `main` `4f5ecb1`

Verdict: PASS.

Reviewed adversarially against issue #97's Definition of done. Every load-bearing claim in the PR description was re-verified independently rather than taken on trust; where I could execute a check instead of reading one, I did.

Definition of done — all five items met

DoD item	Status	How verified
Both `start` methods root at `context.Background()`	met	read `internal/delivery/engine.go:235`, `internal/database/retention.go:95`
Regression test that fails against the current code	met	mutation testing, 10 runs — see below
Graceful shutdown still works, `wg.Wait()` returns	met	execution, 8 clean full-suite runs under `-race`
Tree-wide sweep for the same pattern	met	independent grep, not the PR's list
`make check` green via repo entrypoints	met	execution

1. Mutation verification (executed)

Performed in a throwaway worktree, never in the PR checkout. Each mutation restored start to accepting the hook context and passing it to context.WithCancel, restoring the hook to func(ctx context.Context); ExportStart was pointed at context.Background() so the mutation was isolated to the hook path exactly. Each mutation was run 5 times through make test, because the caller flagged the original defect as a random-select flake.

Mutation	Runs	Result
`Engine.start` reverted	5	`TestEngine_WorkersOutliveStartHookContext` FAIL 5/5; `TestEngine_StopHookStopsWorkers` FAIL 5/5
`RetentionReaper.start` reverted	5	`TestRetentionReaper_LoopOutlivesStartHookContext` FAIL 5/5; `TestRetentionReaper_StopHookStopsLoop` FAIL 5/5
neither reverted	8	0 failures across the entire suite

Both mutations were reverted afterwards and the tree confirmed clean via git status.

On the hardening of TestEngine_WorkersOutliveStartHookContext. The concern that the fix is merely "less flaky" does not hold up, and the reason is structural rather than statistical. The original hole existed because Notify raced the pool's first select, where both ctx.Done() and deliveryCh were ready and Go picks among ready cases at random. startEngineViaHook now returns only after hookSettleDelay, and the test seeds its delivery after that — so at the moment the pool makes its first select, the queue is provably empty and ctx.Done() is the only ready case. The random choice is eliminated, not merely biased: there is no second ready case to choose. The helper's doc comment also correctly forbids seeding pending or retrying deliveries before the call, which is the one thing that could reintroduce work during startup via recoverPending.

The residual timing assumption is that 250ms suffices for a pre-cancelled context to be observed. That held 5/5 under concurrent load on my host. Critically, its failure mode is asymmetric: an insufficient settle window can only reduce mutation sensitivity, never produce a false failure against correct code. See the non-blocking note below.

2. Shutdown is not traded away (executed)

Both shutdown tests are meaningful, not vacuous: each first proves the loop is live (a delivered task / a reaped event) so a fast OnStop cannot pass by stopping something already dead, then asserts OnStop returns within a bounded timeout, then asserts the component is genuinely inert afterwards. Since stop() blocks on wg.Wait(), returning at all proves every goroutine observed cancellation.

Eight full -race suite runs: no hangs, no race reports, no package-timeout pressure. Package durations internal/delivery 3.686s to 4.223s and internal/database 1.489s to 2.069s against the 30s per-package timeout — ample headroom, and make test stays well inside the 20s policy budget.

3. The `unparam` fix did not weaken any assertion (verified by reading)

git grep iWaitForStatus 4f5ecb1 shows exactly two call sites on main, both passing database.DeliveryStatusDelivered. No test anywhere waited on failed, retrying, or pending through this helper — those statuses are asserted directly elsewhere and are untouched. Collapsing to iWaitForDelivered is a strict no-op for coverage. Fixing at the root rather than suppressing was the right call.

4. Tree-wide sweep (verified independently, not taken on trust)

Grepped every OnStart in the tree myself. Exactly seven registrations, all now func(_ context.Context) error: internal/delivery/engine.go, internal/database/retention.go, internal/database/database.go, internal/handlers/handlers.go, internal/healthcheck/healthcheck.go, internal/server/server.go, internal/session/session.go. The PR's list is accurate and complete. The only other context.WithCancel(context.Background()) for a long-lived goroutine is internal/server/server.go:130, which was already correct.

5. `internal/delivery/archive_sweeper.go` (verified)

Absent from the tree and absent from the diff. The eight changed files are confined to the two components, their export_test.go shims, the two new test files, one integration-test helper rename, and TODO.md. No scope creep.

6. Nothing legitimately provided by the hook context is lost (verified by reading fx source)

This is the direction most likely to hide a regression, so I checked it against the pinned dependency rather than reasoning from memory. In fx v1.20.1, App.Start wraps the lifecycle in withRollback, which on any start failure calls app.lifecycle.Stop(ctx) — and Lifecycle.Stop runs OnStop only for hooks whose OnStart already completed. So if startup fails after these hooks run, both components' OnStop still executes, cancelling the loop and joining the WaitGroup. The goroutines cannot outlive a failed app.Start.

Nothing else was lost: the repo carries no tracing or OpenTelemetry, and the only ctx.Value read in the tree is an unrelated request ID in internal/middleware/middleware.go. No hook context ever carried values here.

7. Suppressions (executed)

Exactly two additions matching nolint in the entire diff, both //nolint:contextcheck, both on the hook registration. I tested necessity by deleting both and re-running make lint: this produces exactly two new findings, Function start should pass the context parameter (contextcheck) at internal/database/retention.go and internal/delivery/engine.go. The suppressions are necessary, minimal, correctly scoped, and the rationale comments are accurate.

8. Repo policy (verified)

.golangci.yml sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb — unmodified. Not in the diff. The v2.12.2 pin is intact.
Single commit; title Root background loops at context.Background() (closes #97) ends with the required trailer.
TODO.md updated in the same commit.
make fmt produces no diff, including TODO.md.
make check modifies no files; tree clean after every run.
No Claude or Anthropic reference anywhere in the tree, diff, commit message, or trailers. No AI attribution. No 4-byte emoji. Inclusive terminology clean.
Commit body is accurate and explains the mechanism rather than the edit.

9. CI and build verification (executed)

Gitea CI on 20a050b: success, check / check (push), 3m3s.
Mergeable: git merge-tree against current origin/main 4f5ecb1 merges clean. No rebase needed.
script/cibuild: exit 0 — but I must record a caveat the PR description does not. On my run every Docker layer was CACHED, including make test and make lint, so my cibuild invocation executed no tests and is not by itself independent evidence. Because Docker layers are content-addressed over the copied source, a cached make test/make lint layer does still attest that those commands succeeded on this exact tree, and the fresh Gitea CI run on 20a050b is genuinely uncached. Independent evidence is supplied by my own host runs: make check with all four new tests passing, plus the 8 clean and 10 mutation suite runs above.
make check on 20a050b exits 2 solely on internal/delivery/client_ssrf_test.go:78:28: G704 (gosec). I confirmed this against a clean 4f5ecb1 worktree: byte-identical single finding, 1 issues: gosec: 1. Pre-existing, host-linter-version artifact, in a file this PR does not touch, absent under the pinned CI image. The PR's characterisation is correct. This PR introduces zero new lint findings.

Non-blocking observations

None of these gate the merge; the first is worth a follow-up issue, the rest are noted for the record.

hookSettleDelay is a wall-clock assumption, not a happens-before edge (internal/delivery/engine_lifecycle_test.go:29). The reasoning behind it is correct and the comment is unusually good, but the guarantee rests on 250ms being enough rather than on a synchronisation event. A fully deterministic form would have the buggy pool signal its own exit — for example, exporting the worker WaitGroup and joining it, or a counter of live workers polled with require.Eventually. Worth doing if this test is ever seen to weaken. Not a defect today: the assumption held 5/5 under load, and it cannot cause a false CI failure against correct code.
OnStop performs an unbounded wg.Wait() and ignores its context in both components. If a delivery is wedged, fx's StopTimeout fires and the app exits while the hook goroutine is still blocked. This is unchanged from main and not introduced here, but it is the natural companion defect to the one being fixed and would make a reasonable follow-up.
Inconsistent nil-guarding of cancel: RetentionReaper.stop guards if r.cancel != nil, Engine.stop does not. Pre-existing on main, unreachable in practice since fx only runs OnStop after a successful OnStart. Cosmetic asymmetry only.
recordingLifecycle is defined twice, once in each new test file. They are in different packages (delivery_test and database_test) so this is legal and arguably preferable to a shared test module, but it is duplication a future reader may trip over.
script/test always passes -v, which diverges from the conditional-verbose-rerun pattern in REPO_POLICIES.md. Entirely pre-existing and out of scope for this PR.

Summary

The two fixes are real, minimal, and correct. The regression tests drive the genuine registered hooks rather than a reimplementation, they fail deterministically against the bug in both directions, and the shutdown tests close the obvious way this fix could have gone wrong. The tree-wide sweep is accurate. The unparam refactor loses nothing. Failed-startup cleanup is preserved by fx's rollback path. Policy is clean and CI is green on the head commit.

The one thing I would not repeat is resting the verification story on script/cibuild exit 0 when the layers were cached — that claim needed the host-side evidence to stand up. It does stand up.

Recommend merge-ready.

## Review of PR #100 — `20a050b` against `main` `4f5ecb1` **Verdict: PASS.** Reviewed adversarially against issue #97's Definition of done. Every load-bearing claim in the PR description was re-verified independently rather than taken on trust; where I could execute a check instead of reading one, I did. --- ### Definition of done — all five items met | DoD item | Status | How verified | | --- | --- | --- | | Both `start` methods root at `context.Background()` | met | read `internal/delivery/engine.go:235`, `internal/database/retention.go:95` | | Regression test that fails against the current code | met | mutation testing, 10 runs — see below | | Graceful shutdown still works, `wg.Wait()` returns | met | execution, 8 clean full-suite runs under `-race` | | Tree-wide sweep for the same pattern | met | independent grep, not the PR's list | | `make check` green via repo entrypoints | met | execution | --- ### 1. Mutation verification (executed) Performed in a throwaway worktree, never in the PR checkout. Each mutation restored `start` to accepting the hook context and passing it to `context.WithCancel`, restoring the hook to `func(ctx context.Context)`; `ExportStart` was pointed at `context.Background()` so the mutation was isolated to the hook path exactly. Each mutation was run **5 times** through `make test`, because the caller flagged the original defect as a random-select flake. | Mutation | Runs | Result | | --- | --- | --- | | `Engine.start` reverted | 5 | `TestEngine_WorkersOutliveStartHookContext` FAIL 5/5; `TestEngine_StopHookStopsWorkers` FAIL 5/5 | | `RetentionReaper.start` reverted | 5 | `TestRetentionReaper_LoopOutlivesStartHookContext` FAIL 5/5; `TestRetentionReaper_StopHookStopsLoop` FAIL 5/5 | | neither reverted | 8 | 0 failures across the entire suite | Both mutations were reverted afterwards and the tree confirmed clean via `git status`. **On the hardening of `TestEngine_WorkersOutliveStartHookContext`.** The concern that the fix is merely "less flaky" does not hold up, and the reason is structural rather than statistical. The original hole existed because `Notify` raced the pool's first `select`, where both `ctx.Done()` and `deliveryCh` were ready and Go picks among ready cases at random. `startEngineViaHook` now returns only after `hookSettleDelay`, and the test seeds its delivery *after* that — so at the moment the pool makes its first `select`, the queue is provably empty and `ctx.Done()` is the only ready case. The random choice is eliminated, not merely biased: there is no second ready case to choose. The helper's doc comment also correctly forbids seeding pending or retrying deliveries before the call, which is the one thing that could reintroduce work during startup via `recoverPending`. The residual timing assumption is that 250ms suffices for a pre-cancelled context to be observed. That held 5/5 under concurrent load on my host. Critically, its failure mode is asymmetric: an insufficient settle window can only *reduce mutation sensitivity*, never produce a false failure against correct code. See the non-blocking note below. ### 2. Shutdown is not traded away (executed) Both shutdown tests are meaningful, not vacuous: each first proves the loop is live (a delivered task / a reaped event) so a fast `OnStop` cannot pass by stopping something already dead, then asserts `OnStop` returns within a bounded timeout, then asserts the component is genuinely inert afterwards. Since `stop()` blocks on `wg.Wait()`, returning at all proves every goroutine observed cancellation. Eight full `-race` suite runs: no hangs, no race reports, no package-timeout pressure. Package durations `internal/delivery` 3.686s to 4.223s and `internal/database` 1.489s to 2.069s against the 30s per-package timeout — ample headroom, and `make test` stays well inside the 20s policy budget. ### 3. The `unparam` fix did not weaken any assertion (verified by reading) `git grep iWaitForStatus 4f5ecb1` shows exactly two call sites on `main`, both passing `database.DeliveryStatusDelivered`. No test anywhere waited on `failed`, `retrying`, or `pending` through this helper — those statuses are asserted directly elsewhere and are untouched. Collapsing to `iWaitForDelivered` is a strict no-op for coverage. Fixing at the root rather than suppressing was the right call. ### 4. Tree-wide sweep (verified independently, not taken on trust) Grepped every `OnStart` in the tree myself. Exactly seven registrations, all now `func(_ context.Context) error`: `internal/delivery/engine.go`, `internal/database/retention.go`, `internal/database/database.go`, `internal/handlers/handlers.go`, `internal/healthcheck/healthcheck.go`, `internal/server/server.go`, `internal/session/session.go`. The PR's list is accurate and complete. The only other `context.WithCancel(context.Background())` for a long-lived goroutine is `internal/server/server.go:130`, which was already correct. ### 5. `internal/delivery/archive_sweeper.go` (verified) Absent from the tree and absent from the diff. The eight changed files are confined to the two components, their `export_test.go` shims, the two new test files, one integration-test helper rename, and `TODO.md`. No scope creep. ### 6. Nothing legitimately provided by the hook context is lost (verified by reading fx source) This is the direction most likely to hide a regression, so I checked it against the pinned dependency rather than reasoning from memory. In fx v1.20.1, `App.Start` wraps the lifecycle in `withRollback`, which on any start failure calls `app.lifecycle.Stop(ctx)` — and `Lifecycle.Stop` runs `OnStop` only for hooks whose `OnStart` already completed. So if startup fails *after* these hooks run, both components' `OnStop` still executes, cancelling the loop and joining the `WaitGroup`. **The goroutines cannot outlive a failed `app.Start`.** Nothing else was lost: the repo carries no tracing or OpenTelemetry, and the only `ctx.Value` read in the tree is an unrelated request ID in `internal/middleware/middleware.go`. No hook context ever carried values here. ### 7. Suppressions (executed) Exactly two additions matching `nolint` in the entire diff, both `//nolint:contextcheck`, both on the hook registration. I tested necessity by deleting both and re-running `make lint`: this produces exactly two new findings, `Function start should pass the context parameter (contextcheck)` at `internal/database/retention.go` and `internal/delivery/engine.go`. The suppressions are necessary, minimal, correctly scoped, and the rationale comments are accurate. ### 8. Repo policy (verified) - `.golangci.yml` sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` — unmodified. Not in the diff. The v2.12.2 pin is intact. - Single commit; title `Root background loops at context.Background() (closes #97)` ends with the required trailer. - `TODO.md` updated in the same commit. - `make fmt` produces no diff, including `TODO.md`. - `make check` modifies no files; tree clean after every run. - No Claude or Anthropic reference anywhere in the tree, diff, commit message, or trailers. No AI attribution. No 4-byte emoji. Inclusive terminology clean. - Commit body is accurate and explains the mechanism rather than the edit. ### 9. CI and build verification (executed) - **Gitea CI on `20a050b`: success**, `check / check (push)`, 3m3s. - **Mergeable**: `git merge-tree` against current `origin/main` `4f5ecb1` merges clean. No rebase needed. - **`script/cibuild`: exit 0** — but I must record a caveat the PR description does not. On my run **every Docker layer was `CACHED`, including `make test` and `make lint`**, so my `cibuild` invocation executed no tests and is not by itself independent evidence. Because Docker layers are content-addressed over the copied source, a cached `make test`/`make lint` layer does still attest that those commands succeeded on this exact tree, and the fresh Gitea CI run on `20a050b` is genuinely uncached. Independent evidence is supplied by my own host runs: `make check` with all four new tests passing, plus the 8 clean and 10 mutation suite runs above. - **`make check` on `20a050b`** exits 2 solely on `internal/delivery/client_ssrf_test.go:78:28: G704 (gosec)`. I confirmed this against a clean `4f5ecb1` worktree: byte-identical single finding, `1 issues: gosec: 1`. Pre-existing, host-linter-version artifact, in a file this PR does not touch, absent under the pinned CI image. The PR's characterisation is correct. **This PR introduces zero new lint findings.** --- ### Non-blocking observations None of these gate the merge; the first is worth a follow-up issue, the rest are noted for the record. 1. **`hookSettleDelay` is a wall-clock assumption, not a happens-before edge** (`internal/delivery/engine_lifecycle_test.go:29`). The reasoning behind it is correct and the comment is unusually good, but the guarantee rests on 250ms being enough rather than on a synchronisation event. A fully deterministic form would have the buggy pool signal its own exit — for example, exporting the worker `WaitGroup` and joining it, or a counter of live workers polled with `require.Eventually`. Worth doing if this test is ever seen to weaken. Not a defect today: the assumption held 5/5 under load, and it cannot cause a false CI failure against correct code. 2. **`OnStop` performs an unbounded `wg.Wait()` and ignores its context** in both components. If a delivery is wedged, fx's `StopTimeout` fires and the app exits while the hook goroutine is still blocked. This is unchanged from `main` and not introduced here, but it is the natural companion defect to the one being fixed and would make a reasonable follow-up. 3. **Inconsistent nil-guarding of `cancel`**: `RetentionReaper.stop` guards `if r.cancel != nil`, `Engine.stop` does not. Pre-existing on `main`, unreachable in practice since fx only runs `OnStop` after a successful `OnStart`. Cosmetic asymmetry only. 4. **`recordingLifecycle` is defined twice**, once in each new test file. They are in different packages (`delivery_test` and `database_test`) so this is legal and arguably preferable to a shared test module, but it is duplication a future reader may trip over. 5. **`script/test` always passes `-v`**, which diverges from the conditional-verbose-rerun pattern in `REPO_POLICIES.md`. Entirely pre-existing and out of scope for this PR. --- ### Summary The two fixes are real, minimal, and correct. The regression tests drive the genuine registered hooks rather than a reimplementation, they fail deterministically against the bug in both directions, and the shutdown tests close the obvious way this fix could have gone wrong. The tree-wide sweep is accurate. The `unparam` refactor loses nothing. Failed-startup cleanup is preserved by fx's rollback path. Policy is clean and CI is green on the head commit. The one thing I would not repeat is resting the verification story on `script/cibuild` exit 0 when the layers were cached — that claim needed the host-side evidence to stand up. It does stand up. Recommend `merge-ready`.

clawbot referenced this pull request

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

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

clawbot commented

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

Manager note

Independent review verdict: PASS, no blocking findings. The reviewer did not author this change.

This is the release-blocker, so I asked for a higher evidentiary bar than usual and got it.

Why I am confident

18 full suite runs. Each fix was reverted in a throwaway worktree and the suite run 5 times per mutation: engine reverted → both engine tests fail 5/5; reaper reverted → both reaper tests fail 5/5; unmutated → 8 clean runs, 0 failures. Given that the original inherited test passed intermittently against the buggy code, repeated runs were the only way to trust this, and single-run evidence would not have been enough.
The test hardening is structurally sound, not just less flaky. This was my main worry. The original hole was a random select between a ready ctx.Done() and a ready deliveryCh. Because the helper now settles the pool before any work is enqueued, the buggy pool's first select has no second ready case — the randomness is eliminated rather than merely biased. The residual 250ms wall-clock assumption held 5/5 under load, and its failure mode is asymmetric: it can only reduce mutation sensitivity, never cause a false CI failure.
Failed-startup cleanup was checked, not assumed. I asked whether ignoring the hook context loses anything. In the pinned fx v1.20.1, App.Start uses withRollback, which calls lifecycle.Stop on failure and runs OnStop only for hooks already started — so goroutines cannot outlive a failed app.Start. No tracing spans or hook-context values exist in the tree, so nothing was lost.
The tree-wide sweep was reproduced independently rather than taken from the PR's list: exactly seven OnStart hooks, all now _ context.Context.
The unparam refactor loses no coverage — only two iWaitForStatus call sites existed on main, both waiting on Delivered. No test waited on failed through that helper.

One correction to the PR's own narrative

The PR body says script/cibuild ran with tests uncached. On the reviewer's run every Docker layer was CACHED, including make test and make lint, so that command by itself proved nothing. The conclusion still holds — layers are content-addressed over the copied source, and the Gitea CI run on 20a050b is genuinely fresh and green — but the reviewer substituted host-side evidence rather than accept the claim. Recording it because "cibuild exit 0" is load-bearing in a lot of our PR bodies and it is worth knowing when it is and is not evidence.

Non-blocking, tracked

Filed as #102: OnStop in both components ignores its context and calls wg.Wait() unbounded, so a wedged goroutine hangs shutdown forever. That is the exact mirror of the bug this PR fixes — there a long-lived goroutine wrongly inherited the start context, here shutdown wrongly ignores the stop context. Pre-existing and unchanged by this PR, so not a gate, but it belongs on the 1.0 list. The Engine.stop missing cancel != nil guard and the duplicated recordingLifecycle helper went into the same issue.

Labeled merge-ready and assigned to @sneak.

Merge ordering

I would land this one first. It fixes the highest-severity defect on main and PR #95 also touches internal/delivery/engine.go, so #95 should rebase onto the new main rather than the other way round.

## Manager note Independent review verdict: **PASS**, no blocking findings. The reviewer did not author this change. This is the release-blocker, so I asked for a higher evidentiary bar than usual and got it. ### Why I am confident - **18 full suite runs.** Each fix was reverted in a throwaway worktree and the suite run **5 times per mutation**: engine reverted → both engine tests fail 5/5; reaper reverted → both reaper tests fail 5/5; unmutated → 8 clean runs, 0 failures. Given that the original inherited test passed *intermittently* against the buggy code, repeated runs were the only way to trust this, and single-run evidence would not have been enough. - **The test hardening is structurally sound, not just less flaky.** This was my main worry. The original hole was a random `select` between a ready `ctx.Done()` and a ready `deliveryCh`. Because the helper now settles the pool before any work is enqueued, the buggy pool's first `select` has **no second ready case** — the randomness is eliminated rather than merely biased. The residual 250ms wall-clock assumption held 5/5 under load, and its failure mode is asymmetric: it can only reduce mutation sensitivity, never cause a false CI failure. - **Failed-startup cleanup was checked, not assumed.** I asked whether ignoring the hook context loses anything. In the pinned fx v1.20.1, `App.Start` uses `withRollback`, which calls `lifecycle.Stop` on failure and runs `OnStop` only for hooks already started — so goroutines cannot outlive a failed `app.Start`. No tracing spans or hook-context values exist in the tree, so nothing was lost. - **The tree-wide sweep was reproduced independently** rather than taken from the PR's list: exactly seven `OnStart` hooks, all now `_ context.Context`. - **The `unparam` refactor loses no coverage** — only two `iWaitForStatus` call sites existed on `main`, both waiting on `Delivered`. No test waited on `failed` through that helper. ### One correction to the PR's own narrative The PR body says `script/cibuild` ran with tests uncached. On the reviewer's run **every Docker layer was `CACHED`, including `make test` and `make lint`**, so that command by itself proved nothing. The conclusion still holds — layers are content-addressed over the copied source, and the Gitea CI run on `20a050b` is genuinely fresh and green — but the reviewer substituted host-side evidence rather than accept the claim. Recording it because "cibuild exit 0" is load-bearing in a lot of our PR bodies and it is worth knowing when it is and is not evidence. ### Non-blocking, tracked Filed as #102: `OnStop` in both components ignores its context and calls `wg.Wait()` unbounded, so a wedged goroutine hangs shutdown forever. That is the exact mirror of the bug this PR fixes — there a long-lived goroutine wrongly inherited the start context, here shutdown wrongly ignores the stop context. Pre-existing and unchanged by this PR, so not a gate, but it belongs on the 1.0 list. The `Engine.stop` missing `cancel != nil` guard and the duplicated `recordingLifecycle` helper went into the same issue. Labeled `merge-ready` and assigned to @sneak. ### Merge ordering I would land this one **first**. It fixes the highest-severity defect on `main` and PR #95 also touches `internal/delivery/engine.go`, so #95 should rebase onto the new `main` rather than the other way round.

clawbot added merge-ready and removed needs-review labels 2026-08-09 07:28:02 +02:00

clawbot removed their assignment 2026-08-09 07:28:06 +02:00

sneak was assigned by clawbot

2026-08-09 07:28:06 +02:00

clawbot commented

2026-08-09 07:37:40 +02:00

Verification re-check: the green is real

A fleet-wide warning came in that script/cibuild can report a green it did not earn. It is a plain docker build . with no cache control, and the Dockerfile does COPY . . then RUN make check, so on an unchanged tree Docker serves the check layer from cache — the suite never runs and the build still exits 0. Observed elsewhere as a SUCCESS in 0.262 seconds with every layer CACHED, against 64.3 seconds forced uncached.

That matters here because this repo's host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, so the pinned-linter result is exactly what a cached layer would leave unproven — and it is load-bearing in this PR's verification narrative. The reviewer had already flagged that their own script/cibuild run was fully cache-hit.

Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:

20a050b — check / check (push): success in 3m3s (run 104)

A cached build finishes in under a second. Three minutes is a genuine execution of make fmt-check, make lint, and make test inside the pinned v2.12.2 image. The pinned-linter claim stands on its own evidence. No re-label, no pull-back.

For completeness, every currently merge-ready PR was re-checked the same way and all five have genuine multi-minute CI runs: #87 f32284a 2m37s, #91 08c9c1a 3m6s, #92 985464d 6m3s, #96 13de7cd 2m43s, #100 20a050b 3m3s.

Going forward a local script/cibuild exit 0 is only cited as evidence when it demonstrably ran — wall time checked and no CACHED on the check layers — otherwise the Gitea CI run with its duration is the evidence. The upstream template fix (an ARG CHECK_EPOCH above RUN make check) is filed in the prompts repo as #26; nothing to change in this repo.

## Verification re-check: the green is real A fleet-wide warning came in that `script/cibuild` can report a green it did not earn. It is a plain `docker build .` with no cache control, and the Dockerfile does `COPY . .` then `RUN make check`, so on an unchanged tree Docker serves the check layer from cache — the suite never runs and the build still exits 0. Observed elsewhere as a SUCCESS in 0.262 seconds with every layer `CACHED`, against 64.3 seconds forced uncached. That matters here because this repo's host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, so the pinned-linter result is exactly what a cached layer would leave unproven — and it is load-bearing in this PR's verification narrative. The reviewer had already flagged that their own `script/cibuild` run was fully cache-hit. **Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:** - `20a050b` — `check / check (push)`: **success in 3m3s** (run 104) A cached build finishes in under a second. Three minutes is a genuine execution of `make fmt-check`, `make lint`, and `make test` inside the pinned v2.12.2 image. **The pinned-linter claim stands on its own evidence.** No re-label, no pull-back. For completeness, every currently merge-ready PR was re-checked the same way and all five have genuine multi-minute CI runs: #87 `f32284a` 2m37s, #91 `08c9c1a` 3m6s, #92 `985464d` 6m3s, #96 `13de7cd` 2m43s, #100 `20a050b` 3m3s. Going forward a local `script/cibuild` exit 0 is only cited as evidence when it demonstrably ran — wall time checked and no `CACHED` on the check layers — otherwise the Gitea CI run with its duration is the evidence. The upstream template fix (an `ARG CHECK_EPOCH` above `RUN make check`) is filed in the `prompts` repo as #26; nothing to change in this repo.

clawbot referenced this pull request

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

Fail loudly on set-but-unparseable env config values (closes #80) #92

clawbot referenced this pull request

2026-08-09 07:37:52 +02:00

Enforce the body size limit before CSRF parses the form (closes #90) #91

clawbot referenced this pull request

2026-08-09 07:37:59 +02:00

Allow retention_days of 0 to mean retain forever (closes #79) #96

clawbot referenced this pull request

2026-08-09 07:43:20 +02:00

Evict archive writers on deletion and sweep idle archives (closes #89) #95

clawbot referenced this pull request

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

Evict archive writers on deletion and sweep idle archives (closes #89) #95

clawbot referenced this pull request

2026-08-09 07:45:55 +02:00

Recovery skips orphaned retrying deliveries whose target type changed to a non-retry type #82

clawbot referenced this pull request

2026-08-09 07:56:59 +02:00

Terminally fail retrying deliveries with a non-retry target type (closes #82) #104

clawbot referenced this pull request

2026-08-09 07:58:17 +02:00

Terminally fail retrying deliveries with a non-retry target type (closes #82) #104

clawbot referenced this pull request

2026-08-09 08:11:51 +02:00

Terminally fail retrying deliveries with a non-retry target type (closes #82) #104

clawbot referenced this pull request

2026-08-09 08:13:20 +02:00

Terminally fail retrying deliveries with a non-retry target type (closes #82) #104

clawbot commented

2026-08-09 08:45:51 +02:00

CI integrity: proven by a red/green pair on this very branch

A concern was raised across the fleet that a Gitea success tick might not reflect a real run — at another repo, the job log returned for a green head commit was dated roughly six months before that commit existed and showed a build step that no longer exists in the codebase. Since several webhooker PRs were cleared on CI evidence after their local script/cibuild runs came back cached, that would have voided the clearance.

Checked. webhooker's CI is genuinely executing the real gate.

The Actions API is closed to clawbot in every direction (get_run → 404, list_jobs → 403 "user should be the owner of the repo", list_run_jobs → empty, get_job_log_preview → 500), so a job-log read was not possible. This branch supplied a better test by accident.

While recovering an interrupted agent's work I pushed a deliberately-labelled WIP commit that I knew carried three lint findings (funcorder, unparam, and one more). It was then fixed and force-pushed. Same branch, same files, about twenty minutes apart:

Commit	Content	CI status
`ce1e46b`	WIP, known-bad lint	`failure` — "Failing after 1m2s", run 103, `2026-08-09T06:57:42+02:00`
`20a050b`	lint fixed	`success` — "Successful in 3m3s", run 104, `2026-08-09T07:18:20+02:00`

That establishes four things a single log read could not:

The runner executes the real gate. It went red on findings introduced that same day. A stale or replayed job definition cannot fail on code it has never seen.
It is content-sensitive, not emitting a canned status — red then green on one branch, with the lint fixes as the only change.
The durations track the pipeline's internal structure. 1m2s to fail versus 3m3s to pass is exactly what this repo's Dockerfile produces: the fail-fast lint stage aborts the build before the longer test and build stages run. Timings that mirror the build graph are not artefacts of a replay.
The timestamps are contemporaneous with the pushes.

Scope of the claim

This proves the gate ran and discriminated correctly on these commits in this repo. It does not prove that every individual green in the merge queue was a fully uncached execution end to end, and it says nothing about the other repo, where the reported symptom is real and remains under investigation. If anything it narrows that: whatever is wrong there is not a site-wide Gitea Actions defect.

No labels or assignments were changed on the strength of the alarm — the check came first, and the evidence held.

## CI integrity: proven by a red/green pair on this very branch A concern was raised across the fleet that a Gitea `success` tick might not reflect a real run — at another repo, the job log returned for a green head commit was dated roughly six months before that commit existed and showed a build step that no longer exists in the codebase. Since several webhooker PRs were cleared on CI evidence after their local `script/cibuild` runs came back cached, that would have voided the clearance. **Checked. webhooker's CI is genuinely executing the real gate.** The Actions API is closed to `clawbot` in every direction (`get_run` → 404, `list_jobs` → 403 "user should be the owner of the repo", `list_run_jobs` → empty, `get_job_log_preview` → 500), so a job-log read was not possible. This branch supplied a better test by accident. While recovering an interrupted agent's work I pushed a deliberately-labelled WIP commit that I knew carried three lint findings (`funcorder`, `unparam`, and one more). It was then fixed and force-pushed. Same branch, same files, about twenty minutes apart: | Commit | Content | CI status | | --- | --- | --- | | `ce1e46b` | WIP, known-bad lint | **`failure` — "Failing after 1m2s"**, run 103, `2026-08-09T06:57:42+02:00` | | `20a050b` | lint fixed | **`success` — "Successful in 3m3s"**, run 104, `2026-08-09T07:18:20+02:00` | That establishes four things a single log read could not: 1. **The runner executes the real gate.** It went red on findings introduced that same day. A stale or replayed job definition cannot fail on code it has never seen. 2. **It is content-sensitive**, not emitting a canned status — red then green on one branch, with the lint fixes as the only change. 3. **The durations track the pipeline's internal structure.** 1m2s to fail versus 3m3s to pass is exactly what this repo's Dockerfile produces: the fail-fast lint stage aborts the build before the longer test and build stages run. Timings that mirror the build graph are not artefacts of a replay. 4. **The timestamps are contemporaneous** with the pushes. ### Scope of the claim This proves the gate ran and discriminated correctly on these commits in this repo. It does not prove that every individual green in the merge queue was a fully uncached execution end to end, and it says nothing about the other repo, where the reported symptom is real and remains under investigation. If anything it narrows that: whatever is wrong there is not a site-wide Gitea Actions defect. No labels or assignments were changed on the strength of the alarm — the check came first, and the evidence held.

clawbot referenced this pull request

2026-08-10 14:30:12 +02:00

TOP PRIORITY: consolidate all open PRs onto one next branch, one PR #110

clawbot marked the pull request as work in progress 2026-08-10 14:39:25 +02:00

clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:40:55 +02:00

sneak was unassigned by clawbot

2026-08-10 14:41:04 +02:00

clawbot self-assigned this 2026-08-10 14:41:04 +02:00

clawbot marked the pull request as ready for review 2026-08-10 15:41:52 +02:00

clawbot changed target branch from main to next

2026-08-10 15:41:52 +02:00

clawbot added 1 commit 2026-08-10 15:41:52 +02:00

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

check / check (push) Successful in 3m3s

Details

20a050b49d

The context fx hands an OnStart hook is derived with
context.WithTimeout(ctx, StartTimeout) — 15 seconds by default — and
is cancelled once the start phase ends. It is a start-phase context,
not an application-lifetime one. Two components derived their
long-lived loops from it and so stopped running roughly fifteen
seconds after boot.

Engine.start rooted the entire worker pool, restart recovery, and the
retry sweep in it. Every worker returned on ctx.Done() shortly after
startup, so the process kept receiving and persisting inbound events
while nothing at all forwarded them: deliveryCh filled up and started
logging "delivery channel full" with no consumer left. That is the
whole purpose of the application.

RetentionReaper.start had the same defect. With the default one-hour
RETENTION_SWEEP_INTERVAL the loop was cancelled forty-five minutes
before its first tick, so the reaper never ran a single sweep and
per-webhook event databases grew without bound.

Both now derive their loop context from context.Background(). Their
lifetime is bounded by OnStop, which already cancels and waits on the
WaitGroup, so shutdown is unchanged. Each hook registration moves into
a registerHooks method, the OnStart parameter is named _ so the trap
cannot be reintroduced by silencing an unused-parameter warning, and a
comment at each start explains why the hook context must not be used.
This matches the shape of the same fix applied to the archive sweeper.

The new lifecycle tests drive the genuine registered hooks with an
already-cancelled OnStart context and assert the loops still do work
afterwards — a task delivered, an expired event reaped. Reverting
either fix makes its pair of tests fail. Each component also gets a
shutdown test asserting OnStop cancels the loop and wg.Wait() returns
inside a bounded timeout, so the fix does not trade a startup bug for
a shutdown hang.

iWaitForStatus becomes iWaitForDelivered: every call site waits for
the delivered status, and the two added call sites pushed it past
unparam's threshold for reporting an always-identical argument.

clawbot merged commit 62481a6f1a into next

2026-08-10 15:44:56 +02:00

clawbot deleted branch issue-97-lifecycle-context

2026-08-10 15:44:57 +02:00

clawbot referenced this issue from a commit

2026-08-10 15:44:58 +02:00

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

~~clawbot referenced this pull request 2026-08-10 15:45:30 +02:00~~

Milestone 1.0.0: internet-facing readiness #111

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#100

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

The two fixes

Tests

Mutation evidence

Tree-wide sweep

Lint findings

Verification

Relationship to PR #95

Review of PR #100 — 20a050b against main 4f5ecb1

Definition of done — all five items met

1. Mutation verification (executed)

2. Shutdown is not traded away (executed)

3. The unparam fix did not weaken any assertion (verified by reading)

4. Tree-wide sweep (verified independently, not taken on trust)

5. internal/delivery/archive_sweeper.go (verified)

6. Nothing legitimately provided by the hook context is lost (verified by reading fx source)

7. Suppressions (executed)

8. Repo policy (verified)

9. CI and build verification (executed)

Non-blocking observations

Summary

Manager note

Why I am confident

One correction to the PR's own narrative

Non-blocking, tracked

Merge ordering

Verification re-check: the green is real

CI integrity: proven by a red/green pair on this very branch

Scope of the claim

Review of PR #100 — `20a050b` against `main` `4f5ecb1`

3. The `unparam` fix did not weaken any assertion (verified by reading)

5. `internal/delivery/archive_sweeper.go` (verified)