feat(videos): unified /v1/videos submit/poll/content surface (Phase 1, DashScope)#811
Merged
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a unified ChangesUnified video API
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-proxy/src/videos.rs`:
- Around line 585-611: Bound the success-path telemetry model label in the
submit handler before calling telemetry.finish, using the existing
metric_model_label(&snap, &model_name) logic already used by the error path.
Pass the bounded label instead of the raw caller-controlled model_name, while
preserving the existing success response and usage-event behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 953ff6ea-e0b3-40c6-a4b2-0d7e87691529
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/aisix-proxy/Cargo.tomlcrates/aisix-proxy/src/error.rscrates/aisix-proxy/src/jobs.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/videos.rstests/e2e/src/cases/videos-e2e.test.ts
…hScope mapping Add the typed video-generation endpoint family (api7/AISIX-Cloud#1118 Phase 1): - POST /v1/videos — submit a task. OpenAI-shape request (model, prompt, seconds, size); returns the video job object (object:"video", status queued/in_progress/completed/failed, progress, created_at). - GET /v1/videos/{video_id} — poll status. - GET /v1/videos/{video_id}/content — 302 redirect to the provider's video URL once the task succeeded. Stateless task addressing: video_id = base64url_nopad(model_entry_id + ":" + upstream_task_id). The GET routes decode it, re-resolve the Model and ProviderKey from the snapshot, and call the provider's task endpoint — nothing is persisted, so multi-instance DPs need no shared task store. Unknown/undecodable ids are a typed 404 (video_not_found). Provider coverage in this PR: Alibaba DashScope (Wan) — submit POST {api_base}/api/v1/services/aigc/video-generation/video-synthesis with the mandatory X-DashScope-Async: enable header, poll GET {api_base}/api/v1/tasks/{task_id}; task statuses normalise PENDING→queued, RUNNING→in_progress, SUCCEEDED→completed, FAILED/CANCELED/UNKNOWN→failed. seconds → parameters.duration; size "WIDTHxHEIGHT" → parameters.size "WIDTH*HEIGHT". Other providers return a typed 501 not_implemented. Wiring matches the typed-endpoint family: ACL + client-IP allowlist, input guardrail chain over the submit prompt (before the rate-limit reservation), model-level quota enforcement on submit, one zero-token UsageEvent per accepted submit, bounded metric labels. The two GET routes are exempt from model-level rate limits by design — polling a task must not burn the model RPM the submit already paid. Deferred within Phase 1 (tracked in api7/AISIX-Cloud#1118): Zhipu CogVideoX and Volcano Seedance mappings, the CP-side per-second cost schema, and a streaming content proxy for large files.
Remediations from the independent PR audit (api7/AISIX-Cloud#1118): - Bounded metric labels on the GET routes: the unsupported-provider branch recorded the caller-supplied video id as the Prometheus model label — an authenticated caller could explode metric cardinality by minting random ids. GET error paths now record the fixed `unresolved` sentinel. - Wildcard-alias ACL parity: the video id now carries the requested alias as an inner base64url segment (entry_id : b64url(alias) : task_id), and the GET routes run the key ACL against that alias — the identical check the submit performed — so a key allowlisted for `wan/turbo` can poll the task it submitted on a `wan/*` Model. - Disclosure-oracle fold on GETs: ACL denials and the unsupported- provider case both surface as a uniform 404 `video_not_found`, so crafted ids cannot probe the Model table (403 echoed a model identity; 501 revealed entry existence + provider family). Submit keeps its distinct 403/501 — the caller already knows the name there. - Redirect scheme validation: the content route parses the provider- supplied video URL and only forwards absolute http(s) targets; any other scheme is a typed upstream-decode error. - api_base tolerance: `dashscope_root()` strips one trailing `/compatible-mode/v1`, `/api/v1`, or `/v1` from the ProviderKey base so keys configured with the OpenAI-compatible root still reach the native task endpoints instead of opaque upstream 404s. - Hardening: `require_safe_upstream_id` rejects bare `.` / `..` path segments (shared with the jobs surface); submit responses report progress 100 when the provider returns an already-completed status. New unit coverage: metric-label boundedness, wildcard submit→poll→ content journey, restricted-key 404 with zero upstream calls, compatible-mode base URL rewrite, non-http scheme rejection, allowed_cidrs on the poll route, extended codec roundtrip/tamper table.
…lias Review follow-up: on the submit success (and 501) path the raw requested model name reached the Prometheus model label; a wildcard Model accepts unbounded alias strings, so this was the #451 cardinality shape on the new route. Label by the resolved entry's display_name (exact matches unchanged; wildcard traffic aggregates under the pattern), with a registry-rendering test pinning that the alias never appears.
moonming
force-pushed
the
feat/videos-endpoint-p1
branch
from
July 23, 2026 09:40
333ce37 to
f7808eb
Compare
This was referenced Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the typed video-generation endpoint family — Phase 1 of the unified
/v1/videossurface. Design decisions are pinned in the internal issueapi7/AISIX-Cloud#1118(#1116surfaced the gap: video traffic could only ride the raw passthrough tunnel, so model routing, model-level rate limiting, budgets, and guardrail attribution were absent for it).Three routes in
crates/aisix-proxy/src/videos.rs:POST /v1/videos— submit a generation task. OpenAI-shape request (model,prompt, optionalsecondsas string or int, optionalsizeasWIDTHxHEIGHT); responds with the video job object.GET /v1/videos/{video_id}— poll status.GET /v1/videos/{video_id}/content—302redirect to the provider's video URL when the task succeeded;400 invalid_request_errorwhile queued/in-progress or when generation failed.Contract sources
openai-pythonSDK (src/openai/types/video.py,video_create_params.py,video_create_error.py):id,object: "video",model,status(queued/in_progress/completed/failed),progress,created_at,seconds,size, and the{code, message}errorobject on failed jobs. The docs page rejects unauthenticated fetches, so the generated SDK types serve as the verifiable spec snapshot.POST {api_base}/api/v1/services/aigc/video-generation/video-synthesiswith the mandatoryX-DashScope-Async: enableheader and the{model, input: {prompt}, parameters}envelope; pollGET {api_base}/api/v1/tasks/{task_id}; the video URL arrives in the poll response (output.video_url). Status mapping:PENDING→queued,RUNNING→in_progress,SUCCEEDED→completed,FAILED/CANCELED/UNKNOWN→failed.A unified submit/poll/content surface with this exact status enum is the shape established gateways converge on for video generation; the differentiation here is provider coverage, starting with DashScope.
Design decisions (binding, from the internal issue)
video_id = base64url_nopad(model_entry_id + ":" + upstream_task_id). GET routes decode the id, re-resolve the Model row and its ProviderKey from the live snapshot, and call the provider's task endpoint. Nothing is persisted — multi-instance DPs need no shared task store. Unknown or undecodable ids → typed 404 (video_not_found, newProxyError::VideoNotFoundvariant). Decoded task ids pass the same charset guard the jobs surface applies before URL interpolation (jobs::require_safe_upstream_id, widened topub(crate)).quota::enforce(state, auth, Some(&model_rl))— the same shape as chat/embeddings and the layer#1116added to passthrough. The two GET routes passNonefor the model layer by design: a client polling a task it already paid an RPM slot to submit must not starve itself. Key-level layers still apply to polls.prompt(same resolution path as embeddings), before the rate-limit reservation so a policy block doesn't burn an RPM slot; blocked-pattern detail stays in operator logs only. No output scan on the GET routes (the outputs are URLs/status, not text).UsageEventper accepted submit (inbound_protocol: "openai", labelvideos), with per-PK attribution and guardrail observability fields; failed submits emit the standard zero-token error event. Per-second cost is deliberately not modeled here — theModelCostper-second extension is a separate control-plane PR (see Deferred). GET routes emit access logs + bounded metrics but no usage events (poll traffic carries no billing signal).auth.key().can_access(...)+ the model client-IP allowlist are enforced on all three routes before any upstream contact. Metricmodellabels are the resolved display name or the fixedunresolvedsentinel — never raw caller input.Mapping notes / divergences
seconds→parameters.duration(integer). Accepted as string (the SDK's wire type) or bare integer.size"WIDTHxHEIGHT"→parameters.size"WIDTH*HEIGHT"— the explicit-dimension parameter the Wan model families document. The Alibaba reference notes the wan2.7 line replacedsizewithresolution/ratioquality tiers ("wan2.7 不再使用 size 字段…而早期模型直接使用 size 字段"); an arbitraryWIDTHxHEIGHTcannot be represented in that tier system without a lossy invented mapping, so this PR maps to the documented explicit-dimension form and callers targeting tier-based models should omitsize. A tier mapping for those models is a follow-up on the same surface.created_atis populated at submit time; DashScope's poll response reports no machine-readable creation timestamp and the gateway stores nothing, so poll responses carry0.secondson completed polls comes from the provider'susageblock when present.api_basefor thealibabavendor — the ProviderKey must carry the regional DashScope endpoint (existingresolve_base_urlerror path).501 not_implementednaming the provider, so "wrong provider" is distinguishable from "wrong request".Deferred (explicitly out of this PR, tracked in the internal issue)
cp-admin.yamlModelCostextension + dashboard form) — a separate control-plane PR; until it lands, video usage events carry zero tokens and no cost.Audit remediation (independent cold review of 028299f)
The audit returned 1 HIGH + 5 MEDIUM gating findings; all six are addressed in the follow-up commit:
modellabel. Fixed — GET error paths record the fixedunresolvedsentinel; pinned by a unit test that renders the metrics registry and asserts the sentinel appears and the raw id does not.wan/turboon awan/*Model could not poll its own task. Fixed — the id now carries the requested alias as an inner base64url segment (b64url(entry_id : b64url(alias) : task_id)) and the GETs runcan_access(alias), the identical check the submit performed. Covered by a wildcard submit→poll→content unit journey.video_not_found. Submit keeps its distinct 403/501: the caller already knows the model name they asked for, so there is nothing to disclose. Covered by a restricted-key test asserting 404 + zero upstream calls + no name in the message.http/httpstargets; other schemes are a typed upstream-decode error. Accepted residual: the redirect target is provider-supplied — the gateway validates scheme and header-safety, but an operator-misconfigured or compromised upstream can still choose the host; the upstream is operator-trusted infrastructure and the gateway never fetches the URL itself.…/compatible-mode/v1), which produced…/compatible-mode/v1/api/v1/services/…upstream 404s. Fixed —dashscope_root()strips one trailing/compatible-mode/v1//api/v1//v1in both URL builders; unit tests pin the rewrite including the outbound path seen by a mock upstream.allowed_cidrsenforcement case on the poll route.Non-gating fixes taken:
require_safe_upstream_idnow rejects bare./..segments (shared with the jobs surface); submit responses reportprogress: 100when the provider returns an already-completed status.Low-severity dispositions (not changed in this PR):
http_clienthas no default timeout and every handler applies the model's configuredtimeoutidentically; changing the default is a family-wide decision, not a videos-surface one.provider == "alibaba"is the Phase 1 gate; a modality marker is a control-plane schema question (canonical incp-admin.yaml) and is tracked with the per-second cost work.Test evidence
videos.rsmod tests, 25 cases): status-mapping table;video_idencode/decode roundtrip incl. task ids containing:; tamper rejection (non-base64, missing separator, empty halves, non-UTF8, raw provider ids); submit param mapping incl. omitted-params and malformed-sizerejection;secondsstring/int normalisation; wiremock handler tests for submit happy path (asserts the DashScope wire body +X-DashScope-Asyncheader), poll status mapping, failed-task error object, content 302 +Location, content not-ready 400, unknown-id 404 (both undecodable and ghost-model), unsupported-provider 501, missing-field 400 envelope, guardrail block 422 withexpect(0)upstream calls, modelrpm=1submit 429 with polling exempt, and upstream 4xx message passthrough.tests/e2e/src/cases/videos-e2e.test.ts, source-blind, real binary against etcd + scripted DashScope-shaped mock upstreams): submit→poll→content-302 happy path asserting the client envelope and the upstream wire shape; modelrpm=1→ second submit 429rate_limit_exceededwithRetry-Afterand zero extra upstream calls; polling not blocked after the cap; unknown/ghost video ids → 404 with no upstream contact.cargo fmtclean;cargo clippy --workspace --testszero warnings;cargo test -p aisix-proxy --lib688 passed;cargo build --bin aisixok; new e2e file 3/3 passed; existing passthrough e2e family (4 files, 7 tests) all passed. Branch rebased onto latestmain(post-fix(proxy): estimate ensemble panel + judge sub-call usage on missing-usage backends #806) and re-verified; full gate re-run after the audit remediation commit (fmt clean, workspace clippy 0 warnings, 688 lib tests, binary build, videos e2e 3/3 + passthrough family 7/7).Summary by CodeRabbit
New Features
Bug Fixes