fix(admin): unblock webhook drains when subscribers stall

[SebTardif]

What Problem This Solves

Fixes post-mutation webhook drains hanging when a subscriber never responds.

User Impact

A timed-out attempt records timeout after two seconds, advances through that attempted entry, and lets other subscribers proceed. Unattempted entries stay queued for the next drain.

Why This Change Was Made

This preserves the notify-only delivery contract: attempted failures are not retried. The branch is updated to current main, with dashboard-operator documentation and an Unreleased entry. Thanks @SebTardif for the original fix; the contributor commits and co-author credit are preserved.

Evidence

The regression uses synthetic D1 fixtures without a competing background drain. It verifies one timed-out attempt, preservation of the next queued entry, delivery to a healthy subscriber, recovery, and no duplicate delivery. Against the original implementation it exceeds the three-second test limit; with the fix it passes. The complete gate on a clean AWS Crabbox install with Node 24.19.0 and pnpm 11.28.0 passes lint, typecheck, 277 tests, dashboard/demo/CLI builds, docs, and packed SDK verification. Independent Codex review is clean through P2.

No production endpoint was contacted. Exact-head CI and CodeQL passed at 192ef4db835ba7dde845f208367394359892e325. This PR awaits the required code-owner approval from the requested reviewers.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed October 6, 2026, 11:59 PM ET / October 7, 2026, 03:59 UTC (Revision 33).

ClawSweeper review

What this changes

The PR bounds webhook notification attempts to two seconds, records timeouts, preserves untouched queued entries, and adds recovery coverage and operator documentation.

Merge readiness

⛔ Blocked before merge - 2 items remain

The fix remains necessary and no actionable code defect remains. The prior contract, regression-test, and changelog concerns are resolved; real transport proof through the production Worker drain remains incomplete.

Priority: P2
Reviewed head: 192ef4db835ba7dde845f208367394359892e325

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The focused patch now has sound regression coverage and documentation, but available runtime evidence still leaves the production transport path unproven.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the updated D1 regression exercises drainWebhooks with simulated fetch clients, while preserved earlier Node cancellation evidence does not connect real transport to the Worker drain and remainder recovery. Provide redacted local Worker logs or terminal output showing timeout, persisted cursor, healthy-subscriber progress, and later untouched-entry delivery; production deployment is unnecessary. Screenshots or recordings count when they show that behavior. Redact credentials, IP addresses, private endpoints, and personal details. Updating the PR body should trigger review; otherwise ask a maintainer to comment @clawsweeper re-review. No stored-data contract changes require migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the updated D1 regression exercises drainWebhooks with simulated fetch clients, while preserved earlier Node cancellation evidence does not connect real transport to the Worker drain and remainder recovery. Provide redacted local Worker logs or terminal output showing timeout, persisted cursor, healthy-subscriber progress, and later untouched-entry delivery; production deployment is unnecessary. Screenshots or recordings count when they show that behavior. Redact credentials, IP addresses, private endpoints, and personal details. Updating the PR body should trigger review; otherwise ask a maintainer to comment @clawsweeper re-review. No stored-data contract changes require migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	9 items	Verified scope and parentage: The pinned merge-base-to-head delta contains 25 files. Raw head parents are current main followed by the earlier contributor head. Comparing the exact head with its current-main parent leaves only webhook source, its regression test, operator documentation, and one changelog entry; inherited CLI, dependency, and workflow changes are already on main. The stale test merge was not used. Still needed on current main: Current main awaits each subscriber POST without an abort signal. A subscriber that never returns can prevent progress to subsequent subscribers and cursor persistence. Application release and canonical check: The v0.5.0 application source also contains the unbounded POST. The bounded GitHub pull-list search found this PR and the distinct, open destination-security PR #75, with no merged webhook timeout replacement.
Findings	None	None.
Security	None	None.

How this fits together

Successful admin mutations append changes to D1 and schedule background webhook notifications. The drain sends changes to enabled subscribers and saves delivery cursors and status for the dashboard.

flowchart TD
  A[Admin mutation] --> B[D1 change log]
  A --> C[Background webhook drain]
  B --> C
  D[Subscriber URLs and cursors] --> C
  C --> E[POST with deadline]
  E --> F[Save attempted cursor and status]
  F --> G[Dashboard delivery result]
  F --> H[Untouched entries await next drain]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the updated D1 regression exercises drainWebhooks with simulated fetch clients, while preserved earlier Node cancellation evidence does not connect real transport to the Worker drain and remainder recovery. Provide redacted local Worker logs or terminal output showing timeout, persisted cursor, healthy-subscriber progress, and later untouched-entry delivery; production deployment is unnecessary. Screenshots or recordings count when they show that behavior. Redact credentials, IP addresses, private endpoints, and personal details. Updating the PR body should trigger review; otherwise ask a maintainer to comment @clawsweeper re-review. No stored-data contract changes require migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Complete next step (P2) - Add redacted real-transport evidence through the local production Worker drain demonstrating timeout persistence, healthy-subscriber progress, and later delivery of untouched entries.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	webhook production +29/-7; tests +90/-0	The production growth implements the stated deadline, while regression coverage verifies healthy subscribers and recovery.
Change relative to current main	4 files differ	The remaining 21 files in the pinned introduction delta already match current main.

Technical review

Best possible solution:

Keep the accepted notify-only deadline and preservation of untouched entries, supported by a real Worker transport trace demonstrating timeout recovery.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes the hang path: current main awaits an unbounded subscriber POST before advancing to other hooks. No failing runtime reproduction was executed during this read-only review.

Is this the best way to solve the issue?

Yes, the focused deadline and attempted-entry cursor tracking match the now-recorded delivery decision; the isolated regression addresses the previous race and remainder concerns.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 7be7204292c9.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This repairs bounded background notification reliability without demonstrated disruption to public flag evaluation.
• merge-risk: 🚨 compatibility: Existing subscribers taking longer than two seconds lose their former unlimited wait; the repository administrator explicitly adopted and documented this tradeoff.
• merge-risk: 🚨 message-delivery: A timed-out attempted notification is permanently skipped under the accepted notify-only policy, while untouched entries remain queued.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Needs real behavior proof before merge: the updated D1 regression exercises drainWebhooks with simulated fetch clients, while preserved earlier Node cancellation evidence does not connect real transport to the Worker drain and remainder recovery. Provide redacted local Worker logs or terminal output showing timeout, persisted cursor, healthy-subscriber progress, and later untouched-entry delivery; production deployment is unnecessary. Screenshots or recordings count when they show that behavior. Redact credentials, IP addresses, private endpoints, and personal details. Updating the PR body should trigger review; otherwise ask a maintainer to comment @clawsweeper re-review. No stored-data contract changes require migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Verified scope and parentage: The pinned merge-base-to-head delta contains 25 files. Raw head parents are current main followed by the earlier contributor head. Comparing the exact head with its current-main parent leaves only webhook source, its regression test, operator documentation, and one changelog entry; inherited CLI, dependency, and workflow changes are already on main. The stale test merge was not used. (apps/api/src/admin/webhooks.ts:181, 192ef4db835b)
• Still needed on current main: Current main awaits each subscriber POST without an abort signal. A subscriber that never returns can prevent progress to subsequent subscribers and cursor persistence. (apps/api/src/admin/webhooks.ts:171, 7be7204292c9)
• Application release and canonical check: The v0.5.0 application source also contains the unbounded POST. The bounded GitHub pull-list search found this PR and the distinct, open destination-security PR fix(admin): webhook create accepts private and metadata URLs #75, with no merged webhook timeout replacement. (apps/api/src/admin/webhooks.ts:171, 7be7204292c9)
• Recorded deadline decision: The exact head authored by Peter Steinberger explicitly chooses two-second delivery, advancement only through attempted entries, and preservation of the remainder. GitHub independently verifies steipete has repository admin permission. This addresses the earlier request for owner acceptance: slow attempted notifications may be skipped under the documented notify-only contract, and that accepted tradeoff is retained rather than requested again. (apps/api/src/admin/webhooks.ts:181, 192ef4db835b)
• Decision authority: GitHub returned permission admin for steipete, corroborating authority for the explicit delivery decision in the updated head.
• Prior findings addressed: The new regression seeds D1 directly, attempts one hanging entry, delivers both entries to a healthy hook, then delivers only the untouched remainder after recovery and verifies no redelivery. CHANGELOG.md and docs/admin-console.md now document the timeout and cursor outcome. The hanging and recovered fetch clients remain simulated. (apps/api/test/webhooks.test.ts:131, 192ef4db835b)

Likely related people:

• vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add a redacted real-transport trace through the local Worker drain showing timeout persistence, healthy-subscriber delivery, and untouched remainder recovery.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (32 earlier review cycles; latest 8 shown)

• reviewed 2026-09-01T20:40:24.929Z sha e12a356 :: found issues before merge. :: [P1] Obtain approval for the two-second delivery cutoff | [P3] Add the required Unreleased changelog entry
• reviewed 2026-09-03T07:57:00.477Z sha e12a356 :: blocked before merge. :: [P1] Preserve slow subscriber deliveries | [P3] Add the required Unreleased changelog entry
• reviewed 2026-09-03T13:39:26.940Z sha e12a356 :: blocked before merge. :: [P1] Obtain approval for the two-second delivery cutoff | [P3] Document the new timeout behavior in Unreleased
• reviewed 2026-09-09T19:21:19.348Z sha e12a356 :: needs real behavior proof before merge. :: [P1] Make the new delivery cutoff an explicit upgrade contract | [P2] Assert remaining entries instead of an empty second drain | [P3] Document the timeout and cursor outcome in Unreleased
• reviewed 2026-09-11T16:37:17.984Z sha 0ad54b8 :: needs real behavior proof before merge. :: [P1] Make the new delivery cutoff an explicit upgrade contract | [P2] Assert remaining entries with an isolated drain fixture | [P3] Document the timeout and cursor outcome in Unreleased
• reviewed 2026-09-18T09:53:29.144Z sha 0ad54b8 :: needs real behavior proof before merge. :: [P1] Resolve the delivery cutoff contract for existing subscribers | [P2] Assert remaining entries with an isolated drain fixture | [P3] Document the timeout and cursor outcome in Unreleased
• reviewed 2026-10-02T10:42:48.014Z sha 0ad54b8 :: needs real behavior proof before merge. :: [P1] [P1] Make the delivery cutoff an explicit upgrade contract | [P2] [P2] Isolate pending entries and assert remainder delivery | [P3] [P3] Document the timeout behavior in Unreleased
• reviewed 2026-10-04T14:26:49.351Z sha 0ad54b8 :: needs real behavior proof before merge. :: [P1] [P1] Resolve the delivery cutoff contract for existing subscribers | [P2] [P2] Isolate pending rows and assert remainder delivery | [P3] [P3] Document the timeout behavior in Unreleased

[SebTardif]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[SebTardif]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[SebTardif]

@clawsweeper

[P1] Preserve slow webhook deliveries across the new cutoff
A subscriber that responds just after two seconds now reaches the existing cursor-advance path and permanently misses the batch.

Pushed 63f6c5e. Timeout still aborts the hung POST. The cursor stays put so the next drain retries that batch. HTTP and unreachable outcomes still advance (notify-only).

@clawsweeper re-review

[SebTardif]

@clawsweeper

[P1] Get approval for retry-on-timeout delivery semantics
The new timeout path leaves the cursor unchanged, but the released webhook contract says failed posts are notify-only and never retried. A timeout can follow a completed remote side effect, so the next drain may duplicate entries; either preserve cursor advancement or obtain explicit approval.

Pushed e12a356. Contract chosen: notify-only, matching the released drain docs. Timeout now advances the cursor like HTTP and unreachable, then stops the batch.

@clawsweeper re-review

[steipete]

Approved: exact-head CI green and reviewed.

The merge-base changed after approval.