Terminally fail retrying deliveries with a non-retry target type (closes #82) #104
Reference in New Issue
Block a user
Delete Branch "issue-82-orphaned-retrying"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #82.
Behaviour chosen, and why
Option 1 from the issue, as settled in the manager's "Implementation
requirements" comment: an orphaned
retryingdelivery whose target type nolonger supports retries is terminally marked
failed, with a recordedreason.
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 recoverysweepSingleRetry— the 60s retry sweepIf a target's
typewas edited from a retry type (http/slack) to afire-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 stayedretryingforever.
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:
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
DeliveryResultvia the existingrecordResult(attemptcountAttempts+1,success=false, error text naming the current type) and marks the deliveryfailedvia the existingupdateDeliveryStatus. Status is never set without aresult row, so the transition is not opaque in the event log.
The normal path is untouched: fire-and-forget targets never set status
retryingthemselves, so a normally-operatingdatabaseorlogdelivery isstill never seen by recovery or the sweep.
The diff is confined to the two
reschedulersites, the new helper, and the twostale doc comments, so the rebase over PRs #95 and #100 stays mechanical.
Tests
internal/delivery/engine_integration_test.go, plus one newExportSweepWebhookRetriesso the sweep path is reachable from the black-boxtests:
TestRecoverSingleRetry_TypeNoLongerRetries— target type nowlog;recovery ends the delivery
failedwith a second result row naming the typeTestSweepSingleRetry_TypeNoLongerRetries— same, target type nowdatabase, driven through the sweepTestRecoverSingleRetry_UnknownTargetType/TestSweepSingleRetry_UnknownTargetType— typenot-a-target-type; sameoutcome through both paths, no panic
TestRecoverWebhookDeliveries_RetryingDeliveriesgains the regression guard:a
retryingdelivery whose target is stillhttpis rescheduled normally,stays
retrying, and gains no synthetic result rowEach 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 inplace),
make test:Each failed on
expected: "failed" / actual: "retrying"and on the missingresult 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
migrate in-flight deliveries: any delivery still
retryingunder the oldtype is terminally failed with a recorded reason and can be redelivered
manually.
TODO.mdupdated in the same commit.Verification
make fmt— cleanmake check— green,0 issuesfrom the pinned linterscript/cibuild— genuinely executed, not cached: 4m28s wall, theRUN make checklayer ran for 81s with livego testoutput (noCACHEDmarker on it); the tree had changed, so the
COPY . .layer invalidated thecache below it
.golangci.ymluntouched (sha256021cc83f...); the v2.12.2 Dockerfile pinis unchanged
What this builds
Single commit
0384de4onissue-82-orphaned-retrying, branched frommain@
4f5ecb1.Both places that reschedule an orphaned
retryingdelivery —recoverSingleRetry(restart recovery) andsweepSingleRetry(the 60s sweep)— used to
returnsilently when the delivery's target did not implement theinternal
rescheduler. They now call one shared helper,failUnretryableRetry, which:webhook, delivery, target id/name, and the current target type;
DeliveryResultthrough the existingrecordResult— attemptcountAttempts+1,success=false, error text naming the current type; andfailedthrough the existingupdateDeliveryStatus.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 thereschedulerassertion 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 newExportSweepWebhookRetries),internal/delivery/engine_integration_test.go,README.md,TODO.md. Nothing else inengine.gomoved, so the rebase overPRs #95 and #100 should be mechanical.
How I verified it
Mutation test. I reverted both call sites to a bare
return, leaving thehelper and the tests in place, and ran
make test:Every one failed on both assertions that matter —
expected: "failed" / actual: "retrying", and the result row that should haverecorded the reason being absent (1 row, not 2). The regression guard added to
TestRecoverWebhookDeliveries_RetryingDeliveriespassed under the mutation, asit must: it asserts that an
httptarget 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 issuesfrom the pinned v2.12.2 linter. The onlyoutput beyond that is the pre-existing
gomodguarddeprecation warning thatmainalso emits;.golangci.ymlis untouched (sha256021cc83f...).script/cibuild— genuinely executed, not served from cache. 4m28swall clock; the
RUN make checklayer is#27 DONE 81.0swith livego testoutput streaming through it and no
CACHEDmarker. The commit changed thetree, so the
COPY . .layer invalidated everything below it. This also meansthe full suite ran once from scratch inside the pinned CI image after the
mutation was reverted.
Gitea CI on head commit
0384de4is running; I will note the result belowif it is not visible on the PR by the time a reviewer picks this up.
Gitea CI on head commit
0384de4finished 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.
Lint evidence re-validated (and one correction to the record)
script/lintinvokes the hostgolangci-lint, not the pinned Dockerimage, 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), andcross-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 cleanorigin/main4f5ecb1baseline — each retried untila run was valid, i.e. contained no lock message and named no file path outside
its own worktree.
0384de40 issues.main4f5ecb10 issues.I then ran a full
make checkat the head commit and scanned the complete2037-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
gosecG704 ininternal/delivery/client_ssrf_test.go. The hostbinary 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. Bothlint to
0 issues. So there is no pre-existing finding to discount here, andno host/CI version skew on this machine right now. I did not modify
.golangci.yml(sha256021cc83f...) 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.
Independent review of PR #104 — head
0384de4, basemain@4f5ecb1Verdict: PASS
No blocking defects. The change delivers option 1 as specified in the
## Implementation requirementscomment on #82, both recovery sites aregenuinely 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 time0.53s (
real 0m0.528s), every layer from#11through#32reportedCACHED, including theRUN make checklayer. I am not repeating the author'sclaim 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
0384de4ischeck / check (push)= success, describedSuccessful in 4m4s, run 106. A four-minute run is a genuine full build andcheck 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 anyreported path began with
../or with an absolute path outside the worktree Ilaunched from.
make lint, from my own worktree): VOID. 34 issues, every oneattributed to
../wt82-lint/internal/...— a different worktree entirely.Cross-worktree cache contamination, exactly the
../-relative form. Notrecorded in either direction.
golangci-lint cache clean, run 2: VALID.0 issues.No lockerror, no path outside my worktree. Only other output is the tracked
gomodguarddeprecation warning (#98), which is a warning, not a finding.make check: VALID,rc=0,0 issues.The first
make checkI 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 recorda lint result from it.
Note on the pre-existing
gosecG704 ininternal/delivery/client_ssrf_test.go:it does not reproduce. My valid runs report
0 issues.There was nothingto 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:
Each failed on both
expected: "failed" / actual: "retrying"and on themissing result row. Because the two call sites are byte-identical, a single
replace_alledit hit both — confirming neither site was left with a silentreturnand neither carries a divergent copy of the transition.Mutations the author did NOT list. I ran three more:
recordResult, keep the status flip — 4 FAIL, onshould have 2 item(s), but has 1. The result row is independentlyasserted; a status-only flip cannot pass.
updateDeliveryStatus, keep the result row — 4 FAIL, onexpected: "failed". The status transition is independently asserted.Warn->Error— suite stays green. This isa 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 !okinrecoverSingleRetry, so astill-
httptarget would also be terminally failed. Result:--- FAIL: TestRecoverWebhookDeliveries_RetryingDeliveries. The guard is notvacuous — 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 checkrc=0; tests green (I have one full uncached verboserun plus five mutation runs); lint
0 issues.on a valid run;make fmt-checkclean.
make checkmodified nothing —git status --porcelainemptyafterwards.
Mergeable. Merge base is
4f5ecb1, which is the currentorigin/maintip, 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, andnil.(rescheduler)yields(nil, false)— no panic, same terminal path. Thereasonstring renders it astarget type "" does not support retries.TargetTypeis a barestringwith no DB-level enum, so""is reachable bya hand edit. It is not separately tested, but the code path is identical to the
tested
not-a-target-typecase (both are missing map keys), so this is not agap worth blocking on.
The result row is real and useful. Attempt number is
countAttempts(...)+1= 2 against the seeded single prior attempt — sane andnon-colliding.
success=false. The error text names the current target typevia
%qand is asserted with two independentContainschecks (the typestring, and
does not support retries). Not opaque.Idempotency / double-transition. Both
recoverRetryingDeliveriesandsweepWebhookRetriesselect onstatus = retrying. Once the helper flips therow to
failed, neither query selects it again, so a second sweep is a no-opand restart recovery followed by a sweep cannot double-transition or duplicate
the result row. Traced; clean.
Idiom consistency.
recordResult(...)immediately followed byupdateDeliveryStatus(..., DeliveryStatusFailed)is the established pattern inthis package — see
internal/delivery/target_http.go:299-306. The new helpermatches 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 doccomments),
internal/delivery/export_test.go(one new export),internal/delivery/engine_integration_test.go,README.md,TODO.md. Nothingelse in
engine.gomoved. The rebase over PRs #95 and #100 stays mechanical.Policy.
.golangci.ymluntouched; sha256 is021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcbasrequired.
Dockerfilepin intact:golangci/golangci-lint:v2.12.2@sha256:5cceeef0....(closes #82).TODO.mdupdated in the same commit.in the commit message, body, code, or docs — checked; zero hits.
//nolint.script/fmt/script/fmt-checkare gofmt-only, so
make fmtis clean by the repo's own definition. The addedREADME and
TODO.mdprose wraps at <=73 columns, matching the surroundingfile exactly.
Non-blocking observations
Log level is untested. Mutating
Warn->Errorleaves the suitegreen, 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
recordResultsucceeds andupdateDeliveryStatusthen fails, the row staysretryingand the nextsweep 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-typedelivery 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
loadTargeterrors (target deleted rather thanretyped), both sites still log and
return, leaving the delivery stuck inretrying— the same class of unbounded state #82 closed, via a differenttrigger. Distinct from #82's scenario and correctly out of scope here.
Worth its own issue.
TODO.mdstaleness is pre-existing. The# Next Stepsection stilldescribes retention cleanup, which already landed, and
# Statuscitesmain (afe88c6). Both were already stale on4f5ecb1; this PR's obligationwas a
Completed Stepsentry in the same commit, which it delivers.Labels and assignees deliberately left unchanged.
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:
script/cibuildwas fully cached — 0.528s wall, every layerCACHEDincludingRUN make check. It said so plainly, did not repeat the author's claimed 4m28s genuine run, and substituted the Gitea CI status on0384de4: 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.parallel golangci-lint is running, then a run reporting 34 issues all attributed to../wt82-lint/internal/..., the relative-path contamination form. It rangolangci-lint cache clean, retried, and got a valid0 issues.Nothing was recorded from a void run.It also independently confirmed the correction I circulated: no host/CI linter version skew, and the
gosecG704 inclient_ssrf_test.godoes not reproduce. Nothing was waved away on the strength of a stale note.What was verified by execution
recordResultfails four tests onshould have 2 item(s), but has 1; removing onlyupdateDeliveryStatusfails four onexpected: "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.if !ok— so retryablehttptargets would also be failed — failsTestRecoverWebhookDeliveries_RetryingDeliveries. The guard is load-bearing, not incidental.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→Errorstays 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.retryingforever, becauseloadTargeterrors 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-readyand assigned to @sneak. Merge base is still4f5ecb1, clean fast-forward — but #95 and #100 also touchinternal/delivery/engine.go, so this should land after them.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.