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

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

New Issue
Closed
opened 2026-08-09 08:13:05 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 08:13:05 +02:00
Collaborator
Copy Link

Both surfaced by the independent review of PR #104 (#82) and correctly held out of its scope. They are the two remaining paths where a delivery reaches a bad terminal state without the engine recording why, or fails to reach one at all.

#82 fixed the retyped-target case. These are its siblings.

1. processDelivery fails an unknown target type with no result row

internal/delivery/engine.go:783-796. When a delivery's target type is not in the registry, processDelivery marks the delivery failed and records nothing.

That is precisely the opacity #82's spec forbade for the recovery and sweep paths — a delivery that ends failed with an empty event log is unreadable to anyone trying to work out what happened. #104 fixed the two recovery/sweep sites and deliberately did not touch this one, so the codebase is now inconsistent: two paths explain themselves, one does not.

Done when: the unknown-target-type branch in processDelivery records a DeliveryResult naming the offending type before marking the delivery failed, following the same recordResult-then-updateDeliveryStatus idiom used by #104 and by internal/delivery/target_http.go:299-306; with a test asserting the result row exists and names the type, mutation-verified.

2. A deleted target leaves a retrying delivery stuck forever

Distinct trigger, same end state as #82. Both recoverSingleRetry and sweepSingleRetry begin with loadTarget(d.TargetID); if the target row is gone rather than retyped, loadTarget errors and both sites log and return. The delivery stays retrying for the lifetime of the database.

#82 only reached the case where a target still exists but no longer implements rescheduler. Deleting a target while one of its deliveries is mid-retry is a more ordinary operator action than editing a target's type, so this is arguably the likelier of the two to happen in practice.

Done when: a retrying delivery whose target row no longer exists is terminally resolved rather than left hanging — the natural choice is the same treatment #82 settled on (record a result row explaining the target is gone, mark failed), so all three cases behave alike. Cover both the recovery and sweep paths, mutation-verified.

Note the interaction with soft deletes: confirm whether target deletion is soft (GORM default scope) or hard here, since a soft-deleted target may still be loadable depending on the query, and the fix differs accordingly. Establish that first.

Why one issue

Both are the same theme — a delivery ending in a state the engine cannot explain — both live in internal/delivery/engine.go, and both should follow the idiom #104 established. Fixing them together keeps the three terminal paths consistent, which is the actual goal.

Definition of done

  • Items 1 and 2 implemented with tests, each mutation-verified.
  • No code path can land a delivery in failed with an empty event log, and no path can leave one in retrying indefinitely.
  • make check green via the repo's own entrypoints; .golangci.yml untouched.
Both surfaced by the independent review of PR #104 (#82) and correctly held out of its scope. They are the two remaining paths where a delivery reaches a bad terminal state without the engine recording why, or fails to reach one at all. #82 fixed the *retyped-target* case. These are its siblings. ## 1. `processDelivery` fails an unknown target type with no result row `internal/delivery/engine.go:783-796`. When a delivery's target type is not in the registry, `processDelivery` marks the delivery `failed` and records **nothing**. That is precisely the opacity #82's spec forbade for the recovery and sweep paths — a delivery that ends `failed` with an empty event log is unreadable to anyone trying to work out what happened. #104 fixed the two recovery/sweep sites and deliberately did not touch this one, so the codebase is now inconsistent: two paths explain themselves, one does not. **Done when:** the unknown-target-type branch in `processDelivery` records a `DeliveryResult` naming the offending type before marking the delivery `failed`, following the same `recordResult`-then-`updateDeliveryStatus` idiom used by #104 and by `internal/delivery/target_http.go:299-306`; with a test asserting the result row exists and names the type, mutation-verified. ## 2. A deleted target leaves a retrying delivery stuck forever Distinct trigger, same end state as #82. Both `recoverSingleRetry` and `sweepSingleRetry` begin with `loadTarget(d.TargetID)`; if the target **row is gone** rather than retyped, `loadTarget` errors and both sites log and `return`. The delivery stays `retrying` for the lifetime of the database. #82 only reached the case where a target still exists but no longer implements `rescheduler`. Deleting a target while one of its deliveries is mid-retry is a more ordinary operator action than editing a target's type, so this is arguably the likelier of the two to happen in practice. **Done when:** a `retrying` delivery whose target row no longer exists is terminally resolved rather than left hanging — the natural choice is the same treatment #82 settled on (record a result row explaining the target is gone, mark `failed`), so all three cases behave alike. Cover both the recovery and sweep paths, mutation-verified. Note the interaction with soft deletes: confirm whether target deletion is soft (GORM default scope) or hard here, since a soft-deleted target may still be loadable depending on the query, and the fix differs accordingly. Establish that first. ## Why one issue Both are the same theme — a delivery ending in a state the engine cannot explain — both live in `internal/delivery/engine.go`, and both should follow the idiom #104 established. Fixing them together keeps the three terminal paths consistent, which is the actual goal. ## Definition of done - Items 1 and 2 implemented with tests, each mutation-verified. - No code path can land a delivery in `failed` with an empty event log, and no path can leave one in `retrying` indefinitely. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.
clawbot added this to the 1.0.0 milestone 2026-08-24 00:58:21 +02:00
clawbot commented 2026-08-24 03:53:56 +02:00
Author
Collaborator
Copy Link

