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

Recovery skips orphaned retrying deliveries whose target type changed to a non-retry type #82

New Issue
Closed
opened 2026-08-07 17:12:07 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-07 17:12:07 +02:00
Collaborator
Copy Link

Low-severity edge case surfaced by the independent review of PR #81 (delivery Target-interface refactor).

Background

After #81, restart recovery and the 60s sweep look each orphaned retrying delivery's target up in the registry and only reschedule it if the target implements the internal rescheduler (i.e. http/slack). Fire-and-forget targets (database/log) never set status retrying, so under normal operation nothing is dropped — this is correct and was verified in review.

The edge case

If a target's type is mutated in the DB from a retry type (http/slack) to a fire-and-forget type (or an unknown type) while it still has an orphaned retrying delivery, that delivery now stays stuck in retrying forever. The pre-refactor code would eventually re-dispatch it and let the new target type resolve it to delivered/failed.

This requires editing a target's type out from under an in-flight retrying delivery, so it does not arise in normal operation, and neither the old nor the new code handles it "correctly" (old: re-dispatches under a type that never produced the retry; new: leaves it stuck). Noted for completeness; it was explicitly not a merge blocker.

Definition of done

Decide the intended behaviour for a retrying delivery whose target type no longer supports retries, and make recovery/sweep do that deterministically. Options:

  • terminally mark such orphaned deliveries failed (with a recorded reason) during recovery, or
  • re-dispatch them once through the current target type (closer to the old behaviour), or
  • explicitly document that mutating a target's type does not migrate in-flight deliveries and leave them for manual cleanup.

Pick one, implement it, and add a recovery test that seeds a retrying delivery whose target type has changed and asserts the chosen outcome.

Low-severity edge case surfaced by the independent review of PR #81 (delivery Target-interface refactor). ## Background After #81, restart recovery and the 60s sweep look each orphaned `retrying` delivery's target up in the registry and only reschedule it if the target implements the internal `rescheduler` (i.e. `http`/`slack`). Fire-and-forget targets (`database`/`log`) never set status `retrying`, so under normal operation nothing is dropped — this is correct and was verified in review. ## The edge case If a target's `type` is mutated in the DB from a retry type (`http`/`slack`) to a fire-and-forget type (or an unknown type) while it still has an orphaned `retrying` delivery, that delivery now stays stuck in `retrying` forever. The pre-refactor code would eventually re-dispatch it and let the new target type resolve it to `delivered`/`failed`. This requires editing a target's type out from under an in-flight retrying delivery, so it does not arise in normal operation, and neither the old nor the new code handles it "correctly" (old: re-dispatches under a type that never produced the retry; new: leaves it stuck). Noted for completeness; it was explicitly not a merge blocker. ## Definition of done Decide the intended behaviour for a `retrying` delivery whose target type no longer supports retries, and make recovery/sweep do that deterministically. Options: - terminally mark such orphaned deliveries `failed` (with a recorded reason) during recovery, or - re-dispatch them once through the current target type (closer to the old behaviour), or - explicitly document that mutating a target's type does not migrate in-flight deliveries and leave them for manual cleanup. Pick one, implement it, and add a recovery test that seeds a `retrying` delivery whose target type has changed and asserts the chosen outcome.
clawbot commented 2026-08-09 07:45:55 +02:00
Author
Collaborator
Copy Link

Implementation requirements

Baseline: main @ 4f5ecb1.

Decision on the open question

The issue asks for a behaviour to be chosen from three options. Choose option 1: terminally mark such orphaned deliveries failed, with a recorded reason. Reasoning, so the implementer and reviewer both have it:

  • Re-dispatching (option 2) risks a delivery the operator did not ask for. The target's type was changed out from under an in-flight retry; firing it at the new type is a side effect with external consequences, and "closer to the old behaviour" is not a virtue when the old behaviour was also wrong.
  • Documenting and leaving it stuck (option 3) leaves rows in retrying forever, which is exactly the unbounded-state problem the rest of this milestone has been closing.
  • Marking failed is deterministic, has no external side effect, and the event itself remains durably stored in the per-webhook event database — so manual redelivery (already on the roadmap) can recover it deliberately.

This is a judgement call the issue explicitly delegated. @sneak, it is easy to reverse if you disagree.

Confirmed state

Two sites in internal/delivery/engine.go do the same thing:

  • line ~476, restart recovery: rs, ok := e.targets[target.Type].(rescheduler); if !ok { return }
  • line ~671, the 60s retry sweep: identical.

Both silently return, leaving the delivery in retrying forever. Note this also covers an unknown target type: e.targets[type] returns nil, the assertion fails, and it takes the same path.

1. Behaviour

