Skip to content

fix(server): cap delivery history for oversized coalesced webhooks - #24046

Open
only21mil wants to merge 2 commits into
pingdotgg:mainfrom
only21mil:r13/n015-bound-oversized-coalesced-webhook-failur
Open

only21mil wants to merge 2 commits into
pingdotgg:mainfrom
only21mil:r13/n015-bound-oversized-coalesced-webhook-failur

Conversation

@only21mil

Copy link
Copy Markdown
Contributor

Problem

A webhook task with a coalesce key keeps every oversized-prompt failure in its delivery history, past the 20-row limit #22234 set for them.

Webhook delivery history keeps three buckets per task: main (50 rows), filtered (20) and suppression (20). #22234 puts oversized-prompt failures in the suppression bucket so a flood of them cannot push out real runs.

A coalesced delivery escapes that limit. It is logged as accepted when its window opens, so insertion prunes the main bucket. When the window closes and the rendered prompt is longer than the provider limit, markDeliveryFailed changes the row to dispatch_failed with "The filled-in prompt is too long." That moves the row into the suppression bucket, but only recordDelivery prunes, and it prunes the bucket of the row it just inserted. Nothing trims the suppression bucket after the update.

So a webhook task with a coalesce key that keeps receiving oversized payloads, one per window, keeps every failed row. Each row keeps up to 64 KiB of the request body. The Deliveries dialog in Settings lists every retained row, so the list grows without bound too.

Steps: create a webhook task whose prompt includes {{body.text}}, with coalesce key {{body.group}} and a 10-second window. Send a JSON body whose text is 130,000 characters, once per window, 25 times. On main the Deliveries dialog shows 25 failed rows. The limit is 20.

Change

apps/server/src/scheduledTasks/ScheduledTaskService.ts only.

  • The bucket predicate and the retention DELETE move out of recordDelivery into deliveryBucket and pruneDeliveries, unchanged. recordDelivery calls pruneDeliveries as before.
  • markDeliveryFailed runs its UPDATE ... RETURNING task_id and then prunes the row's new bucket, in one transaction. It still ignores errors, as before.

The three limits, the received-time and rowid ordering and every caller stay the same. For "The run failed to start." and the other failure messages the row stays in the main bucket, so the extra prune removes nothing new.

Scope and approval

No issue. This is a focused fix of a broken invariant: #22234 documents that oversized-prompt failures share the suppression bucket's 20-row limit, and the immediate path already keeps it. Only the coalesced path, which fails after the row was logged, misses it. One production file and one test.

Verification

Run on Linux. A new test in ScheduledTaskService.webhook.test.ts drives 21 coalesced windows with a 120,000-character body through the real service and SQLite, and counts the logged rows. It uses the Effect test clock, like the neighbouring coalescing tests.

On main the test fails with 21 rows kept. On this branch it passes.

The new test on main: 21 dispatch_failed rows kept where 20 are expected

This branch: the new test and all 80 scheduledTasks tests pass

Real app on Linux: production web and server builds of main and of this branch, each server with a fresh data directory on loopback, driven from Chromium. In each, I added an empty project and created the task above in Settings > Scheduled Tasks. Then I sent 25 oversized requests, each in its own coalescing window, 13 seconds apart. All 25 got HTTP 202, and no run started.

main this branch
Rows in the Deliveries dialog 25 20
dispatch_failed rows in SQLite 25 20
Logged body characters kept 1,638,400 1,310,720

On main the dialog lists all 25 failed deliveries, oldest 6 minutes ago:

Deliveries dialog on a main build: 25 "Run failed" rows

On this branch it keeps the newest 20:

Deliveries dialog on a build of this branch: 20 "Run failed" rows

Also run on this branch, in apps/server:

  • vp test run src/scheduledTasks: 8 files, 80 tests pass.
  • tsc --noEmit: exit 0, no errors.
  • vp fmt --check and vp lint on both changed files: pass. Lint prints one existing warning in an untouched test at line 521.

Not checked:

  • macOS and Windows runs. The change is SQL inside the server and has no platform branch.
  • Database file size on disk. SQLite reuses freed pages, so the gain is in rows and the size of the Deliveries list.
  • Repository-wide tests and CI.

Prior PRs: #22234 by @juliusmarminge added the three retention buckets and the suppression rule for oversized prompts. This PR reuses its bucket predicate and DELETE unchanged. Open #23579 by @juliusmarminge moves the pending-window drain that calls markDeliveryFailed; it does not change the helper this PR edits, and the two merge cleanly.

Production fix, tests, PR description and images written with Claude Opus 5.5 in T3 Code Nightly.

🤖 Generated with Claude Opus 5.5 in T3 Code Nightly

only21mil and others added 2 commits October 22, 2026 16:44
… limit

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A coalesced window whose rendered prompt is too long moves its logged
delivery into the suppression bucket when it fails, but only insertion
pruned a bucket, so these rows were never trimmed. Prune the row's
bucket after a failure update too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 23, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 92734be

Macroscope's review found this PR approvable — This focused server fix closes a retention gap for oversized coalesced webhook failures while preserving the existing bucket limits and failure behavior. A targeted test verifies that the suppression history remains capped at 20 rows.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between d7a8d6f and 92734be.

📒 Files selected for processing: 2
  • apps/server/src/scheduledTasks/ScheduledTaskService.ts
  • apps/server/src/scheduledTasks/ScheduledTaskService.webhook.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Webhook delivery retention now uses shared bucket classification and pruning logic. Recording and failure updates prune the affected bucket. A test checks that repeated oversized deliveries leave 20 dispatch_failed entries.

Changes: Webhook delivery retention

Layer / File(s) Summary
Bucket classification and pruning
apps/server/src/scheduledTasks/ScheduledTaskService.ts, apps/server/src/scheduledTasks/ScheduledTaskService.webhook.test.ts
Delivery outcomes are classified into filtered, dedupe, or main buckets. Pruning retains the newest 20 filtered or dedupe rows, or 50 main rows, per task. recordDelivery and markDeliveryFailed invoke pruning. The test checks retention after repeated oversized deliveries.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 92734

No actionable merge-blocking risk is identified for the delivery-history cap; the change is ready for normal merge checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: limiting delivery history for oversized coalesced webhooks.
Description check Passed The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the defect, implementation, testing, observed results, and unverified environments. Th…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the defect, implementation, testing, observed results, and unverified environments. The focused-fix rationale addresses the absence of a linked issue.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant