Repository navigation
fix(webhooks): stop admission-refusal retry loops and per-retry log writes - #8870
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 34 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
94db255 to
5b2a564
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 34 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 34 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…locked-run logs Usage-limit, suspended-account, and missing-billing-account refusals now carry stable codes, so surfaces can tell a refusal that holds until a person acts from a transient one. Unattended surfaces can opt into throttleErrorLogs: each refusal of a workflow by the same gate records at most one execution error row per 15-minute window (Redis SET NX, in-process LRU without Redis, fail-open on Redis errors). A sender resending a refused delivery no longer writes an execution row, trace archive, and workspace_files row per attempt. A caller-supplied logging session is always completed.
…egram and Slack Telegram resends a non-2xx update until it succeeds or 24 hours pass, and Slack disables an app's event subscription once most deliveries fail, so answering a usage-limit refusal with 402 only loops. Providers opt in with acknowledgeAdmissionRejections and get an empty 200 with an ignored outcome; transient refusals (rate limit, concurrency, reservation outage) still fail so the sender retries. Generic webhooks keep the 402. Polling receives the raw refusal with its code. Webhook preprocessing throttles its error rows.
…sources The poll orchestrator checks each workspace's payer once per tick and skips its webhooks while the payer is over its usage limit: nothing is fetched, marked seen, or counted as a failure, so items deliver once the payer is back under the limit and triggers are no longer auto-disabled over billing state. A webhook with consecutive poll failures waits 2^(n-1) minutes (capped at an hour) before the next fetch, and a source's Retry-After or FLOOD_WAIT_<n> is persisted and honored. RSS logs a source's 4xx once at warn, and an admission refusal mid-poll leaves the remaining items unseen.
…og them once A POST to a path with no webhook keeps its 404 but now carries x-slack-no-retry, sent unconditionally so it reveals nothing the 404 does not. The processor and route lines for an unknown path drop to debug, leaving the route handler's single client-error line.
Register a generated secret_token with setWebhook, store it as providerConfig.secretToken, and reject deliveries whose X-Telegram-Bot-Api-Secret-Token header does not match (401). Webhooks registered before this change have no stored secret and stay accepted until their next deploy registers one. A new registration reuses the active deployment's secret for the same bot so deliveries keep verifying during cutover. Drops the stale empty User-Agent warning: the proxy exempts webhook trigger routes from the empty-UA block.
An unreadable usage ledger fails closed as exceeded; it said nothing about the payer, yet it was tagged USAGE_LIMIT_EXCEEDED and acknowledged-and-dropped for Telegram and Slack. It now stays untagged and retryable. Reservation headroom denials clear as in-flight runs settle, so they leave the deterministic set too. The blocked-run log claim drops its in-process fallback: usage refusals only happen on hosted billing deployments, which run Redis, and without Redis every refusal records its row. A Redis failure logs at debug.
Active rows store the bot token as authored, often a {{VAR}} reference, while
subscription calls receive it resolved, so the comparison never matched: every
deploy minted a fresh secret (a candidate that never activated left the bot
rejected by the active row), and retiring an old version could delete the
webhook the active version still used. Stored tokens are now resolved against
the background webhook env before comparing.
…source failures The per-tick payer pre-check read billing attribution and the usage ledger for every polled workspace, including healthy idle ones. A deterministic admission refusal of a polled event now records the workspace in Redis for five minutes, and the orchestrator skips only those workspaces in one MGET; healthy payers cost no billing reads, and billing-disabled deployments skip the mechanism. Backoff now follows source fetch failures only, tracked in providerConfig and stamped from the failed poll's start, so transient item-processing refusals never back a webhook off. Poll state keys are system-managed so deploy change detection ignores them.
…backoff Every poller's outer catch now records a source failure, so a failing Gmail, Outlook, IMAP, Drive, Sheets, Calendar, or HubSpot source backs off like RSS instead of only RSS. markWebhookSuccess clears the backoff in its existing reset write, the window uses the shared jittered backoff, and the orchestrator asks a boolean isPollBackedOff. Smaller cleanups: one isDroppedDispatch predicate for the Slack and path fan-outs, explicit precedence for a polled refusal's code, a typed RSS refusal error instead of a flag, Telegram resolves only the stored bot token, and PollOutcome lives with the polling types.
Poll outcome docs describe what a skipped poll actually does, source failures keep logging the full error object as the pollers did before, the backoff table pins the clock past the poll start so it proves the window is anchored there, and the RSS rate-limit test drives a Retry-After header.
… refusal Only RSS stopped at a refused item; Gmail, Outlook, and IMAP advanced their cursors past refused emails, and every poller counted the refusals toward auto-disable. A shared PollAdmissionRefusedError now leaves the idempotency callback, stops the batch, and returns skipped before any cursor update or failure count; items that already ran replay as idempotent no-ops. Source backoff goes back to RSS only, where the rate-limited feed was: the other pollers' fetch helpers do not carry status or Retry-After, so routing their failures through it would back off on a guess. The block-missing 404 also tells Slack not to redeliver.
…an-out failure A poller stops its batch on a deterministic refusal only while nothing in the batch has completed; once an item has run, the refusal is an ordinary item failure, so the poller saves its completed work exactly as before and no completed event can replay after the idempotency window. In a multi-target delivery a missing block's no-retry 404 no longer stands in for a target that failed and needs the sender to retry. A source's Retry-After is counted from its answer rather than the poll's start. The RSS backoff keys are cleared by RSS's own state write, so other pollers' success path is unchanged, and two fields nothing reads are dropped.
A workspace without a billing account fails inside payer resolution and takes the retryable attribution-error path; the branch that tagged BILLING_ACCOUNT_REQUIRED only ran for an attribution with no actor, which system attribution never produces. The branch goes back to its staging form and the deterministic set keeps the usage limit and suspended accounts.
…lling utils mock Two gates that fail without a code (a ban lookup error and a usage lookup error) no longer share one throttle claim, so neither hides the other's row. The polling utils module gets one central mock in @sim/testing, replacing the partial importOriginal mocks and the hand-rolled factory in the table trigger test.
410b107 to
3a141dd
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 37 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
…its token with Subscription creation resolves the incoming bot token with the deployer's env, while cleanup resolves with the background env; the active-row matcher now uses the same env as its caller, so a bot referenced through a personal variable still reuses the active secret and is not deleted from under the active deployment. Tests: the idempotency service gets one central mock in @sim/testing, used by every test that mocked it locally, and the new tests import single factory and mock files instead of the @sim/testing barrel.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 40 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
…n the central mock
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 40 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
Summary
Stops webhook senders from looping on refused deliveries, stops each retry from writing a new execution log, and stops pollers from hammering failing or blocked sources.
Root causes (from the webhook and poller logs)
workspace_filesrow after being refused.429 FLOOD_WAIT_*) every minute.Admission codes
WORKFLOW_NOT_DEPLOYED:USAGE_LIMIT_EXCEEDEDandACCOUNT_SUSPENDED.Ack-and-drop opt-in (
acknowledgeAdmissionRejectionson the provider handler)ignoredoutcome. Transient refusals still fail so the sender retries.Blocked-run log dedupe
SET NX EX; fails open).Polling
PollAdmissionRefusedErrorout of the item's idempotency callback.skippedwithout advancing its cursor or counting a failure, so the refused items stay pending. Once an item has completed, the refusal is an ordinary item failure, so completed work is saved exactly as before and never replays. RSS, which tracks items by GUID, always records the completed ones and leaves the rest unseen.MGET: no fetch, no failure count, no state change. Healthy payers cost no billing reads, and billing-disabled deployments skip this.Retry-After/FLOOD_WAIT_<n>, counted from its answer, asks for more.Deleted path
x-slack-no-retry: 1, sent unconditionally, so it reveals nothing the 404 doesn't.Telegram secret token
setWebhookregisters a per-webhooksecret_token, and deliveries without it get 401.{{VAR}}bot tokens are resolved before comparing, which also stops retiring an old version from deleting the active bot's webhook.Behavior changes
What to watch after deploy
/api/webhooks/trigger/*should collapse for Telegram and Slack paths. Generic webhooks keep their 402.429,FLOOD_WAIT) should log one warn per backed-off attempt instead of errors every minute.Type of Change
Testing
processor.test.ts: ack seam, transient and headroom refusals still fail, polled raw code, refusal recording.preprocessing.test.ts: codes,usage_unavailable, throttle per gate, supplied session.route.test.ts: 404 no-retry header, mixed fan-out (refusal vs. failure, missing block vs. failure).slack-dispatch.test.ts: mixed fan-out.rss.test.ts: refused item unseen,Retry-After.polling/utils.test.ts: backoff anchored at poll start,Retry-After.polling/orchestrator.test.ts: refusal skip, backoff skip, billing-disabled.polling/google-calendar.test.ts: a refused first event stops the batch with no cursor advance or failure count; a refusal after a completed event saves the cursor as before.telegram.test.ts: env-var token secret reuse and delete guard.blocked-run-log.integration.tsagainst real Redis: one winner of 25 concurrent claims, per-gate keys, expiry, fail-open.type-checkpassed.check:realtime-prunepasses on its own. CI runs the fullcheck:audits(includingcheck:unused-exports) and integration suites.Checklist
test-auditauthoring gate)