A pending delivery whose target was deleted is stranded forever #293
Reference in New Issue
Block a user
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?
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.