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

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

clawbot commented

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

Closes #82.

Behaviour chosen, and why

Option 1 from the issue, as settled in the manager's "Implementation
requirements" comment: an orphaned retrying delivery whose target type no
longer supports retries is terminally marked failed, with a recorded
reason.

Both recovery paths previously looked the delivery's target up in the registry
and, when it did not implement the internal rescheduler, returned silently:

recoverSingleRetry — restart recovery
sweepSingleRetry — the 60s retry sweep

If a target's type was edited from a retry type (http/slack) to a
fire-and-forget type (database/log) — or to a type unknown to the registry,
where the map lookup yields nil and the assertion fails identically — while one
of its deliveries was still retrying, that delivery stayed retrying
forever.

Re-dispatching under the new type was rejected: it would perform a delivery the
operator never asked for, with external consequences. Leaving the row stuck is
the unbounded-state problem the rest of this milestone has been closing. The
event itself remains durably stored in the per-webhook event database, so
manual redelivery can recover it deliberately.

The shared helper

Both sites now call one helper, so the terminal transition exists exactly once
and cannot drift:

func (e *Engine) failUnretryableRetry(
    webhookDB *gorm.DB,
    webhookID string,
    d *database.Delivery,
    target *database.Target,
)

It logs at warn, not error — this is operator-caused state, not a system
fault — naming the delivery, the target (id and name), and the current type, so
the operator can connect it to their own edit. It then records a
DeliveryResult via the existing recordResult (attempt countAttempts+1,
success=false, error text naming the current type) and marks the delivery
failed via the existing updateDeliveryStatus. Status is never set without a
result row, so the transition is not opaque in the event log.

The normal path is untouched: fire-and-forget targets never set status
retrying themselves, so a normally-operating database or log delivery is
still never seen by recovery or the sweep.

The diff is confined to the two rescheduler sites, the new helper, and the two
stale doc comments, so the rebase over PRs #95 and #100 stays mechanical.

Tests

internal/delivery/engine_integration_test.go, plus one new
ExportSweepWebhookRetries so the sweep path is reachable from the black-box
tests:

TestRecoverSingleRetry_TypeNoLongerRetries — target type now log;
recovery ends the delivery failed with a second result row naming the type
TestSweepSingleRetry_TypeNoLongerRetries — same, target type now
database, driven through the sweep
TestRecoverSingleRetry_UnknownTargetType /
TestSweepSingleRetry_UnknownTargetType — type not-a-target-type; same
outcome through both paths, no panic
TestRecoverWebhookDeliveries_RetryingDeliveries gains the regression guard:
a retrying delivery whose target is still http is rescheduled normally,
stays retrying, and gains no synthetic result row

Each terminal-failure assertion also checks the delivery was not pushed onto
the retry channel.

Mutation evidence

With both call sites reverted to a bare return (helper and tests left in
place), make test:

--- FAIL: TestRecoverSingleRetry_TypeNoLongerRetries (1.52s)
--- FAIL: TestSweepSingleRetry_TypeNoLongerRetries (1.47s)
--- FAIL: TestRecoverSingleRetry_UnknownTargetType (1.37s)
--- FAIL: TestSweepSingleRetry_UnknownTargetType (1.37s)
FAIL	sneak.berlin/go/webhooker/internal/delivery	3.467s

Each failed on expected: "failed" / actual: "retrying" and on the missing
result row. The regression guard passed under the mutation, as it should — it
asserts unchanged behaviour. The fix was then restored and the suite is green
again.

Docs

README "Recovery paths" now states that changing a target's type does not
migrate in-flight deliveries: any delivery still retrying under the old
type is terminally failed with a recorded reason and can be redelivered
manually.
TODO.md updated in the same commit.

Verification

make fmt — clean
make check — green, 0 issues from the pinned linter
script/cibuild — genuinely executed, not cached: 4m28s wall, the
RUN make check layer ran for 81s with live go test output (no CACHED
marker on it); the tree had changed, so the COPY . . layer invalidated the
cache below it
.golangci.yml untouched (sha256 021cc83f...); the v2.12.2 Dockerfile pin
is unchanged

Closes #82. ## Behaviour chosen, and why Option 1 from the issue, as settled in the manager's "Implementation requirements" comment: an orphaned `retrying` delivery whose target type no longer supports retries is **terminally marked `failed`, with a recorded reason**. Both recovery paths previously looked the delivery's target up in the registry and, when it did not implement the internal `rescheduler`, returned silently: - `recoverSingleRetry` — restart recovery - `sweepSingleRetry` — the 60s retry sweep If a target's `type` was edited from a retry type (`http`/`slack`) to a fire-and-forget type (`database`/`log`) — or to a type unknown to the registry, where the map lookup yields nil and the assertion fails identically — while one of its deliveries was still `retrying`, that delivery stayed `retrying` forever. Re-dispatching under the new type was rejected: it would perform a delivery the operator never asked for, with external consequences. Leaving the row stuck is the unbounded-state problem the rest of this milestone has been closing. The event itself remains durably stored in the per-webhook event database, so manual redelivery can recover it deliberately. ## The shared helper Both sites now call one helper, so the terminal transition exists exactly once and cannot drift: ``` func (e *Engine) failUnretryableRetry( webhookDB *gorm.DB, webhookID string, d *database.Delivery, target *database.Target, ) ``` It logs at **warn**, not error — this is operator-caused state, not a system fault — naming the delivery, the target (id and name), and the current type, so the operator can connect it to their own edit. It then records a `DeliveryResult` via the existing `recordResult` (attempt `countAttempts+1`, `success=false`, error text naming the current type) and marks the delivery `failed` via the existing `updateDeliveryStatus`. Status is never set without a result row, so the transition is not opaque in the event log. The normal path is untouched: fire-and-forget targets never set status `retrying` themselves, so a normally-operating `database` or `log` delivery is still never seen by recovery or the sweep. The diff is confined to the two `rescheduler` sites, the new helper, and the two stale doc comments, so the rebase over PRs #95 and #100 stays mechanical. ## Tests `internal/delivery/engine_integration_test.go`, plus one new `ExportSweepWebhookRetries` so the sweep path is reachable from the black-box tests: - `TestRecoverSingleRetry_TypeNoLongerRetries` — target type now `log`; recovery ends the delivery `failed` with a second result row naming the type - `TestSweepSingleRetry_TypeNoLongerRetries` — same, target type now `database`, driven through the sweep - `TestRecoverSingleRetry_UnknownTargetType` / `TestSweepSingleRetry_UnknownTargetType` — type `not-a-target-type`; same outcome through both paths, no panic - `TestRecoverWebhookDeliveries_RetryingDeliveries` gains the regression guard: a `retrying` delivery whose target is still `http` is rescheduled normally, stays `retrying`, and gains no synthetic result row Each terminal-failure assertion also checks the delivery was not pushed onto the retry channel. ## Mutation evidence With both call sites reverted to a bare `return` (helper and tests left in place), `make test`: ``` --- FAIL: TestRecoverSingleRetry_TypeNoLongerRetries (1.52s) --- FAIL: TestSweepSingleRetry_TypeNoLongerRetries (1.47s) --- FAIL: TestRecoverSingleRetry_UnknownTargetType (1.37s) --- FAIL: TestSweepSingleRetry_UnknownTargetType (1.37s) FAIL sneak.berlin/go/webhooker/internal/delivery 3.467s ``` Each failed on `expected: "failed" / actual: "retrying"` and on the missing result row. The regression guard passed under the mutation, as it should — it asserts unchanged behaviour. The fix was then restored and the suite is green again. ## Docs - README "Recovery paths" now states that changing a target's type does not migrate in-flight deliveries: any delivery still `retrying` under the old type is terminally failed with a recorded reason and can be redelivered manually. - `TODO.md` updated in the same commit. ## Verification - `make fmt` — clean - `make check` — green, `0 issues` from the pinned linter - `script/cibuild` — **genuinely executed, not cached**: 4m28s wall, the `RUN make check` layer ran for 81s with live `go test` output (no `CACHED` marker on it); the tree had changed, so the `COPY . .` layer invalidated the cache below it - `.golangci.yml` untouched (sha256 `021cc83f...`); the v2.12.2 Dockerfile pin is unchanged

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

clawbot self-assigned this 2026-08-09 07:57:17 +02:00

clawbot commented

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

What this builds

Single commit 0384de4 on issue-82-orphaned-retrying, branched from main
@ 4f5ecb1.

Both places that reschedule an orphaned retrying delivery —
recoverSingleRetry (restart recovery) and sweepSingleRetry (the 60s sweep)
— used to return silently when the delivery's target did not implement the
internal rescheduler. They now call one shared helper,
failUnretryableRetry, which:

logs at warn (operator-caused state, not a system fault) naming the
webhook, delivery, target id/name, and the current target type;
records a DeliveryResult through the existing recordResult — attempt
countAttempts+1, success=false, error text naming the current type; and
marks the delivery failed through the existing updateDeliveryStatus.

That is option 1 from the issue, as settled in the manager's implementation
requirements. The delivery is deliberately not re-dispatched under the new
type — that would be an outbound delivery the operator never requested — and
the event itself stays in the per-webhook event database, so manual redelivery
can recover it on purpose.

An unknown/garbage target type takes the same path for free:
e.targets[type] yields nil and the rescheduler assertion fails identically.

Files touched: internal/delivery/engine.go (two call sites, the new helper,
two now-stale doc comments), internal/delivery/export_test.go (one new
ExportSweepWebhookRetries), internal/delivery/engine_integration_test.go,
README.md, TODO.md. Nothing else in engine.go moved, so the rebase over
PRs #95 and #100 should be mechanical.

How I verified it

Mutation test. I reverted both call sites to a bare return, leaving the
helper and the tests in place, and ran make test:

--- FAIL: TestRecoverSingleRetry_TypeNoLongerRetries (1.52s)
--- FAIL: TestSweepSingleRetry_TypeNoLongerRetries (1.47s)
--- FAIL: TestRecoverSingleRetry_UnknownTargetType (1.37s)
--- FAIL: TestSweepSingleRetry_UnknownTargetType (1.37s)
FAIL	sneak.berlin/go/webhooker/internal/delivery	3.467s

Every one failed on both assertions that matter —
expected: "failed" / actual: "retrying", and the result row that should have
recorded the reason being absent (1 row, not 2). The regression guard added to
TestRecoverWebhookDeliveries_RetryingDeliveries passed under the mutation, as
it must: it asserts that an http target is still rescheduled normally,
behaviour this change does not alter. I then restored both call sites and the
suite went green again.

make fmt — clean, no diff produced beyond my edits.

make check — green, 0 issues from the pinned v2.12.2 linter. The only
output beyond that is the pre-existing gomodguard deprecation warning that
main also emits; .golangci.yml is untouched (sha256 021cc83f...).

script/cibuild — genuinely executed, not served from cache. 4m28s
wall clock; the RUN make check layer is #27 DONE 81.0s with live go test
output streaming through it and no CACHED marker. The commit changed the
tree, so the COPY . . layer invalidated everything below it. This also means
the full suite ran once from scratch inside the pinned CI image after the
mutation was reverted.

Gitea CI on head commit 0384de4 is running; I will note the result below
if it is not visible on the PR by the time a reviewer picks this up.

## What this builds Single commit `0384de4` on `issue-82-orphaned-retrying`, branched from `main` @ `4f5ecb1`. Both places that reschedule an orphaned `retrying` delivery — `recoverSingleRetry` (restart recovery) and `sweepSingleRetry` (the 60s sweep) — used to `return` silently when the delivery's target did not implement the internal `rescheduler`. They now call one shared helper, `failUnretryableRetry`, which: 1. logs at **warn** (operator-caused state, not a system fault) naming the webhook, delivery, target id/name, and the current target type; 2. records a `DeliveryResult` through the existing `recordResult` — attempt `countAttempts+1`, `success=false`, error text naming the current type; and 3. marks the delivery `failed` through the existing `updateDeliveryStatus`. That is option 1 from the issue, as settled in the manager's implementation requirements. The delivery is deliberately **not** re-dispatched under the new type — that would be an outbound delivery the operator never requested — and the event itself stays in the per-webhook event database, so manual redelivery can recover it on purpose. An unknown/garbage target type takes the same path for free: `e.targets[type]` yields nil and the `rescheduler` assertion fails identically. Files touched: `internal/delivery/engine.go` (two call sites, the new helper, two now-stale doc comments), `internal/delivery/export_test.go` (one new `ExportSweepWebhookRetries`), `internal/delivery/engine_integration_test.go`, `README.md`, `TODO.md`. Nothing else in `engine.go` moved, so the rebase over PRs #95 and #100 should be mechanical. ## How I verified it **Mutation test.** I reverted both call sites to a bare `return`, leaving the helper and the tests in place, and ran `make test`: ``` --- FAIL: TestRecoverSingleRetry_TypeNoLongerRetries (1.52s) --- FAIL: TestSweepSingleRetry_TypeNoLongerRetries (1.47s) --- FAIL: TestRecoverSingleRetry_UnknownTargetType (1.37s) --- FAIL: TestSweepSingleRetry_UnknownTargetType (1.37s) FAIL sneak.berlin/go/webhooker/internal/delivery 3.467s ``` Every one failed on both assertions that matter — `expected: "failed" / actual: "retrying"`, and the result row that should have recorded the reason being absent (1 row, not 2). The regression guard added to `TestRecoverWebhookDeliveries_RetryingDeliveries` passed under the mutation, as it must: it asserts that an `http` target is still rescheduled normally, behaviour this change does not alter. I then restored both call sites and the suite went green again. **`make fmt`** — clean, no diff produced beyond my edits. **`make check`** — green, `0 issues` from the pinned v2.12.2 linter. The only output beyond that is the pre-existing `gomodguard` deprecation warning that `main` also emits; `.golangci.yml` is untouched (sha256 `021cc83f...`). **`script/cibuild`** — **genuinely executed, not served from cache.** 4m28s wall clock; the `RUN make check` layer is `#27 DONE 81.0s` with live `go test` output streaming through it and no `CACHED` marker. The commit changed the tree, so the `COPY . .` layer invalidated everything below it. This also means the full suite ran once from scratch inside the pinned CI image after the mutation was reverted. **Gitea CI** on head commit `0384de4` is running; I will note the result below if it is not visible on the PR by the time a reviewer picks this up.

clawbot commented

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

Gitea CI on head commit 0384de4 finished success (check / check (push),
run 106). It took roughly 4m40s from queue to green — a genuine full build and
check inside the pinned image, not an instant cached pass.

Gitea CI on head commit `0384de4` finished **success** (`check / check (push)`, run 106). It took roughly 4m40s from queue to green — a genuine full build and check inside the pinned image, not an instant cached pass.

clawbot commented

2026-08-09 08:05:34 +02:00

Lint evidence re-validated (and one correction to the record)

script/lint invokes the host golangci-lint, not the pinned Docker
image, so it shares one cache and one lock with every other session on this
box. Two known failure modes make a local lint verdict untrustworthy: a lock
collision (parallel golangci-lint is running, which is not a result), and
cross-contamination where findings from an unrelated worktree appear in the
output. I re-ran the gate treating any run exhibiting either as void.

Method: two fresh worktrees, linted sequentially — one at this PR's head
0384de4, one at a clean origin/main 4f5ecb1 baseline — each retried until
a run was valid, i.e. contained no lock message and named no file path outside
its own worktree.

Run	Valid on	Verdict
head `0384de4`	attempt 1	`0 issues.`
clean `main` `4f5ecb1`	attempt 1	`0 issues.`

I then ran a full make check at the head commit and scanned the complete
2037-line output, not just the tail: zero occurrences of the lock message, zero
file paths outside the worktree, 0 issues., no test failures, exit 0.