At both sites, when the target does not implement rescheduler, instead of returning silently:

  • record a DeliveryResult capturing why (target type no longer supports retries, naming the current type), and
  • mark the delivery failed via the existing status-update path.

Do not just set the status — a delivery that ends failed with no result row is opaque to anyone reading the event log later.

Factor the two sites into one shared helper. They are already near-duplicates, and two copies of a terminal-state transition is how they drift.

2. Distinguish this from a genuine failure

Log at warn, not error — this is an operator-caused state, not a system fault. The message should name the delivery, the target, and the current type, so the operator can see the connection to their own edit.

3. Do not disturb the normal path

Fire-and-forget targets (database/log) never set status retrying under normal operation, so this path should be unreachable for them in practice. The review of #81 verified that and it must stay true: this change must not cause a normally-operating database or log delivery to be touched by recovery or the sweep.

4. Tests

  • Recovery: seed a retrying delivery whose target type has been changed to a non-retry type, run recovery, assert the delivery ends failed and a result row records the reason.
  • The retry sweep: same, through the sweep path.
  • Unknown/garbage target type: same outcome, no panic.
  • A retrying delivery whose target is still http/slack is still rescheduled normally — the regression guard.
  • Mutation-verify: with the fix reverted, each new test must fail. State the evidence in the PR body.

5. Docs

README should state that changing a target's type does not migrate in-flight deliveries: any delivery still retrying under the old type is terminally failed and can be redelivered manually. TODO.md in the same commit.

Definition of done

