fix(web): stop workflow status polls draining the shared AI rate-limit bucket - #1518
Conversation
…t bucket #1507 added /api/workflows to AI_ROUTE_PREFIXES. isAiRoute classified by path prefix alone, so the polled GET status endpoint was metered against the AI budget (default 12/min) while pollVideoToActions polled at 40/min. The 12th request 429'd ~17s into a 30s window, before a transcript fetch plus an agent call could finish. The bucket is keyed by class, not path, so every AI prefix shares one ai:<ip> counter -- a single Studio run also 429'd /api/chat, /api/transcribe and /api/pipeline as collateral. Move the classifier into auth-paths.ts, which exists as the home for path policy free of Next.js request types so vitest can import it offline, and make it method-aware. GET/HEAD on /api/workflows falls to the general budget; POST stays AI-class because starting a run does real model work. The exemption is keyed per-prefix rather than exempting GET globally, so it cannot widen another route that later serves model work over GET. An omitted method defaults to POST so the failure mode is the stricter limit. Also retune the poller to 30 attempts x 2s: 30 req/min leaves roughly half the general allowance for the rest of the page, and the wall-clock window doubles to 60s, which better fits the work the run actually does. Closes #1517
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 111 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
groupthinking
added
bug
Claude
|
@coderabbitai review Auto-review skipped this PR because it had no labels — the config requires at least one of 26, and a PR opened via the API starts unlabeled. Added Worth noting this is the failure mode Under gate 3 a config-skip would satisfy the review gate on its own, but this PR changes rate-limiting behaviour and deserves an actual read rather than a technicality. Generated by Claude Code |
|
|
…e AI budget Self-review follow-up. The exemption looked the prefix up with the same loose startsWith used for class membership, so a future sibling surface whose name merely starts with an exempted prefix -- /api/workflows-admin -- would silently inherit the GET carve-out and drop onto the looser budget. No route in the tree does this today (checked every directory under apps/web/src/app/api), so this is latent rather than live. It is worth closing while the file is open: the same shape, an incidental block quietly becoming an allow, is what #1486 had to fix in the SSRF guard. Class membership keeps its original loose matching. Narrowing that would move routes off the stricter budget, which this change has no business doing; the exemption is the widening, so only the exemption is tightened.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
2 participants
Canonical issue
Closes #1517
Outcome
Studio's Act on findings can complete a run instead of 429ing partway through it, and doing so no longer takes
/api/chatand/api/transcribedown as collateral.isAiRouteis now method-aware.GET/HEADon/api/workflowsfalls to the general budget (60/min);POST .../video-to-actionsstays AI-class because starting a run genuinely does model work.Scope
auth-paths.ts—isAiRoute(pathname, method)plusAI_ROUTE_PREFIXES, moved here fromproxy.ts. This file exists as the home for path policy "free of Next.js request types so vitest can import it offline" and already hosts the siblingshouldSkipRateLimit, so the classifier becomes unit-testable without a Next.js request harness.proxy.ts— imports the classifier, threadsrequest.methodthroughgetRateLimitand the bucket-class decision. Net −18 lines.studio-workflow.ts— poll cadence retuned to 30 × 2s.auth-paths.test.ts— 10 new cases.ai:<ip>/api:<ip>class keying is what makes one route's polling everyone else's problem. Splitting buckets per prefix is the more thorough fix and a larger design change; this PR corrects the classification that made it bite.AI_LIMITdefault of 12/min. Unchanged and unexamined here.needsAuthenticationand the gate modes are untouched.Risk
methodargument defaults toPOST, so a future caller that forgets it lands on the tighter budget rather than silently widening one.Verification
Head
3c0d78d. Measured, not inferred.Focused tests — 10 new cases in
auth-paths.test.ts, all passing.Non-vacuity proven by running the suite against each pre-fix baseline, not by reasoning about them:
isAiRoute(the behaviour onmain)startsWith)The remaining 6 new cases pass on every baseline by design — they are controls.
POSTstays AI-class, mutating verbs stay AI-class,GETon/api/chatand/api/transcribestays AI-class, and an omitted method still defaults strict. Without them, a "fix" that simply dropped/api/workflowsfrom the list, or exemptedGETglobally, would also go green.Full
apps/websuite — 58 files, 355 passed, 0 failed. No regressions.npx tsc --noEmit— cleannpm run lint— cleanVercel preview — READY on this branch (gate 4).
Required CI — first run pending on this head.
Review threads resolved — none open.
These tests actually gate, which was not true a few hours ago: #1480 added the
test-frontendjob that runs theapps/webvitest suite in CI.Production evidence
Not applicable as a deployed artefact. The observable effect is the absence of a 429 on a polling endpoint, and reproducing it against a preview needs provider keys plus a real YouTube URL. The rate-limit classification is pure path/method policy with no I/O, so the unit tests measure it directly — which is the reason for moving it out of
proxy.ts.Agent handoff
Note for reviewers
Three things worth flagging beyond the diff.
1. This was found on #1507 before it merged, and merged anyway. The Vercel Agent review flagged the prefix addition, and a follow-up comment quantified it. #1507 merged as
5135f4ewithproxy.tsunchanged from the flagged state, so the defect is onmainnow rather than in review. Not a criticism of the change — it is a good feature — but the finding existed and the merge did not pick it up.2. The poll retune is part of the same defect, not unrelated tidying. Fixing only the classifier moves the poller from 40/min against a 12/min ceiling to 40/min against a 60/min ceiling. That clears, but leaves ~20 req/min for everything else the Studio page does on the shared
api:<ip>bucket — thin enough that ordinary page traffic could still 429. At 2s the run spends 30/min and leaves roughly half the allowance. The wall-clock window doubling to 60s is a side benefit: the previous 30s was shorter than a transcript fetch plus an agent call, which is why the UI'sStill running after N pollsbranch was so easy to hit.3. The second commit is a self-review catch, and it is the same shape as #1486's bug. The exemption originally reused the loose
startsWiththat class membership uses, so a future/api/workflows-adminwould have inherited theGETcarve-out and silently landed on the looser budget. No route in the tree does that today — I checked every directory underapps/web/src/app/api— so it was latent, not live. But "an incidental block quietly becomes an allow" is exactly what the SSRF guard had to fix in #1486, and it was cheap to close here. Class membership deliberately keeps loose matching: tightening it would move routes off the stricter budget, which is a widening this PR has no business making.The deeper issue is that the bucket is keyed by class rather than by path, so any one AI route's traffic is charged against every other's. That is out of scope here and noted under Scope above.