Correction. My brief carried a stale premise worth flagging for anyone
working this repo: it stated the host linter is v2.10.1 and that it reports a
pre-existing gosec G704 in internal/delivery/client_ssrf_test.go. The host
binary is in fact v2.12.2 — the same version the Dockerfile pins — and that
G704 finding does not reproduce, on my branch or on a clean main. Both
lint to 0 issues. So there is no pre-existing finding to discount here, and
no host/CI version skew on this machine right now. I did not modify
.golangci.yml (sha256 021cc83f...) or the Dockerfile pin.

Since a local gate on this host can currently produce an unearned green, an
unearned red, or another codebase's findings, the strongest evidence for this
PR remains the Gitea CI run on the pushed head commit 0384de4: success,
~4m40s — a genuine full build and check inside the pinned image.

## Lint evidence re-validated (and one correction to the record) `script/lint` invokes the **host** `golangci-lint`, not the pinned Docker image, so it shares one cache and one lock with every other session on this box. Two known failure modes make a local lint verdict untrustworthy: a lock collision (`parallel golangci-lint is running`, which is not a result), and cross-contamination where findings from an unrelated worktree appear in the output. I re-ran the gate treating any run exhibiting either as void. Method: two fresh worktrees, linted sequentially — one at this PR's head `0384de4`, one at a clean `origin/main` `4f5ecb1` baseline — each retried until a run was valid, i.e. contained no lock message and named no file path outside its own worktree. | Run | Valid on | Verdict | | --- | --- | --- | | head `0384de4` | attempt 1 | `0 issues.` | | clean `main` `4f5ecb1` | attempt 1 | `0 issues.` | I then ran a full `make check` at the head commit and scanned the **complete** 2037-line output, not just the tail: zero occurrences of the lock message, zero file paths outside the worktree, `0 issues.`, no test failures, exit 0. **Correction.** My brief carried a stale premise worth flagging for anyone working this repo: it stated the host linter is v2.10.1 and that it reports a pre-existing `gosec` G704 in `internal/delivery/client_ssrf_test.go`. The host binary is in fact **v2.12.2** — the same version the Dockerfile pins — and that G704 finding does **not** reproduce, on my branch or on a clean `main`. Both lint to `0 issues`. So there is no pre-existing finding to discount here, and no host/CI version skew on this machine right now. I did not modify `.golangci.yml` (sha256 `021cc83f...`) or the Dockerfile pin. Since a local gate on this host can currently produce an unearned green, an unearned red, or another codebase's findings, the strongest evidence for this PR remains the **Gitea CI run on the pushed head commit `0384de4`: `success`, ~4m40s** — a genuine full build and check inside the pinned image.

clawbot commented

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

Independent review of PR #104 — head `0384de4`, base `main` @ `4f5ecb1`

Verdict: PASS

No blocking defects. The change delivers option 1 as specified in the
## Implementation requirements comment on #82, both recovery sites are
genuinely covered by one shared helper, and the tests are load-bearing under
mutation. Six non-blocking observations are listed at the end; none should
hold up a merge.

Tooling honesty statement

script/cibuild: CACHED, not executed — my run is NOT evidence. Wall time
0.53s (real 0m0.528s), every layer from #11 through #32 reported
CACHED, including the RUN make check layer. I am not repeating the author's
claim of a 4m28s genuine run; I could not reproduce it, because the cache
already held a layer built from this exact tree. I substituted real evidence
below.

Substitute CI evidence (verified): the Gitea commit status on head
0384de4 is check / check (push) = success, described
Successful in 4m4s, run 106. A four-minute run is a genuine full build and
check inside the pinned image, not an instant cached pass.

Lint runs — which were valid and by what test. I treated a run as VOID if
the output contained parallel golangci-lint is running, or if any
reported path began with ../ or with an absolute path outside the worktree I
launched from.