The issue's own DoD with option 1 chosen, plus items 1-5, make check green via the repo's own entrypoints, .golangci.yml untouched, single commit whose title ends with (closes #82), no attribution trailers.

Conflict note

PR #95 and PR #100 both modify internal/delivery/engine.go and are merge-ready but unmerged. Keep this diff tight and confined to the two rescheduler sites and the shared helper, so the eventual rebase is mechanical.

## Implementation requirements Baseline: `main` @ `4f5ecb1`. ### Decision on the open question The issue asks for a behaviour to be chosen from three options. **Choose option 1: terminally mark such orphaned deliveries `failed`, with a recorded reason.** Reasoning, so the implementer and reviewer both have it: - Re-dispatching (option 2) risks a *delivery* the operator did not ask for. The target's type was changed out from under an in-flight retry; firing it at the new type is a side effect with external consequences, and "closer to the old behaviour" is not a virtue when the old behaviour was also wrong. - Documenting and leaving it stuck (option 3) leaves rows in `retrying` forever, which is exactly the unbounded-state problem the rest of this milestone has been closing. - Marking `failed` is deterministic, has no external side effect, and the event itself remains durably stored in the per-webhook event database — so manual redelivery (already on the roadmap) can recover it deliberately. This is a judgement call the issue explicitly delegated. @sneak, it is easy to reverse if you disagree. ### Confirmed state Two sites in `internal/delivery/engine.go` do the same thing: - **line ~476**, restart recovery: `rs, ok := e.targets[target.Type].(rescheduler); if !ok { return }` - **line ~671**, the 60s retry sweep: identical. Both silently `return`, leaving the delivery in `retrying` forever. Note this also covers an **unknown** target type: `e.targets[type]` returns nil, the assertion fails, and it takes the same path. ### 1. Behaviour At both sites, when the target does not implement `rescheduler`, instead of returning silently: - record a `DeliveryResult` capturing why (target type no longer supports retries, naming the current type), and - mark the delivery `failed` via the existing status-update path. Do not just set the status — a delivery that ends `failed` with no result row is opaque to anyone reading the event log later. Factor the two sites into **one** shared helper. They are already near-duplicates, and two copies of a terminal-state transition is how they drift. ### 2. Distinguish this from a genuine failure Log at warn, not error — this is an operator-caused state, not a system fault. The message should name the delivery, the target, and the current type, so the operator can see the connection to their own edit. ### 3. Do not disturb the normal path Fire-and-forget targets (`database`/`log`) never set status `retrying` under normal operation, so this path should be unreachable for them in practice. The review of #81 verified that and it must stay true: this change must not cause a normally-operating `database` or `log` delivery to be touched by recovery or the sweep. ### 4. Tests - Recovery: seed a `retrying` delivery whose target type has been changed to a non-retry type, run recovery, assert the delivery ends `failed` **and** a result row records the reason. - The retry sweep: same, through the sweep path. - Unknown/garbage target type: same outcome, no panic. - A `retrying` delivery whose target is still `http`/`slack` is still rescheduled normally — the regression guard. - Mutation-verify: with the fix reverted, each new test must fail. State the evidence in the PR body. ### 5. Docs README should state that changing a target's type does not migrate in-flight deliveries: any delivery still retrying under the old type is terminally failed and can be redelivered manually. `TODO.md` in the **same commit**. ### Definition of done The issue's own DoD with option 1 chosen, plus items 1-5, `make check` green via the repo's own entrypoints, `.golangci.yml` untouched, single commit whose title ends with ` (closes #82)`, no attribution trailers. ### Conflict note PR #95 and PR #100 both modify `internal/delivery/engine.go` and are merge-ready but unmerged. Keep this diff tight and confined to the two rescheduler sites and the shared helper, so the eventual rebase is mechanical.
clawbot commented 2026-08-09 07:47:57 +02:00
Author
Collaborator
Copy Link

Implementation plan

Implementing option 1 per the manager comment (terminally mark failed with a
recorded reason). Branch issue-82-orphaned-retrying off main @ 4f5ecb1.

Code

One new helper in internal/delivery/engine.go:

func (e *Engine) failUnretryableRetry(
    webhookDB *gorm.DB,
    webhookID string,
    d *database.Delivery,
    target *database.Target,
)

It logs at warn naming the delivery, target and current type; records a
DeliveryResult via the existing recordResult (attempt countAttempts+1,
success=false, error text naming the current target type and stating the
delivery was terminally failed because that type no longer supports retries);
then marks the delivery failed via the existing updateDeliveryStatus.

Both existing sites change from a bare return to a call plus return:

  • recoverSingleRetry (restart recovery, line ~476)
  • sweepSingleRetry (60s sweep, line ~671)

This also covers an unknown/garbage target type, since e.targets[type] yields
a nil Target and the rescheduler assertion fails identically. Nothing else
in engine.go is touched, so the rebase over PRs #95/#100 stays mechanical.

export_test.go gains ExportSweepWebhookRetries so the sweep path is
reachable from the black-box tests (recovery already has
ExportRecoverWebhookDeliveries).

Tests (engine_integration_test.go)

  1. Recovery: retrying delivery whose target type is now log — asserts
    status failed and a DeliveryResult row whose Error names the type.
  2. Sweep: same seed, driven through ExportSweepWebhookRetries.
  3. Unknown/garbage target type (type = "wat"): same outcome, no panic.
  4. Regression guard: a retrying delivery on a still-http target is
    rescheduled normally and is not marked failed.

Plus mutation evidence: revert the two call sites, show each new test fails,
restore.

Docs

README "Recovery paths" gains a note that changing a target's type does not
migrate in-flight deliveries — any delivery still retrying under the old type
is terminally failed and can be redelivered manually. TODO.md updated in the
same commit.

Verification

make fmt, make check, script/cibuild, plus the Gitea CI run on the head
commit. .golangci.yml untouched.

## Implementation plan Implementing option 1 per the manager comment (terminally mark `failed` with a recorded reason). Branch `issue-82-orphaned-retrying` off `main` @ `4f5ecb1`. ### Code One new helper in `internal/delivery/engine.go`: ``` func (e *Engine) failUnretryableRetry( webhookDB *gorm.DB, webhookID string, d *database.Delivery, target *database.Target, ) ``` It logs at **warn** naming the delivery, target and current type; records a `DeliveryResult` via the existing `recordResult` (attempt `countAttempts+1`, `success=false`, error text naming the current target type and stating the delivery was terminally failed because that type no longer supports retries); then marks the delivery `failed` via the existing `updateDeliveryStatus`. Both existing sites change from a bare `return` to a call plus `return`: - `recoverSingleRetry` (restart recovery, line ~476) - `sweepSingleRetry` (60s sweep, line ~671) This also covers an unknown/garbage target type, since `e.targets[type]` yields a nil `Target` and the `rescheduler` assertion fails identically. Nothing else in `engine.go` is touched, so the rebase over PRs #95/#100 stays mechanical. `export_test.go` gains `ExportSweepWebhookRetries` so the sweep path is reachable from the black-box tests (recovery already has `ExportRecoverWebhookDeliveries`). ### Tests (`engine_integration_test.go`) 1. Recovery: `retrying` delivery whose target type is now `log` — asserts status `failed` **and** a `DeliveryResult` row whose `Error` names the type. 2. Sweep: same seed, driven through `ExportSweepWebhookRetries`. 3. Unknown/garbage target type (`type = "wat"`): same outcome, no panic. 4. Regression guard: a `retrying` delivery on a still-`http` target is rescheduled normally and is **not** marked `failed`. Plus mutation evidence: revert the two call sites, show each new test fails, restore. ### Docs README "Recovery paths" gains a note that changing a target's type does not migrate in-flight deliveries — any delivery still `retrying` under the old type is terminally failed and can be redelivered manually. `TODO.md` updated in the same commit. ### Verification `make fmt`, `make check`, `script/cibuild`, plus the Gitea CI run on the head commit. `.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#82
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?