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

Close the two remaining delivery terminal-state gaps (closes #107) #292

Merged
clawbot merged 1 commits from issue-107-terminal-state-gaps into next 2026-08-24 05:12:03 +02:00
Conversation 1 Commits 1 Files Changed 5 +789 -10
clawbot commented 2026-08-24 04:32:55 +02:00
Collaborator
Copy Link

Closes #107.

What was wrong

Opaque failure. An unknown target type marked the delivery failed and wrote no DeliveryResult. The event log read failed / 0 attempts / No attempts recorded yet, with one server log line as the only account of why anywhere.

Deletion-orphaned retries — three distinct failures, not one. Deletes are soft (BaseModel.DeletedAt) and loadTarget uses the default scope, so the row is invisible to the engine:

  1. recoverSingleRetry and sweepSingleRetry logged the failed lookup and returned, leaving the delivery retrying for the life of the database while the sweep repeated record not found every 60 seconds.
  2. Worse, and not in the issue text: deleting a target did not stop deliveries to it. A scheduled retry is a time.AfterFunc holding the target's config from when the chain began, and nothing on that path read the target row, so the timer kept firing real POSTs at the destination the operator had removed. Terminalising in recovery and the sweep alone only catches this after a restart.
  3. Nothing distinguished "you deleted this target" from "this id never named a row".

What changed — internal/delivery/engine.go only

  • processDelivery records a result naming the offending type before failing the delivery.
  • recoverSingleRetry and sweepSingleRetry terminalise a retrying delivery whose target row is gone, through the shared failMissingTargetRetry.
  • processRetryTask confirms the target still exists before attempting, and abandons the chain when it does not.
  • One Unscoped() lookup, confined to these terminal paths, supplies the reason text and the deleted row's type. The engine's normal target loading stays scoped — widening it would mean deleting a target stopped nothing. Same narrow scoping as #211.
  • recordResult-then-updateDeliveryStatus is factored into one failDelivery, shared with the existing failUnretryableRetry, so the three terminal paths behave identically. Both now return errors: a failed result write goes to bookkeepingFailed and the status is left alone.

Two decisions worth reviewing

Only a target confirmed gone stops anything. Every new branch is gated on errors.Is(err, gorm.ErrRecordNotFound). Any other lookup failure is the main database being unreadable — transient — and every path leaves the delivery exactly as it was. A guard that terminally failed every retrying delivery of every webhook on one bad read would be a larger fault than the strand it clears. Two tests pin this in both directions.

#256's ownership gate stays the only thing deciding re-dispatch. Terminal writes reached from recovery go through retainIdle/release like every other write those paths make. The retry path writes directly, exactly as a target's own Deliver does, because the worker already holds that delivery — claiming it again through the recovery gate would only fail against the worker's own reference. Nothing was added to either sweep dispatch arm.

Test fixtures

Retry fixtures that drove processRetryTask for a target id with no row in the main database now create one. That state is not reachable in service — the handler reads the target to build the task in the first place — so those fixtures were modelling something the engine can now legitimately refuse.

Verification

make check green with GOFLAGS=-count=1, on the rebased branch. Lint ran in Docker and executed (0 issues, 52.2s, not cached).

Both halves reproduced on unmodified next first, with the built binary, a real HTTP sink and the web UI.

Half 2, before — target deleted at 1787536937.375, sink POSTs at:

1787536932.335  1787536933.341  1787536935.354   <- before deletion
1787536939.367  1787536947.375                   <- AFTER deletion

then across a restart:

02:02:47 ERROR failed to load target for retrying delivery recovery  loading target: record not found
02:03:47 ERROR retry sweep: failed to load target                    loading target: record not found
02:04:47 ERROR retry sweep: failed to load target                    loading target: record not found

status retrying, 5 attempts, never terminalising.

Half 2, after: 0 post-deletion POSTs; the delivery reaches failed at the next scheduled retry with attempt 4 recording target "doomed" (type http) was deleted; the delivery cannot be retried and has been failed terminally; after restart, 0 recovery errors and 0 sweep errors across two sweep cycles.

A second live run deletes the target while the process is stopped, so no in-memory chain exists and restart recovery is the only thing that can see it: 0 errors, terminalised with the same reason, 0 POSTs after restart.

Half 1, before: failed, delivery_results rows 0. After: 1 row, unknown target type "pubsub": this build has no delivery implementation for it, so no attempt was made.

No #256 regression, measured rather than argued: 250 events across 3 targets on a healthy database, no reader. 750 deliveries, 750 sink POSTs, 750 result rows; both sweep arms ran against those 750 rows (confirmed by their queries in the log, one tick at +60s); restart added 0 further requests.

Each of the seven new guards was mutation-verified individually — reason row, recovery terminalisation, sweep terminalisation, retry-chain liveness check, the errors.Is discrimination on the retry and recovery paths, Unscoped(), and the ownership release — each failing only its own tests.

TODO.md untouched, .golangci.yml untouched, internal/database, internal/config and internal/handlers untouched.

Deliberately not fixed here

A pending delivery whose target was deleted strands the same way: sendRecoveredDeliveries logs target not found for delivery and skips it on every sweep. Distinct trigger, distinct status, and it lives in the sweep arm this change deliberately does not touch. Filed separately as #293.

Closes https://git.eeqj.de/sneak/webhooker/issues/107. ## What was wrong **Opaque failure.** An unknown target type marked the delivery `failed` and wrote no `DeliveryResult`. The event log read `failed / 0 attempts / No attempts recorded yet`, with one server log line as the only account of why anywhere. **Deletion-orphaned retries — three distinct failures, not one.** Deletes are soft (`BaseModel.DeletedAt`) and `loadTarget` uses the default scope, so the row is invisible to the engine: 1. `recoverSingleRetry` and `sweepSingleRetry` logged the failed lookup and returned, leaving the delivery `retrying` for the life of the database while the sweep repeated `record not found` every 60 seconds. 2. Worse, and not in the issue text: deleting a target did not stop deliveries to it. A scheduled retry is a `time.AfterFunc` holding the target's config from when the chain began, and nothing on that path read the target row, so the timer kept firing real POSTs at the destination the operator had removed. Terminalising in recovery and the sweep alone only catches this after a restart. 3. Nothing distinguished "you deleted this target" from "this id never named a row". ## What changed — `internal/delivery/engine.go` only - `processDelivery` records a result naming the offending type before failing the delivery. - `recoverSingleRetry` and `sweepSingleRetry` terminalise a retrying delivery whose target row is gone, through the shared `failMissingTargetRetry`. - `processRetryTask` confirms the target still exists before attempting, and abandons the chain when it does not. - One `Unscoped()` lookup, confined to these terminal paths, supplies the reason text and the deleted row's type. The engine's normal target loading stays scoped — widening it would mean deleting a target stopped nothing. Same narrow scoping as https://git.eeqj.de/sneak/webhooker/issues/211. - `recordResult`-then-`updateDeliveryStatus` is factored into one `failDelivery`, shared with the existing `failUnretryableRetry`, so the three terminal paths behave identically. Both now return errors: a failed result write goes to `bookkeepingFailed` and the status is left alone. ### Two decisions worth reviewing **Only a target confirmed gone stops anything.** Every new branch is gated on `errors.Is(err, gorm.ErrRecordNotFound)`. Any other lookup failure is the main database being unreadable — transient — and every path leaves the delivery exactly as it was. A guard that terminally failed every retrying delivery of every webhook on one bad read would be a larger fault than the strand it clears. Two tests pin this in both directions. **#256's ownership gate stays the only thing deciding re-dispatch.** Terminal writes reached from recovery go through `retainIdle`/`release` like every other write those paths make. The retry path writes directly, exactly as a target's own `Deliver` does, because the worker already holds that delivery — claiming it again through the recovery gate would only fail against the worker's own reference. **Nothing was added to either sweep dispatch arm.** ### Test fixtures Retry fixtures that drove `processRetryTask` for a target id with no row in the main database now create one. That state is not reachable in service — the handler reads the target to build the task in the first place — so those fixtures were modelling something the engine can now legitimately refuse. ## Verification `make check` **green** with `GOFLAGS=-count=1`, on the rebased branch. Lint ran in Docker and executed (`0 issues`, 52.2s, not cached). **Both halves reproduced on unmodified `next` first**, with the built binary, a real HTTP sink and the web UI. Half 2, before — target deleted at `1787536937.375`, sink POSTs at: ``` 1787536932.335 1787536933.341 1787536935.354 <- before deletion 1787536939.367 1787536947.375 <- AFTER deletion ``` then across a restart: ``` 02:02:47 ERROR failed to load target for retrying delivery recovery loading target: record not found 02:03:47 ERROR retry sweep: failed to load target loading target: record not found 02:04:47 ERROR retry sweep: failed to load target loading target: record not found ``` status `retrying`, 5 attempts, never terminalising. Half 2, after: **0 post-deletion POSTs**; the delivery reaches `failed` at the next scheduled retry with attempt 4 recording `target "doomed" (type http) was deleted; the delivery cannot be retried and has been failed terminally`; after restart, **0** recovery errors and **0** sweep errors across two sweep cycles. A second live run deletes the target while the process is stopped, so no in-memory chain exists and restart recovery is the only thing that can see it: 0 errors, terminalised with the same reason, 0 POSTs after restart. Half 1, before: `failed`, `delivery_results` rows **0**. After: 1 row, `unknown target type "pubsub": this build has no delivery implementation for it, so no attempt was made`. **No #256 regression**, measured rather than argued: 250 events across 3 targets on a healthy database, no reader. 750 deliveries, **750** sink POSTs, 750 result rows; both sweep arms ran against those 750 rows (confirmed by their queries in the log, one tick at +60s); restart added **0** further requests. Each of the seven new guards was mutation-verified individually — reason row, recovery terminalisation, sweep terminalisation, retry-chain liveness check, the `errors.Is` discrimination on the retry and recovery paths, `Unscoped()`, and the ownership release — each failing only its own tests. `TODO.md` untouched, `.golangci.yml` untouched, `internal/database`, `internal/config` and `internal/handlers` untouched. ## Deliberately not fixed here A **pending** delivery whose target was deleted strands the same way: `sendRecoveredDeliveries` logs `target not found for delivery` and skips it on every sweep. Distinct trigger, distinct status, and it lives in the sweep arm this change deliberately does not touch. Filed separately as https://git.eeqj.de/sneak/webhooker/issues/293.
clawbot added 1 commit 2026-08-24 04:32:55 +02:00
Close the two remaining delivery terminal-state gaps (closes #107)
Some checks failed
check / check (push) Has been cancelled
Details
918533d897 
A delivery could reach a bad end without the engine recording why, and
a retrying delivery could fail to reach an end at all.

An unknown target type marked the delivery failed and wrote no
DeliveryResult, so the event log showed "failed" with no attempts and
the only account of why was one line in the server log. It now records
a result naming the type before failing the delivery.

A deleted target left its retrying deliveries stranded. Both recovery
and the sweep began with a scoped loadTarget, which cannot see a soft
deleted row, so both logged and returned: the delivery stayed retrying
for the life of the database while the sweep repeated the same error
every minute. Both now terminalise it with a recorded reason.

Deleting a target also did not stop deliveries to it. A scheduled
retry is a time.AfterFunc holding the target's configuration from when
the chain began, and nothing on that path read the target row, so the
timer kept firing and kept sending to the destination the operator had
removed for the rest of the backoff chain; terminalising in recovery
and the sweep alone would only have caught it after a restart.
processRetryTask now confirms the target still exists before it
attempts, and abandons the chain when it does not.

Only a target confirmed gone stops anything. A lookup that fails for
any other reason is the main database being unreadable, which is
transient, and every path leaves the delivery exactly as it was rather
than failing it.

The reason text comes from one Unscoped lookup confined to these
terminal paths, because a soft deleted row is what distinguishes a
target the operator deleted from an id that never named one. The
engine's normal target loading stays scoped, or deleting a target
would stop nothing.

Terminal writes reached from recovery keep going through the existing
retainIdle ownership gate; the retry path writes directly, as a
target's own Deliver does, because the worker already holds that
delivery. Nothing was added to either sweep dispatch arm.

Retry fixtures that drove processRetryTask for a target id with no row
in the main database now create one. That state is not reachable in
service: the handler reads the target to build the task.
clawbot self-assigned this 2026-08-24 04:33:40 +02:00
clawbot added the needs-review label 2026-08-24 04:33:46 +02:00
clawbot force-pushed issue-107-terminal-state-gaps from 918533d897 to 3f429f9a4f 2026-08-24 04:35:14 +02:00 Compare
clawbot commented 2026-08-24 05:11:01 +02:00
Author
Collaborator
Copy Link

PASS. Independently verified by execution: both halves reproduced on unmodified next and closed on this head; the seven changed fixtures still fail when the behaviour they guard regresses; #256's no-duplicate guarantee holds at 750/750 with 0 added by restart; make check green (21 packages, 0 (cached), lint 0 issues in Docker, 1m20s); CI green on 3f429f9; mergeable, no trailers, no scope creep.

Disclosures and anomalies (none blocking):

  • Mutation and probe work was done in throwaway copies under /tmp/review-292-work, driven only through make test; the reviewed tree was never modified.
  • TestProcessRetryTask_LargeBody_FetchFromDB asserts only the final delivery status and never that the body reached the sink — it survives a mutation that removes the event-body fetch entirely. Verified identical on unmodified next, so this is pre-existing and not caused by the fixture change here. Worth a separate issue.
  • missingTargetReason discards the error from its Unscoped() lookup, so a lookup that fails for a reason other than not-found yields the "no longer exists" wording instead of "was deleted". Reason text only; the terminal decision is still made on the scoped ErrRecordNotFound.
  • Measured the residual left by #293 on this build: a pending delivery whose target was deleted still strands at pending and still logs target not found for delivery every 60s indefinitely (3 cycles observed). No post-deletion POSTs in that case, so the delivering-to-a-removed-destination half is closed for both statuses.
**PASS.** Independently verified by execution: both halves reproduced on unmodified `next` and closed on this head; the seven changed fixtures still fail when the behaviour they guard regresses; #256's no-duplicate guarantee holds at 750/750 with 0 added by restart; `make check` green (21 packages, 0 `(cached)`, lint `0 issues` in Docker, 1m20s); CI green on `3f429f9`; mergeable, no trailers, no scope creep. Disclosures and anomalies (none blocking): - Mutation and probe work was done in throwaway copies under `/tmp/review-292-work`, driven only through `make test`; the reviewed tree was never modified. - `TestProcessRetryTask_LargeBody_FetchFromDB` asserts only the final delivery status and never that the body reached the sink — it survives a mutation that removes the event-body fetch entirely. Verified identical on unmodified `next`, so this is pre-existing and **not** caused by the fixture change here. Worth a separate issue. - `missingTargetReason` discards the error from its `Unscoped()` lookup, so a lookup that fails for a reason other than not-found yields the "no longer exists" wording instead of "was deleted". Reason text only; the terminal decision is still made on the scoped `ErrRecordNotFound`. - Measured the residual left by https://git.eeqj.de/sneak/webhooker/issues/293 on this build: a `pending` delivery whose target was deleted still strands at `pending` and still logs `target not found for delivery` every 60s indefinitely (3 cycles observed). No post-deletion POSTs in that case, so the delivering-to-a-removed-destination half is closed for both statuses.
clawbot merged commit af3703d748 into next 2026-08-24 05:12:03 +02:00
clawbot deleted branch issue-107-terminal-state-gaps 2026-08-24 05:12:03 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#292
Delete Branch "issue-107-terminal-state-gaps"

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?