Codex/group media fixed pricing - #4333
Conversation
Enhance empty/loading states and unify spacing/typography for a more product-grade look. Made-with: Cursor
UI/creative center design
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Creative Center assets and archive, async image/video task submission and polling, multi-dimensional pricing (group/seconds/resolution) and billing/quota refinements, task upsert/persistence on submit, Sora/Grok/XAI adaptor enhancements, and broad frontend integrations (asset UI, playground, pricing, task-logs). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / Web
participant AsyncCtrl as Async Controller
participant TaskModel as Task Model (DB)
participant Relay as Relay / Upstream
participant Storage as Service (downloads/archive)
Client->>AsyncCtrl: POST /v1/images/async-generations
AsyncCtrl->>TaskModel: Insert pending Task (metadata, body ref)
TaskModel-->>AsyncCtrl: TaskID
AsyncCtrl-->>Client: 202 AsyncImageTaskResponse (task id)
AsyncCtrl->>Relay: (goroutine) forward saved request to upstream
Relay->>Storage: fetch media or store response bytes
Storage-->>Relay: response bytes / error
alt upstream 2xx
Relay->>TaskModel: Update Task (status=success, result URL, store response bytes)
else non-2xx
Relay->>TaskModel: Update Task (status=failure, fail reason, optionally store body)
end
Client->>AsyncCtrl: GET /v1/images/async-generations/:task_id
AsyncCtrl->>TaskModel: Load Task
TaskModel-->>AsyncCtrl: Task (status/progress/result)
AsyncCtrl-->>Client: AsyncImageTaskResponse
sequenceDiagram
participant Caller as Price resolver
participant PriceHelper as relay/helper/price.go
participant RatioStore as setting/ratio_setting store
participant PriceData as PriceData result
Caller->>PriceHelper: Request price (model, group, ratios)
alt Group seconds price exists
PriceHelper->>RatioStore: GetGroupModelPriceBySeconds(group, model, seconds)
RatioStore-->>PriceHelper: price
PriceHelper->>PriceData: Set GroupPriceOverride, BaseQuota
else Group resolution price exists
PriceHelper->>RatioStore: GetGroupModelPriceByResolution(group, model, resolution)
RatioStore-->>PriceHelper: price
PriceHelper->>PriceData: Set GroupPriceOverride, BaseQuota
else Fallback
PriceHelper->>RatioStore: GetModelPrice or apply ratio_setting
RatioStore-->>PriceHelper: price/ratios
PriceHelper->>PriceData: Compute quota using ratios
end
PriceHelper-->>Caller: PriceData (Quota, OtherRatios, override flags)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
service/quota.go (1)
347-399:⚠️ Potential issue | 🟠 MajorMirror
TokenUnlimitedin post-consume token debits.
PreConsumeTokenQuotanow skips token pre-consumption for unlimited tokens, butPostConsumeQuotastill adjusts token quota for every non-playground request. That can still decrement token balance during final settlement forTokenUnlimitedtraffic.💳 Proposed fix
- if !relayInfo.TokenUnlimited && token.RemainQuota < quota { + if token.RemainQuota < quota { return fmt.Errorf("token quota is not enough, token remain quota: %s, need quota: %s", logger.FormatQuota(token.RemainQuota), logger.FormatQuota(quota)) }- if !relayInfo.IsPlayground { + if !relayInfo.IsPlayground && !relayInfo.TokenUnlimited { if quota > 0 { err = model.DecreaseTokenQuota(relayInfo.TokenId, relayInfo.TokenKey, quota) } else { err = model.IncreaseTokenQuota(relayInfo.TokenId, relayInfo.TokenKey, -quota) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/quota.go` around lines 347 - 399, PostConsumeQuota currently always adjusts token quota for non-playground requests which will wrongly debit unlimited tokens; update PostConsumeQuota to skip token adjustments when relayInfo.TokenUnlimited is true (same guard used in PreConsumeTokenQuota). Concretely, in the block that calls model.DecreaseTokenQuota/model.IncreaseTokenQuota, add a condition checking relayInfo.TokenUnlimited (e.g., if !relayInfo.IsPlayground && !relayInfo.TokenUnlimited) so that model.DecreaseTokenQuota and model.IncreaseTokenQuota are not invoked for unlimited tokens.web/src/hooks/dashboard/useDashboardData.js (1)
178-270:⚠️ Potential issue | 🟡 MinorMissing
buildDashboardStaleKeyin theuseCallbackdependency arrays.
loadQuotaData(L226),loadUptimeData(L251), andgetUserData(L270) all callbuildDashboardStaleKey(...)but none list it in their deps. AlthoughbuildDashboardStaleKeyitself is memoized, its identity depends onisAdminUser;getUserData's deps currently omit both, so ifisAdmin()ever transitioned during a session the callback would keep issuing requests under the stale "user"/"admin" cache namespace. This will also be flagged byreact-hooks/exhaustive-deps.🔧 Proposed fix
- }, [inputs, dataExportDefaultTime, isAdminUser, now]); + }, [inputs, dataExportDefaultTime, isAdminUser, now, buildDashboardStaleKey]); ... - }, [activeUptimeTab]); + }, [activeUptimeTab, buildDashboardStaleKey]); ... - }, [userDispatch]); + }, [userDispatch, buildDashboardStaleKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/dashboard/useDashboardData.js` around lines 178 - 270, The callbacks loadQuotaData, loadUptimeData, and getUserData call buildDashboardStaleKey but do not include it (and in getUserData also isAdminUser) in their useCallback dependency arrays; update the dependency arrays to include buildDashboardStaleKey for loadQuotaData, loadUptimeData, and getUserData, and add isAdminUser to getUserData's deps so the memoized callbacks update when buildDashboardStaleKey or admin state changes (refer to functions loadQuotaData, loadUptimeData, getUserData).relay/channel/xai/dto.go (1)
16-31:⚠️ Potential issue | 🔴 CriticalRemove unsupported
SeedandSeedsfields from the ImageRequest DTO.The xAI Grok image generation API (endpoint
/v1/images/generations) does not supportseedorseedsparameters in its official specification. The official request parameters are:aspect_ratio,model,n,prompt,quality,resolution,response_format, anduseronly. Including fields that the upstream API doesn't accept will cause requests to fail or be silently ignored by the relay. Delete bothSeed *float64andSeeds []intfrom the struct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/xai/dto.go` around lines 16 - 31, The ImageRequest DTO currently contains unsupported fields Seed and Seeds which the xAI Grok image generation API does not accept; remove the Seed *float64 and Seeds []int fields from the ImageRequest struct (type ImageRequest) so the struct only includes supported parameters (e.g., Model, Prompt, N, Image, Size, AspectRatio, OutputResolution, ResponseFormat) and ensure any code that marshals or references ImageRequest.Seed/Seeds is updated or removed to avoid compile errors.web/src/helpers/utils.jsx (1)
155-189:⚠️ Potential issue | 🟠 Major
backendMessageis computed but never used — backend error strings are still dropped.
extractBackendErrorMessage(error)is assigned tobackendMessageat line 155, but nothing insideshowErrorsubsequently reads it. The genericdefaultbranch and the final fallback still only useerror.message/error, so structured backend payloads like{ error: { message } }or{ message }are silently discarded — defeating the purpose of the new helper. Consider preferringbackendMessagewhere it's available (at least in the default branch and the non-Axios path).🔧 Proposed wiring
- default: - Toast.error('错误：' + error.message); + default: + Toast.error('错误：' + (backendMessage || error.message)); } return; } - Toast.error('错误：' + error.message); + Toast.error('错误：' + (backendMessage || error.message)); } else { - Toast.error('错误：' + error); + Toast.error('错误：' + (backendMessage || error)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/helpers/utils.jsx` around lines 155 - 189, The showError function computes backendMessage via extractBackendErrorMessage(error) but never uses it; update showError to prefer backendMessage when present (use backendMessage instead of error.message) in the Axios default case, in the non-Axios path (the final Toast.error fallback), and any other branches that currently call Toast.error('错误：' + error.message) or Toast.error('错误：' + error) so structured backend payloads are surfaced; keep existing fallback to error.message if backendMessage is falsy.model/pricing.go (1)
346-373:⚠️ Potential issue | 🟠 MajorQuotaType classification ignores group-only fixed pricing.
pricing.GroupModelPrice/GroupModelPriceBySeconds/GroupModelPriceByResolutionare populated unconditionally at Lines 347-349, but theQuotaTypedecision at Lines 353-373 only inspects the default (non-group) mapsmodelPriceBySecondsMap[formattedModelName],modelPriceByResolutionMap[formattedModelName], andratio_setting.GetModelPrice(model, false). A media model that has been configured exclusively with group-scoped fixed pricing (the very feature this PR is introducing — "Add group fixed pricing for media models") but no default fixed/seconds/resolution price will fall through to theelsebranch and be reported to the frontend asQuotaType=0(ratio-based), withModelRatio/CompletionRatiopopulated instead of the expected fixed-pricing badge.Please also consult the group maps when deciding
QuotaType, e.g.:♻️ Proposed classification fix
- if hasSecondsPrice && len(secondsPriceMap) > 0 { + if hasSecondsPrice && len(secondsPriceMap) > 0 { pricing.ModelPriceBySeconds = make(map[string]float64, len(secondsPriceMap)) for seconds, price := range secondsPriceMap { pricing.ModelPriceBySeconds[seconds] = price } pricing.QuotaType = 2 - } else if hasResolutionPrice && len(resolutionPriceMap) > 0 { + } else if len(pricing.GroupModelPriceBySeconds) > 0 { + pricing.QuotaType = 2 + } else if hasResolutionPrice && len(resolutionPriceMap) > 0 { pricing.ModelPriceByResolution = make(map[string]float64, len(resolutionPriceMap)) for resolution, price := range resolutionPriceMap { pricing.ModelPriceByResolution[resolution] = price } pricing.QuotaType = 3 - } else if findPrice { + } else if len(pricing.GroupModelPriceByResolution) > 0 { + pricing.QuotaType = 3 + } else if findPrice || len(pricing.GroupModelPrice) > 0 { pricing.ModelPrice = modelPrice pricing.QuotaType = 1 } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/pricing.go` around lines 346 - 373, The QuotaType logic ignores group-level fixed pricing: when GroupModelPrice / GroupModelPriceBySeconds / GroupModelPriceByResolution contain entries for formattedModelName, the code should treat the model as fixed-priced instead of falling back to ratio-based. Update the decision block (the if/else that inspects modelPriceBySecondsMap, modelPriceByResolutionMap, and ratio_setting.GetModelPrice) to also check the group maps (groupModelPriceMap[formattedModelName], groupModelPriceBySecondsMap[formattedModelName], groupModelPriceByResolutionMap[formattedModelName]) and, when those group maps exist and are non-empty, populate the corresponding pricing.ModelPrice / ModelPriceBySeconds / ModelPriceByResolution values from the group maps and set pricing.QuotaType to the appropriate value (2 for seconds, 3 for resolution, 1 for fixed price) before falling back to ratio_setting.GetModelRatio/GetCompletionRatio and QuotaType=0.controller/playground.go (1)
621-698:⚠️ Potential issue | 🟡 Minor
playgroundBodyCaptureWriterbuffers the full response in memory for every non-stream playground request.Wrapping
c.Writerwith abytes.Bufferduplicate (Lines 51-68) is fine for small JSON responses, but it fires for everyPlayground,PlaygroundImageGenerations, andPlaygroundImageEditscall. Image/edit responses with inlineb64_jsonpayloads commonly run into several MiB per item, and the buffer is held until the handler returns. Consider: (a) capping the capture buffer (e.g., stop appending past N MiB, still stream-through via the inner writer) and treating over-cap responses as "no media URL extracted", or (b) only attaching the capture writer when the action is image/media and the request was known-non-stream, skipping for plain chat completions where media extraction will return nothing anyway. As-is, a burst of concurrent playground requests with base64 image responses could pressure the process RSS materially.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/playground.go` around lines 621 - 698, The playgroundBodyCaptureWriter currently buffers entire responses (used in Playground, PlaygroundImageGenerations, PlaygroundImageEdits) which can OOM on large b64_json image payloads; either (A) add a capped buffer behavior in playgroundBodyCaptureWriter (introduce a constant like maxPlaygroundCaptureBytes) so Write() continues to write-through to the embedded ResponseWriter but stops appending to the internal buffer after the cap and sets a flag indicating "truncated", and ensure Status()/body accessors handle truncated payloads, or (B) only wrap c.Writer with playgroundBodyCaptureWriter for requests that are non-stream and have an image/media action (use inferPlaygroundChatRequestAction or the action passed into createPendingPlaygroundMediaTask to detect image/edit flows) and skip wrapping for plain chat completions; update call sites (Playground, PlaygroundImageGenerations, PlaygroundImageEdits) to apply one of these fixes and ensure recordPlayground* / updatePlaygroundMediaTask treat truncated/no-buffer as "no media URL extracted."relay/relay_task.go (1)
621-624:⚠️ Potential issue | 🟠 MajorUse the task-scoped key for realtime follow-up fetches.
Realtime fetch currently uses
channelModel.Key, which can be different from the key that created the async task. Prefer the persisted per-task key when present.Proposed fix
+ fetchKey := channelModel.Key + if task.PrivateData.Key != "" { + fetchKey = task.PrivateData.Key + } resp, err := adaptor.FetchTask(baseURL, channelModel.Key, map[string]any{ "task_id": task.GetUpstreamTaskID(), "action": task.Action, }, proxy)- resp, err := adaptor.FetchTask(baseURL, channelModel.Key, map[string]any{ + resp, err := adaptor.FetchTask(baseURL, fetchKey, map[string]any{Based on learnings, async video follow-up requests should prefer
task.PrivateData.Keyoverchannel.Key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 621 - 624, The realtime fetch is using channelModel.Key but should prefer the per-task persisted key; update the adaptor.FetchTask call in relay_task.go (the caller that currently passes channelModel.Key) to pass task.PrivateData.Key when present (falling back to channelModel.Key) so follow-up fetches use the task-scoped key (keep the same task.GetUpstreamTaskID() payload and proxy argument).
🧹 Nitpick comments (25)
docs/linksky-api-usage.md (2)
509-523: Use environment variable for API key in Python example.The Python SDK example uses a hardcoded string placeholder
"YOUR_LINKSKY_API_KEY"(line 513), which is inconsistent with the Node.js example (line 495) and curl examples that use environment variables. For security best practices and consistency, the Python example should also demonstrate environment variable usage.🔐 Suggested security improvement
+import os from openai import OpenAI client = OpenAI( - api_key="YOUR_LINKSKY_API_KEY", + api_key=os.environ.get("LINKSKY_API_KEY"), base_url="https://linksky.top/v1", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/linksky-api-usage.md` around lines 509 - 523, The Python example currently hardcodes the API key when constructing OpenAI(...) which is insecure; change the OpenAI client instantiation (the OpenAI(...) call that assigns to client) to read the API key from an environment variable (e.g., os.environ["LINKSKY_API_KEY"]) instead of the literal "YOUR_LINKSKY_API_KEY" so it matches the Node.js and curl examples and follows best practices for secrets management.
158-186: Consider clarifying the duplicate parameter structure.The example shows both top-level parameters and nested
extra_body.google.image_configwith overlapping fields:
aspect_ratioappears at both line 174 and line 180output_resolution(line 175) vs.image_size(line 181)While this may be intentional for API compatibility, users might be confused about which parameters take precedence or whether both are required. Consider adding a brief explanation of why both structures are included.
📝 Suggested clarification
Add a note before or after the example explaining:
说明: 当前项目同时支持顶层参数和 `extra_body.google.image_config` 嵌套结构。 为确保兼容性，建议同时传递这两个结构，其中 `output_resolution` 对应 `image_size`。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/linksky-api-usage.md` around lines 158 - 186, The example duplicates image params at the top level ("aspect_ratio", "output_resolution") and inside extra_body.google.image_config ("aspect_ratio", "image_size"), which may confuse users; update the docs by adding a short clarifying note near the curl example stating that the API supports both top-level parameters and the nested extra_body.google.image_config for compatibility, that users should send both when targeting Google-specific features, and that output_resolution maps to image_size (i.e., output_resolution ⇄ image_size), referencing the keys "aspect_ratio", "output_resolution", "extra_body.google.image_config", and "image_size" so readers can locate the fields in the example.replace.js (1)
1-16: Remove the one-off source-mutation script after applying the color migration.This hard-coded script rewrites
web/src/pages/CreativeCenter/index.jsxin place and is not documented or wired into tooling. Keeping it at the repo root makes accidental future rewrites more likely.Suggested cleanup
-const fs = require('fs'); -let content = fs.readFileSync('web/src/pages/CreativeCenter/index.jsx', 'utf8'); - -content = content.replace(/indigo-600/g, 'blue-600'); -content = content.replace(/indigo-500/g, 'blue-500'); -content = content.replace(/indigo-400/g, 'blue-400'); -content = content.replace(/indigo-300/g, 'blue-300'); -content = content.replace(/indigo-200/g, 'blue-200'); -content = content.replace(/indigo-100/g, 'blue-100'); -content = content.replace(/indigo-50/g, 'blue-50'); -content = content.replace(/purple-600/g, 'sky-500'); -content = content.replace(/purple-500/g, 'sky-400'); -content = content.replace(/99,102,241/g, '59,130,246'); // indigo-500 to blue-500 - -fs.writeFileSync('web/src/pages/CreativeCenter/index.jsx', content, 'utf8'); -console.log('Done replacing colors.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@replace.js` around lines 1 - 16, Delete the one-off root script that mutates source in place (the file using fs.readFileSync/fs.writeFileSync and the content/console.log flow) to prevent accidental future rewrites; if you want to keep it for reproducibility instead, move it into a documented repo scripts/migrations area, add a clear README entry and a safe flag or dry-run option rather than overwriting files in-place, and ensure any retained script is not executable by default.web/src/components/layout/SiderBar.jsx (1)
43-113: Minor: inconsistent path betweenrouterMap.assetand nav itemto.
routerMap.assetis/console/assetsbut the nav item'stois/assets(Line 112). The Link actually usesrouterMapState[itemKey](Line 420), so navigation works, and selection matching also usesrouterMapState, so this is consistent with the existing convention used by sibling items (e.g.,token,log). No functional issue, but theto: '/assets'field is effectively unused — consider aligning it with/console/assetsfor clarity, or remove it if it's truly redundant across all items.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/layout/SiderBar.jsx` around lines 43 - 113, The workspaceItems entry for "资产库" in the SiderBar component has a mismatched to value ('to: \'/assets\'') versus routerMap.asset ('/console/assets'); update the workspaceItems item (in the SiderBar function where workspaceItems is defined) to use the same canonical path (change to '/console/assets') or remove the unused to property entirely so it matches routerMap.asset/routerMapState lookups (refer to routerMap.asset, routerMapState, and the workspaceItems itemKey 'asset').relay/constant/relay_mode_test.go (1)
5-22: LGTM.Good table-driven coverage for the new playground and async image route mappings. Consider also adding cases for
/pg/video/generationsand/pg/video/async-generationsifPath2RelayModeis expected to resolve those asRelayModeVideoSubmit(or whichever constant applies), to match the router additions inrouter/relay-router.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/constant/relay_mode_test.go` around lines 5 - 22, Add test cases in TestPath2RelayModeSupportsPlaygroundImageRoutes to cover the new video route mappings: include entries for "/pg/video/generations" and "/pg/video/async-generations" and assert they map to RelayModeVideoSubmit by calling Path2RelayMode; update the tests slice (in relay/constant/relay_mode_test.go) to include these two paths with want set to RelayModeVideoSubmit so the unit test reflects the router changes in router/relay-router.go.common/endpoint_type.go (1)
40-47: Possible duplicateEndpointTypeOpenAIVideofor Sora channel.For
ChannelTypeSorathe switch setsendpointTypes = [OpenAIVideo], and if the same request also satisfiesIsOpenAIVideoModel(modelName)the prepend at Line 41 produces[OpenAIVideo, OpenAIVideo]. Depending on downstream consumers iterating endpoint types, this can lead to redundant routing/fallback attempts. A cheap guard is to only prepend when the type isn't already first:♻️ Suggested tweak
- if IsOpenAIVideoModel(modelName) { - endpointTypes = append([]constant.EndpointType{constant.EndpointTypeOpenAIVideo}, endpointTypes...) - } else if IsImageEditModel(modelName) { - endpointTypes = append([]constant.EndpointType{constant.EndpointTypeImageEdit}, endpointTypes...) - } else if IsImageGenerationModel(modelName) { - // add to first - endpointTypes = append([]constant.EndpointType{constant.EndpointTypeImageGeneration}, endpointTypes...) - } + var prepend constant.EndpointType + switch { + case IsOpenAIVideoModel(modelName): + prepend = constant.EndpointTypeOpenAIVideo + case IsImageEditModel(modelName): + prepend = constant.EndpointTypeImageEdit + case IsImageGenerationModel(modelName): + prepend = constant.EndpointTypeImageGeneration + } + if prepend != "" && (len(endpointTypes) == 0 || endpointTypes[0] != prepend) { + endpointTypes = append([]constant.EndpointType{prepend}, endpointTypes...) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/endpoint_type.go` around lines 40 - 47, The code may prepend constant.EndpointTypeOpenAIVideo into endpointTypes even when ChannelTypeSora already set it, causing duplicate entries; update the prepending logic in the block that checks IsOpenAIVideoModel(modelName) to only prepend constant.EndpointTypeOpenAIVideo if endpointTypes is empty or endpointTypes[0] != constant.EndpointTypeOpenAIVideo (i.e., guard the append with a check against the first element) so endpointTypes does not become [OpenAIVideo, OpenAIVideo]; reference the IsOpenAIVideoModel function and the endpointTypes slice when making this change.service/task_billing_test.go (1)
728-745: Strengthen assertions to actually prove “no adjustment occurred.”Both tests only assert
NoError, but their names (SkipsUnlimitedToken…) imply a behavioral guarantee: that no user/token quota mutation and no billing log is written whenTokenUnlimitedis true. Without side-effect assertions, the tests would still pass if someone accidentally removed the skip branch (as long as the code path happened not to error out for a zero-valuedWalletFunding). Consider asserting on user quota and log count, mirroring the pattern used in other tests in this file (e.g.TestRecalculate_ZeroDelta).♻️ Suggested hardening
func TestPreConsumeTokenQuota_SkipsUnlimitedToken(t *testing.T) { truncate(t) + const userID = 39 + seedUser(t, userID, 10000) relayInfo := &relaycommon.RelayInfo{ + UserId: userID, TokenId: 0, TokenKey: "playground_1_playground-video", TokenUnlimited: true, } require.NoError(t, PreConsumeTokenQuota(relayInfo, 1000)) + assert.Equal(t, 10000, getUserQuota(t, userID)) + assert.Equal(t, int64(0), countLogs(t)) } func TestBillingSessionSettle_SkipsUnlimitedTokenAdjustment(t *testing.T) { truncate(t) const userID = 40 seedUser(t, userID, 10000) session := &BillingSession{ relayInfo: &relaycommon.RelayInfo{ UserId: userID, TokenId: 0, TokenKey: "playground_1_playground-video", TokenUnlimited: true, }, funding: &WalletFunding{userId: userID}, } require.NoError(t, session.Settle(1000)) + assert.Equal(t, 10000, getUserQuota(t, userID)) + assert.Equal(t, int64(0), countLogs(t)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_billing_test.go` around lines 728 - 745, The test TestBillingSessionSettle_SkipsUnlimitedTokenAdjustment currently only asserts no error; update it to verify the skip behavior by checking that calling BillingSession.Settle when BillingSession.relayInfo.TokenUnlimited == true does not change the user's quota and does not write any billing logs: seed the user (seedUser), capture the current quota (e.g., via whatever read function or repository used elsewhere in tests), call session.Settle(1000), then assert the quota equals the pre-call value and that billing log count (the logs store used elsewhere in tests) did not increase; mirror the pattern from TestRecalculate_ZeroDelta for assertions and use the same WalletFunding/funding and log-check utilities used in other tests.service/billing_session.go (1)
95-113: CaptureTokenUnlimitedinto a local before the goroutine for consistency.The
Refundclosure snapshotstokenId,tokenKey,isPlayground, etc. into locals (clearly intentional to avoid readings.relayInfooff-thread), but the newly addeds.relayInfo.TokenUnlimitedcheck on line 108 is read from the shared struct inside the goroutine. For consistency — and to guard against any future mutation ofrelayInfoafterRefundreturns — copy it alongsideisPlayground.Proposed tweak
isPlayground := s.relayInfo.IsPlayground + tokenUnlimited := s.relayInfo.TokenUnlimited tokenConsumed := s.tokenConsumed funding := s.funding @@ - if tokenConsumed > 0 && !isPlayground && !s.relayInfo.TokenUnlimited { + if tokenConsumed > 0 && !isPlayground && !tokenUnlimited {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/billing_session.go` around lines 95 - 113, The goroutine captures some relayInfo fields but reads s.relayInfo.TokenUnlimited directly inside the closure; create a local variable (e.g., tokenUnlimited := s.relayInfo.TokenUnlimited) alongside tokenId, tokenKey, isPlayground, tokenConsumed, funding before the gopool.Go call and use that local (tokenUnlimited) inside the closure when checking tokenUnlimited in the model.IncreaseTokenQuota branch to avoid accessing shared state from the goroutine.service/text_quota.go (1)
251-282: Optional: hoist duplicatedOtherRatiosfiltering out of both branches.The filter + write-back + multiply block is identical in both the
!UsePriceandUsePricepaths. Consider computing and persisting the filtered ratios once before theif/elseand sharing the multiplication logic to reduce drift risk.♻️ Sketch
+ otherRatios := common.FilterOtherRatiosForBillingModel(relayInfo.OriginModelName, relayInfo.PriceData.OtherRatios) + relayInfo.PriceData.OtherRatios = otherRatios + if !relayInfo.PriceData.UsePrice { ... - otherRatios := common.FilterOtherRatiosForBillingModel(...) - relayInfo.PriceData.OtherRatios = otherRatios for _, otherRatio := range otherRatios { quotaCalculateDecimal = quotaCalculateDecimal.Mul(decimal.NewFromFloat(otherRatio)) } ... } else { ... - otherRatios := common.FilterOtherRatiosForBillingModel(...) - relayInfo.PriceData.OtherRatios = otherRatios for _, otherRatio := range otherRatios { quotaCalculateDecimal = quotaCalculateDecimal.Mul(decimal.NewFromFloat(otherRatio)) } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/text_quota.go` around lines 251 - 282, Hoist the duplicated filtering/multiplication: call common.FilterOtherRatiosForBillingModel(relayInfo.OriginModelName, relayInfo.PriceData.OtherRatios) once before the if/else, assign back to relayInfo.PriceData.OtherRatios, and then apply the loop that multiplies quotaCalculateDecimal by each decimal.NewFromFloat(otherRatio) in a shared place after both branches compute the base quotaCalculateDecimal; ensure both branches stop performing the same filtering/multiplication so summary.Quota is set from the final quotaCalculateDecimal.Round(0).IntPart() in the same shared location.setting/ratio_setting/model_price_by_resolution_test.go (1)
9-34: LGTM — nice case-insensitivity coverage; consider adding one missing-resolution negative case.The three case permutations (
"1k"/"1K","2K"/"2k","4k"/"4K") are a good regression guard. For parity withTestGetModelPriceBySeconds, consider appending a quick_, ok := GetModelPriceByResolution("nano-banana", "8k"); require.False(t, ok)to lock in negative behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/ratio_setting/model_price_by_resolution_test.go` around lines 9 - 34, Add a negative-resolution assertion to TestGetModelPriceByResolution: after the existing positive checks call GetModelPriceByResolution("nano-banana", "8k") (or another non-existent resolution) and assert ok is false (e.g., _, ok := GetModelPriceByResolution(...); require.False(t, ok)). Keep the existing UpdateModelPriceByResolutionByJSONString setup and defer restore; only append this failing-resolution check to the end of TestGetModelPriceByResolution to mirror TestGetModelPriceBySeconds.service/task_polling_transient_test.go (1)
10-96: Guard against future parallel tests mutatingTaskNotFoundGraceMinutes.Both tests toggle the package-global
constant.TaskNotFoundGraceMinutes. This is fine today because the tests run serially, but if any test inservice/later callst.Parallel()the global will race. Two cheap protections:
- Call
t.Setenv-style helpers or wrap the mutation in a small helper that panics if called concurrently, or- Factor
isTransientVideoNotFoundResponseto take the grace minutes as a parameter so tests don't touch globals at all.Not a blocker for this PR, just a hardening step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling_transient_test.go` around lines 10 - 96, The tests mutate the package-global constant.TaskNotFoundGraceMinutes which can race if tests run in parallel; refactor isTransientVideoNotFoundResponse to accept a graceMinutes parameter (e.g., isTransientVideoNotFoundResponse(statusCode int, body []byte, submitTime, now, graceMinutes int64) or similar), update all call sites to pass constant.TaskNotFoundGraceMinutes in production code, and update the two tests to stop modifying the global by passing the desired grace (10 or 0) directly to the function; ensure function and tests compile and remove any defer-based restoration of the global.router/api-router.go (1)
346-352: Protect the/asset/*/downloadendpoints against abuse.
POST /asset/self/downloadandPOST /asset/download(presumably ZIP-archive users' media assets) are authenticated but have no rate-limit middleware. If the handlers stream large archives assembled from many assets, concurrent requests can exhaust disk/IO and memory. Consider attachingmiddleware.CriticalRateLimit()(or a dedicated download limiter) and an archive-size cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/api-router.go` around lines 346 - 352, The two download endpoints under the assetRoute (handlers controller.DownloadUserCreativeCenterAssets and controller.DownloadAllCreativeCenterAssets) lack rate-limiting and can be abused to exhaust IO/memory; add middleware.CriticalRateLimit() (or a dedicated download limiter) to the POST routes for "/self/download" and "/download" and also enforce an archive-size cap inside the handlers (e.g., limit number of asset entries, total bytes, or deny oversized archive requests in DownloadUserCreativeCenterAssets and DownloadAllCreativeCenterAssets) so concurrent archive generation cannot overwhelm disk/IO.web/src/hooks/common/useHeaderBar.js (1)
92-114: Nit:actualThemeis now a constant, so the memoization/effect wiring is dead weight.Since
actualThemeis hardcoded to'light', theuseEffectdependency[actualTheme]will never re-fire after mount — which is fine, but the variable and dep can be removed for clarity. If there's any plan to bring theming back, leaving a// TODOcomment pointing to the intentional removal would help future readers (and match the companion change incontext/Theme/index.jsx).♻️ Optional cleanup
- const actualTheme = 'light'; - // Logo loading effect @@ // Send theme to iframe useEffect(() => { try { const iframe = document.querySelector('iframe'); const cw = iframe && iframe.contentWindow; if (cw) { - cw.postMessage({ themeMode: actualTheme }, '*'); + cw.postMessage({ themeMode: 'light' }, '*'); } } catch (e) { // Silently ignore cross-origin or access errors } - }, [actualTheme]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/common/useHeaderBar.js` around lines 92 - 114, actualTheme is a hardcoded constant so the second useEffect's dependency array is unnecessary and the constant is dead weight; remove the actualTheme variable, inline the static 'light' value in the iframe postMessage call inside the useEffect, and change the effect dependency array to [] (or add a brief // TODO noting theming is intentionally removed if you want to keep a reminder). Update the reference to actualTheme in the useEffect to use the string 'light' so there are no unused variables.relay/channel/xai/adaptor_test.go (1)
119-151: Minor: misleading model name in XAI adaptor test.
TestConvertImageRequestPreservesAspectRatioAndOutputResolutionpassesModel: "nano-banana-pro"through the XAI adaptor to verifyAspectRatio/OutputResolution/Seed/Seedsare preserved. Sincenano-banana-prois not an XAI model, a future reader may reasonably think the XAI adaptor is meant to handle it. Swapping in a real Grok image model (or a clearly-fake name likexai-test-model) keeps the assertion intent (field pass-through) while avoiding the cross-provider confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/xai/adaptor_test.go` around lines 119 - 151, In TestConvertImageRequestPreservesAspectRatioAndOutputResolution change the DTO's Model value from the non-XAI name "nano-banana-pro" to an XAI-appropriate identifier (e.g., a real Grok image model string or a clearly-fake XAI name like "xai-test-model") so the test's intent (verifying field pass-through in adaptor.ConvertImageRequest for dto.ImageRequest) isn't confusing about provider ownership; update the dto.ImageRequest.Model field in the test accordingly.relay/channel/openai/adaptor.go (1)
439-482: When multipart parsing fails, we now silently fall back to JSON — worth logging.The new condition only parses the multipart form when
Content-Typecontainsmultipart/form-data. If a client does send a multipart body butc.MultipartForm()fails for some reason (e.g. truncated upload), the old code returned"failed to parse multipart form"; with the new structure that error path still exists, but for non-multipart edit requests we silently dropmaskand any non-whitelisted form fields that a richer future request might carry. The whitelisted fallback currently coversprompt/n/size/quality/response_formatonly. Fields likebackground,model_version,user,image_urlson the DTO won't propagate.Suggest either:
- Explicitly iterating a broader set (including anything new on
dto.ImageRequestyou want to support), or- Marshaling
requestto a struct tagged for form fields and writing whatever is non-empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openai/adaptor.go` around lines 439 - 482, ConvertImageRequest currently swallows multipart form fields when c.MultipartForm() fails or when Content-Type detection skips parsing, losing fields like mask/background/model_version/user/image_urls; update ConvertImageRequest to log the multipart parse error (when c.MultipartForm() returns an error) and ensure all DTO fields propagate by either iterating a broader set of fields from dto.ImageRequest or marshaling dto.ImageRequest into form tags and writing every non-empty field to the multipart writer; specifically modify the code around c.MultipartForm()/mf handling in ConvertImageRequest to (1) call processLogger or a logger with the parsing error when c.MultipartForm() fails, and (2) add logic that writes mask, background, model_version, user, image_urls, and any other dto.ImageRequest fields to writer when mf is nil so non-multipart fallback preserves those values.web/src/components/table/task-logs/modals/ContentModal.jsx (1)
84-95: Download via<a download>can fail for cross-origin HTTP(S) URLs.When
imagePreviewUrlpoints to a non–same-origin HTTP(S) resource (e.g., when the proxy is disabled or returns a redirect to a third-party CDN), thedownloadattribute is ignored by browsers and the link will just navigate/open the image instead of saving it. Also, the filenametask-preview-imageis extension-less, so saved files lose their mime hint.Consider fetching the URL as a blob and using
URL.createObjectURL, and deriving an extension from the URL or content-type. Not blocking—just worth hardening if downloads are a common flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/task-logs/modals/ContentModal.jsx` around lines 84 - 95, The current handleDownloadImage uses an <a download> with imagePreviewUrl which fails for cross-origin HTTP(S) URLs and produces extension-less filenames; change handleDownloadImage to fetch the image URL as a blob (use fetch with response.blob()), derive a file extension from the response.headers.get('content-type') or fallback to parsing the imagePreviewUrl path, create an object URL via URL.createObjectURL(blob), set link.href to that object URL and link.download to a name including the extension (e.g., task-preview-image.<ext>), call link.click(), then revoke the object URL with URL.revokeObjectURL and handle fetch errors with a fallback to the existing link-based behavior.dto/openai_image.go (1)
161-169:imagePriceRatioforced to 1 for resolution-only models — confirm the end-to-end billing path.Setting
ImagePriceRatio = 1here means the DALL·E-stylesizeRatio * qualityRatio * ncontribution is neutralized for resolution-only models (e.g.nano-banana-pro). That's only correct if the resolution-based price is folded in later byFilterOtherRatiosForBillingModel/PriceData.ModelPrice(which this PR does by emptyingOtherRatiosand driving cost from the fixed resolution price). Worth an inline comment cross-referencingcommon.IsResolutionOnlyBillingModeland the resolution price lookup inrelay/helper/price.goso this isn't read as "free" by future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dto/openai_image.go` around lines 161 - 169, The imagePriceRatio is being forced to 1 when common.IsResolutionOnlyBillingModel(i.Model) returns true which can look like the image cost is being zeroed out; add an inline comment next to the imagePriceRatio assignment explaining that for resolution-only models we neutralize size/quality/n here because the final resolution-based price is applied later (see FilterOtherRatiosForBillingModel and PriceData.ModelPrice) and reference the resolution price lookup in relay/helper/price.go so future readers understand the end-to-end billing path.service/task_polling.go (2)
73-96: Transient-404 classifier: explicit trim set + fallback semantics look correct.One note: when
submitTime <= 0 || now <= 0the function returnstrue(treat as transient and keep polling). Without a valid timestamp this could loop indefinitely for a given upstream response, butsweepTimedOutTasks/TaskTimeoutMinuteswill still terminate the task eventually, so this is an acceptable tradeoff. Worth a short comment on the fallback so future readers don't re-question it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` around lines 73 - 96, Add a short clarifying comment inside isTransientVideoNotFoundResponse (near the submitTime/now check) explaining that when submitTime <= 0 || now <= 0 the function conservatively returns true to treat the 404 as transient and continue polling, and that this fallback may allow longer-lived polling but is bounded by the global sweepTimedOutTasks / TaskTimeoutMinutes mechanism which will eventually terminate the task; reference isTransientVideoNotFoundResponse, constant.TaskNotFoundGraceMinutes, and the sweepTimedOutTasks/TaskTimeoutMinutes timeout behavior in the comment so future readers understand the tradeoff.
582-592: Consume-log duration update: guard against clock-skewFinishTime.Logic is sound (prefers
SubmitTime, falls back toStartTime, only updates whenFinishTime > startAt). One edge case: if upstream reports aFinishTimethat's wildly in the future relative tostartAt(or ifstartAtis a legacy sentinel seconds-vs-ms mismatch), the computeduse_timecould be stored as a bogus large integer. Not a bug today, but a small sanity cap (e.g., ignore if delta exceeds a reasonable upper bound) would be a cheap defense.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` around lines 582 - 592, The consume-log update may record a bogus large use_time if FinishTime is far in the future or units are mismatched; compute delta := task.FinishTime - startAt and add a sanity cap (e.g., maxUseTime := 24*3600 seconds) before calling model.UpdateConsumeLogUseTimeByRequestId: only call the update when delta > 0 && delta <= maxUseTime, otherwise skip and logger.LogWarn with context (task.TaskID, RequestId, delta) so bad upstream values are ignored; keep symbols to change: task.SubmitTime, task.StartTime, task.FinishTime, task.PrivateData.RequestId and the call to model.UpdateConsumeLogUseTimeByRequestId.model/task_stats_test.go (1)
110-125: Brittle expected counts (7,2,5) forgetTaskActionsForMediaType.These are tightly coupled to the current action set and will break on every legitimate addition (e.g., a new Grok image variant added elsewhere in this PR). Consider asserting on set membership of expected actions instead of raw counts, so the test documents intent and survives additive changes:
Proposed tightening
- allActions := getTaskActionsForMediaType(TaskMediaTypeAll) - if len(allActions) != 7 { - t.Fatalf("expected 7 actions for all media type, got %d", len(allActions)) - } + allActions := getTaskActionsForMediaType(TaskMediaTypeAll) + for _, want := range []string{"imageGenerate", "imageEdit", "generate", "textGenerate", "remixGenerate" /* ... */} { + if !slices.Contains(allActions, want) { + t.Fatalf("expected action %q in all-media actions, got %v", want, allActions) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/task_stats_test.go` around lines 110 - 125, The test TestGetTaskActionsForMediaType is brittle because it asserts exact counts; change it to assert that the expected action IDs/names are present for each media type rather than exact lengths: call getTaskActionsForMediaType(TaskMediaTypeAll|Image|Video) and for each result ensure the known required actions (e.g., the canonical image and video action identifiers your code expects) are contained in the returned slice (use a helper contains/map lookup to check membership), and only optionally assert that the returned set is non-empty instead of hardcoded numeric expectations; update references in the test to use contains checks against getTaskActionsForMediaType.relay/helper/price_test.go (1)
17-178: LGTM — good coverage of the new override semantics.Tests cleanly assert the intended behavior: when a user-group override is present,
GroupPriceOverride=true,GroupPriceOverrideGroup="vip", the override price is used, andGroupRatioInfo.GroupRatiostays at theUsingGroupratio (1.0) rather than being multiplied in — matching the "Prefer user group for media fixed pricing" commit.Minor: the tests mutate package-level globals (
common.QuotaPerUnit,ratio_settingJSON). Since none of them callt.Parallel()this is safe today, but a single mis-addition oft.Parallel()in this package will silently break state isolation. Consider wrapping global mutations in a smallwithPricingConfig(t, fn)helper (or usingt.Cleanup) so future parallelization doesn't silently race.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/helper/price_test.go` around lines 17 - 178, Tests mutate package-level globals (common.QuotaPerUnit and ratio_setting JSON settings) which can race if tests are parallelized; wrap each test's global mutations in a helper such as withPricingConfig (or use t.Cleanup) that captures originals (e.g., original := ratio_setting.ModelPriceBySeconds2JSONString(), originalQuotaPerUnit := common.QuotaPerUnit), applies the temporary updates via ratio_setting.Update... and setting common.QuotaPerUnit, and guarantees restoration by registering cleanup to call the Update... back to originals and reset common.QuotaPerUnit; update the tests (e.g., TestModelPriceHelperUsesSecondsPriceForChatCompatibleVideo, TestModelPriceHelperUsesGroupResolutionPriceWithoutGroupRatio, TestModelPriceHelperUsesGroupPerCallPriceWithoutGroupRatio, TestModelPriceHelperFallsBackToSecondsMinPrice) to use that helper so state isolation is enforced even if t.Parallel() is added later.common/billing_model.go (1)
28-50: Minor: allocation in resolution-only branch is wasted.
filtered := make(map[string]float64, len(ratios))at line 33 is allocated unconditionally, but theIsResolutionOnlyBillingModelbranch returns its own (empty)filteredat line 40 via early return of a freshly referenced map — fine, but you could simplify by short-circuiting before the allocation, or by returningmap[string]float64{}explicitly to make intent obvious.Proposed tidy-up
func FilterOtherRatiosForBillingModel(modelName string, ratios map[string]float64) map[string]float64 { if len(ratios) == 0 { return map[string]float64{} } + if IsResolutionOnlyBillingModel(modelName) { + return map[string]float64{} + } filtered := make(map[string]float64, len(ratios)) switch { case IsDurationOnlyBillingModel(modelName): if ratio, ok := ratios["seconds"]; ok && ratio > 0 { filtered["seconds"] = ratio } - case IsResolutionOnlyBillingModel(modelName): - return filtered default: for key, ratio := range ratios { if ratio > 0 { filtered[key] = ratio } } } return filtered }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/billing_model.go` around lines 28 - 50, The allocation of filtered in FilterOtherRatiosForBillingModel is done before checking IsResolutionOnlyBillingModel, wasting an allocation for the resolution-only branch; move the IsResolutionOnlyBillingModel(modelName) check before creating filtered (or return map[string]float64{} immediately in that branch) and only call filtered := make(map[string]float64, len(ratios)) when needed (e.g., after handling IsResolutionOnlyBillingModel and IsDurationOnlyBillingModel branches) so the empty map allocation is avoided.controller/asset.go (1)
146-172:sliceCreativeCenterAssetscopies every element for the "hide username" case — small but avoidable.You allocate a fresh
CreativeCenterAssetper item just to clearUsername. IfincludeUsernameis true, you could return the subslice directly (same pointers), and if it's false, clear the field in a single pass withoutcopyAsset := *asset. Micro-optimization, but since this runs on every list request it's worth the pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/asset.go` around lines 146 - 172, The current sliceCreativeCenterAssets always makes a deep copy of each element to blank Username, which is unnecessary; change it to return the subslice assets[startIdx:endIdx] directly when includeUsername is true, and when includeUsername is false allocate items once (capacity endIdx-startIdx) and in a single pass append the original pointers but set asset.Username = "" on each non-nil element (instead of doing copyAsset := *asset), preserving nil checks and bounds logic using startIdx/pageSize/endIdx as already computed.web/src/hooks/task-logs/useTaskLogsData.js (1)
38-73: Hardcoded Chineselabelstrings in a reusable hook.
TASK_STATS_RANGE_PRESETSandTASK_MEDIA_TYPE_OPTIONSembed Chinese labels ('今天','全部', …) and are re-exported viataskStatsRangePresets/taskMediaTypeOptions. Consumers will either render those verbatim (bypassing i18n) or need to callt(preset.label)at the call site, which is fragile and easy to miss. Prefer returning only the i18n key (e.g.,labelKey: '今天') and document that the consumer must wrap it int(...), or compute the label inside the hook viauseTranslation. As per coding guidelines: "UseuseTranslation()hook and callt('中文key')in components."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/task-logs/useTaskLogsData.js` around lines 38 - 73, TASK_STATS_RANGE_PRESETS and TASK_MEDIA_TYPE_OPTIONS currently contain hardcoded Chinese label strings; update the hook to compute localized labels inside it by importing and using useTranslation() and replacing label fields with t(...) results (or alternatively expose labelKey instead of label) so consumers no longer receive raw Chinese text. Specifically, modify TASK_STATS_RANGE_PRESETS and TASK_MEDIA_TYPE_OPTIONS and the exported taskStatsRangePresets / taskMediaTypeOptions to call const { t } = useTranslation() and set each preset/option label to t('your.i18n.key') (or expose labelKey if you prefer the consumer to translate) and ensure all references to label in consumers use the new shape.controller/async_video.go (1)
65-75: Dead-defensive nil check after already dereferencingc.Request.
c.Request.Methodandc.Request.Header.Clone()are read unconditionally at Lines 68-69, yet Line 73 checksc.Request != nil && c.Request.URL != nil. Ifc.Requestwere actually nil the goroutine would have panicked three lines earlier. Move the guard before the first dereference or drop it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/async_video.go` around lines 65 - 75, The code defensively checks c.Request only when setting job.RawQuery but already dereferences c.Request earlier (c.Request.Method and c.Request.Header.Clone()), which is inconsistent and can panic; either move the guard (c.Request != nil && c.Request.URL != nil) to run before any use of c.Request or remove the guard entirely if c.Request is guaranteed non-nil. Update the asyncVideoJob construction so that access to c.Request.Method, c.Request.Header.Clone(), and c.Request.URL.RawQuery are performed only after verifying c.Request (and c.Request.URL where needed), referring to asyncVideoJob, c.Request.Method, c.Request.Header.Clone(), and job.RawQuery to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb2d0fcd-c926-44e0-988e-3c58499aa73c
⛔ Files ignored due to path filters (1)
web/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (144)
common/billing_model.gocommon/billing_model_test.gocommon/endpoint_defaults.gocommon/endpoint_type.gocommon/endpoint_type_test.gocommon/init.gocommon/model.goconstant/endpoint_type.goconstant/env.goconstant/task.gocontroller/asset.gocontroller/async_image.gocontroller/async_video.gocontroller/async_video_test.gocontroller/creative_center_history.gocontroller/creative_center_upload.gocontroller/playground.gocontroller/relay.gocontroller/task.godocs/linksky-api-usage.mddto/asset.godto/async_image.godto/async_video.godto/openai_image.godto/openai_image_test.godto/openai_request.godto/openai_request_zero_value_test.godto/task.godto/task_stats.gomiddleware/distributor.gomodel/creative_center_asset.gomodel/creative_center_asset_test.gomodel/creative_center_history.gomodel/log.gomodel/main.gomodel/option.gomodel/pricing.gomodel/task.gomodel/task_cas_test.gomodel/task_stats.gomodel/task_stats_test.gomodel/user.goold_index.jsxrelay/channel/openai/adaptor.gorelay/channel/openai/adaptor_test.gorelay/channel/task/gemini/adaptor.gorelay/channel/task/sora/adaptor.gorelay/channel/task/sora/adaptor_test.gorelay/channel/task/vertex/adaptor.gorelay/channel/xai/adaptor.gorelay/channel/xai/adaptor_test.gorelay/channel/xai/constants.gorelay/channel/xai/dto.gorelay/common/relay_info.gorelay/common/relay_info_test.gorelay/common/relay_utils.gorelay/constant/relay_mode.gorelay/constant/relay_mode_test.gorelay/helper/price.gorelay/helper/price_test.gorelay/relay_adaptor.gorelay/relay_task.gorelay/relay_task_test.goreplace.jsrouter/api-router.gorouter/relay-router.gorouter/video-router.goservice/billing_session.goservice/channel_affinity_usage_cache_test.goservice/creative_center_asset_archive.goservice/creative_center_asset_archive_test.goservice/log_info_generate.goservice/quota.goservice/task_billing.goservice/task_billing_test.goservice/task_polling.goservice/task_polling_transient_test.goservice/text_quota.goservice/text_quota_test.gosetting/ratio_setting/group_model_price_test.gosetting/ratio_setting/model_price_by_resolution_test.gosetting/ratio_setting/model_price_by_seconds_test.gosetting/ratio_setting/model_ratio.gosetting/system_setting/image_bed.gotypes/price_data.goweb/src/App.jsxweb/src/components/layout/NoticeModal.jsxweb/src/components/layout/PageLayout.jsxweb/src/components/layout/SiderBar.jsxweb/src/components/layout/headerbar/ActionButtons.jsxweb/src/components/layout/headerbar/index.jsxweb/src/components/playground/SettingsPanel.jsxweb/src/components/settings/OtherSetting.jsxweb/src/components/settings/RatioSetting.jsxweb/src/components/settings/SystemSetting.jsxweb/src/components/settings/personal/cards/NotificationSettings.jsxweb/src/components/table/channels/modals/ModelTestModal.jsxweb/src/components/table/model-pricing/filter/PricingDisplaySettings.jsxweb/src/components/table/model-pricing/filter/PricingQuotaTypes.jsxweb/src/components/table/model-pricing/layout/PricingSidebar.jsxweb/src/components/table/model-pricing/layout/content/PricingContent.jsxweb/src/components/table/model-pricing/layout/header/PricingTopSection.jsxweb/src/components/table/model-pricing/layout/header/PricingVendorIntro.jsxweb/src/components/table/model-pricing/layout/header/SearchActions.jsxweb/src/components/table/model-pricing/modal/PricingFilterModal.jsxweb/src/components/table/model-pricing/modal/components/FilterModalContent.jsxweb/src/components/table/model-pricing/modal/components/ModelPricingTable.jsxweb/src/components/table/model-pricing/view/card/PricingCardView.jsxweb/src/components/table/model-pricing/view/table/PricingTableColumns.jsxweb/src/components/table/models/ModelsColumnDefs.jsxweb/src/components/table/models/modals/EditModelModal.jsxweb/src/components/table/models/modals/EditPrefillGroupModal.jsxweb/src/components/table/task-logs/TaskLogsColumnDefs.jsxweb/src/components/table/task-logs/TaskLogsDashboard.jsxweb/src/components/table/task-logs/TaskLogsFilters.jsxweb/src/components/table/task-logs/TaskLogsTable.jsxweb/src/components/table/task-logs/index.jsxweb/src/components/table/task-logs/modals/ContentModal.jsxweb/src/components/table/usage-logs/UsageLogsColumnDefs.jsxweb/src/constants/common.constant.jsweb/src/constants/playground.constants.jsweb/src/context/Theme/index.jsxweb/src/helpers/api.jsweb/src/helpers/render.jsxweb/src/helpers/utils.jsxweb/src/hooks/common/useHeaderBar.jsweb/src/hooks/common/useNavigation.jsweb/src/hooks/common/useSidebar.jsweb/src/hooks/dashboard/useDashboardData.jsweb/src/hooks/model-pricing/useModelPricingData.jsxweb/src/hooks/playground/useApiRequest.jsxweb/src/hooks/task-logs/useTaskLogsData.jsweb/src/hooks/usage-logs/useUsageLogsData.jsxweb/src/pages/About/index.jsxweb/src/pages/Asset/index.jsxweb/src/pages/CreativeCenter/index.jsxweb/src/pages/Home/index.jsxweb/src/pages/Setting/Operation/SettingsHeaderNavModules.jsxweb/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsxweb/src/pages/Setting/Ratio/GroupRatioSettings.jsxweb/src/pages/Setting/Ratio/ModelRatioSettings.jsxweb/src/pages/Setting/Ratio/components/ModelPricingEditor.jsxweb/src/pages/Setting/Ratio/hooks/useModelPricingEditorState.jsweb/tailwind.config.js
💤 Files with no reviewable changes (8)
- web/src/components/table/model-pricing/modal/PricingFilterModal.jsx
- web/src/components/table/model-pricing/layout/header/PricingVendorIntro.jsx
- web/src/components/table/model-pricing/layout/content/PricingContent.jsx
- web/src/components/layout/headerbar/ActionButtons.jsx
- web/src/components/table/model-pricing/layout/PricingSidebar.jsx
- web/src/components/layout/headerbar/index.jsx
- web/src/components/table/model-pricing/layout/header/PricingTopSection.jsx
- web/src/components/table/model-pricing/modal/components/FilterModalContent.jsx
| RelayTask(ctx) | ||
|
|
||
| responseBody := recorder.Body.Bytes() | ||
| statusCode := recorder.Code | ||
| if statusCode == 0 { | ||
| statusCode = http.StatusOK | ||
| } | ||
| if statusCode < http.StatusOK || statusCode >= http.StatusMultipleChoices { | ||
| updateAsyncVideoTaskFailure(task, responseBody, extractPlaygroundTaskErrorMessage(responseBody, "async video request failed")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the sync/async video submit path persists UpstreamTaskID reliably before RelayTask returns 2xx.
rg -nP --type=go -C5 'UpstreamTaskID\s*='
rg -nP --type=go -C5 'RefreshVideoTask'Repository: QuantumNous/new-api
Length of output: 2137
🏁 Script executed:
# Get full context of async_video.go around lines 240-250
sed -n '220,260p' controller/async_video.goRepository: QuantumNous/new-api
Length of output: 1344
🏁 Script executed:
# Check what updateAsyncVideoTaskRunning does
rg -nP --type=go 'func updateAsyncVideoTaskRunning' -A 10Repository: QuantumNous/new-api
Length of output: 763
🏁 Script executed:
# Find updateAsyncImageTaskSuccess for comparison
rg -nP --type=go 'updateAsyncImageTaskSuccess' -B 5 -A 10Repository: QuantumNous/new-api
Length of output: 2055
🏁 Script executed:
# Check the RelayTask function for video context
rg -nP --type=go 'func.*RelayTask\(ctx' -A 30 | head -80Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check if there's a finalization function for async video success similar to image
rg -nP --type=go 'updateAsyncVideoTaskSuccess|updateAsyncVideoTaskCompleted|updateAsyncVideoTaskDone' -B 2 -A 15Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check the background scheduler logic to see if it gates on UpstreamTaskID
rg -nP --type=go 'RefreshVideoTask|UpstreamTaskID' controller/async_video.go service/task_polling.go -B 3 -A 3Repository: QuantumNous/new-api
Length of output: 2317
Add success handler to match async image flow.
When RelayTask completes with 2xx, the task remains in TaskStatusInProgress without finalization—unlike controller/async_image.go line 224, which calls updateAsyncImageTaskSuccess on success. The video task then depends entirely on client-side polling (RefreshVideoTask at line 100) or background scheduler refresh (which gates on PrivateData.UpstreamTaskID being non-empty at controller/async_video.go:126).
This creates an asymmetry: if the upstream provider does not return an UpstreamTaskID in the initial response, the task cannot progress via polling and will never finalize, leaving it permanently in IN_PROGRESS. Consider adding a success handler (e.g., updateAsyncVideoTaskSuccess) to capture UpstreamTaskID and finalize the task state, mirroring the image flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/async_video.go` around lines 240 - 250, The handler currently only
treats non-2xx as failures after calling RelayTask, leaving tasks in
TaskStatusInProgress for successful 2xx responses; add a success path similar to
controller/async_image.go by checking when statusCode is in the 2xx range
(http.StatusOK <= statusCode < http.StatusMultipleChoices) and calling
updateAsyncVideoTaskSuccess(task, responseBody) (or the appropriate signature)
to record the UpstreamTaskID and finalize the task state; keep the existing
updateAsyncVideoTaskFailure(task, responseBody,
extractPlaygroundTaskErrorMessage(...)) for non-2xx responses and ensure you
still default statusCode from recorder.Code the same way.
| // TaskBillingContext 记录任务提交时的计费参数，以便轮询阶段可以重新计算额度。 | ||
| type TaskBillingContext struct { | ||
| ModelPrice float64 `json:"model_price,omitempty"` // 模型单价 | ||
| GroupRatio float64 `json:"group_ratio,omitempty"` // 分组倍率 | ||
| ModelRatio float64 `json:"model_ratio,omitempty"` // 模型倍率 | ||
| OtherRatios map[string]float64 `json:"other_ratios,omitempty"` // 附加倍率（时长、分辨率等） | ||
| OriginModelName string `json:"origin_model_name,omitempty"` // 模型名称，必须为OriginModelName | ||
| PerCallBilling bool `json:"per_call_billing,omitempty"` // 按次计费：跳过轮询阶段的差额结算 | ||
| ModelPrice float64 `json:"model_price,omitempty"` // 模型单价 | ||
| GroupRatio float64 `json:"group_ratio,omitempty"` // 分组倍率 | ||
| ModelRatio float64 `json:"model_ratio,omitempty"` // 模型倍率 | ||
| OtherRatios map[string]float64 `json:"other_ratios,omitempty"` // 附加倍率（时长、分辨率等） | ||
| OriginModelName string `json:"origin_model_name,omitempty"` // 模型名称，必须为OriginModelName | ||
| PerCallBilling bool `json:"per_call_billing,omitempty"` // 按次计费：跳过轮询阶段的差额结算 | ||
| GroupPriceOverride bool `json:"group_price_override,omitempty"` | ||
| GroupPriceOverrideGroup string `json:"group_price_override_group,omitempty"` | ||
| UsingGroup string `json:"using_group,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all read-sites of the new fields to confirm graceful fallback for pre-PR tasks.
rg -nP --type=go -C3 'GroupPriceOverride\b|GroupPriceOverrideGroup\b|UsingGroup\s*:'Repository: QuantumNous/new-api
Length of output: 20146
🏁 Script executed:
rg -nP --type=go 'updateVideoSingleTask|settle|doRefund|refundTask' -A 5 | head -100Repository: QuantumNous/new-api
Length of output: 6616
🏁 Script executed:
# Find where TaskBillingContext is used in task polling/refund
rg -nP --type=go 'task\.PrivateData|TaskBillingContext' service/task_polling.go -B 2 -A 5Repository: QuantumNous/new-api
Length of output: 2237
🏁 Script executed:
# Check task_billing.go for how it uses these fields during refund
rg -nP --type=go -A 10 'func.*[Rr]efund|func.*[Ss]ettle' service/task_billing.goRepository: QuantumNous/new-api
Length of output: 409
🏁 Script executed:
rg -nP --type=go 'func RecalculateTaskQuota' -A 20Repository: QuantumNous/new-api
Length of output: 2293
🏁 Script executed:
# Look for how UsingGroup and GroupPriceOverrideGroup are actually used in billing/refund
rg -nP --type=go 'UsingGroup|GroupPriceOverrideGroup' service/task_billing.go -B 3 -A 3Repository: QuantumNous/new-api
Length of output: 699
🏁 Script executed:
# Check if there are any fallback patterns when these fields are empty
rg -nP --type=go 'task\.Group|billing.*group' service/task_billing.go -iRepository: QuantumNous/new-api
Length of output: 224
🏁 Script executed:
# Search for any code that reads BillingContext fields during billing operations
rg -nP --type=go 'BillingContext|PrivateData\..*Group' service/task_billing.go -B 2 -A 5Repository: QuantumNous/new-api
Length of output: 796
🏁 Script executed:
# Check if UsingGroup or GroupPriceOverrideGroup from BillingContext are used anywhere in actual quota calculation
rg -nP --type=go 'bc\.(UsingGroup|GroupPriceOverrideGroup|GroupPriceOverride)' service/ -B 2 -A 3Repository: QuantumNous/new-api
Length of output: 735
🏁 Script executed:
# Verify the complete flow of RecalculateTaskQuotaByTokens to see if it ever considers the stored billing context
cat -n service/task_billing.go | sed -n '264,330p'Repository: QuantumNous/new-api
Length of output: 1453
🏁 Script executed:
# Check if GroupPriceOverride from BillingContext actually affects quota calculation in refund flow
rg -nP --type=go 'BillingContext.*GroupPrice|if.*GroupPrice' service/task_billing.go -B 3 -A 8Repository: QuantumNous/new-api
Length of output: 498
🏁 Script executed:
# Verify that RecalculateTaskQuota itself doesn't use BillingContext
cat -n service/task_billing.go | sed -n '200,260p'Repository: QuantumNous/new-api
Length of output: 2103
🏁 Script executed:
# Look for any billing context usage in the actual refund path
rg -nP --type=go 'RefundTaskQuota|taskAdjustFunding' service/task_billing.go -A 15 | head -60Repository: QuantumNous/new-api
Length of output: 2055
🏁 Script executed:
# Check if RecalculateTaskQuotaByTokens respects GroupPriceOverride from BillingContext
rg -nP --type=go 'GroupPriceOverride' service/task_billing.goRepository: QuantumNous/new-api
Length of output: 300
🏁 Script executed:
# Comprehensive check: does any refund/settle code path use GroupPriceOverride from BillingContext to skip group ratio?
rg -nP --type=go 'PerCallBilling|GroupPriceOverride' service/task_billing.go -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 576
🏁 Script executed:
# Check the actual flow in task_polling.go settle path
cat -n service/task_polling.go | sed -n '636,670p'Repository: QuantumNous/new-api
Length of output: 841
🏁 Script executed:
# Check if there's any TODO or FIXME related to using BillingContext in settle
rg -nP --type=go 'TODO|FIXME' service/task_billing.goRepository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check the learnings context - verify what the intended behavior should be
# by looking at how it's supposed to work based on test expectations
cat -n service/task_billing_test.go | sed -n '425,485p'Repository: QuantumNous/new-api
Length of output: 2334
🏁 Script executed:
# Verify: does RecalculateTaskQuotaByTokens ever consult BillingContext to check GroupPriceOverride?
rg -nP --type=go 'RecalculateTaskQuotaByTokens' -A 50 service/task_billing.go | grep -i 'billingcontext\|groupprice'Repository: QuantumNous/new-api
Length of output: 45
Stored billing-override fields in TaskBillingContext are not consulted during polling settle/refund — tokens path will ignore GroupPriceOverride.
GroupPriceOverride, GroupPriceOverrideGroup, and UsingGroup are persisted into task.PrivateData.BillingContext to carry "fixed media pricing per group" decisions into async refund/settlement. However, the actual quota recalculation paths do not read these fields:
settleTaskBillingOnComplete()only checksPerCallBilling, notGroupPriceOverride.RecalculateTaskQuotaByTokens()always appliesgroupRatio(line 290–301) regardless of whetherbc.GroupPriceOverridewas true at submit time.
If a task was originally submitted with GroupPriceOverride=true (meaning fixed pricing at the group's media rate, without per-user ratio scaling), and polling later triggers the token-recalculation path, the quota will be incorrectly recalculated with the group ratio applied. This violates the original pricing intent.
The BillingContext fields must be consulted during quota recalculation to skip group ratio multiplication when GroupPriceOverride=true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/task.go` around lines 115 - 126, The quota recalculation paths must
honor TaskBillingContext.GroupPriceOverride: update settleTaskBillingOnComplete
and RecalculateTaskQuotaByTokens to read the saved billing context
(task.PrivateData.BillingContext -> TaskBillingContext) and, when
bc.GroupPriceOverride == true, skip applying bc.GroupRatio (i.e., do not
multiply by GroupRatio) and instead use the stored group pricing intent (use
GroupPriceOverrideGroup / UsingGroup as needed) so the original fixed-per-group
pricing is preserved during polling/settlement. Ensure both functions check
bc.PerCallBilling as before and short-circuit consistently when
bc.GroupPriceOverride is set.
| const normalizeStaleTimestamp = (value) => { | ||
| const text = typeof value === 'string' ? value.trim() : ''; | ||
| return text ? text.slice(0, 16) : ''; | ||
| }; | ||
|
|
||
| const buildLogsStaleKey = (scope, filters = {}) => | ||
| [ | ||
| 'usage-logs', | ||
| scope, | ||
| isAdminUser ? 'admin' : 'user', | ||
| filters.type ?? '', | ||
| filters.username || '', | ||
| filters.token_name || '', | ||
| filters.model_name || '', | ||
| filters.channel || '', | ||
| filters.group || '', | ||
| filters.request_id || '', | ||
| normalizeStaleTimestamp(filters.start_timestamp), | ||
| normalizeStaleTimestamp(filters.end_timestamp), | ||
| ].join(':'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'staleCacheKey' web/src/helpers/api.jsRepository: QuantumNous/new-api
Length of output: 437
🏁 Script executed:
#!/bin/bash
# Get more context around buildGetRequestKey and how staleCacheKey is used
rg -nP -B2 -A15 'function buildGetRequestKey' web/src/helpers/api.jsRepository: QuantumNous/new-api
Length of output: 681
🏁 Script executed:
#!/bin/bash
# Find where buildLogsStaleKey is called and what filters are passed
rg -nP 'buildLogsStaleKey' web/src/hooks/usage-logs/useUsageLogsData.jsxRepository: QuantumNous/new-api
Length of output: 288
🏁 Script executed:
#!/bin/bash
# Check how stat-self and list API calls use filters/pagination
rg -nP -B3 -A3 "(stat-self|list)" web/src/hooks/usage-logs/useUsageLogsData.jsx | head -80Repository: QuantumNous/new-api
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Get full list API call context
rg -nP -A10 "staleCacheKey: buildLogsStaleKey\('list'" web/src/hooks/usage-logs/useUsageLogsData.jsxRepository: QuantumNous/new-api
Length of output: 375
🏁 Script executed:
#!/bin/bash
# Get full stat-self API call context
rg -nP -A10 "staleCacheKey: buildLogsStaleKey\('stat-self'" web/src/hooks/usage-logs/useUsageLogsData.jsxRepository: QuantumNous/new-api
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Find 429 handling/stale cache logic in api.js
rg -nP -B5 -A5 '429|stale|GET_STALE_CACHE' web/src/helpers/api.js | head -100Repository: QuantumNous/new-api
Length of output: 2128
🏁 Script executed:
#!/bin/bash
# Get the full list API call including all config parameters
rg -nP -B5 -A20 "staleCacheKey: buildLogsStaleKey\('list'" web/src/hooks/usage-logs/useUsageLogsData.jsxRepository: QuantumNous/new-api
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Check if page/pageSize/startIdx appear anywhere in the list request or hook
rg -nP 'page|startIdx|pageSize' web/src/hooks/usage-logs/useUsageLogsData.jsx | head -30Repository: QuantumNous/new-api
Length of output: 1134
Include pagination parameters in cache key for list requests.
buildLogsStaleKey('list', ...) omits page (via p query param) and page_size parameters from the cache key. The URL embeds these as /api/log/?p=${startIdx}&page_size=${pageSize}&..., but since staleCacheKey is used as a custom key, buildGetRequestKey ignores the query params entirely. This creates a cache collision: requests to different pages generate identical cache keys. When a 429 error occurs on page 2, tryResolveStaleGetResponse returns the cached page 1 response because the keys match.
Include pagination in buildLogsStaleKey('list', ...):
buildLogsStaleKey = (scope, filters = {}) => [
'usage-logs',
scope,
isAdminUser ? 'admin' : 'user',
filters.type ?? '',
filters.username || '',
filters.token_name || '',
filters.model_name || '',
filters.channel || '',
filters.group || '',
filters.request_id || '',
filters.start_timestamp || '',
filters.end_timestamp || '',
filters.page || '', // Add
filters.page_size || '', // Add
].join(':');
Then pass them when calling the list endpoint:
staleCacheKey: buildLogsStaleKey('list', {
type: currentLogType,
username,
token_name,
model_name,
channel,
group,
request_id,
start_timestamp,
end_timestamp,
page: startIdx, // Add
page_size: pageSize, // Add
}),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/hooks/usage-logs/useUsageLogsData.jsx` around lines 46 - 65, The
cache key built by buildLogsStaleKey (used as staleCacheKey for list requests)
omits pagination, causing different pages to share the same key and return wrong
cached responses; update buildLogsStaleKey (and keep normalizeStaleTimestamp
as-is) to include filters.page and filters.page_size in the joined key, and
update callers that invoke buildLogsStaleKey('list', ...) to pass page and
page_size (the p/startIdx and pageSize values) so each paginated list request
gets a unique staleCacheKey; no other behavior changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/task_polling.go (1)
487-518:⚠️ Potential issue | 🟠 MajorNew transient/not-found handling is skipped for non-JSON response bodies.
The new logic at lines 502-515 only runs when the outer
common.Unmarshal(responseBody, &errorResult)succeeds (line 490). If an upstream returns a non-JSON body — e.g. a bareNot Foundfrom an edge proxy/nginx, an HTML error page, or any text —Unmarshalfails, the whole block is skipped,taskResult.Statusstays"", and the switch falls through todefault:on line 565, returning"unknown task status for task ...". Net effect: the exact "generic 404 while result is publishing" case this PR is meant to handle is silently bypassed whenever the body isn't valid JSON, which is very common for gateway-level 404s.Consider running the transient check before/outside the JSON branch so it is independent of
GeneralErrorResponseparsing.🛠️ Suggested restructure
if taskResult.Status == "" { - //taskResult = relaycommon.FailTaskInfo("upstream returned empty status") - errorResult := &dto.GeneralErrorResponse{} - if err = common.Unmarshal(responseBody, &errorResult); err == nil { - openaiError := errorResult.TryToOpenAIError() - if openaiError != nil { - if openaiError.Code == "429" { - return nil - } - taskResult = relaycommon.FailTaskInfo("upstream returned error") - } else { - bodyLower := strings.ToLower(string(responseBody)) - if isTransientVideoNotFoundResponse(resp.StatusCode, responseBody, task.SubmitTime, now, task.Properties.OriginModelName, task.Properties.UpstreamModelName) { - logger.LogInfo(ctx, fmt.Sprintf("Task %s upstream result not ready yet, keep polling, response: %s", taskId, string(responseBody))) - return nil - } - if strings.Contains(bodyLower, "not found") { - taskResult = relaycommon.FailTaskInfo("upstream task not found") - taskResult.Reason = strings.TrimSpace(string(responseBody)) - } else { - logger.LogError(ctx, fmt.Sprintf("Task %s returned empty status with unrecognized error format, keep polling, response: %s", taskId, string(responseBody))) - return nil - } - } - } + // Handle transient 404s even when the body isn't valid JSON (bare "Not Found" + // from gateways, HTML error pages, etc.). + if isTransientVideoNotFoundResponse(resp.StatusCode, responseBody, task.SubmitTime, now, task.Properties.OriginModelName, task.Properties.UpstreamModelName) { + logger.LogInfo(ctx, fmt.Sprintf("Task %s upstream result not ready yet, keep polling, response: %s", taskId, string(responseBody))) + return nil + } + bodyLower := strings.ToLower(string(responseBody)) + errorResult := &dto.GeneralErrorResponse{} + if err = common.Unmarshal(responseBody, &errorResult); err == nil { + if openaiError := errorResult.TryToOpenAIError(); openaiError != nil { + if openaiError.Code == "429" { + return nil + } + taskResult = relaycommon.FailTaskInfo("upstream returned error") + } else if strings.Contains(bodyLower, "not found") { + taskResult = relaycommon.FailTaskInfo("upstream task not found") + taskResult.Reason = strings.TrimSpace(string(responseBody)) + } else { + logger.LogError(ctx, fmt.Sprintf("Task %s returned empty status with unrecognized error format, keep polling, response: %s", taskId, string(responseBody))) + return nil + } + } else if strings.Contains(bodyLower, "not found") { + taskResult = relaycommon.FailTaskInfo("upstream task not found") + taskResult.Reason = strings.TrimSpace(string(responseBody)) + } else { + logger.LogError(ctx, fmt.Sprintf("Task %s returned empty status with non-JSON body, keep polling, response: %s", taskId, string(responseBody))) + return nil + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` around lines 487 - 518, When taskResult.Status is empty, the transient "not found" detection (isTransientVideoNotFoundResponse) should run regardless of whether common.Unmarshal(responseBody, &errorResult) succeeds; move or duplicate the call so the transient check executes before attempting to parse JSON (i.e., in the block where taskResult.Status == "" and before/independent of invoking common.Unmarshal and dto.GeneralErrorResponse.TryToOpenAIError), and only fall back to JSON-specific handling (TryToOpenAIError and extracting error.Code/strings.Contains) if parsing succeeds—this ensures non-JSON upstream bodies (plain "Not Found" or HTML) are properly recognized as transient and keep polling instead of falling through to the default unknown status.
🧹 Nitpick comments (1)
service/task_polling.go (1)
52-55: Align withCacheGetChannelfor consistency across polling functions.
updateSunoTasksandupdateVideoTasksboth usemodel.CacheGetChannel()to retrieve channel info during polling.RefreshVideoTaskshould do the same—there's no documented reason why this manual user-triggered refresh requires a direct DB hit instead of the cached variant, and changing it would improve consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` around lines 52 - 55, RefreshVideoTask currently calls model.GetChannelById(task.ChannelId, true) while other polling functions (updateSunoTasks, updateVideoTasks) use model.CacheGetChannel for consistency; change RefreshVideoTask to call model.CacheGetChannel with the same semantics used elsewhere (pass the task.ChannelId and the same cache flags) instead of GetChannelById so the polling path uses cached channel lookups consistent with updateSunoTasks/updateVideoTasks and avoids an unnecessary direct DB hit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/task_polling.go`:
- Around line 92-99: The current check returns true when submitTime <= 0 (or now
<= 0), which incorrectly keeps legacy/missing-submit-time tasks polling; change
the permissive fallback to fail-safe by returning false instead: update the
condition around submitTime and now (the lines checking "if submitTime <= 0 ||
now <= 0") to return false instead of true so the logic that uses
constant.TaskNotFoundGraceMinutes enforces the stricter path and lets
sweepTimedOutTasks handle eventual cleanup.
---
Outside diff comments:
In `@service/task_polling.go`:
- Around line 487-518: When taskResult.Status is empty, the transient "not
found" detection (isTransientVideoNotFoundResponse) should run regardless of
whether common.Unmarshal(responseBody, &errorResult) succeeds; move or duplicate
the call so the transient check executes before attempting to parse JSON (i.e.,
in the block where taskResult.Status == "" and before/independent of invoking
common.Unmarshal and dto.GeneralErrorResponse.TryToOpenAIError), and only fall
back to JSON-specific handling (TryToOpenAIError and extracting
error.Code/strings.Contains) if parsing succeeds—this ensures non-JSON upstream
bodies (plain "Not Found" or HTML) are properly recognized as transient and keep
polling instead of falling through to the default unknown status.
---
Nitpick comments:
In `@service/task_polling.go`:
- Around line 52-55: RefreshVideoTask currently calls
model.GetChannelById(task.ChannelId, true) while other polling functions
(updateSunoTasks, updateVideoTasks) use model.CacheGetChannel for consistency;
change RefreshVideoTask to call model.CacheGetChannel with the same semantics
used elsewhere (pass the task.ChannelId and the same cache flags) instead of
GetChannelById so the polling path uses cached channel lookups consistent with
updateSunoTasks/updateVideoTasks and avoids an unnecessary direct DB hit.
🪄 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
Run ID: df3ff3cc-c63e-4e8e-a892-9b936fd479fb
📒 Files selected for processing (2)
service/task_polling.goservice/task_polling_transient_test.go
✅ Files skipped from review due to trivial changes (1)
- service/task_polling_transient_test.go
| if constant.TaskNotFoundGraceMinutes <= 0 { | ||
| return false | ||
| } | ||
| if submitTime <= 0 || now <= 0 { | ||
| return true | ||
| } | ||
| return now-submitTime <= int64(constant.TaskNotFoundGraceMinutes)*60 | ||
| } |
There was a problem hiding this comment.
Permissive fallback when submitTime <= 0.
When TaskNotFoundGraceMinutes > 0 but submitTime is missing or zero (e.g. persisted before SubmitTime was reliably set, or imported/legacy rows), this returns true unconditionally, so the task stays in "keep polling" forever until sweepTimedOutTasks eventually kills it. Returning false here would fail-safe to the stricter path and rely on the submit-time invariant the rest of the codebase assumes.
🛡️ Suggested fix
- if submitTime <= 0 || now <= 0 {
- return true
- }
+ if submitTime <= 0 || now <= 0 {
+ return false
+ }
return now-submitTime <= int64(constant.TaskNotFoundGraceMinutes)*60📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if constant.TaskNotFoundGraceMinutes <= 0 { | |
| return false | |
| } | |
| if submitTime <= 0 || now <= 0 { | |
| return true | |
| } | |
| return now-submitTime <= int64(constant.TaskNotFoundGraceMinutes)*60 | |
| } | |
| if constant.TaskNotFoundGraceMinutes <= 0 { | |
| return false | |
| } | |
| if submitTime <= 0 || now <= 0 { | |
| return false | |
| } | |
| return now-submitTime <= int64(constant.TaskNotFoundGraceMinutes)*60 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/task_polling.go` around lines 92 - 99, The current check returns true
when submitTime <= 0 (or now <= 0), which incorrectly keeps
legacy/missing-submit-time tasks polling; change the permissive fallback to
fail-safe by returning false instead: update the condition around submitTime and
now (the lines checking "if submitTime <= 0 || now <= 0") to return false
instead of true so the logic that uses constant.TaskNotFoundGraceMinutes
enforces the stricter path and lets sweepTimedOutTasks handle eventual cleanup.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/channel/task/gemini/dto.go`:
- Around line 73-82: Change the OperationError struct's Code field from string
to any (interface{}) so numeric codes (e.g., int32) can unmarshal correctly;
update the type declaration of OperationError (the Code field) and keep the
custom UnmarshalJSON implementation (operationErrorAlias / Unmarshal JSON)
unchanged so the alias-based unmarshal picks up the new any type and preserves
compatibility with string codes.
In `@relay/channel/task/sora/adaptor.go`:
- Around line 143-148: The helper relayInfoUpstreamModelName incorrectly returns
"" when info.ChannelMeta is nil even if info.UpstreamModelName is set, causing
BuildRequestURL to pick the wrong endpoint; update relayInfoUpstreamModelName to
only guard against a nil info (i.e., check info == nil) and return
info.UpstreamModelName directly without gating on ChannelMeta so BuildRequestURL
can correctly detect an upstream model name.
- Around line 510-568: The multipart path currently skips writing the "seconds"
field for Sora models (the continue in the formData.Value loop that checks
isSoraVideoModel and key == "seconds"), so requests like seconds=8 get dropped
when duration is synthesized; change the loop logic to stop skipping "seconds"
(i.e., remove "key == \"seconds\"" from the continue condition) so original
seconds values are written via writer.WriteField, and keep the existing duration
fallback logic (durationValue defaulting to "4" and writing "duration") so both
seconds and duration are preserved for Sora models; refer to the formData.Value
loop, the hasSeconds/durationValue variables,
isSoraVideoModel(info.UpstreamModelName), and writer.WriteField usages to locate
the change.
🪄 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
Run ID: 241df049-f1b1-4376-9a72-a54e4e172663
📒 Files selected for processing (6)
relay/channel/task/gemini/adaptor_test.gorelay/channel/task/gemini/dto.gorelay/channel/task/sora/adaptor.gorelay/channel/task/sora/adaptor_test.gorelay/channel/task/vertex/adaptor.gorelay/channel/task/vertex/adaptor_test.go
✅ Files skipped from review due to trivial changes (1)
- relay/channel/task/sora/adaptor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- relay/channel/task/vertex/adaptor.go
| type OperationError struct { | ||
| Message string `json:"message"` | ||
| Code string `json:"code"` | ||
| } | ||
|
|
||
| func (e *OperationError) UnmarshalJSON(data []byte) error { | ||
| switch common.GetJsonType(data) { | ||
| case "object": | ||
| type operationErrorAlias OperationError | ||
| return common.Unmarshal(data, (*operationErrorAlias)(e)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google Cloud long-running Operation error field schema code type
💡 Result:
In Google Cloud long-running operations (LROs), defined in google.longrunning.Operation protobuf, the error field is of type google.rpc.Status. This Status message has: - code: int32 (an enum value from google.rpc.Code, e.g., 1 for CANCELLED, 5 for NOT_FOUND) - message: string (developer-facing English error message) - details: repeated google.protobuf.Any (list of error detail messages) When done=true and error is set (oneof result), it indicates failure or cancellation. This schema is standard across Google Cloud APIs. Protobuf definitions: - Operation.error = google.rpc.Status error = 4; - Status: message Status { int32 code = 1; string message = 2; repeated google.protobuf.Any details = 3; }
Citations:
- 1: https://cloud.google.com/resource-manager/reference/rest/Shared.Types/Operation
- 2: https://github.com/googleapis/api-common-protos/blob/master/google/longrunning/operations.proto
- 3: https://github.com/googleapis/googleapis/blob/master/google/longrunning/operations.proto
- 4: https://github.com/googleapis/api-common-protos/blob/87185dfffad4afa5a33a8c153f0e1ea53b4f85dc/google/rpc/status.proto
- 5: https://github.com/googleapis/api-common-protos/blob/master/google/rpc/status.proto
- 6: https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto
🏁 Script executed:
fd -t f "dto.go" relay/channel/task/gemini/Repository: QuantumNous/new-api
Length of output: 95
🏁 Script executed:
if [ -f "relay/channel/task/gemini/dto.go" ]; then
echo "=== File exists, checking OperationError context ==="
sed -n '70,90p' relay/channel/task/gemini/dto.go
echo ""
echo "=== Checking usage of OperationError ==="
rg -n "OperationError" relay/channel/task/gemini/
fiRepository: QuantumNous/new-api
Length of output: 904
🏁 Script executed:
rg -n "ParseTaskResult\|OperationError" relay/channel/task/gemini/ -A 5 -B 2Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check the broader context of how this DTO is used
rg -n "json:" relay/channel/task/gemini/dto.go | head -20Repository: QuantumNous/new-api
Length of output: 1176
🏁 Script executed:
# Look for the complete file structure around line 70 and how errors are handled
sed -n '65,100p' relay/channel/task/gemini/dto.goRepository: QuantumNous/new-api
Length of output: 633
🏁 Script executed:
# Search for where these DTOs are unmarshaled
rg -n "Unmarshal\|json.Unmarshal\|ParseTaskResult" relay/channel/task/gemini/*.go -B 2 -A 3Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check files in relay/channel/task/gemini to find usage
ls -la relay/channel/task/gemini/Repository: QuantumNous/new-api
Length of output: 480
🏁 Script executed:
# Search across the gemini task implementation
rg -n "ParseTaskResult\|parseTaskResult\|TaskResult" relay/channel/task/gemini/*.go -B 1 -A 3Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Search for where TaskResult (the struct containing OperationError) is used
rg -n "TaskResult" relay/channel/task/gemini/*.go -B 2 -A 5Repository: QuantumNous/new-api
Length of output: 1976
🏁 Script executed:
# Look at the entire dto.go file to understand the structure
cat -n relay/channel/task/gemini/dto.goRepository: QuantumNous/new-api
Length of output: 3712
🏁 Script executed:
# Check the ParseTaskResult function to see how it handles errors
sed -n '208,230p' relay/channel/task/gemini/adaptor.goRepository: QuantumNous/new-api
Length of output: 642
🏁 Script executed:
# Search for any actual usage/examples of OperationError with numeric code
rg -n "code" relay/channel/task/gemini/adaptor_test.goRepository: QuantumNous/new-api
Length of output: 314
Change OperationError.Code from string to any to accept numeric error codes.
Google Cloud long-running operations return error responses with numeric code fields (int32). The current Code string declaration causes unmarshal to fail on standard error objects, preventing the error message from being recorded. Changing to Code any preserves compatibility with both string and numeric codes.
Proposed fix
type OperationError struct {
Message string `json:"message"`
- Code string `json:"code"`
+ Code any `json:"code"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type OperationError struct { | |
| Message string `json:"message"` | |
| Code string `json:"code"` | |
| } | |
| func (e *OperationError) UnmarshalJSON(data []byte) error { | |
| switch common.GetJsonType(data) { | |
| case "object": | |
| type operationErrorAlias OperationError | |
| return common.Unmarshal(data, (*operationErrorAlias)(e)) | |
| type OperationError struct { | |
| Message string `json:"message"` | |
| Code any `json:"code"` | |
| } | |
| func (e *OperationError) UnmarshalJSON(data []byte) error { | |
| switch common.GetJsonType(data) { | |
| case "object": | |
| type operationErrorAlias OperationError | |
| return common.Unmarshal(data, (*operationErrorAlias)(e)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/channel/task/gemini/dto.go` around lines 73 - 82, Change the
OperationError struct's Code field from string to any (interface{}) so numeric
codes (e.g., int32) can unmarshal correctly; update the type declaration of
OperationError (the Code field) and keep the custom UnmarshalJSON implementation
(operationErrorAlias / Unmarshal JSON) unchanged so the alias-based unmarshal
picks up the new any type and preserves compatibility with string codes.
| func relayInfoUpstreamModelName(info *relaycommon.RelayInfo) string { | ||
| if info == nil || info.ChannelMeta == nil { | ||
| return "" | ||
| } | ||
| return info.UpstreamModelName | ||
| } |
There was a problem hiding this comment.
Don’t gate UpstreamModelName on ChannelMeta.
BuildRequestURL uses this helper to choose /v1/video/generations; if RelayInfo.UpstreamModelName is set but ChannelMeta is nil, this returns "" and falls back to /v1/videos.
Proposed fix
func relayInfoUpstreamModelName(info *relaycommon.RelayInfo) string {
- if info == nil || info.ChannelMeta == nil {
+ if info == nil {
return ""
}
return info.UpstreamModelName
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func relayInfoUpstreamModelName(info *relaycommon.RelayInfo) string { | |
| if info == nil || info.ChannelMeta == nil { | |
| return "" | |
| } | |
| return info.UpstreamModelName | |
| } | |
| func relayInfoUpstreamModelName(info *relaycommon.RelayInfo) string { | |
| if info == nil { | |
| return "" | |
| } | |
| return info.UpstreamModelName | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/channel/task/sora/adaptor.go` around lines 143 - 148, The helper
relayInfoUpstreamModelName incorrectly returns "" when info.ChannelMeta is nil
even if info.UpstreamModelName is set, causing BuildRequestURL to pick the wrong
endpoint; update relayInfoUpstreamModelName to only guard against a nil info
(i.e., check info == nil) and return info.UpstreamModelName directly without
gating on ChannelMeta so BuildRequestURL can correctly detect an upstream model
name.
| hasSeconds := false | ||
| hasDuration := false | ||
| durationValue := "" | ||
| hasSize := false | ||
| sizeValue := "" | ||
| hasAspectRatio := false | ||
| aspectRatioValue := "" | ||
| for key, values := range formData.Value { | ||
| if key == "model" { | ||
| continue | ||
| } | ||
| if key == "seconds" && len(values) > 0 && strings.TrimSpace(values[0]) != "" { | ||
| hasSeconds = true | ||
| } | ||
| if key == "duration" && len(values) > 0 && strings.TrimSpace(values[0]) != "" { | ||
| hasDuration = true | ||
| } | ||
| if key == "duration" && len(values) > 0 && durationValue == "" { | ||
| durationValue = strings.TrimSpace(values[0]) | ||
| } | ||
| if key == "size" && len(values) > 0 && strings.TrimSpace(values[0]) != "" { | ||
| hasSize = true | ||
| if sizeValue == "" { | ||
| sizeValue = strings.TrimSpace(values[0]) | ||
| } | ||
| } | ||
| if key == "aspect_ratio" && len(values) > 0 && strings.TrimSpace(values[0]) != "" { | ||
| hasAspectRatio = true | ||
| } | ||
| if key == "aspect_ratio" && len(values) > 0 && aspectRatioValue == "" { | ||
| aspectRatioValue = strings.TrimSpace(values[0]) | ||
| } | ||
| if isSoraVideoModel(info.UpstreamModelName) && (key == "seconds" || key == "size") { | ||
| continue | ||
| } | ||
| for _, v := range values { | ||
| writer.WriteField(key, v) | ||
| } | ||
| } | ||
| if info.UpstreamModelName == "grok-imagine-1.0-video" && !hasSeconds && durationValue != "" { | ||
| writer.WriteField("seconds", durationValue) | ||
| } | ||
| if isSoraVideoModel(info.UpstreamModelName) { | ||
| if !hasDuration { | ||
| if durationValue == "" { | ||
| durationValue = "4" | ||
| } | ||
| writer.WriteField("duration", durationValue) | ||
| } | ||
| if !hasAspectRatio { | ||
| if aspectRatioValue == "" && hasSize { | ||
| aspectRatioValue = soraAspectRatioFromSize(sizeValue) | ||
| } | ||
| if aspectRatioValue == "" { | ||
| aspectRatioValue = "9:16" | ||
| } | ||
| writer.WriteField("aspect_ratio", aspectRatioValue) | ||
| } | ||
| writer.WriteField("async", "true") |
There was a problem hiding this comment.
Preserve multipart seconds when mapping Sora duration.
The JSON path maps seconds to duration, but the multipart path only records hasSeconds; for Sora it then drops the seconds field and defaults missing duration to 4, ignoring a request like seconds=8.
Proposed fix
hasSeconds := false
+ secondsValue := ""
hasDuration := false
durationValue := ""
@@
if key == "seconds" && len(values) > 0 && strings.TrimSpace(values[0]) != "" {
hasSeconds = true
+ if secondsValue == "" {
+ secondsValue = strings.TrimSpace(values[0])
+ }
}
@@
if isSoraVideoModel(info.UpstreamModelName) {
if !hasDuration {
if durationValue == "" {
- durationValue = "4"
+ durationValue = secondsValue
+ }
+ if durationValue == "" {
+ durationValue = "4"
}
writer.WriteField("duration", durationValue)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/channel/task/sora/adaptor.go` around lines 510 - 568, The multipart
path currently skips writing the "seconds" field for Sora models (the continue
in the formData.Value loop that checks isSoraVideoModel and key == "seconds"),
so requests like seconds=8 get dropped when duration is synthesized; change the
loop logic to stop skipping "seconds" (i.e., remove "key == \"seconds\"" from
the continue condition) so original seconds values are written via
writer.WriteField, and keep the existing duration fallback logic (durationValue
defaulting to "4" and writing "duration") so both seconds and duration are
preserved for Sora models; refer to the formData.Value loop, the
hasSeconds/durationValue variables, isSoraVideoModel(info.UpstreamModelName),
and writer.WriteField usages to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
relay/relay_task.go (2)
315-335:⚠️ Potential issue | 🟡 MinorAvoid returning stale ratio metadata.
X-New-Api-Other-Ratiosis written beforeAdjustBillingOnSubmit, but the final ratios can change afterward. If this header is user-visible billing metadata, move header emission until after adjustment or make the header explicitly represent the estimate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 315 - 335, The header X-New-Api-Other-Ratios is set from otherRatios before calling adaptor.AdjustBillingOnSubmit, which can change info.PriceData.OtherRatios and lead to stale user-visible billing metadata; move the c.Header("X-New-Api-Other-Ratios", ...) emission to after the AdjustBillingOnSubmit / calcTaskQuotaWithRatios block (or explicitly label it as an estimate) so the header reflects final ratios—update the sequence around otherRatios, adaptor.AdjustBillingOnSubmit, calcTaskQuotaWithRatios, and info.PriceData.OtherRatios accordingly.
303-326:⚠️ Potential issue | 🟠 MajorClean up the pre-created task on submit failures.
Because the pending task is inserted before the upstream call, these early returns can leave a task permanently
submittedeven though the submit failed. Also close the non-2xx response body after reading it.Suggested direction
resp, err := adaptor.DoRequest(c, info, requestBody) if err != nil { + // Mark the pre-created local task as failed, or defer creation until submit succeeds. return nil, service.TaskErrorWrapper(err, "do_request_failed", http.StatusInternalServerError) } if resp != nil && !isSuccessfulTaskSubmitStatus(resp.StatusCode) { + defer resp.Body.Close() responseBody, _ := io.ReadAll(resp.Body) + // Mark the pre-created local task as failed, or defer creation until submit succeeds. return nil, service.TaskErrorWrapper(fmt.Errorf("%s", string(responseBody)), "fail_to_fetch_task", resp.StatusCode) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 303 - 326, The pending task inserted by upsertPendingRelayTaskRecord must be removed when upstream submission fails: add a cleanup call (e.g. deletePendingRelayTaskRecord(c, info, platform) or updatePendingRelayTaskToFailed(...)) whenever adaptor.DoRequest returns an error, when resp is non-nil but !isSuccessfulTaskSubmitStatus(resp.StatusCode), and when adaptor.DoResponse returns taskErr; also ensure you close resp.Body after reading it (call resp.Body.Close() after io.ReadAll and in all error paths where resp is non-nil) to avoid leaking connections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/async_video.go`:
- Around line 137-145: Multipart form handling in async_video.go never copies
the request_id into the request, so task.PrivateData.ClientRequestId is lost;
update the multipart parsing block (where req.Prompt, req.Model, req.Image,
req.ImageURL, req.Size, req.Seconds, req.Duration are set using
firstAsyncVideoFormValue(form.Value, ...)) to also read and set req.RequestId =
strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "request_id")) (or
similar) so that req.RequestId is populated for multipart submissions and
preserved into task.PrivateData.ClientRequestId downstream.
- Around line 52-70: The code double-copies the upload by calling
storage.Bytes() then copying it again with append when building asyncVideoJob;
to fix, avoid the second copy by either (A) assigning the already-read slice
directly (set asyncVideoJob.Body = bodyBytes instead of append([]byte(nil),
bodyBytes...)) or (preferred) change asyncVideoJob to carry a handle (e.g.,
BodyStorage or an io.ReadSeeker/io.ReadCloser backed by temp file) and pass the
storage handle from where readAsyncVideoTaskRequest and initAsyncVideoTask are
called so large multipart uploads are kept on disk rather than duplicated in
memory.
- Around line 257-268: The updateAsyncVideoTaskFailure function currently writes
raw responseBody and failReason into task.Data and task.FailReason; instead
sanitize and JSON-wrap them: call redactVideoResponseBody(responseBody) to get a
redacted []byte (removing bytesBase64Encoded and truncating data: URLs),
sanitize failReason to strip or replace any data: URLs/HTML (reuse
redactVideoResponseBody or a small helper), then construct a JSON object like
{"error": "<sanitized-failReason>", "response":
<redacted-response-as-string-or-object>} and marshal it using the project's JSON
wrapper (e.g., common.JSONMarshal or equivalent) and assign that JSON to
task.Data; keep task.FailReason set to a short sanitized string (no data: URLs),
and preserve other fields as before so DB JSON field updates succeed across
SQLite/MySQL/Postgres.
In `@relay/relay_task.go`:
- Around line 85-109: The submit flow fails to persist resolved header overrides
so async follow-ups lose client_header passthrough values; update the submit
logic where task is populated (around task.Action, task.PrivateData, and
task.PrivateData.BillingContext) to call the same resolution used elsewhere
(e.g., processHeaderOverride or the function that returns the resolved header
map) and assign its result to task.PrivateData.ResolvedHeaderOverride so the
resolved headers are stored on the task at creation and available for async
follow-ups.
---
Outside diff comments:
In `@relay/relay_task.go`:
- Around line 315-335: The header X-New-Api-Other-Ratios is set from otherRatios
before calling adaptor.AdjustBillingOnSubmit, which can change
info.PriceData.OtherRatios and lead to stale user-visible billing metadata; move
the c.Header("X-New-Api-Other-Ratios", ...) emission to after the
AdjustBillingOnSubmit / calcTaskQuotaWithRatios block (or explicitly label it as
an estimate) so the header reflects final ratios—update the sequence around
otherRatios, adaptor.AdjustBillingOnSubmit, calcTaskQuotaWithRatios, and
info.PriceData.OtherRatios accordingly.
- Around line 303-326: The pending task inserted by upsertPendingRelayTaskRecord
must be removed when upstream submission fails: add a cleanup call (e.g.
deletePendingRelayTaskRecord(c, info, platform) or
updatePendingRelayTaskToFailed(...)) whenever adaptor.DoRequest returns an
error, when resp is non-nil but !isSuccessfulTaskSubmitStatus(resp.StatusCode),
and when adaptor.DoResponse returns taskErr; also ensure you close resp.Body
after reading it (call resp.Body.Close() after io.ReadAll and in all error paths
where resp is non-nil) to avoid leaking connections.
🪄 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
Run ID: 4b76154b-379f-48ec-a9a2-052ca911bc39
📒 Files selected for processing (4)
controller/async_video.gocontroller/async_video_test.gorelay/relay_task.goweb/src/pages/CreativeCenter/index.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/async_video_test.go
| bodyBytes, err := storage.Bytes() | ||
| if err != nil { | ||
| respondAsyncVideoOpenAIError(c, http.StatusBadRequest, err.Error(), types.ErrorCodeReadRequestBodyFailed) | ||
| return | ||
| } | ||
|
|
||
| req := readAsyncVideoTaskRequest(c, bodyBytes) | ||
| task := initAsyncVideoTask(c, req) | ||
| if err := task.Insert(); err != nil { | ||
| respondAsyncVideoOpenAIError(c, http.StatusInternalServerError, err.Error(), types.ErrorCodeQueryDataError) | ||
| return | ||
| } | ||
|
|
||
| job := asyncVideoJob{ | ||
| TaskID: task.TaskID, | ||
| Path: asyncVideoGeneration, | ||
| Method: c.Request.Method, | ||
| Header: c.Request.Header.Clone(), | ||
| Body: append([]byte(nil), bodyBytes...), |
There was a problem hiding this comment.
Avoid duplicating large media bodies in memory.
storage.Bytes() materializes the full upload, then Line 70 copies it again into the async job. For multipart media, concurrent requests can retain 2x body size until relay completion. Prefer passing a BodyStorage/temp-backed handle into the worker; at minimum avoid the second copy.
Minimal improvement to remove the extra copy
job := asyncVideoJob{
TaskID: task.TaskID,
Path: asyncVideoGeneration,
Method: c.Request.Method,
Header: c.Request.Header.Clone(),
- Body: append([]byte(nil), bodyBytes...),
+ Body: bodyBytes,
Keys: cloneAsyncImageContextKeys(c),
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/async_video.go` around lines 52 - 70, The code double-copies the
upload by calling storage.Bytes() then copying it again with append when
building asyncVideoJob; to fix, avoid the second copy by either (A) assigning
the already-read slice directly (set asyncVideoJob.Body = bodyBytes instead of
append([]byte(nil), bodyBytes...)) or (preferred) change asyncVideoJob to carry
a handle (e.g., BodyStorage or an io.ReadSeeker/io.ReadCloser backed by temp
file) and pass the storage handle from where readAsyncVideoTaskRequest and
initAsyncVideoTask are called so large multipart uploads are kept on disk rather
than duplicated in memory.
| req.Prompt = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "prompt")) | ||
| req.Model = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "model")) | ||
| req.Image = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "image")) | ||
| req.ImageURL = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "image_url")) | ||
| req.Size = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "size")) | ||
| req.Seconds = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "seconds")) | ||
| if duration, err := strconv.Atoi(strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "duration"))); err == nil { | ||
| req.Duration = duration | ||
| } |
There was a problem hiding this comment.
Preserve request_id for multipart submissions.
JSON requests can populate req.RequestId, but multipart requests never copy the request_id form field, so task.PrivateData.ClientRequestId is lost for those clients.
Proposed fix
req.ImageURL = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "image_url"))
req.Size = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "size"))
req.Seconds = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "seconds"))
+ req.RequestId = strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "request_id"))
if duration, err := strconv.Atoi(strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "duration"))); err == nil {
req.Duration = duration
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/async_video.go` around lines 137 - 145, Multipart form handling in
async_video.go never copies the request_id into the request, so
task.PrivateData.ClientRequestId is lost; update the multipart parsing block
(where req.Prompt, req.Model, req.Image, req.ImageURL, req.Size, req.Seconds,
req.Duration are set using firstAsyncVideoFormValue(form.Value, ...)) to also
read and set req.RequestId =
strings.TrimSpace(firstAsyncVideoFormValue(form.Value, "request_id")) (or
similar) so that req.RequestId is populated for multipart submissions and
preserved into task.PrivateData.ClientRequestId downstream.
| func updateAsyncVideoTaskFailure(task *model.Task, responseBody []byte, failReason string) { | ||
| if task == nil { | ||
| return | ||
| } | ||
| task.Status = model.TaskStatusFailure | ||
| task.Progress = taskcommon.ProgressComplete | ||
| task.FinishTime = time.Now().Unix() | ||
| task.FailReason = strings.TrimSpace(failReason) | ||
| task.PrivateData.ResultURL = "" | ||
| if len(responseBody) > 0 { | ||
| task.Data = append([]byte(nil), responseBody...) | ||
| } |
There was a problem hiding this comment.
Sanitize and JSON-wrap failure payloads before persisting.
task.Data is a JSON field, but this stores arbitrary upstream response bytes; HTML/plain-text failures can make task.Update() fail on JSON-enforcing databases. It also bypasses the existing video response redaction path, so base64/data URL payloads can be persisted in Data or FailReason.
Proposed direction
task.Status = model.TaskStatusFailure
task.Progress = taskcommon.ProgressComplete
task.FinishTime = time.Now().Unix()
task.FailReason = strings.TrimSpace(failReason)
+ if strings.HasPrefix(strings.ToLower(task.FailReason), "data:") {
+ task.FailReason = "async video request failed"
+ }
task.PrivateData.ResultURL = ""
if len(responseBody) > 0 {
- task.Data = append([]byte(nil), responseBody...)
+ sanitizedBody := redactVideoResponseBody(responseBody)
+ var decoded any
+ if err := common.Unmarshal(sanitizedBody, &decoded); err != nil {
+ if wrappedBody, marshalErr := common.Marshal(gin.H{
+ "error": strings.TrimSpace(string(sanitizedBody)),
+ }); marshalErr == nil {
+ sanitizedBody = wrappedBody
+ } else {
+ sanitizedBody = nil
+ }
+ }
+ if len(sanitizedBody) > 0 {
+ task.Data = append([]byte(nil), sanitizedBody...)
+ }
}Based on learnings, redactVideoResponseBody removes bytesBase64Encoded fields/truncates base64, and data: URLs should not be stored in task.FailReason. As per coding guidelines, database code must be compatible with SQLite, MySQL, and PostgreSQL, and Go JSON operations must use common wrappers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/async_video.go` around lines 257 - 268, The
updateAsyncVideoTaskFailure function currently writes raw responseBody and
failReason into task.Data and task.FailReason; instead sanitize and JSON-wrap
them: call redactVideoResponseBody(responseBody) to get a redacted []byte
(removing bytesBase64Encoded and truncating data: URLs), sanitize failReason to
strip or replace any data: URLs/HTML (reuse redactVideoResponseBody or a small
helper), then construct a JSON object like {"error": "<sanitized-failReason>",
"response": <redacted-response-as-string-or-object>} and marshal it using the
project's JSON wrapper (e.g., common.JSONMarshal or equivalent) and assign that
JSON to task.Data; keep task.FailReason set to a short sanitized string (no
data: URLs), and preserve other fields as before so DB JSON field updates
succeed across SQLite/MySQL/Postgres.
| task.Action = info.Action | ||
| task.Status = model.TaskStatusSubmitted | ||
| task.Progress = taskcommon.ProgressSubmitted | ||
| task.PrivateData.RequestId = info.RequestId | ||
| task.PrivateData.BillingSource = info.BillingSource | ||
| task.PrivateData.SubscriptionId = info.SubscriptionId | ||
| task.PrivateData.TokenId = info.TokenId | ||
| task.PrivateData.UpstreamRequestPath = strings.TrimSpace(info.RequestURLPath) | ||
| if prompt := extractTaskPromptFromContext(c); prompt != "" { | ||
| task.Properties.Input = prompt | ||
| } | ||
| if clientRequestID := extractTaskClientRequestIDFromContext(c); clientRequestID != "" { | ||
| task.PrivateData.ClientRequestId = clientRequestID | ||
| } | ||
| task.PrivateData.BillingContext = &model.TaskBillingContext{ | ||
| ModelPrice: info.PriceData.ModelPrice, | ||
| GroupRatio: info.PriceData.GroupRatioInfo.GroupRatio, | ||
| ModelRatio: info.PriceData.ModelRatio, | ||
| OtherRatios: info.PriceData.OtherRatios, | ||
| OriginModelName: info.OriginModelName, | ||
| PerCallBilling: common.StringsContains(constant.TaskPricePatches, info.OriginModelName), | ||
| GroupPriceOverride: info.PriceData.GroupPriceOverride, | ||
| GroupPriceOverrideGroup: info.PriceData.GroupPriceOverrideGroup, | ||
| UsingGroup: info.UsingGroup, | ||
| } |
There was a problem hiding this comment.
Persist resolved header overrides with the task.
ResolvedHeaderOverride is not copied into task.PrivateData, so async follow-up requests can lose {client_header:*} passthrough values after the submit request context is gone.
Suggested fix
task.PrivateData.RequestId = info.RequestId
+ task.PrivateData.ResolvedHeaderOverride = info.ResolvedHeaderOverride
task.PrivateData.BillingSource = info.BillingSourceBased on learnings: In the Sora2/OpenAI async video task flow, the resolved header map from processHeaderOverride must be persisted into task.PrivateData.ResolvedHeaderOverride at submit time.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| task.Action = info.Action | |
| task.Status = model.TaskStatusSubmitted | |
| task.Progress = taskcommon.ProgressSubmitted | |
| task.PrivateData.RequestId = info.RequestId | |
| task.PrivateData.BillingSource = info.BillingSource | |
| task.PrivateData.SubscriptionId = info.SubscriptionId | |
| task.PrivateData.TokenId = info.TokenId | |
| task.PrivateData.UpstreamRequestPath = strings.TrimSpace(info.RequestURLPath) | |
| if prompt := extractTaskPromptFromContext(c); prompt != "" { | |
| task.Properties.Input = prompt | |
| } | |
| if clientRequestID := extractTaskClientRequestIDFromContext(c); clientRequestID != "" { | |
| task.PrivateData.ClientRequestId = clientRequestID | |
| } | |
| task.PrivateData.BillingContext = &model.TaskBillingContext{ | |
| ModelPrice: info.PriceData.ModelPrice, | |
| GroupRatio: info.PriceData.GroupRatioInfo.GroupRatio, | |
| ModelRatio: info.PriceData.ModelRatio, | |
| OtherRatios: info.PriceData.OtherRatios, | |
| OriginModelName: info.OriginModelName, | |
| PerCallBilling: common.StringsContains(constant.TaskPricePatches, info.OriginModelName), | |
| GroupPriceOverride: info.PriceData.GroupPriceOverride, | |
| GroupPriceOverrideGroup: info.PriceData.GroupPriceOverrideGroup, | |
| UsingGroup: info.UsingGroup, | |
| } | |
| task.Action = info.Action | |
| task.Status = model.TaskStatusSubmitted | |
| task.Progress = taskcommon.ProgressSubmitted | |
| task.PrivateData.RequestId = info.RequestId | |
| task.PrivateData.ResolvedHeaderOverride = info.ResolvedHeaderOverride | |
| task.PrivateData.BillingSource = info.BillingSource | |
| task.PrivateData.SubscriptionId = info.SubscriptionId | |
| task.PrivateData.TokenId = info.TokenId | |
| task.PrivateData.UpstreamRequestPath = strings.TrimSpace(info.RequestURLPath) | |
| if prompt := extractTaskPromptFromContext(c); prompt != "" { | |
| task.Properties.Input = prompt | |
| } | |
| if clientRequestID := extractTaskClientRequestIDFromContext(c); clientRequestID != "" { | |
| task.PrivateData.ClientRequestId = clientRequestID | |
| } | |
| task.PrivateData.BillingContext = &model.TaskBillingContext{ | |
| ModelPrice: info.PriceData.ModelPrice, | |
| GroupRatio: info.PriceData.GroupRatioInfo.GroupRatio, | |
| ModelRatio: info.PriceData.ModelRatio, | |
| OtherRatios: info.PriceData.OtherRatios, | |
| OriginModelName: info.OriginModelName, | |
| PerCallBilling: common.StringsContains(constant.TaskPricePatches, info.OriginModelName), | |
| GroupPriceOverride: info.PriceData.GroupPriceOverride, | |
| GroupPriceOverrideGroup: info.PriceData.GroupPriceOverrideGroup, | |
| UsingGroup: info.UsingGroup, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/relay_task.go` around lines 85 - 109, The submit flow fails to persist
resolved header overrides so async follow-ups lose client_header passthrough
values; update the submit logic where task is populated (around task.Action,
task.PrivateData, and task.PrivateData.BillingContext) to call the same
resolution used elsewhere (e.g., processHeaderOverride or the function that
returns the resolved header map) and assign its result to
task.PrivateData.ResolvedHeaderOverride so the resolved headers are stored on
the task at creation and available for async follow-ups.
2 participants
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
New Features
Enhancements