Plan. Deletes are soft (BaseModel.DeletedAt), and loadTarget uses the default scope, so a deleted target's row is invisible to the engine — confirmed, and it is what produces both the endless record not found sweep log and the "never existed" ambiguity.

  1. Unknown target type. processDelivery records a DeliveryResult naming the type before settling failed, via the recordResult-then-updateDeliveryStatus idiom. Both now return errors, so a failed result write goes to bookkeepingFailed and the status is left alone.

  2. Deleted target, recovery and sweep. recoverSingleRetry and sweepSingleRetry terminalise only on gorm.ErrRecordNotFound; any other loadTarget error stays a log-and-return, because terminally failing every retrying delivery on a transient main-database fault would be worse than the bug. The reason text is resolved by one narrow Unscoped() lookup used only on this terminal path, so "you deleted this target" and "this id never named a row" read differently. Both go through the existing retainIdle/release gate, sharing one terminal helper with failUnretryableRetry.

  3. The cached-config retry chain (the half the audit found worse than filed): a scheduled time.AfterFunc carries a TargetConfig snapshot and keeps POSTing to a destination the operator removed. processRetryTask will check target liveness before dispatching and terminalise instead — the worker already owns the delivery there, so it writes directly as any target does inside Deliver, rather than adding a second ownership path. A lookup that fails for a reason other than not-found proceeds as before, so the new guard cannot itself stall retries. Fresh deliveries are unaffected: their config is read in the request that queues them.

Nothing is added to the sweep's dispatch arms, so the #256 ownership gate stays the only thing deciding re-dispatch. Verification includes a several-hundred-delivery no-duplicate run to prove that.

Plan. Deletes are soft (`BaseModel.DeletedAt`), and `loadTarget` uses the default scope, so a deleted target's row is invisible to the engine — confirmed, and it is what produces both the endless `record not found` sweep log and the "never existed" ambiguity. 1. **Unknown target type.** `processDelivery` records a `DeliveryResult` naming the type before settling `failed`, via the `recordResult`-then-`updateDeliveryStatus` idiom. Both now return errors, so a failed result write goes to `bookkeepingFailed` and the status is left alone. 2. **Deleted target, recovery and sweep.** `recoverSingleRetry` and `sweepSingleRetry` terminalise only on `gorm.ErrRecordNotFound`; any other `loadTarget` error stays a log-and-return, because terminally failing every retrying delivery on a transient main-database fault would be worse than the bug. The reason text is resolved by one narrow `Unscoped()` lookup used *only* on this terminal path, so "you deleted this target" and "this id never named a row" read differently. Both go through the existing `retainIdle`/`release` gate, sharing one terminal helper with `failUnretryableRetry`. 3. **The cached-config retry chain** (the half the audit found worse than filed): a scheduled `time.AfterFunc` carries a `TargetConfig` snapshot and keeps POSTing to a destination the operator removed. `processRetryTask` will check target liveness before dispatching and terminalise instead — the worker already owns the delivery there, so it writes directly as any target does inside `Deliver`, rather than adding a second ownership path. A lookup that fails for a reason other than not-found proceeds as before, so the new guard cannot itself stall retries. Fresh deliveries are unaffected: their config is read in the request that queues them. Nothing is added to the sweep's dispatch arms, so the #256 ownership gate stays the only thing deciding re-dispatch. Verification includes a several-hundred-delivery no-duplicate run to prove that.
clawbot commented 2026-08-24 04:35:34 +02:00
Author
Collaborator
Copy Link

Done in #292 (branch issue-107-terminal-state-gaps, base next). make check green.

Item 2 turned out to be three failures, not one. Beyond the two stranded-recovery sites the issue names, deleting a target did not stop deliveries to it at all: a scheduled retry is a time.AfterFunc carrying the target config from when the chain began, and nothing on that path read the target row, so the timer kept firing real POSTs at the removed destination. processRetryTask now confirms the target exists before attempting.

Both halves were reproduced on unmodified next with the real binary and an HTTP sink before anything was changed. Half 2: target deleted at t=5s, two further POSTs confirmed at the sink afterwards, then across a restart one recovery error and a sweep error every 60s indefinitely, delivery still retrying. Half 1: failed with zero delivery_results rows.

After: zero post-deletion POSTs; the delivery reaches failed with a row reading target "doomed" (type http) was deleted; the delivery cannot be retried and has been failed terminally; zero recovery and sweep errors across two cycles. A separate run deletes the target while the process is stopped, so restart recovery is the only path that can see it — same outcome. Half 1 now records unknown target type "pubsub": this build has no delivery implementation for it, so no attempt was made.

Only a target confirmed gone terminalises anything: every new branch is gated on gorm.ErrRecordNotFound, so an unreadable main database leaves deliveries alone rather than failing all of them. The deleted-versus-never-existed distinction comes from one Unscoped() lookup confined to these terminal paths; normal target loading stays scoped.

Seven guards, each mutation-verified individually. No-duplicate non-regression measured at 250 events across 3 targets: 750 deliveries, 750 sink POSTs, both sweep arms fired, restart added zero.

The equivalent strand for a pending delivery is real but a distinct trigger in a sweep arm this change does not touch; filed as #293.

Done in https://git.eeqj.de/sneak/webhooker/pulls/292 (branch `issue-107-terminal-state-gaps`, base `next`). `make check` green. Item 2 turned out to be three failures, not one. Beyond the two stranded-recovery sites the issue names, deleting a target did not stop deliveries to it at all: a scheduled retry is a `time.AfterFunc` carrying the target config from when the chain began, and nothing on that path read the target row, so the timer kept firing real POSTs at the removed destination. `processRetryTask` now confirms the target exists before attempting. Both halves were reproduced on unmodified `next` with the real binary and an HTTP sink before anything was changed. Half 2: target deleted at t=5s, two further POSTs confirmed at the sink afterwards, then across a restart one recovery error and a sweep error every 60s indefinitely, delivery still `retrying`. Half 1: `failed` with zero `delivery_results` rows. After: zero post-deletion POSTs; the delivery reaches `failed` with a row reading `target "doomed" (type http) was deleted; the delivery cannot be retried and has been failed terminally`; zero recovery and sweep errors across two cycles. A separate run deletes the target while the process is stopped, so restart recovery is the only path that can see it — same outcome. Half 1 now records `unknown target type "pubsub": this build has no delivery implementation for it, so no attempt was made`. Only a target confirmed gone terminalises anything: every new branch is gated on `gorm.ErrRecordNotFound`, so an unreadable main database leaves deliveries alone rather than failing all of them. The deleted-versus-never-existed distinction comes from one `Unscoped()` lookup confined to these terminal paths; normal target loading stays scoped. Seven guards, each mutation-verified individually. No-duplicate non-regression measured at 250 events across 3 targets: 750 deliveries, 750 sink POSTs, both sweep arms fired, restart added zero. The equivalent strand for a `pending` delivery is real but a distinct trigger in a sweep arm this change does not touch; filed as https://git.eeqj.de/sneak/webhooker/issues/293.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

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

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?