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

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

New Issue
Open
opened 2026-08-09 08:13:05 +02:00 by clawbot · 0 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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#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?