Found while fixing the retrying half of #107, and deliberately held out of its scope: that issue's definition of done is bounded to deliveries in retrying, and this lives in the sweep dispatch arm that change does not touch.
sendRecoveredDeliveries (internal/delivery/engine.go) resolves each recovered delivery's target through loadTargetMap, which uses the default scope. Deletes are soft, so a deleted target is absent from the map and the delivery is skipped:
e.log.Error("target not found for delivery", ...)
continue
The row stays pending with no result row. Nothing else moves it, so from pendingSweepMinAge (15 minutes) onward the sweep selects it, logs that same line and skips it again, every 60 seconds, for the life of the database. It is the same end state #107 fixes for retrying — a delivery that never reaches a terminal status, with an empty event log — reached from a different status.
Reachable by deleting a target while one of its deliveries is queued or stranded at pending: the delivery row is written before the task is queued, so the window is not just the failed-bookkeeping case.
Done when: a pending delivery whose target row no longer exists is terminally resolved rather than skipped indefinitely, with a DeliveryResult recording why, matching what #107 settled on for the three retrying cases — failDelivery, and missingTargetReason for the deleted-versus-never-existed distinction, both already exist. Mutation-verified.
Two things to be careful of, both load-bearing:
The write must go through the retainIdle/release ownership gate, like every other write the recovery paths make. See internal/delivery/inflight.go and #256.
Only a target confirmed gone may terminalise anything. loadTargetMap currently reports one error for the whole batch and returns nil on a failed query, so "this target is absent from the map" today conflates "deleted" with "the query failed" — terminalising on the latter would fail every pending delivery of every webhook on one bad read. Distinguishing them is part of the work.
Verify at scale against a healthy database as well: the same arm is what #256 made exact, and re-dispatching a healthy pending delivery is the regression to avoid.
Found while fixing the `retrying` half of https://git.eeqj.de/sneak/webhooker/issues/107, and deliberately held out of its scope: that issue's definition of done is bounded to deliveries in `retrying`, and this lives in the sweep dispatch arm that change does not touch.
`sendRecoveredDeliveries` (`internal/delivery/engine.go`) resolves each recovered delivery's target through `loadTargetMap`, which uses the default scope. Deletes are soft, so a deleted target is absent from the map and the delivery is skipped:
```
e.log.Error("target not found for delivery", ...)
continue
```
The row stays `pending` with no result row. Nothing else moves it, so from `pendingSweepMinAge` (15 minutes) onward the sweep selects it, logs that same line and skips it again, every 60 seconds, for the life of the database. It is the same end state https://git.eeqj.de/sneak/webhooker/issues/107 fixes for `retrying` — a delivery that never reaches a terminal status, with an empty event log — reached from a different status.
Reachable by deleting a target while one of its deliveries is queued or stranded at `pending`: the delivery row is written before the task is queued, so the window is not just the failed-bookkeeping case.
**Done when:** a `pending` delivery whose target row no longer exists is terminally resolved rather than skipped indefinitely, with a `DeliveryResult` recording why, matching what https://git.eeqj.de/sneak/webhooker/issues/107 settled on for the three retrying cases — `failDelivery`, and `missingTargetReason` for the deleted-versus-never-existed distinction, both already exist. Mutation-verified.
Two things to be careful of, both load-bearing:
- The write must go through the `retainIdle`/`release` ownership gate, like every other write the recovery paths make. See `internal/delivery/inflight.go` and https://git.eeqj.de/sneak/webhooker/issues/256.
- Only a target confirmed gone may terminalise anything. `loadTargetMap` currently reports one error for the whole batch and returns `nil` on a failed query, so "this target is absent from the map" today conflates "deleted" with "the query failed" — terminalising on the latter would fail every pending delivery of every webhook on one bad read. Distinguishing them is part of the work.
Verify at scale against a healthy database as well: the same arm is what https://git.eeqj.de/sneak/webhooker/issues/256 made exact, and re-dispatching a healthy pending delivery is the regression to avoid.
Plan. The code the issue names is still on next (83740b1): sendRecoveredDeliveries in internal/delivery/engine.go logs "target not found for delivery" and continues, every sweep, forever.
Fix: do what recoverSingleRetry already does for retrying. On a map miss, confirm with loadTarget:
gorm.ErrRecordNotFound means the target is gone. Fail the delivery terminally through the same ownership-gated path as failMissingTargetRetry (retainIdle, missingTargetReason, failDelivery, release). Generalise that function rather than copying it, and make its log line and reason text fit a pending delivery as well as a retrying one.
Any other error leaves the delivery alone for the next sweep. So a failed loadTargetMap query can never terminalise a batch.
Tests:
A pending delivery whose target is soft-deleted ends failed with one DeliveryResult giving the reason, from both the restart path and the sweep.
A failing target read leaves it pending.
A healthy pending delivery is still sent, once.
Mutation-verify: each test fails when its branch is removed.
Scale check: re-run the at-scale check from #256 against a healthy database. No healthy pending delivery is re-sent or failed. Verify it; publish none of the evidence.
Model: opus-5-5
Plan. The code the issue names is still on `next` (`83740b1`): `sendRecoveredDeliveries` in `internal/delivery/engine.go` logs "target not found for delivery" and `continue`s, every sweep, forever.
- **Fix:** do what `recoverSingleRetry` already does for `retrying`. On a map miss, confirm with `loadTarget`:
- `gorm.ErrRecordNotFound` means the target is gone. Fail the delivery terminally through the same ownership-gated path as `failMissingTargetRetry` (`retainIdle`, `missingTargetReason`, `failDelivery`, `release`). Generalise that function rather than copying it, and make its log line and reason text fit a `pending` delivery as well as a `retrying` one.
- Any other error leaves the delivery alone for the next sweep. So a failed `loadTargetMap` query can never terminalise a batch.
- **Tests:**
- A `pending` delivery whose target is soft-deleted ends `failed` with one `DeliveryResult` giving the reason, from both the restart path and the sweep.
- A failing target read leaves it `pending`.
- A healthy `pending` delivery is still sent, once.
- Mutation-verify: each test fails when its branch is removed.
- **Scale check:** re-run the at-scale check from https://git.eeqj.de/sneak/webhooker/issues/256 against a healthy database. No healthy `pending` delivery is re-sent or failed. Verify it; publish none of the evidence.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 04:13:07 +02:00
Built in #329 (branch issue-293-pending-deleted-target, base next), following the plan above. The PR body has what changed and the disclosures.
Model: opus-5-5
Built in https://git.eeqj.de/sneak/webhooker/pulls/329 (branch `issue-293-pending-deleted-target`, base `next`), following the plan above. The PR body has what changed and the disclosures.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Found while fixing the
retryinghalf of #107, and deliberately held out of its scope: that issue's definition of done is bounded to deliveries inretrying, and this lives in the sweep dispatch arm that change does not touch.sendRecoveredDeliveries(internal/delivery/engine.go) resolves each recovered delivery's target throughloadTargetMap, which uses the default scope. Deletes are soft, so a deleted target is absent from the map and the delivery is skipped:The row stays
pendingwith no result row. Nothing else moves it, so frompendingSweepMinAge(15 minutes) onward the sweep selects it, logs that same line and skips it again, every 60 seconds, for the life of the database. It is the same end state #107 fixes forretrying— a delivery that never reaches a terminal status, with an empty event log — reached from a different status.Reachable by deleting a target while one of its deliveries is queued or stranded at
pending: the delivery row is written before the task is queued, so the window is not just the failed-bookkeeping case.Done when: a
pendingdelivery whose target row no longer exists is terminally resolved rather than skipped indefinitely, with aDeliveryResultrecording why, matching what #107 settled on for the three retrying cases —failDelivery, andmissingTargetReasonfor the deleted-versus-never-existed distinction, both already exist. Mutation-verified.Two things to be careful of, both load-bearing:
retainIdle/releaseownership gate, like every other write the recovery paths make. Seeinternal/delivery/inflight.goand #256.loadTargetMapcurrently reports one error for the whole batch and returnsnilon a failed query, so "this target is absent from the map" today conflates "deleted" with "the query failed" — terminalising on the latter would fail every pending delivery of every webhook on one bad read. Distinguishing them is part of the work.Verify at scale against a healthy database as well: the same arm is what #256 made exact, and re-dispatching a healthy pending delivery is the regression to avoid.
Plan. The code the issue names is still on
next(83740b1):sendRecoveredDeliveriesininternal/delivery/engine.gologs "target not found for delivery" andcontinues, every sweep, forever.recoverSingleRetryalready does forretrying. On a map miss, confirm withloadTarget:gorm.ErrRecordNotFoundmeans the target is gone. Fail the delivery terminally through the same ownership-gated path asfailMissingTargetRetry(retainIdle,missingTargetReason,failDelivery,release). Generalise that function rather than copying it, and make its log line and reason text fit apendingdelivery as well as aretryingone.loadTargetMapquery can never terminalise a batch.pendingdelivery whose target is soft-deleted endsfailedwith oneDeliveryResultgiving the reason, from both the restart path and the sweep.pending.pendingdelivery is still sent, once.pendingdelivery is re-sent or failed. Verify it; publish none of the evidence.Model: opus-5-5
Built in #329 (branch
issue-293-pending-deleted-target, basenext), following the plan above. The PR body has what changed and the disclosures.Model: opus-5-5