Skip to content

fix(web): scope /api/workflows AI rate limit to the start (POST), not status polls - #1524

Closed
groupthinking wants to merge 1 commit into
mainfrom
fix/workflows-poll-rate-limit
Closed

fix(web): scope /api/workflows AI rate limit to the start (POST), not status polls#1524
groupthinking wants to merge 1 commit into
mainfrom
fix/workflows-poll-rate-limit

Conversation

@groupthinking

Copy link
Copy Markdown
Owner

Problem (live on main)

The durable video-to-actions status poll — GET /api/workflows/video-to-actions/:runId — fires every ~1.5s (20 attempts, studio-workflow.ts). But /api/workflows was blanket-listed in AI_ROUTE_PREFIXES (proxy.ts), so every poll drew from the strict AI budget (12/min) on the shared ai:<ip> bucket.

Consequence: after ~11 polls (~16s) the client is 429'd mid-run. getVideoToActionsStatus treats a 429 as non-terminal (not 404, no runStatus), so pollVideoToActions never breaks early — it burns all 20 attempts and the UI hangs on "still running". The polling also starves the same ai:<ip> bucket used by /api/chat, /api/pipeline, etc.

Flagged by the Vercel Agent (VADE) reviewer on #1507 (merged before the fix landed); this is the follow-up.

Fix

Classify /api/workflows by method instead of a blanket prefix:

  • POST (start = real transcription + action-gen work) → keeps the 12/min AI budget.
  • GET (status poll) → 60/min general budget.

A single run's ~20 polls/30s sits comfortably under the general limit, and the expensive start stays protected.

Tests

New apps/web/src/__tests__/proxy-rate-limit-classification.test.ts (4 cases), asserting the applied budget via the X-RateLimit-Limit header:

  • workflow poll (GET) → 60 (was 12 before the fix — the regression guard)
  • workflow start (POST) → 12
  • other AI routes (e.g. /api/chat) → 12 regardless of method
  • ordinary API routes → 60

Existing proxy-auth-gate + auth-paths + studio-workflow suites still pass; tsc --noEmit and ESLint clean on the changed files.

Refs #1507 (VADE logic finding).

🤖 Generated with Claude Code

… polls

The video-to-actions status poll (GET /api/workflows/.../:runId) fires every
~1.5s (studio-workflow.ts), but /api/workflows was blanket-listed as an AI
route (12/min). After ~11 polls the client exhausted the AI budget and was
429'd mid-run; getVideoToActionsStatus treats 429 as non-terminal, so
pollVideoToActions burned all 20 attempts and the UI hung on "still running".
The poll also starved the shared ai:<ip> bucket used by /api/chat, /api/pipeline.

Classify /api/workflows by method instead of a blanket prefix: POST (start =
real AI work) keeps the 12/min AI budget; GET (status poll) uses the 60/min
general budget. Adds proxy-rate-limit-classification.test.ts.

Addresses PR #1507 review (VADE logic finding).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added javascript Pull requests that update javascript code tests labels Aug 13, 2026
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3166d88c-c610-4100-9989-1587b6f8eba1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

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

@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 2432330.
Ensure 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 Files

None

@groupthinking groupthinking self-assigned this Aug 13, 2026

Copy link
Copy Markdown
Owner Author

Reconciliation: #1518 merged the same fix — this one needs a decision, and one piece of it is worth keeping

We independently fixed the same defect. #1518 merged as 12f5c44 at 07:57 UTC, ~5 hours after this PR was opened. Same root cause, same diagnosis (isAiRoute classifying by prefix alone, so the ~1.5s status poll drew from the 12/min AI budget). Under MERGE_POLICY.md gate 6 this is a reconciliation, and the merged one is the winner by default — but not on every axis, and the delta is worth recording rather than just closing.

This PR will not apply cleanly any more

Both hunks target lines that no longer exist. #1518 moved AI_ROUTE_PREFIXES and isAiRoute out of proxy.ts into auth-paths.ts — the file whose docstring designates it for path policy "free of Next.js request types so vitest can import it offline," and which already hosted the sibling shouldSkipRateLimit. proxy.ts now imports the classifier. So this is a rebase-onto-a-moved-function, not a textual conflict to resolve in place.

Where the merged version differs, and why

this PR #1518 (on main)
Workflow methods that stay AI-class POST only everything except GET/HEAD
Prefix match for the exemption loose startsWith requires a segment boundary
Poller cadence unchanged (20 × 1.5s = 40/min) 30 × 2s = 30/min

Two of those are worth being concrete about:

method === 'POST' sends PUT/PATCH/DELETE to the general budget. No such handler exists on /api/workflows today, so nothing is broken — but the direction of the default matters. If someone later adds a mutating verb that reaches the workflow runtime, this version silently meters it at 60/min. #1518 inverts that: only reads are exempt, so an unanticipated verb lands on the stricter budget.

startsWith('/api/workflows') catches /api/workflows-admin. With this PR, a future route by that name would hit the workflow branch and get the general budget on any non-POST method. I shipped the same bug in #1518's first commit and caught it on self-review — it is the shape #1486 had to fix in the SSRF guard, where an incidental block quietly became an allow. Latent in both cases; no such route exists.

The poll retune isn't cosmetic. Fixing only the classifier moves the poller from 40/min under a 12/min ceiling to 40/min under a 60/min one. That clears, but leaves ~20 req/min for everything else the Studio page does on the shared api:<ip> bucket. 2s spends 30/min and leaves about half.

What this PR has that main does not

Your tests are end-to-end and main's are not. proxy-rate-limit-classification.test.ts drives the real proxy() through a NextRequest with env stubbed, and asserts the applied budget off the X-RateLimit-Limit header. #1518's 10 cases test the extracted classifier as a pure function — they pin the decision, but nothing pins that the decision actually reaches the response header. A refactor that broke the wiring between isAiRoute and checkRateLimit would pass all 10 and fail all 4 of yours.

That is real coverage and it survives the move: the tests import @/proxy, not the classifier, so they should pass against main unmodified.

Suggested resolution

Close this and port proxy-rate-limit-classification.test.ts to main as a test-only PR. Gate 6's 72-hour rule says don't leave both open, and the implementation half is already merged — but closing without salvaging the test loses the only end-to-end assertion this behaviour has.

I have not closed it myself, since the salvage decision is yours and closing would bury it. Happy to open the test-only PR if you'd rather not.

Unrelated blocker on this PR

Vercel — Account is blocked is failing here, and it is not about your diff. It is account-level and repo-wide: the same failure is on #1518 and #1528, and Vercel Deployments – garv_projects has been stuck pending ("Required and affected projects deploying") since at least 02:41 UTC. No previews are building, which makes MERGE_POLICY.md gate 4 unsatisfiable for every apps/web/** PR until the account is unblocked — precisely the "gate nobody could satisfy" failure mode the policy was rewritten to avoid.


Generated by Claude Code

@groupthinking
groupthinking enabled auto-merge (squash) August 13, 2026 11:41
@groupthinking

Copy link
Copy Markdown
Owner Author

Closing as superseded by #1518 (merged on main at 12f5c44ac).

#1524 and #1518 both stop GET /api/workflows/... status polls from drawing on the 12/min AI budget. #1518 is the landed form: method-aware classifier moved into auth-paths.ts, segment-boundary exemption, poller retuned to 30×2s, and tests. This PR is CONFLICTING/DIRTY against that work — rebasing it would only duplicate a fix that is already on main.

No further action on this branch.

@groupthinking

Copy link
Copy Markdown
Owner Author

Superseded by merged #1518 (12f5c44). Not rebasing a conflicting duplicate.

auto-merge was automatically disabled August 13, 2026 11:52

Pull request was closed

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

Labels

javascript Pull requests that update javascript code tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant