Design: should delivery targets own their recovery/sweep loop, not just backoff? #85

New Issue

clawbot · 2026-08-07T18:07:15+02:00

clawbot commented

2026-08-07 18:07:15 +02:00

Raised by @sneak during the #77 Target-interface refactor; capturing it so the design question is not lost.

The merged design keeps restart-recovery and the 60-second sweep engine-owned (querying retrying deliveries, loading events/targets, re-enqueue), and delegates only the backoff recompute to each target. @sneak's instinct was that a target could own its whole recovery too — hand it the DB and let it find and re-drive its own retrying rows.

Trade-off noted in review: engine-owned recovery avoids duplicating the DB-scan/recovery machinery in every target (that plumbing is identical for all types); target-owned recovery is more consistent with "the target owns its retries" but duplicates the scan per target.

Not a bug and not 1.0-blocking — the current engine-owned recovery is correct (verified by the independent review of #81). Decide whether to leave it engine-owned or move recovery into the targets. Cross-ref #82 (a concrete edge case in the current recovery path).

Raised by @sneak during the #77 Target-interface refactor; capturing it so the design question is not lost. The merged design keeps restart-recovery and the 60-second sweep engine-owned (querying `retrying` deliveries, loading events/targets, re-enqueue), and delegates only the backoff recompute to each target. @sneak's instinct was that a target could own its whole recovery too — hand it the DB and let it find and re-drive its own `retrying` rows. Trade-off noted in review: engine-owned recovery avoids duplicating the DB-scan/recovery machinery in every target (that plumbing is identical for all types); target-owned recovery is more consistent with "the target owns its retries" but duplicates the scan per target. Not a bug and not 1.0-blocking — the current engine-owned recovery is correct (verified by the independent review of #81). Decide whether to leave it engine-owned or move recovery into the targets. Cross-ref #82 (a concrete edge case in the current recovery path).

clawbot referenced this issue

2026-08-07 18:07:44 +02:00

1.0/mvp #33

clawbot commented

2026-08-07 18:52:38 +02:00

Owner decision needed: where should retry recovery/sweep live?

Question: after the #77/#81 Target-interface refactor, restart-recovery and the 60-second sweep are engine-owned (the engine queries retrying rows, loads events/targets, and re-enqueues), while each target owns only the backoff recompute via the rescheduler interface. Should targets instead own their whole recovery loop?

Options

Keep recovery engine-owned, targets own policy only (status quo). The engine owns the mechanism (scan the per-webhook DBs, load state, re-enqueue); targets own the policy (does this type retry at all; has the backoff elapsed). One copy of the scan/load/re-enqueue plumbing; the Target interface stays small; fire-and-forget targets need nothing.
Move recovery fully into each target: hand the target the DB and let it find and re-drive its own retrying rows. Most consistent with "the target owns its retries", and a future exotic target (e.g. one with its own durable queue) could recover however it likes. Cost: every retrying target reimplements the identical scan/load/re-enqueue machinery, each copy can drift (bug class multiplies — #82 would need fixing N times instead of once), and targets become coupled to DB scanning concerns they currently know nothing about.
Hybrid: engine keeps the scan, but instead of the narrow backoffElapsed hook it hands each orphaned delivery to a richer per-target RecoverOrphan(...) hook that decides requeue / drop / fail. Keeps one scan, gives targets full policy control, and gives the engine a natural place to handle deliveries whose target no longer retries.

Recommendation

Option 1, unchanged, with one small borrow from option 3 to fix #82: when the sweep finds a retrying delivery whose current target type does not implement rescheduler (the type was changed after the delivery was orphaned), the engine should mark it Failed with a recorded result instead of skipping it forever. That is a ~10-line change in sweepSingleRetry/recoverSingleRetry, keeps a single copy of the recovery machinery, and preserves the engine-mechanism/target-policy split that the independent review of #81 verified correct. Option 2's consistency benefit is real but purely aesthetic today — no current or planned target needs a different recovery mechanism, and the duplication cost is concrete.

If you confirm option 1, #82 becomes a small engine-side fix and this issue can close; if you prefer option 2 or 3, I would fold #82 into that refactor instead. Assigning to @sneak for the call.

## Owner decision needed: where should retry recovery/sweep live? Question: after the #77/#81 Target-interface refactor, restart-recovery and the 60-second sweep are engine-owned (the engine queries `retrying` rows, loads events/targets, and re-enqueues), while each target owns only the backoff recompute via the `rescheduler` interface. Should targets instead own their whole recovery loop? ### Options 1. Keep recovery engine-owned, targets own policy only (status quo). The engine owns the mechanism (scan the per-webhook DBs, load state, re-enqueue); targets own the policy (does this type retry at all; has the backoff elapsed). One copy of the scan/load/re-enqueue plumbing; the Target interface stays small; fire-and-forget targets need nothing. 2. Move recovery fully into each target: hand the target the DB and let it find and re-drive its own `retrying` rows. Most consistent with "the target owns its retries", and a future exotic target (e.g. one with its own durable queue) could recover however it likes. Cost: every retrying target reimplements the identical scan/load/re-enqueue machinery, each copy can drift (bug class multiplies — #82 would need fixing N times instead of once), and targets become coupled to DB scanning concerns they currently know nothing about. 3. Hybrid: engine keeps the scan, but instead of the narrow `backoffElapsed` hook it hands each orphaned delivery to a richer per-target `RecoverOrphan(...)` hook that decides requeue / drop / fail. Keeps one scan, gives targets full policy control, and gives the engine a natural place to handle deliveries whose target no longer retries. ### Recommendation Option 1, unchanged, with one small borrow from option 3 to fix #82: when the sweep finds a `retrying` delivery whose current target type does not implement `rescheduler` (the type was changed after the delivery was orphaned), the engine should mark it `Failed` with a recorded result instead of skipping it forever. That is a ~10-line change in `sweepSingleRetry`/`recoverSingleRetry`, keeps a single copy of the recovery machinery, and preserves the engine-mechanism/target-policy split that the independent review of #81 verified correct. Option 2's consistency benefit is real but purely aesthetic today — no current or planned target needs a different recovery mechanism, and the duplication cost is concrete. If you confirm option 1, #82 becomes a small engine-side fix and this issue can close; if you prefer option 2 or 3, I would fold #82 into that refactor instead. Assigning to @sneak for the call.

sneak was assigned by clawbot

2026-08-07 18:52:41 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#85