Run 1 (make lint, from my own worktree): VOID. 34 issues, every one
attributed to ../wt82-lint/internal/... — a different worktree entirely.
Cross-worktree cache contamination, exactly the ../-relative form. Not
recorded in either direction.
After golangci-lint cache clean, run 2: VALID. 0 issues. No lock
error, no path outside my worktree. Only other output is the tracked
gomodguard deprecation warning (#98), which is a warning, not a finding.
Final consolidated make check: VALID, rc=0, 0 issues.

The first make check I launched also produced a VOID lint tail
(Error: parallel golangci-lint is running, make: *** [Makefile:25: check] Error 3) — its test phase had already completed green, but I did not record
a lint result from it.

Note on the pre-existing gosec G704 in internal/delivery/client_ssrf_test.go:
it does not reproduce. My valid runs report 0 issues. There was nothing
to discount, and I discounted nothing.

Verified by execution

Both sites covered, and the helper is genuinely shared. Mutation A —
reverting both call sites to a bare return, helper and tests left in place —
reproduces the author's reported result exactly:

--- FAIL: TestRecoverSingleRetry_TypeNoLongerRetries
--- FAIL: TestSweepSingleRetry_TypeNoLongerRetries
--- FAIL: TestRecoverSingleRetry_UnknownTargetType
--- FAIL: TestSweepSingleRetry_UnknownTargetType
--- PASS: TestRecoverWebhookDeliveries_RetryingDeliveries

Each failed on both expected: "failed" / actual: "retrying" and on the
missing result row. Because the two call sites are byte-identical, a single
replace_all edit hit both — confirming neither site was left with a silent
return and neither carries a divergent copy of the transition.

Mutations the author did NOT list. I ran three more:

Remove only recordResult, keep the status flip — 4 FAIL, on
should have 2 item(s), but has 1. The result row is independently
asserted; a status-only flip cannot pass.
Remove only updateDeliveryStatus, keep the result row — 4 FAIL, on
expected: "failed". The status transition is independently asserted.
Change the log level Warn -> Error — suite stays green. This is
a real coverage gap, but non-blocking; see observation 1.

The regression guard actually guards. I applied the helper to retryable
targets by hoisting the call above the if !ok in recoverSingleRetry, so a
still-http target would also be terminally failed. Result:
--- FAIL: TestRecoverWebhookDeliveries_RetryingDeliveries. The guard is not
vacuous — it fails when the helper is wrongly applied, and passes under
mutation A because it asserts behaviour this change does not alter. Both
directions confirmed.

Checks. make check rc=0; tests green (I have one full uncached verbose
run plus five mutation runs); lint 0 issues. on a valid run; make fmt-check
clean. make check modified nothing — git status --porcelain empty
afterwards.

Mergeable. Merge base is 4f5ecb1, which is the current origin/main
tip, so the branch is a clean fast-forward; the tracker also reports
mergeable: true. No rebase needed.

Verified by reading

Empty-string target type. e.targets[""] returns the nil zero value, and
nil.(rescheduler) yields (nil, false) — no panic, same terminal path. The
reason string renders it as target type "" does not support retries.
TargetType is a bare string with no DB-level enum, so "" is reachable by
a hand edit. It is not separately tested, but the code path is identical to the
tested not-a-target-type case (both are missing map keys), so this is not a
gap worth blocking on.

The result row is real and useful. Attempt number is
countAttempts(...)+1 = 2 against the seeded single prior attempt — sane and
non-colliding. success=false. The error text names the current target type
via %q and is asserted with two independent Contains checks (the type
string, and does not support retries). Not opaque.

Idempotency / double-transition. Both recoverRetryingDeliveries and
sweepWebhookRetries select on status = retrying. Once the helper flips the
row to failed, neither query selects it again, so a second sweep is a no-op
and restart recovery followed by a sweep cannot double-transition or duplicate
the result row. Traced; clean.

Idiom consistency. recordResult(...) immediately followed by
updateDeliveryStatus(..., DeliveryStatusFailed) is the established pattern in
this package — see internal/delivery/target_http.go:299-306. The new helper
matches it exactly. Naming (failUnretryableRetry, ExportSweepWebhookRetries)
is consistent with recoverSingleRetry / sweepSingleRetry /
ExportRecoverWebhookDeliveries. No stutter.

Scope discipline. 5 files, +276/-4:
internal/delivery/engine.go (two call sites, the new helper, two stale doc
comments), internal/delivery/export_test.go (one new export),
internal/delivery/engine_integration_test.go, README.md, TODO.md. Nothing
else in engine.go moved. The rebase over PRs #95 and #100 stays mechanical.

Policy.

.golangci.yml untouched; sha256 is
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb as
required.
Dockerfile pin intact: golangci/golangci-lint:v2.12.2@sha256:5cceeef0....
Single commit; title ends with (closes #82).
TODO.md updated in the same commit.
No Claude/Anthropic references, no attribution or session trailers anywhere
in the commit message, body, code, or docs — checked; zero hits.
No 4-byte emoji.
No new //nolint.
Inclusive terminology clean.
Markdown: the repo has no prettier config and script/fmt / script/fmt-check
are gofmt-only, so make fmt is clean by the repo's own definition. The added
README and TODO.md prose wraps at <=73 columns, matching the surrounding
file exactly.

Non-blocking observations

Log level is untested. Mutating Warn -> Error leaves the suite
green, so requirement 2 of the spec ("log at warn, not error") is satisfied
in the code but not defended by a test. There is no precedent anywhere in
this repo for asserting log levels, so this is consistent with local norms
rather than a defect of this PR. Worth a follow-up only if log-level
assertions are adopted generally.
Write-failure window. If recordResult succeeds and
updateDeliveryStatus then fails, the row stays retrying and the next
sweep adds a second result row. Self-converging once the DB recovers, and
both failures log at error. Acceptable.
Recovery/sweep overlap. Restart recovery runs in its own goroutine while
the 60s sweep ticks. If recovery ran longer than 60s, both could touch the
same row; worst case is one duplicate result row and a redundant identical
write of the same terminal status. No external side effect.
Pre-existing, out of scope: processDelivery
(internal/delivery/engine.go:783-796) fails an unknown-target-type
delivery with updateDeliveryStatus(..., Failed) and no result row —
precisely the opacity the spec forbade for the new path. This PR was
correctly scoped to the two rescheduler sites and should not have touched
it. Worth its own issue.
Deleted target row. If loadTarget errors (target deleted rather than
retyped), both sites still log and return, leaving the delivery stuck in
retrying — the same class of unbounded state #82 closed, via a different
trigger. Distinct from #82's scenario and correctly out of scope here.
Worth its own issue.
TODO.md staleness is pre-existing. The # Next Step section still
describes retention cleanup, which already landed, and # Status cites
main (afe88c6). Both were already stale on 4f5ecb1; this PR's obligation
was a Completed Steps entry in the same commit, which it delivers.

Labels and assignees deliberately left unchanged.

## Independent review of PR #104 — head `0384de4`, base `main` @ `4f5ecb1` ## Verdict: PASS No blocking defects. The change delivers option 1 as specified in the `## Implementation requirements` comment on #82, both recovery sites are genuinely covered by one shared helper, and the tests are load-bearing under mutation. Six non-blocking observations are listed at the end; none should hold up a merge. --- ## Tooling honesty statement **`script/cibuild`: CACHED, not executed — my run is NOT evidence.** Wall time 0.53s (`real 0m0.528s`), every layer from `#11` through `#32` reported `CACHED`, including the `RUN make check` layer. I am not repeating the author's claim of a 4m28s genuine run; I could not reproduce it, because the cache already held a layer built from this exact tree. I substituted real evidence below. **Substitute CI evidence (verified):** the Gitea commit status on head `0384de4` is `check / check (push)` = **success**, described `Successful in 4m4s`, run 106. A four-minute run is a genuine full build and check inside the pinned image, not an instant cached pass. **Lint runs — which were valid and by what test.** I treated a run as VOID if the output contained `parallel golangci-lint is running`, **or** if any reported path began with `../` or with an absolute path outside the worktree I launched from. - Run 1 (`make lint`, from my own worktree): **VOID.** 34 issues, every one attributed to `../wt82-lint/internal/...` — a different worktree entirely. Cross-worktree cache contamination, exactly the `../`-relative form. Not recorded in either direction. - After `golangci-lint cache clean`, run 2: **VALID.** `0 issues.` No lock error, no path outside my worktree. Only other output is the tracked `gomodguard` deprecation warning (#98), which is a warning, not a finding. - Final consolidated `make check`: **VALID**, `rc=0`, `0 issues.` The first `make check` I launched also produced a **VOID** lint tail (`Error: parallel golangci-lint is running`, `make: *** [Makefile:25: check] Error 3`) — its *test* phase had already completed green, but I did not record a lint result from it. **Note on the pre-existing `gosec` G704 in `internal/delivery/client_ssrf_test.go`:** it does **not** reproduce. My valid runs report `0 issues.` There was nothing to discount, and I discounted nothing. --- ## Verified by execution **Both sites covered, and the helper is genuinely shared.** Mutation A — reverting both call sites to a bare `return`, helper and tests left in place — reproduces the author's reported result exactly: ``` --- FAIL: TestRecoverSingleRetry_TypeNoLongerRetries --- FAIL: TestSweepSingleRetry_TypeNoLongerRetries --- FAIL: TestRecoverSingleRetry_UnknownTargetType --- FAIL: TestSweepSingleRetry_UnknownTargetType --- PASS: TestRecoverWebhookDeliveries_RetryingDeliveries ``` Each failed on both `expected: "failed" / actual: "retrying"` and on the missing result row. Because the two call sites are byte-identical, a single `replace_all` edit hit both — confirming neither site was left with a silent `return` and neither carries a divergent copy of the transition. **Mutations the author did NOT list.** I ran three more: - **Remove only `recordResult`, keep the status flip** — 4 FAIL, on `should have 2 item(s), but has 1`. The result row is independently asserted; a status-only flip cannot pass. - **Remove only `updateDeliveryStatus`, keep the result row** — 4 FAIL, on `expected: "failed"`. The status transition is independently asserted. - **Change the log level `Warn` -> `Error`** — suite stays **green**. This is a real coverage gap, but non-blocking; see observation 1. **The regression guard actually guards.** I applied the helper to *retryable* targets by hoisting the call above the `if !ok` in `recoverSingleRetry`, so a still-`http` target would also be terminally failed. Result: `--- FAIL: TestRecoverWebhookDeliveries_RetryingDeliveries`. The guard is not vacuous — it fails when the helper is wrongly applied, and passes under mutation A because it asserts behaviour this change does not alter. Both directions confirmed. **Checks.** `make check` `rc=0`; tests green (I have one full uncached verbose run plus five mutation runs); lint `0 issues.` on a valid run; `make fmt-check` clean. `make check` modified nothing — `git status --porcelain` empty afterwards. **Mergeable.** Merge base is `4f5ecb1`, which is the current `origin/main` tip, so the branch is a clean fast-forward; the tracker also reports `mergeable: true`. No rebase needed. --- ## Verified by reading **Empty-string target type.** `e.targets[""]` returns the nil zero value, and `nil.(rescheduler)` yields `(nil, false)` — no panic, same terminal path. The `reason` string renders it as `target type "" does not support retries`. `TargetType` is a bare `string` with no DB-level enum, so `""` is reachable by a hand edit. It is not separately tested, but the code path is identical to the tested `not-a-target-type` case (both are missing map keys), so this is not a gap worth blocking on. **The result row is real and useful.** Attempt number is `countAttempts(...)+1` = 2 against the seeded single prior attempt — sane and non-colliding. `success=false`. The error text names the current target type via `%q` and is asserted with two independent `Contains` checks (the type string, and `does not support retries`). Not opaque. **Idempotency / double-transition.** Both `recoverRetryingDeliveries` and `sweepWebhookRetries` select on `status = retrying`. Once the helper flips the row to `failed`, neither query selects it again, so a second sweep is a no-op and restart recovery followed by a sweep cannot double-transition or duplicate the result row. Traced; clean. **Idiom consistency.** `recordResult(...)` immediately followed by `updateDeliveryStatus(..., DeliveryStatusFailed)` is the established pattern in this package — see `internal/delivery/target_http.go:299-306`. The new helper matches it exactly. Naming (`failUnretryableRetry`, `ExportSweepWebhookRetries`) is consistent with `recoverSingleRetry` / `sweepSingleRetry` / `ExportRecoverWebhookDeliveries`. No stutter. **Scope discipline.** 5 files, +276/-4: `internal/delivery/engine.go` (two call sites, the new helper, two stale doc comments), `internal/delivery/export_test.go` (one new export), `internal/delivery/engine_integration_test.go`, `README.md`, `TODO.md`. Nothing else in `engine.go` moved. The rebase over PRs #95 and #100 stays mechanical. **Policy.** - `.golangci.yml` untouched; sha256 is `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` as required. - `Dockerfile` pin intact: `golangci/golangci-lint:v2.12.2@sha256:5cceeef0...`. - Single commit; title ends with ` (closes #82)`. - `TODO.md` updated in the same commit. - No Claude/Anthropic references, no attribution or session trailers anywhere in the commit message, body, code, or docs — checked; zero hits. - No 4-byte emoji. - No new `//nolint`. - Inclusive terminology clean. - Markdown: the repo has no prettier config and `script/fmt` / `script/fmt-check` are gofmt-only, so `make fmt` is clean by the repo's own definition. The added README and `TODO.md` prose wraps at <=73 columns, matching the surrounding file exactly. --- ## Non-blocking observations 1. **Log level is untested.** Mutating `Warn` -> `Error` leaves the suite green, so requirement 2 of the spec ("log at warn, not error") is satisfied in the code but not defended by a test. There is no precedent anywhere in this repo for asserting log levels, so this is consistent with local norms rather than a defect of this PR. Worth a follow-up only if log-level assertions are adopted generally. 2. **Write-failure window.** If `recordResult` succeeds and `updateDeliveryStatus` then fails, the row stays `retrying` and the next sweep adds a second result row. Self-converging once the DB recovers, and both failures log at error. Acceptable. 3. **Recovery/sweep overlap.** Restart recovery runs in its own goroutine while the 60s sweep ticks. If recovery ran longer than 60s, both could touch the same row; worst case is one duplicate result row and a redundant identical write of the same terminal status. No external side effect. 4. **Pre-existing, out of scope:** `processDelivery` (`internal/delivery/engine.go:783-796`) fails an unknown-target-type delivery with `updateDeliveryStatus(..., Failed)` and **no result row** — precisely the opacity the spec forbade for the new path. This PR was correctly scoped to the two rescheduler sites and should not have touched it. Worth its own issue. 5. **Deleted target row.** If `loadTarget` errors (target deleted rather than retyped), both sites still log and `return`, leaving the delivery stuck in `retrying` — the same class of unbounded state #82 closed, via a different trigger. Distinct from #82's scenario and correctly out of scope here. Worth its own issue. 6. **`TODO.md` staleness is pre-existing.** The `# Next Step` section still describes retention cleanup, which already landed, and `# Status` cites `main (afe88c6)`. Both were already stale on `4f5ecb1`; this PR's obligation was a `Completed Steps` entry in the same commit, which it delivers. --- Labels and assignees deliberately left unchanged.

clawbot referenced this pull request

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

Two remaining delivery terminal-state gaps: opaque failure and deletion-orphaned retries #107

clawbot commented

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

Manager note

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

Verification quality

This review is worth reading as a model of handling the current tooling hazards, because it hit two of them live and neither corrupted its verdict:

Its own script/cibuild was fully cached — 0.528s wall, every layer CACHED including RUN make check. It said so plainly, did not repeat the author's claimed 4m28s genuine run, and substituted the Gitea CI status on 0384de4: success in 4m4s (run 106). That is the right instinct — the author's run was almost certainly genuine (it produced the cache the reviewer then hit), but a reviewer repeating an unverifiable claim is how unearned greens propagate.
Its first two lint attempts were void — one parallel golangci-lint is running, then a run reporting 34 issues all attributed to ../wt82-lint/internal/..., the relative-path contamination form. It ran golangci-lint cache clean, retried, and got a valid 0 issues. Nothing was recorded from a void run.

It also independently confirmed the correction I circulated: no host/CI linter version skew, and the gosec G704 in client_ssrf_test.go does not reproduce. Nothing was waved away on the strength of a stale note.

What was verified by execution

Both sites genuinely share the helper. Reverting both call sites reproduces the author's four named failures on both the status flip and the missing result row.
Mutations the author did not list. Removing only recordResult fails four tests on should have 2 item(s), but has 1; removing only updateDeliveryStatus fails four on expected: "failed". Both halves of the terminal transition are independently asserted — that is the check that distinguishes a real fix from a status flip with a decorative result row.
The regression guard actually guards. Hoisting the helper call above the if !ok — so retryable http targets would also be failed — fails TestRecoverWebhookDeliveries_RetryingDeliveries. The guard is load-bearing, not incidental.
Idempotency traced: both queries filter status = retrying, so a flipped row is not re-selected.

Non-blocking, and two of them are new work

The log level survives mutation (Warn → Error stays green) — noted, but there is no precedent for log-level assertions anywhere in this repo and I am not going to invent one here.

Two findings were genuinely out of scope and are now tracked as #107:

processDelivery (engine.go:783-796) fails an unknown-target-type delivery with no result row — exactly the opacity this PR's spec forbade for the recovery and sweep paths. The codebase is now inconsistent: two paths explain themselves, one does not.
A deleted target row (rather than a retyped one) still leaves the delivery stuck in retrying forever, because loadTarget errors and both sites return. Deleting a target mid-retry is a more ordinary operator action than editing its type, so this is arguably likelier than the case #82 fixed.

Neither belongs in this PR. Both are the natural next step after it.

Labeled merge-ready and assigned to @sneak. Merge base is still 4f5ecb1, clean fast-forward — but #95 and #100 also touch internal/delivery/engine.go, so this should land after them.

## Manager note Independent review verdict: **PASS**, no blocking findings. The reviewer did not author this change. ### Verification quality This review is worth reading as a model of handling the current tooling hazards, because it hit two of them live and neither corrupted its verdict: - **Its own `script/cibuild` was fully cached** — 0.528s wall, every layer `CACHED` including `RUN make check`. It said so plainly, did **not** repeat the author's claimed 4m28s genuine run, and substituted the Gitea CI status on `0384de4`: **success in 4m4s** (run 106). That is the right instinct — the author's run was almost certainly genuine (it produced the cache the reviewer then hit), but a reviewer repeating an unverifiable claim is how unearned greens propagate. - **Its first two lint attempts were void** — one `parallel golangci-lint is running`, then a run reporting 34 issues all attributed to `../wt82-lint/internal/...`, the relative-path contamination form. It ran `golangci-lint cache clean`, retried, and got a valid `0 issues.` Nothing was recorded from a void run. It also independently confirmed the correction I circulated: no host/CI linter version skew, and the `gosec` G704 in `client_ssrf_test.go` does not reproduce. Nothing was waved away on the strength of a stale note. ### What was verified by execution - **Both sites genuinely share the helper.** Reverting both call sites reproduces the author's four named failures on both the status flip and the missing result row. - **Mutations the author did not list.** Removing only `recordResult` fails four tests on `should have 2 item(s), but has 1`; removing only `updateDeliveryStatus` fails four on `expected: "failed"`. Both halves of the terminal transition are independently asserted — that is the check that distinguishes a real fix from a status flip with a decorative result row. - **The regression guard actually guards.** Hoisting the helper call above the `if !ok` — so retryable `http` targets would also be failed — fails `TestRecoverWebhookDeliveries_RetryingDeliveries`. The guard is load-bearing, not incidental. - Idempotency traced: both queries filter `status = retrying`, so a flipped row is not re-selected. ### Non-blocking, and two of them are new work The log level survives mutation (`Warn` → `Error` stays green) — noted, but there is no precedent for log-level assertions anywhere in this repo and I am not going to invent one here. Two findings were genuinely out of scope and are now tracked as **#107**: 1. `processDelivery` (`engine.go:783-796`) fails an unknown-target-type delivery with **no result row** — exactly the opacity this PR's spec forbade for the recovery and sweep paths. The codebase is now inconsistent: two paths explain themselves, one does not. 2. A **deleted** target row (rather than a retyped one) still leaves the delivery stuck in `retrying` forever, because `loadTarget` errors and both sites return. Deleting a target mid-retry is a more ordinary operator action than editing its type, so this is arguably likelier than the case #82 fixed. Neither belongs in this PR. Both are the natural next step after it. Labeled `merge-ready` and assigned to @sneak. Merge base is still `4f5ecb1`, clean fast-forward — but #95 and #100 also touch `internal/delivery/engine.go`, so this should land after them.

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

clawbot removed their assignment 2026-08-09 08:13:30 +02:00

sneak was assigned by clawbot

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

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

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

sneak was unassigned by clawbot

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

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

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

clawbot changed target branch from main to next

2026-08-10 15:42:07 +02:00

clawbot added 1 commit 2026-08-10 15:42:07 +02:00

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

check / check (push) Successful in 4m4s

Details

0384de4a7b

Restart recovery and the 60s retry sweep both looked an orphaned
`retrying` delivery's target up in the registry and silently returned
when it did not implement `rescheduler`. If a target's type was edited
from a retry type (`http`/`slack`) to a fire-and-forget type
(`database`/`log`) or an unknown one while a delivery was still
retrying, that delivery stayed `retrying` forever.

Both sites now hand the delivery to one shared helper,
`failUnretryableRetry`, which records a `DeliveryResult` naming the
current target type as the reason and marks the delivery `failed`. It
logs at warn, not error: this is operator-caused state, not a system
fault.

Re-dispatching under the new type was rejected as it would perform a
delivery the operator never asked for; the event itself stays in the
per-webhook event database, so manual redelivery can recover it
deliberately.

Fire-and-forget targets never set status `retrying` under normal
operation, so this path stays unreachable for them in practice.

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

Milestone 1.0.0: internet-facing readiness #111

clawbot force-pushed issue-82-orphaned-retrying from 0384de4a7b to ead81298ed

2026-08-10 15:59:32 +02:00

Compare

clawbot merged commit 0ce8565f51 into next

2026-08-10 16:00:04 +02:00

clawbot deleted branch issue-82-orphaned-retrying

2026-08-10 16:00:05 +02:00

clawbot referenced this issue from a commit

2026-08-10 16:00:05 +02:00

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#104

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

Behaviour chosen, and why

The shared helper

Tests

Mutation evidence

Docs

Verification

What this builds

How I verified it

Lint evidence re-validated (and one correction to the record)

Independent review of PR #104 — head 0384de4, base main @ 4f5ecb1

Verdict: PASS

Tooling honesty statement

Verified by execution

Verified by reading

Non-blocking observations

Manager note

Verification quality

What was verified by execution

Non-blocking, and two of them are new work

Independent review of PR #104 — head `0384de4`, base `main` @ `4f5ecb1`