refactor: async task - #2985
Conversation
…service layer Restructure the task relay system for better separation of concerns: - Extract task billing into service/task_billing.go with unified settlement flow - Move task polling loop from controller to service/task_polling.go (supports Suno + video platforms) - Split RelayTask into fetch/submit paths with dedicated retry logic (taskSubmitWithRetry) - Add TaskDto, TaskResponse generics, and FetchReq to dto/task.go - Add taskcommon/helpers.go for shared task adaptor utilities - Remove controller/task_video.go (logic consolidated into service layer) - Update all task adaptors (ali, doubao, gemini, hailuo, jimeng, kling, sora, suno, vertex, vidu) - Simplify frontend task logs to use new TaskDto response format
Add three billing lifecycle methods to the TaskAdaptor interface: - EstimateBilling: compute OtherRatios from user request before pricing - AdjustBillingOnSubmit: adjust ratios from upstream submit response - AdjustBillingOnComplete: determine final quota at task terminal state Introduce BaseBilling as embeddable no-op default for adaptors without custom billing. Move Sora/Ali OtherRatios logic from shared validation into per-adaptor EstimateBilling implementations. Add TaskBillingContext to persist pricing params (model_price, group_ratio, other_ratios) in task private data for async polling settlement. Extract RecalculateTaskQuota as a general-purpose delta settlement function and unify polling billing via settleTaskBillingOnComplete (adaptor-first, then token-based fallback).
- Renamed RelayTask function to RelayTaskFetch for clarity. - Updated routing in relay-router.go and video-router.go to use RelayTaskFetch for fetch operations. - Enhanced error handling in RelayTaskFetch function. - Adjusted task data conversion in TaskAdaptor to include task ID.
- Updated the remix handling in ResolveOriginTask to prioritize extracting OtherRatios from the BillingContext of the original task if available. - Retained the previous logic for extracting seconds and size from task data as a fallback. - Improved clarity and maintainability of the remix logic by separating the new and old approaches.
- Enhanced the RelayTask function to utilize a locked channel when available, allowing for better reuse during retries. - Updated error handling to ensure proper context setup for the selected channel. - Clarified comments in ResolveOriginTask regarding channel locking and retry behavior. - Introduced a new field in TaskRelayInfo to store the locked channel object, improving type safety and reducing import cycles.
… conflicts Replace all bare task.Update() (DB.Save) calls with UpdateWithStatus(), which adds a WHERE status = ? guard to prevent concurrent processes from overwriting each other's state transitions. Key changes: model/task.go: - Add taskSnapshot struct with Equal() method for change detection - Add Snapshot() method to capture pre-update state - Add UpdateWithStatus(fromStatus) using DB.Where().Save() for CAS semantics with full-struct save (no explicit field listing needed) model/midjourney.go: - Add UpdateWithStatus(fromStatus string) with same CAS pattern service/task_polling.go (updateVideoSingleTask): - Snapshot before processing upstream response; skip DB write if unchanged - Terminal transitions (SUCCESS/FAILURE) use UpdateWithStatus CAS: billing/refund only executes if this process wins the transition - Non-terminal updates also use UpdateWithStatus to prevent overwriting a concurrent terminal transition back to IN_PROGRESS - Defer settleTaskBillingOnComplete to after CAS check (shouldSettle flag) relay/relay_task.go (tryRealtimeFetch): - Add snapshot + change detection; use UpdateWithStatus for CAS safety controller/midjourney.go (UpdateMidjourneyTaskBulk): - Capture preStatus before mutations; use UpdateWithStatus CAS - Gate refund (IncreaseUserQuota) on CAS success (won && shouldReturnQuota) This prevents the multi-instance race condition where: 1. Instance A reads task (IN_PROGRESS), fetches upstream (still IN_PROGRESS) 2. Instance B reads same task, fetches upstream (now SUCCESS), writes SUCCESS 3. Instance A's bare Save() overwrites SUCCESS back to IN_PROGRESS
…gration tests
- Updated UpdateWithStatus method to use Model().Select("*").Updates() for conditional updates, preventing GORM's INSERT fallback.
- Introduced comprehensive integration tests for UpdateWithStatus, covering scenarios for winning and losing CAS updates, as well as concurrent updates.
- Added task_cas_test.go to validate the new behavior and ensure data integrity during concurrent state transitions.
WalkthroughAdds async task polling, adapter billing hooks, task CAS updates, comprehensive billing/reconciliation (logs, refunds), relay submission refactors (PublicTaskID/locked channel), video proxy/unified error handling, many adaptor updates to use RelayInfo and common marshal, and frontend locale/usage-log additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Poll as TaskPollingLoop
participant Adaptor as TaskPollingAdaptor
participant Upstream as Upstream Channel/API
participant DB as Database
participant Billing as service/task_billing
Poll->>DB: query unfinished tasks grouped by platform/channel
Poll->>Adaptor: Init(info) and FetchTask(upstream)
Adaptor->>Upstream: HTTP request (FetchTask)
Upstream-->>Adaptor: response (status, data)
Adaptor-->>Poll: ParseTaskResult -> TaskInfo
Poll->>DB: Snapshot + UpdateWithStatus (CAS)
DB-->>Poll: RowsAffected (won/lose)
Poll->>Billing: if won -> settleTaskBillingOnComplete / LogTaskConsumption
Billing->>DB: write consumption/refund logs, update quotas/tokens
Poll->>DB: persist task updates (ResultURL, UpstreamTaskID, status, progress)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…try fix for async tasks
1. Async task model redirection (aligned with sync tasks):
- Integrate ModelMappedHelper in RelayTaskSubmit after model name
determination, populating OriginModelName / UpstreamModelName on RelayInfo.
- All task adaptors now send UpstreamModelName to upstream providers:
- Gemini & Vertex: BuildRequestURL uses UpstreamModelName.
- Doubao & Ali: BuildRequestBody conditionally overwrites body.Model.
- Vidu, Kling, Hailuo, Jimeng: convertToRequestPayload accepts RelayInfo
and unconditionally uses info.UpstreamModelName.
- Sora: BuildRequestBody parses JSON and multipart bodies to replace
the "model" field with UpstreamModelName.
- Frontend log visibility: LogTaskConsumption and taskBillingOther now
emit is_model_mapped / upstream_model_name in the "other" JSON field.
- Billing safety: RecalculateTaskQuotaByTokens reads model name from
BillingContext.OriginModelName (via taskModelName) instead of
task.Data["model"], preventing billing leaks from upstream model names.
2. Per-call billing (TaskPricePatches lifecycle):
- Rename TaskBillingContext.ModelName → OriginModelName; add PerCallBilling
bool field, populated from TaskPricePatches at submission time.
- settleTaskBillingOnComplete short-circuits when PerCallBilling is true,
skipping both adaptor adjustments and token-based recalculation.
- Remove ModelName from TaskSubmitResult; use relayInfo.OriginModelName
consistently in controller/relay.go for billing context and logging.
3. Multipart retry boundary mismatch fix:
- Root cause: after Sora (or OpenAI audio) rebuilds a multipart body with a
new boundary and overwrites c.Request.Header["Content-Type"], subsequent
calls to ParseMultipartFormReusable on retry would parse the cached
original body with the wrong boundary, causing "NextPart: EOF".
- Fix: ParseMultipartFormReusable now caches the original Content-Type in
gin context key "_original_multipart_ct" on first call and reuses it for
all subsequent parses, making multipart parsing retry-safe globally.
- Sora adaptor reverted to the standard pattern (direct header set/get),
which is now safe thanks to the root fix.
4. Tests:
- task_billing_test.go: update makeTask to use OriginModelName; add
PerCallBilling settlement tests (skip adaptor adjust, skip token recalc);
add non-per-call adaptor adjustment test with refund verification.
fc448d6 to
ec5c6b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
controller/midjourney.go (1)
6-6:⚠️ Potential issue | 🟡 MinorDirect
encoding/jsonusage violates coding guidelines.This file uses
json.Marshal(lines 82, 145, 149, 157) andjson.Unmarshal(line 112) directly. Per coding guidelines, all JSON marshal/unmarshal operations must use wrapper functions fromcommon/json.go(common.Marshal(),common.Unmarshal(), etc.).🔧 Replace with common wrappers
- "encoding/json"Then replace usages, e.g.:
- body, _ := json.Marshal(map[string]any{ + body, _ := common.Marshal(map[string]any{- err = json.Unmarshal(responseBody, &responseItems) + err = common.Unmarshal(responseBody, &responseItems)And similarly for lines 145, 149, 157, 235.
As per coding guidelines: "Do NOT directly import or call
encoding/jsonfor marshal/unmarshal in business code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/midjourney.go` at line 6, Replace all direct uses of encoding/json in midjourney.go: remove the "encoding/json" import, add the project's common JSON wrapper import (e.g., common), and replace every json.Marshal and json.Unmarshal call with common.Marshal and common.Unmarshal respectively; specifically update the calls named json.Marshal (occurrences around lines where responses/bodies are encoded: json.Marshal at ~82, ~145, ~149, ~157) and json.Unmarshal (around ~112) to use common.Marshal/common.Unmarshal so the file no longer calls encoding/json directly and complies with the wrapper usage policy.relay/relay_task.go (1)
281-290:⚠️ Potential issue | 🔴 CriticalCritical: missing
returncauses nil function call panic.When
relayModeis not infetchRespBuilders,respBuilderisnilbut execution falls through torespBuilder(c)on line 287, causing a nil pointer dereference panic.🐛 Proposed fix
respBuilder, ok := fetchRespBuilders[relayMode] if !ok { - taskResp = service.TaskErrorWrapperLocal(errors.New("invalid_relay_mode"), "invalid_relay_mode", http.StatusBadRequest) + return service.TaskErrorWrapperLocal(errors.New("invalid_relay_mode"), "invalid_relay_mode", http.StatusBadRequest) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 281 - 290, In RelayTaskFetch, when fetchRespBuilders does not contain relayMode you set taskResp but do not return, leading to a nil respBuilder and a panic when calling respBuilder(c); fix by short-circuiting immediately after detecting the missing key (e.g., return taskResp) or restructure with an else so respBuilder is only invoked when present; reference: RelayTaskFetch, fetchRespBuilders, respBuilder.relay/channel/task/jimeng/adaptor.go (1)
221-224:⚠️ Potential issue | 🟠 Major
a.baseURLused instead of thebaseUrlparameter for new API relay path.Line 223 uses the adaptor's
a.baseURL(set duringInit) instead of thebaseUrlparameter passed toFetchTask. If the adaptor isn'tInit-ed beforeFetchTaskis called (e.g., from polling or real-time fetch paths), this produces a malformed URL. The non-relay path on line 221 correctly uses thebaseUrlparameter.🐛 Proposed fix — consistently use the parameter
if isNewAPIRelay(key) { - uri = fmt.Sprintf("%s/jimeng/?Action=CVSync2AsyncGetResult&Version=2022-08-31", a.baseURL) + uri = fmt.Sprintf("%s/jimeng/?Action=CVSync2AsyncGetResult&Version=2022-08-31", baseUrl) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/jimeng/adaptor.go` around lines 221 - 224, The code builds a relay URI using a.baseURL instead of the FetchTask parameter baseUrl when isNewAPIRelay(key) is true, which breaks callers that don't Init the adaptor; update the branch in FetchTask so the uri variable is constructed from the baseUrl parameter (the same way the non-relay path does) rather than a.baseURL, keeping the format "/jimeng/?Action=CVSync2AsyncGetResult&Version=2022-08-31" and ensuring isNewAPIRelay(key), uri, FetchTask, baseUrl, and a.baseURL are the referenced symbols to locate the change.
🧹 Nitpick comments (22)
model/token.go (1)
363-379: LGTM — optional: alignDecreaseTokenQuota's parameter name for consistency.The rename from
idtotokenIdis applied consistently across both call sites withinIncreaseTokenQuota. However, the siblingDecreaseTokenQuotaat Line 393 still uses the old parameter nameid, leaving the public function pair asymmetric.♻️ Align `DecreaseTokenQuota` parameter name
-func DecreaseTokenQuota(id int, key string, quota int) (err error) { +func DecreaseTokenQuota(tokenId int, key string, quota int) (err error) { if quota < 0 { return errors.New("quota 不能为负数！") } if common.RedisEnabled { gopool.Go(func() { err := cacheDecrTokenQuota(key, int64(quota)) if err != nil { common.SysLog("failed to decrease token quota: " + err.Error()) } }) } if common.BatchUpdateEnabled { - addNewRecord(BatchUpdateTypeTokenQuota, id, -quota) + addNewRecord(BatchUpdateTypeTokenQuota, tokenId, -quota) return nil } - return decreaseTokenQuota(id, quota) + return decreaseTokenQuota(tokenId, quota) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/token.go` around lines 363 - 379, The public function DecreaseTokenQuota should use the same parameter name as IncreaseTokenQuota for consistency: rename its first parameter from id to tokenId in the DecreaseTokenQuota function signature and update all references inside that function (and any local calls) to use tokenId so the pair is symmetric (ensure you update any call sites within the same file that reference the old id name).common/gin.go (1)
248-254: Magic string key — define a package-level constant consistent withKeyRequestBody/KeyBodyStorage.All other context keys in this file are named constants. The new
"_original_multipart_ct"raw literal diverges from that pattern; a typo would silently miss the cache without any compiler error.♻️ Proposed refactor
Add the constant next to the existing ones (lines 20-21):
const KeyRequestBody = "key_request_body" const KeyBodyStorage = "key_body_storage" +const keyOriginalMultipartCT = "_original_multipart_ct"Then replace the raw strings in
ParseMultipartFormReusable:- if saved, ok := c.Get("_original_multipart_ct"); ok { + if saved, ok := c.Get(keyOriginalMultipartCT); ok { if s, ok := saved.(string); ok { contentType = s } else { contentType = c.Request.Header.Get("Content-Type") } } else { contentType = c.Request.Header.Get("Content-Type") - c.Set("_original_multipart_ct", contentType) + c.Set(keyOriginalMultipartCT, contentType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/gin.go` around lines 248 - 254, Add a package-level constant for the "_original_multipart_ct" key (similar to KeyRequestBody / KeyBodyStorage) and use it inside ParseMultipartFormReusable instead of the raw string; specifically define something like KeyOriginalMultipartCT as a const next to the existing context key constants and replace the raw literal occurrences in ParseMultipartFormReusable where contentType is read/saved (the c.Get, c.Set and any other lookups) to use KeyOriginalMultipartCT to avoid silent typos.types/price_data.go (1)
40-41:ToSetting()does not include the newQuotafield.The new per-call billing quota isn't visible in the debug string, which may make it harder to trace billing discrepancies for MJ/Task workflows.
♻️ Proposed addition
-func (p *PriceData) ToSetting() string { - return fmt.Sprintf("ModelPrice: %f, ModelRatio: %f, ... QuotaToPreConsume: %d, ...", ..., p.QuotaToPreConsume, ...) +func (p *PriceData) ToSetting() string { + return fmt.Sprintf("ModelPrice: %f, ModelRatio: %f, ... Quota: %d, QuotaToPreConsume: %d, ...", ..., p.Quota, p.QuotaToPreConsume, ...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/price_data.go` around lines 40 - 41, The ToSetting() debug string for PriceData is missing the new Quota field; update the fmt.Sprintf call inside the ToSetting method to include a placeholder for Quota (e.g., %d or %v as appropriate) and add p.Quota to the argument list (referencing the ToSetting function and the PriceData struct fields like Quota and QuotaToPreConsume) so the returned string shows the per-call billing quota alongside the existing fields.middleware/auth.go (1)
173-188: Populate session context consistently in the session-auth path.Right now the session branch only sets
id. If downstream handlers rely onusername,role,group, or the Auth-Version header (as set inauthHelper), they’ll be missing for dashboard users. Consider mirroring the context setup used byauthHelperto keep behavior consistent.♻️ Suggested adjustment
func TokenOrUserAuth() func(c *gin.Context) { return func(c *gin.Context) { // Try session auth first (dashboard users) session := sessions.Default(c) if id := session.Get("id"); id != nil { - if status, ok := session.Get("status").(int); ok && status == common.UserStatusEnabled { - c.Set("id", id) - c.Next() - return - } + username, uok := session.Get("username").(string) + role, rok := session.Get("role").(int) + status, sok := session.Get("status").(int) + if uok && rok && sok && status == common.UserStatusEnabled && validUserInfo(username, role) { + c.Header("Auth-Version", "864b7076dbcd0a3c01b5520316720ebf") + c.Set("username", username) + c.Set("role", role) + c.Set("id", id) + c.Set("group", session.Get("group")) + c.Set("user_group", session.Get("group")) + c.Set("use_access_token", false) + c.Next() + return + } } // Fall back to token auth (API clients) TokenAuth()(c) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/auth.go` around lines 173 - 188, The TokenOrUserAuth middleware only sets "id" for session-authenticated users; update the session branch in TokenOrUserAuth to mirror authHelper's context population by extracting and setting "username", "role", and "group" (via session.Get like you do for "id") on the gin.Context and also set the Auth-Version response/request header (e.g., "session") so downstream handlers see the same keys and header as token-authenticated requests; keep the existing status/type checks (session.Get("status") == common.UserStatusEnabled) and return after c.Next() as currently implemented.controller/task.go (1)
69-93:tasksToDtomutates the input model objects.Line 87 modifies
task.Usernameon the*model.Taskobjects in the input slice. This is fine if the caller doesn't reuse those model pointers, but it could be surprising. Consider settingUsernameon the DTO instead ifrelay.TaskModel2Dtosupports it, to keep the model objects unmodified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/task.go` around lines 69 - 93, tasksToDto currently mutates the input *model.Task by assigning task.Username, which can surprise callers; instead, call relay.TaskModel2Dto(task) into a dto variable and set dto.Username = user.Username (using the user from userIdMap) so the model objects remain unmodified; update tasksToDto to fill Username on the returned *dto.TaskDto rather than assigning to task.Username and return the list of DTOs.model/midjourney.go (1)
160-164: Duplicate doc comment block.Lines 160–161 and 163–164 repeat the same "UpdateWithStatus performs a conditional UPDATE guarded by fromStatus (CAS)." sentence. Remove one of the two blocks.
✂️ Remove duplicate comment
// UpdateWithStatus performs a conditional UPDATE guarded by fromStatus (CAS). // Returns (true, nil) if this caller won the update, (false, nil) if // another process already moved the task out of fromStatus. -// UpdateWithStatus performs a conditional UPDATE guarded by fromStatus (CAS). // Uses Model().Select("*").Updates() to avoid GORM Save()'s INSERT fallback. func (midjourney *Midjourney) UpdateWithStatus(fromStatus string) (bool, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/midjourney.go` around lines 160 - 164, The doc comment for UpdateWithStatus contains a duplicated sentence; edit the comment block above the UpdateWithStatus method to remove the repeated line ("UpdateWithStatus performs a conditional UPDATE guarded by fromStatus (CAS).") so the comment reads once and still includes the remainder about using Model().Select("*").Updates() to avoid GORM Save()'s INSERT fallback.web/src/components/table/usage-logs/UsageLogsColumnDefs.jsx (1)
377-377: Consider extracting the repeated type-set check into a helper.The condition
record.type === 0 || record.type === 2 || record.type === 5 || record.type === 6is duplicated in 7+ render functions. A small helper (e.g.,const isDataLog = (type) => [0, 2, 5, 6].includes(type)) would reduce repetition and make future type additions less error-prone.Also applies to: 468-468, 491-491, 531-531, 598-598, 632-632, 644-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/usage-logs/UsageLogsColumnDefs.jsx` at line 377, Extract the repeated type-check into a small helper (e.g., const isDataLog = (type) => [0,2,5,6].includes(type)) inside the UsageLogsColumnDefs component file and replace every occurrence of (record.type === 0 || record.type === 2 || record.type === 5 || record.type === 6) in the render functions with isDataLog(record.type); add the helper near the top of web/src/components/table/usage-logs/UsageLogsColumnDefs.jsx (so it’s in scope for the render functions referenced in the diff) and run a quick search/replace for the same pattern at the other affected render sites (the ones noted in the review) to ensure consistency.model/log.go (1)
220-224:GetTokenByIdis a synchronous lookup that may hit Redis/DB.This is called on the billing log write path. If the token cache is cold, this adds latency. The existing
RecordConsumeLogreceivestokenNameas an input parameter instead of looking it up. Consider acceptingtokenNameinRecordTaskBillingLogParamsif callers already have it available, to avoid an extra lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/log.go` around lines 220 - 224, Add a TokenName string field to RecordTaskBillingLogParams and use params.TokenName to set tokenName instead of unconditionally calling GetTokenById; only call GetTokenById(TokenId) as a fallback when params.TokenName is empty and params.TokenId > 0. Update callers of RecordTaskBillingLog (or RecordConsumeLog where applicable) to pass the known tokenName when available so the hot-path avoids the blocking GetTokenById lookup.model/task_cas_test.go (1)
188-207: Variabletshadows*testing.Tinside the goroutine — confusing and error-prone.On Line 191,
t := &Task{}shadows the outert *testing.T. While the goroutine doesn't call anytesting.Tmethods, this makes the code fragile — adding anassertorrequirecall inside the goroutine would silently use the wrongt. Use a different name liketskortask.Rename to avoid shadowing
go func(idx int) { defer wg.Done() - t := &Task{} - *t = Task{ + tsk := &Task{ ID: task.ID, TaskID: task.TaskID, Status: TaskStatusSuccess, Progress: "100%", Quota: task.Quota, Data: json.RawMessage(`{}`), } - t.CreatedAt = task.CreatedAt - t.UpdatedAt = time.Now().Unix() - won, err := t.UpdateWithStatus(TaskStatusInProgress) + tsk.CreatedAt = task.CreatedAt + tsk.UpdatedAt = time.Now().Unix() + won, err := tsk.UpdateWithStatus(TaskStatusInProgress) if err == nil { wins[idx] = won } }(i)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/task_cas_test.go` around lines 188 - 207, The goroutine declares "t := &Task{}" which shadows the outer "t *testing.T"; rename the local task variable (e.g., "tsk" or "localTask") to avoid shadowing and potential misuse, update all subsequent uses in the goroutine (assignments to fields, t.CreatedAt/t.UpdatedAt, and the call to UpdateWithStatus) to the new name, and ensure wins[idx] assignment still uses the boolean returned from that renamed variable's UpdateWithStatus call.web/src/components/table/task-logs/TaskLogsColumnDefs.jsx (1)
295-311: Misleading parameter nameuserIdafterdataIndexchange tousername.The
dataIndexwas changed to'username'(Line 295), so the first render parameter now receives the username string, not a user ID. The parameter is still nameduserIdon Line 296, which is confusing for future readers. It works correctly because Line 300 falls through torecord.username, but the naming is misleading.Suggested rename for clarity
- render: (userId, record, index) => { + render: (text, record, index) => { if (!isAdminUser) { return <></>; } - const displayText = String(record.username || userId || '?'); + const displayText = String(record.username || text || '?');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/task-logs/TaskLogsColumnDefs.jsx` around lines 295 - 311, The render callback for the column with dataIndex 'username' uses a misleading parameter name `userId`; rename that first parameter to something accurate (e.g., `username` or `displayName`) in the render function for this column in TaskLogsColumnDefs.jsx so it reflects the actual value passed, and update any local usages (such as the fallback logic that currently reads `record.username || userId || '?'`) to use the new parameter name (e.g., `record.username || username || '?'`) to keep intent clear.service/task_billing_test.go (2)
109-134: Potential task ID collision inmakeTaskhelper.
TaskIDis generated usingtime.Now().Format("150405.000")which has millisecond resolution. If two calls tomakeTaskhappen within the same millisecond (e.g., in a loop or table-driven test), you'll get duplicate task IDs. Consider adding a counter or random suffix.Use an atomic counter for uniqueness
+var taskSeq atomic.Int64 + func makeTask(userId, channelId, quota, tokenId int, billingSource string, subscriptionId int) *model.Task { return &model.Task{ - TaskID: "task_" + time.Now().Format("150405.000"), + TaskID: fmt.Sprintf("task_test_%d", taskSeq.Add(1)),🤖 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 109 - 134, The TaskID in makeTask is generated with time.Now().Format("150405.000") which can collide at millisecond granularity; modify makeTask to append a unique suffix (e.g., a package-level atomic counter value via atomic.AddUint64 or a random/UUID suffix) to the TaskID to guarantee uniqueness when creating multiple model.Task instances; add the package-level counter (e.g., taskCounter uint64) and use it when building TaskID, and remember to import sync/atomic (or use a UUID library) so makeTask, TaskID and model.Task reliably produce unique IDs in tests.
614-623: Add compile-time interface check formockAdaptor.The mock should include a compile-time assertion to ensure it stays synchronized with the
TaskPollingAdaptorinterface. If the interface changes in the future, the test will fail at compile time rather than silently breaking at runtime.Add this line after the
mockAdaptortype definition:var _ TaskPollingAdaptor = (*mockAdaptor)(nil)This validates that
mockAdaptorimplements all required methods of theTaskPollingAdaptorinterface defined inservice/task_polling.go.🤖 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 614 - 623, Add a compile-time interface assertion to ensure mockAdaptor continues to implement TaskPollingAdaptor: immediately after the mockAdaptor type definition insert a var declaration that assigns a nil *mockAdaptor to the TaskPollingAdaptor interface to force a compile-time check (reference symbols: mockAdaptor and TaskPollingAdaptor).relay/common/relay_info.go (1)
614-627:LockedChannel anytrades type safety for decoupling — acceptable with documented contract.Using
anyto break the import cycle betweenrelay/commonandmodelis a pragmatic choice. The comment on lines 623-626 clearly documents the required type assertion. Consider a future follow-up to introduce a narrow interface if more fields are accessed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/common/relay_info.go` around lines 614 - 627, TaskRelayInfo currently uses LockedChannel any to avoid an import cycle, which loses compile-time type safety; define a small narrow interface (e.g., RelayChannel or ChannelLike) in relay/common that exposes only the methods/fields callers need, change TaskRelayInfo.LockedChannel from any to that interface type, and update call sites to accept/return that interface (or to type-assert to the concrete *model.Channel where necessary); this preserves decoupling while restoring type safety and keeps the documented contract intact.controller/video_proxy.go (1)
127-137:Cache-Controlheader is set after forwarding upstream headers — intentional override.Line 133 correctly uses
Set(notAdd) to override any upstreamCache-Control. The 24-hour cache is reasonable for completed video content. Note that upstreamSet-Cookieor other sensitive headers are also forwarded blindly (Lines 127-131); consider filtering if this proxy is exposed to end users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/video_proxy.go` around lines 127 - 137, The current proxy blindly forwards all headers from resp.Header (loop over resp.Header -> c.Writer.Header().Add) which can leak sensitive upstream headers; update the header copy logic in controller/video_proxy.go (the loop iterating over resp.Header and adding to c.Writer.Header()) to skip sensitive headers such as "Set-Cookie", "Authorization", "WWW-Authenticate", "Proxy-Authenticate", "Proxy-Authorization", and any hop-by-hop headers (e.g., "Connection", "Keep-Alive", "Transfer-Encoding", "TE", "Trailer", "Upgrade"); continue to explicitly set "Cache-Control" with c.Writer.Header().Set("Cache-Control", "public, max-age=86400") after copying allowed headers and then write the status and body as before (io.Copy on resp.Body).relay/channel/task/taskcommon/helpers.go (1)
80-94:BaseBilling.AdjustBillingOnCompletereturns 0 regardless of task success or failure — pre-charged quota is never automatically refunded on failure.Adaptors that should issue a (partial) refund when a task reaches a terminal failure state must override this method. The silent no-op default makes it easy to ship a new adaptor that over-charges users on failure without an obvious compilation error.
Consider documenting this expectation more prominently or providing a helper
FailureRefundBillingbase that returns the full pre-charged amount for failed tasks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/taskcommon/helpers.go` around lines 80 - 94, BaseBilling.AdjustBillingOnComplete silently returns 0 which causes adaptors to never refund pre-charged quota on terminal failures; add a clear expectation and a helper implementation: update the comment above BaseBilling and AdjustBillingOnComplete to state adaptors must override to handle refunds, and add a new FailureRefundBilling type (e.g., type FailureRefundBilling struct{}) that implements AdjustBillingOnComplete(task *model.Task, info *relaycommon.TaskInfo) int to detect terminal failure states on task.Status and return the full pre-charged amount (or appropriate refund amount) so adaptors can embed or use this helper instead of relying on the 0 default.relay/channel/task/doubao/adaptor.go (1)
215-247:UnmarshalMetadataafter buildingContentcan silently discard the prompt/image items.
r.Contentis populated fromreq.Promptandreq.Images, thentaskcommon.UnmarshalMetadata(metadata, &r)merges metadata into the same struct. Because JSON unmarshaling replaces slices when the key is present, any metadata payload that includes"content"will completely overwrite the computedContentslice, silently dropping the prompt and images.If the intent is that metadata values take precedence, document this explicitly. If the intent is that metadata only sets supplemental fields (duration, resolution, etc.), apply the merge before the content-building block so that explicit prompt/image items win.
♻️ Suggested ordering (metadata-as-defaults)
func (a *TaskAdaptor) convertToRequestPayload(req *relaycommon.TaskSubmitReq) (*requestPayload, error) { r := requestPayload{ Model: req.Model, Content: []ContentItem{}, } + + metadata := req.Metadata + if err := taskcommon.UnmarshalMetadata(metadata, &r); err != nil { + return nil, errors.Wrap(err, "unmarshal metadata failed") + } // Add text prompt if req.Prompt != "" { r.Content = append(r.Content, ContentItem{ Type: "text", Text: req.Prompt, }) } // Add images if present if req.HasImage() { for _, imgURL := range req.Images { r.Content = append(r.Content, ContentItem{ Type: "image_url", ImageURL: &ImageURL{URL: imgURL}, }) } } - metadata := req.Metadata - if err := taskcommon.UnmarshalMetadata(metadata, &r); err != nil { - return nil, errors.Wrap(err, "unmarshal metadata failed") - } return &r, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/doubao/adaptor.go` around lines 215 - 247, In convertToRequestPayload the call to taskcommon.UnmarshalMetadata(&r) runs after r.Content is built, so metadata containing "content" can overwrite and drop the prompt/images; to fix, run taskcommon.UnmarshalMetadata(metadata, &r) before you append prompt/images (or unmarshal into a temp struct and merge non-content fields), ensuring that req.Prompt and req.Images are appended after metadata merge so explicit prompt/image items take precedence (refer to function TaskAdaptor.convertToRequestPayload, variable r, r.Content, taskcommon.UnmarshalMetadata, req.Prompt and req.Images).service/task_billing.go (1)
145-175: Early return on funding refund failure leaves token quota un-refunded.On line 154, if
taskAdjustFundingfails, the function returns without restoring token quota (line 158). This is a deliberate fail-fast choice, but it means the token's remaining quota stays lower than it should. Since the funding source adjustment also failed, the system is at least consistently "un-refunded" rather than partially refunded.Worth documenting this trade-off in the comment for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_billing.go` around lines 145 - 175, The current RefundTaskQuota returns immediately when taskAdjustFunding fails, leaving token quota un-refunded; update RefundTaskQuota so that if taskAdjustFunding(task, -quota) returns an error it still calls taskAdjustTokenQuota(ctx, task, -quota) before returning (and keep the logger.LogWarn call), or alternatively document the deliberate trade-off in a clear comment above RefundTaskQuota; reference taskAdjustFunding, taskAdjustTokenQuota and RefundTaskQuota when making the change.relay/relay_task.go (3)
254-273: Reverse-dividing integer-truncated quota is lossy and order-dependent.
baseQuotawas produced byint(float64(…) * ra)(truncation). Dividing that truncated value back byradoes not recover the exact original — each step can drift by ±1 quota unit, and Go maps iterate in random order. Additionally, if a new ratio is0, the result silently becomes0.A more robust approach would be to store the base quota (before
OtherRatios) inPriceDataso thatrecalcQuotaFromRatioscan use it directly instead of reverse-engineering it.♻️ Minimal guard against zero ratios
// 应用新的 ratios result := float64(baseQuota) for _, ra := range ratios { - if ra != 1.0 { + if ra != 1.0 && ra > 0 { result *= ra } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 254 - 273, recalcQuotaFromRatios currently reverse-divides an integer-truncated quota (info.PriceData.Quota) by OtherRatios which is lossy and order-dependent; change the implementation to read a dedicated base quota field (e.g., add and use PriceData.BaseQuota) that stores the quota before OtherRatios instead of reverse-engineering, apply the new ratios multiplicatively to that base (skip or treat a ratio of 0 explicitly to avoid silent zeroing), and keep the function name recalcQuotaFromRatios and parameter types unchanged so callers need no update.
470-472: Silently discardingUpdateWithStatuserrors and CAS outcome.If the CAS update fails (lost race or DB error), the response still contains the freshly fetched data that was never persisted. The caller and downstream user receive a response that doesn't match the stored task state. Consider at least logging the error.
♻️ Suggested improvement
if !snap.Equal(task.Snapshot()) { - _, _ = task.UpdateWithStatus(snap.Status) + if won, err := task.UpdateWithStatus(snap.Status); err != nil { + common.SysLog(fmt.Sprintf("tryRealtimeFetch: UpdateWithStatus failed for task %s: %v", task.TaskID, err)) + } else if !won { + common.SysLog(fmt.Sprintf("tryRealtimeFetch: CAS lost for task %s, response may be stale", task.TaskID)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 470 - 472, The code silently discards the result and error from task.UpdateWithStatus when snapshot CAS fails; change the call in relay_task.go to capture both the returned value and error from task.UpdateWithStatus(snap.Status) and handle them: check the CAS outcome (returned boolean/result) and if it indicates failure or if an error is non-nil, log the failure with contextual details (task ID, expected vs actual status) and propagate or convert the error into the response path as appropriate so callers don't receive an unpersisted state; ensure you reference the existing task variable and its UpdateWithStatus and Snapshot() methods when making this change.
190-197: Sequentialint()truncation may compound rounding errors across multiple ratios.Each iteration truncates the intermediate quota to
int, losing fractional cents. If multipleOtherRatiosexist (e.g.,seconds=1.5andsize=1.666667), the order in which map keys are iterated (non-deterministic in Go) can produce different results. Consider accumulating the product of all ratios first, then applying a singleint()cast:♻️ Suggested fix
if !common.StringsContains(constant.TaskPricePatches, modelName) { - for _, ra := range info.PriceData.OtherRatios { - if ra != 1.0 { - info.PriceData.Quota = int(float64(info.PriceData.Quota) * ra) - } - } + multiplier := 1.0 + for _, ra := range info.PriceData.OtherRatios { + multiplier *= ra + } + if multiplier != 1.0 { + info.PriceData.Quota = int(float64(info.PriceData.Quota) * multiplier) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 190 - 197, The loop in relay_task.go applies int() on quota inside each iteration over info.PriceData.OtherRatios, causing cumulative truncation and non-deterministic results; instead, compute a single cumulative multiplier by iterating over info.PriceData.OtherRatios (e.g., multiply into a float64 accumulator only when ra != 1.0), then after the loop apply info.PriceData.Quota = int(float64(info.PriceData.Quota) * cumulativeMultiplier) in the branch guarded by common.StringsContains(constant.TaskPricePatches, modelName) to perform one final cast and avoid incremental rounding error and order-dependency.service/task_polling.go (2)
320-320: Debug log may emit entire video response bodies (including base64 data).
string(responseBody)is logged before redaction. For Gemini/Vertex responses containing base64-encoded video, this could write megabytes per task per poll cycle into debug logs.Consider logging only a truncated preview or logging after redaction:
♻️ Suggested fix
- logger.LogDebug(ctx, fmt.Sprintf("updateVideoSingleTask response: %s", string(responseBody))) + logger.LogDebug(ctx, fmt.Sprintf("updateVideoSingleTask response (%d bytes): %s", len(responseBody), truncateBase64(string(responseBody))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` at line 320, The debug log in updateVideoSingleTask currently logs the full response body via logger.LogDebug(fmt.Sprintf("updateVideoSingleTask response: %s", string(responseBody))), which can include large base64 video data; change this to either log a redacted/truncated preview (e.g., first N bytes + "...") or apply the existing redaction routine before logging, and ensure you use the same symbol logger.LogDebug and the updateVideoSingleTask response handling path so only a small, safe preview is emitted (or the redacted payload) instead of the entire responseBody.
354-394: Unrecognized upstream status causes an error return without updating the task.The
defaultbranch (line 392-393) returns an error for any status string not in the switch. If an upstream provider introduces a new intermediate status (e.g.,"VALIDATING"), the polling loop will log errors every 15 seconds for that task until the status changes, without updating progress. Consider mapping unknown non-terminal statuses toTaskStatusInProgressinstead.♻️ Suggested improvement
default: - return fmt.Errorf("unknown task status %s for task %s", taskResult.Status, task.TaskID) + logger.LogWarn(ctx, fmt.Sprintf("Unknown task status %q for task %s, treating as in-progress", taskResult.Status, task.TaskID)) + task.Status = model.TaskStatus(model.TaskStatusInProgress) + if task.StartTime == 0 { + task.StartTime = now + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` around lines 354 - 394, The switch on taskResult.Status currently returns an error in the default branch causing noisy logs for unknown intermediate statuses; change the default handling so that unknown non-terminal statuses do not return an error but instead set task.Status = model.TaskStatusInProgress and task.Progress = taskcommon.ProgressInProgress (and set task.StartTime if zero, like the model.TaskStatusInProgress case), while preserving the existing error return only for truly unexpected/invalid terminal values if you can detect them; update the branch that currently returns fmt.Errorf("unknown task status %s for task %s", ...) to instead treat unknown statuses as in-progress by updating task.Progress/Status (referencing taskResult.Status, task.Status, task.Progress, model.TaskStatusInProgress, and taskcommon.ProgressInProgress).
| if saved, ok := c.Get("_original_multipart_ct"); ok { | ||
| contentType = saved.(string) | ||
| } else { | ||
| contentType = c.Request.Header.Get("Content-Type") | ||
| c.Set("_original_multipart_ct", contentType) | ||
| } |
There was a problem hiding this comment.
Unsafe bare type assertion will panic on unexpected stored type.
saved.(string) at Line 250 panics if anything other than a string is stored under "_original_multipart_ct". Use the comma-ok form and fall back to the header read path on failure.
🛡️ Proposed fix
- if saved, ok := c.Get("_original_multipart_ct"); ok {
- contentType = saved.(string)
- } else {
+ if saved, ok := c.Get("_original_multipart_ct"); ok {
+ if s, ok := saved.(string); ok {
+ contentType = s
+ } else {
+ contentType = c.Request.Header.Get("Content-Type")
+ }
+ } else {
contentType = c.Request.Header.Get("Content-Type")
c.Set("_original_multipart_ct", contentType)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/gin.go` around lines 249 - 254, The code in the handler using
gin.Context (variable c) unsafely asserts saved.(string) for the key
"_original_multipart_ct" which can panic if the stored value is not a string;
change this to use the comma-ok form (v, ok := saved.(string)) and if ok is
false fall back to reading c.Request.Header.Get("Content-Type") and store that
string into c.Set("_original_multipart_ct", contentType) so the function uses
the header path on failure instead of panicking.
| task.Data = result.TaskData | ||
| task.Action = relayInfo.Action | ||
| if insertErr := task.Insert(); insertErr != nil { | ||
| common.SysError("insert task error: " + insertErr.Error()) | ||
| } |
There was a problem hiding this comment.
Task record insertion failure is silently swallowed — the user is billed but the task is untracked.
service.SettleBilling (line 557) and the adaptor's DoResponse (which already wrote 200 OK to the client) both succeed before task.Insert() is called. If task.Insert() fails, the quota has been consumed, the upstream task is running, but the task record does not exist — the user can never query or cancel it. The error is only logged to sys-error with no compensating action.
Consider persisting the task record before settling billing (or at least retrying the insert asynchronously), so a billing commit only happens when the task is durably stored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/relay.go` around lines 576 - 580, The task record insertion
(task.Insert()) is performed after service.SettleBilling and adaptor.DoResponse,
so failures can consume quota while leaving the task untracked; move durable
persistence earlier or add a compensating flow: persist the task (call
task.Insert()) before calling service.SettleBilling and before writing the final
response in adaptor.DoResponse, and if immediate pre-billing insert fails
implement a retry/enqueue mechanism (or abort/rollback the billing action) and
surface a server error instead of silently logging via common.SysError; update
the code paths around service.SettleBilling, adaptor.DoResponse, and
task.Insert() to ensure billing only commits after a successful durable insert
(or that failed inserts trigger async retry and billing rollback).
| case constant.ChannelTypeOpenAI, constant.ChannelTypeSora: | ||
| videoURL = fmt.Sprintf("%s/v1/videos/%s/content", baseURL, task.TaskID) | ||
| videoURL = fmt.Sprintf("%s/v1/videos/%s/content", baseURL, task.GetUpstreamTaskID()) | ||
| req.Header.Set("Authorization", "Bearer "+channel.Key) | ||
| default: | ||
| // Video URL is directly in task.FailReason | ||
| videoURL = task.FailReason | ||
| // Video URL is stored in PrivateData.ResultURL (fallback to FailReason for old data) | ||
| videoURL = task.GetResultURL() | ||
| } |
There was a problem hiding this comment.
Default branch: GetResultURL() fallback to FailReason may attempt to fetch non-URL content.
For the default channel type, task.GetResultURL() falls back to FailReason when PrivateData.ResultURL is empty (old data). If FailReason contains error text rather than a URL, url.Parse on Line 105 may succeed (many strings parse as relative URLs), leading to a request to an unintended destination. The error is caught downstream, but consider validating that the URL is absolute before proceeding.
Suggested validation
default:
// Video URL is stored in PrivateData.ResultURL (fallback to FailReason for old data)
videoURL = task.GetResultURL()
+ if videoURL == "" {
+ videoProxyError(c, http.StatusNotFound, "invalid_request_error", "No video URL available for this task")
+ return
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/video_proxy.go` around lines 97 - 103, The default branch uses
task.GetResultURL() (which may return task.FailReason for old data) and then
proceeds to parse and request it; instead validate that the returned URL is an
absolute URL before using it: after obtaining videoURL from task.GetResultURL()
in controller/video_proxy.go (the default case), call url.Parse(videoURL) and
ensure u.IsAbs() and u.Scheme is http/https (or otherwise acceptable); if the
parse fails or the URL is not absolute, return a clear error/HTTP 400 response
(or fail the handler) rather than attempting the downstream request so we never
fetch error text or relative paths. Ensure the validation occurs before any use
of url.Parse results or the request creation to prevent unintended destinations.
| type TaskData interface { | ||
| SunoDataResponse | []SunoDataResponse | string | any | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the current state of dto/task.go
cat -n dto/task.go | head -60Repository: QuantumNous/new-api
Length of output: 2092
🏁 Script executed:
# Check TaskResponse instantiations across the codebase
rg -n 'TaskResponse\[' --type=go -C2Repository: QuantumNous/new-api
Length of output: 3597
🏁 Script executed:
# Also search for TaskResponse usage more broadly
rg -n 'TaskResponse' --type=go | head -30Repository: QuantumNous/new-api
Length of output: 2254
🏁 Script executed:
# Find model.Task definition
rg -n 'type Task struct' --type=go model/ | head -5Repository: QuantumNous/new-api
Length of output: 98
🏁 Script executed:
# Check if there are more type parameters used with TaskResponse that violate the constraint
rg -n 'TaskResponse\[' --type=go | grep -o '\[.*\]' | sort -uRepository: QuantumNous/new-api
Length of output: 148
TaskData constraint is ineffective and violated by actual usage.
Including any in the union makes the constraint equivalent to just any—every type satisfies it, so the listed types are dead code. Additionally, the constraint is violated by actual instantiations: TaskResponse[model.Task] (at service/task_polling.go:326) uses a type not listed in the union.
Either:
- Remove
anyand update the constraint to include all actually-used types:SunoDataResponse | []SunoDataResponse | string | model.Task(and handle[]anyexplicitly), or - Remove the constraint entirely and use just
anyif type flexibility is desired.
A TODO comment does not address the underlying design issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dto/task.go` around lines 16 - 18, The TaskData type constraint currently
includes `any`, making the union meaningless and also doesn't cover actual
usages like `TaskResponse[model.Task]`; remove the `TODO` and either (A) remove
`any` and expand the union to the real set of allowed payloads (for example:
`SunoDataResponse | []SunoDataResponse | string | model.Task` and add explicit
handling for `[]any` if needed), or (B) drop the generic constraint entirely and
make TaskData `any` if you want full flexibility; update the `TaskData`
declaration (symbol: TaskData) accordingly and ensure instantiations such as
`TaskResponse[model.Task]` match the revised constraint.
| func (t *Task) GetResultURL() string { | ||
| if t.PrivateData.ResultURL != "" { | ||
| return t.PrivateData.ResultURL | ||
| } | ||
| return t.FailReason | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "GetResultURL" --type=go -C3Repository: QuantumNous/new-api
Length of output: 2899
🏁 Script executed:
# Also check what video_proxy.go looks like
fd video_proxy.go --type=fRepository: QuantumNous/new-api
Length of output: 88
🏁 Script executed:
# Find where FailReason is assigned to understand its typical content
rg -n "FailReason\s*=" --type=go -C2 | head -80Repository: QuantumNous/new-api
Length of output: 3287
🏁 Script executed:
cat -n controller/video_proxy.go | sed -n '95,120p'Repository: QuantumNous/new-api
Length of output: 1232
🏁 Script executed:
# Also check if there's any validation logic before or after calling url.Parse
cat -n controller/video_proxy.go | sed -n '95,130p'Repository: QuantumNous/new-api
Length of output: 1678
GetResultURL fallback to FailReason could return non-URL content.
For old data where PrivateData.ResultURL is empty, this falls back to FailReason, which contains error text (e.g., "upstream error", timeout messages) rather than actual URLs. While video_proxy.go has error handling for url.Parse() failures, other callers like service/task_polling.go assign the result directly to response fields without validation. Consider adding a check (e.g., validating the scheme or format) before returning from GetResultURL(), or explicitly document to callers that the fallback may contain non-URL content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/task.go` around lines 131 - 136, Task.GetResultURL currently returns
FailReason when PrivateData.ResultURL is empty, which can return non-URL text;
change GetResultURL to validate that the chosen value is a well-formed URL
(e.g., parse with url.Parse and ensure Scheme is "http" or "https") and only
return it if valid, otherwise return an empty string (or explicit sentinel) so
callers like service/task_polling.go do not receive arbitrary error text as a
URL; reference Task.GetResultURL, Task.PrivateData.ResultURL and Task.FailReason
when making the change.
| oldData, _ := common.Marshal(oldTask.Data) | ||
| newData, _ := common.Marshal(newTask.Data) | ||
|
|
||
| sort.Slice(oldData, func(i, j int) bool { | ||
| return oldData[i] < oldData[j] | ||
| }) | ||
| sort.Slice(newData, func(i, j int) bool { | ||
| return newData[i] < newData[j] | ||
| }) | ||
|
|
||
| if string(oldData) != string(newData) { | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
Byte-sorting JSON payloads is not a valid equality check.
Sorting individual bytes of a JSON string destroys its structure. Two semantically different JSON documents that happen to share the same byte histogram would compare as equal, causing the update to be skipped. For example, {"ab":1} and {"ba":1} would be considered identical.
A simpler and correct approach is to compare the raw bytes directly (both come from common.Marshal so key ordering should be deterministic), or use bytes.Equal:
♻️ Suggested fix
- oldData, _ := common.Marshal(oldTask.Data)
- newData, _ := common.Marshal(newTask.Data)
-
- sort.Slice(oldData, func(i, j int) bool {
- return oldData[i] < oldData[j]
- })
- sort.Slice(newData, func(i, j int) bool {
- return newData[i] < newData[j]
- })
-
- if string(oldData) != string(newData) {
+ oldData, _ := common.Marshal(oldTask.Data)
+ newData, _ := common.Marshal(newTask.Data)
+ if string(oldData) != string(newData) {
return true
}📝 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.
| oldData, _ := common.Marshal(oldTask.Data) | |
| newData, _ := common.Marshal(newTask.Data) | |
| sort.Slice(oldData, func(i, j int) bool { | |
| return oldData[i] < oldData[j] | |
| }) | |
| sort.Slice(newData, func(i, j int) bool { | |
| return newData[i] < newData[j] | |
| }) | |
| if string(oldData) != string(newData) { | |
| return true | |
| } | |
| return false | |
| oldData, _ := common.Marshal(oldTask.Data) | |
| newData, _ := common.Marshal(newTask.Data) | |
| if string(oldData) != string(newData) { | |
| return true | |
| } | |
| return false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/task_polling.go` around lines 221 - 234, The code currently sorts the
raw JSON byte slices (oldData, newData) then compares strings, which corrupts
JSON structure; instead remove the sort.Slice calls and replace the comparison
with a direct byte-equality check (e.g., use bytes.Equal(oldData, newData) or
compare the raw []byte/strings returned by common.Marshal), keeping the existing
common.Marshal calls and handling any marshal errors if needed; update the
equality branch in the same function where oldData and newData are defined to
return bytes.Equal(oldData, newData) (or !bytes.Equal -> return true) rather
than sorting the slices.
…keys across multiple languages
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/components/table/task-logs/TaskLogsColumnDefs.jsx (1)
389-411:⚠️ Potential issue | 🟡 MinorComment claims old-data fallback; code doesn't implement it — potential silent regression.
The comment on Line 389 (
兼容旧数据 fail_reason 中的 URL) implies backward compatibility with task records that stored a video URL infail_reasonrather thanresult_url. However,hasResultUrlonly inspectsrecord.result_url. Any historical record with a URL only infail_reasonwill silently fall through to the plain-text path and never render the video preview link.If backward compat is truly intended, a fallback is needed; otherwise remove the misleading comment.
🛡️ Proposed fix (if backward compat is desired)
- const resultUrl = record.result_url; - const hasResultUrl = typeof resultUrl === 'string' && /^https?:\/\//.test(resultUrl); - if (isSuccess && isVideoTask && hasResultUrl) { + const isUrl = (s) => typeof s === 'string' && /^https?:\/\//.test(s); + const resultUrl = isUrl(record.result_url) + ? record.result_url + : isUrl(record.fail_reason) + ? record.fail_reason + : null; + if (isSuccess && isVideoTask && resultUrl) {If backward compat is not needed, just remove the stale comment on Line 389.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/task-logs/TaskLogsColumnDefs.jsx` around lines 389 - 411, The comment promises a fallback for old records with a URL in fail_reason but the code only checks record.result_url; update the logic so resultUrl is computed as record.result_url || parsedUrlFromFailReason, where parsedUrlFromFailReason extracts an http(s) URL from record.fail_reason using the same regex; then change hasResultUrl to test that computed resultUrl and continue to call openVideoModal(resultUrl) when present (and update/remove the comment accordingly if you intend to drop backward compatibility).relay/relay_task.go (1)
281-301:⚠️ Potential issue | 🔴 CriticalBug: missing
returnafter error assignment causes nil-pointer panic.When
respBuilderis not found in the map (!okat line 283),taskRespis set but execution falls through to line 287 whererespBuilder(c)is called on anilfunction, causing a panic.Proposed fix
func RelayTaskFetch(c *gin.Context, relayMode int) (taskResp *dto.TaskError) { respBuilder, ok := fetchRespBuilders[relayMode] if !ok { taskResp = service.TaskErrorWrapperLocal(errors.New("invalid_relay_mode"), "invalid_relay_mode", http.StatusBadRequest) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 281 - 301, In RelayTaskFetch, when fetchRespBuilders[relayMode] is missing you assign taskResp but then continue and call respBuilder(c) which is nil; change the !ok branch (fetchRespBuilders, respBuilder, taskResp) to return immediately after setting taskResp (i.e., set taskResp = service.TaskErrorWrapperLocal(...) and return) so the function does not call respBuilder on a nil value and avoids the panic.relay/channel/task/hailuo/adaptor.go (1)
286-302:⚠️ Potential issue | 🟡 MinorRemove unused utility functions
containsandcontainsInt.These functions are defined but never invoked anywhere in the codebase. If similar functionality is needed in the future, use Go's standard library
slices.Contains(Go 1.21+) instead of custom implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/hailuo/adaptor.go` around lines 286 - 302, Remove the unused helper functions contains and containsInt from adaptor.go: delete the functions named contains(slice []string, item string) and containsInt(slice []int, item int) since they are not referenced anywhere; if similar functionality is later required prefer using slices.Contains from the standard library (Go 1.21+) instead of adding new custom implementations.
🧹 Nitpick comments (8)
web/src/components/table/task-logs/TaskLogsColumnDefs.jsx (1)
295-314: StaleuserIdparameter name and redundant fallback afterdataIndexchange.Since
dataIndexis now'username', the first render parameter already receivesrecord.username. Naming ituserIdis misleading, andrecord.username || userIdis redundant — both resolve to the same value.♻️ Proposed cleanup
- render: (userId, record, index) => { + render: (username, record, index) => { if (!isAdminUser) { return <></>; } - const displayText = String(record.username || userId || '?'); + const displayText = String(username || '?');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/task-logs/TaskLogsColumnDefs.jsx` around lines 295 - 314, The render function for the column still names its first param userId and does a redundant fallback using record.username || userId despite dataIndex: 'username' supplying username as the first arg; rename the first render parameter to username (or displayName) and remove the redundant record.username || userId fallback so displayText is derived directly from username (with the existing fallback of '?'), updating any usages inside render (e.g., displayText, Avatar content, and stringToColor call) to use the new parameter name and eliminate record.username references.relay/channel/task/ali/adaptor.go (1)
328-341: Metadata unmarshalling into the fullAliVideoRequestis broadly permissive.
common.Unmarshal(metadataBytes, aliReq)can overwrite any field ofAliVideoRequestfrom user-provided metadata (e.g.,input.prompt,parameters.*). The only guard is the model-change check at line 339. If the intent is to restrict metadata to just theAliMetadatasubset, consider unmarshalling into a more constrained target. If the broad override is intentional, the current approach works but could benefit from a brief comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/ali/adaptor.go` around lines 328 - 341, The metadata is being unmarshalled directly into AliVideoRequest (aliReq) which lets user metadata overwrite any field; instead unmarshal metadataBytes into a constrained AliMetadata struct and then copy only allowed fields (e.g., Prompt, Parameters, other explicitly permitted fields) into aliReq, leaving model untouched; keep the existing model-check against upstreamModel and return the same errors on marshal/unmarshal failures; if the broad override was intentional, add a short comment above the common.Unmarshal explaining that behavior and why it's safe.controller/task.go (1)
69-93:tasksToDtois clean but mutates the input task objects.Line 89 sets
task.Usernamedirectly on the*model.Taskpointer from the input slice. While theUsernamefield hasgorm:"-"(non-persisted), mutating shared model objects can cause subtle issues if callers reuse them. Since these are fetched fresh from the DB and not reused, this is fine in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/task.go` around lines 69 - 93, tasksToDto currently mutates the input *model.Task by assigning task.Username inside the loop; instead avoid changing the original model by creating a local copy or by supplying the username to the DTO conversion. Update tasksToDto to, for each task where a username is found, make a shallow copy (e.g. copy := *task) and set copy.Username = user.Username, then call relay.TaskModel2Dto(©); or change relay.TaskModel2Dto to accept an optional username parameter and pass it through—ensure you only modify the copied instance so the original tasks slice remains unchanged.service/task_polling.go (1)
38-85:TaskPollingLoopruns an infinite loop with a fixed 15-second sleep — no graceful shutdown.There's no mechanism to stop the loop (e.g., via
context.Done()or a stop channel). If the application starts a shutdown, this goroutine will hang untiltime.Sleepcompletes and a new iteration begins. This is a minor operational concern for now, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/task_polling.go` around lines 38 - 85, TaskPollingLoop currently spins forever with time.Sleep and has no shutdown hook; change it to accept a context.Context (or use a passed-in stop channel) and replace the for { time.Sleep(...) ... } with a select loop that listens for ctx.Done() and a time.Ticker or time.After case so the goroutine can exit immediately on cancellation; ensure you propagate the context into calls like logger.LogError/logger.LogInfo, model.TaskBulkUpdateByID and DispatchPlatformUpdate so ongoing work can be cancelled/observed and the function returns when ctx.Done() is closed.relay/channel/task/gemini/adaptor.go (1)
251-252:ti.TaskIDis set but appears unused by the polling caller.
ParseTaskResultsetsti.TaskIDto the encoded operation name, but the polling loop inupdateVideoSingleTaskdoesn't readtaskResult.TaskID. This is harmless but dead data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/gemini/adaptor.go` around lines 251 - 252, ParseTaskResult assigns ti.TaskID = taskcommon.EncodeLocalTaskID(op.Name) but the polling loop in updateVideoSingleTask never reads taskResult.TaskID, so this is dead data; remove the unused assignment in ParseTaskResult (or alternatively, if intended to be used, update updateVideoSingleTask to read taskResult.TaskID) — search for the assignment to ti.TaskID in ParseTaskResult and either delete that line or wire taskResult.TaskID into updateVideoSingleTask's polling logic so the value is actually consumed.relay/relay_task.go (2)
415-494:tryRealtimeFetchsilently swallows all errors — consider logging for observability.Every error path returns
nilsilently, which is correct for the fallback behavior (caller proceeds with cached state), but makes debugging production issues difficult. Consider addingcommon.SysLogor similar at key failure points (e.g., channel fetch failure, adaptor fetch failure, parse failure).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 415 - 494, The function tryRealtimeFetch currently returns nil on all error paths without logging; add diagnostic logs at key failure points to improve observability: log errors from model.GetChannelById (include task.ChannelId), when adaptor is nil (include channelModel.Type), on adaptor.FetchTask failures (log error and response status/URL if present), on io.ReadAll failures, and on adaptor.ParseTaskResult failures (include response body or parse error); use common.SysLog or processLogger with context (task.TaskID, task.GetUpstreamTaskID(), channelModel.Key) before each early return so failures are visible while preserving the same fallback behavior.
191-197: Quota multiplication loop applies ratios cumulatively but skips1.0via exact float comparison.The
ra != 1.0check (line 193) works because the ratios are set from literal values (e.g.,1.666667,float64(seconds)). However, if any future ratio is computed from division or other floating-point arithmetic, it might not be exactly1.0. This is consistent with the same pattern inrecalcQuotaFromRatios(line 268), so flagging as a minor note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 191 - 197, The loop that multiplies info.PriceData.Quota by each ra in info.PriceData.OtherRatios currently skips only exact 1.0 via `ra != 1.0`, which is brittle for computed floats; change the check to use an epsilon comparison (e.g., math.Abs(ra-1.0) > epsilon) so near-1.0 ratios are treated as 1.0 and not applied. Update the same pattern used in recalcQuotaFromRatios to use the same epsilon constant and reference the same symbols: info.PriceData.OtherRatios, info.PriceData.Quota, and constant.TaskPricePatches.relay/channel/task/hailuo/adaptor.go (1)
162-164: Usetaskcommon.UnmarshalMetadata(req.Metadata, &videoRequest)for consistency with other adaptors in this PR.The vidu, kling, and jimeng adaptors all use the
taskcommon.UnmarshalMetadatahelper function, which is designed to replace the directreq.UnmarshalMetadatapattern. Align hailuo with this refactored approach.Suggested fix
- if err := req.UnmarshalMetadata(&videoRequest); err != nil { + if err := taskcommon.UnmarshalMetadata(req.Metadata, &videoRequest); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/hailuo/adaptor.go` around lines 162 - 164, Replace the direct call to req.UnmarshalMetadata with the shared helper to match other adaptors: use taskcommon.UnmarshalMetadata(req.Metadata, &videoRequest) instead of req.UnmarshalMetadata(&videoRequest), handle the returned error the same way (wrap with "unmarshal metadata to video request failed"), and keep the same variables (req and videoRequest) so behavior remains identical to vidu/kling/jimeng adaptors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/task.go`:
- Around line 187-193: InitTask currently dereferences relayInfo later
(relayInfo.TaskRelayInfo, relayInfo.UserId) while only partially guarding it
earlier, causing a potential nil-pointer panic; add an early nil-check at the
start of InitTask that returns an error or sets sensible defaults when relayInfo
== nil (or alternatively document and enforce non-nil), and update usages of
TaskRelayInfo and UserId to be inside that guard (symbols: InitTask, relayInfo,
TaskRelayInfo, UserId, GenerateTaskID) so GenerateTaskID is only used when
needed and no nil deref can occur.
In `@relay/channel/task/vertex/adaptor.go`:
- Around line 369-371: The current check using task.GetResultURL() will never
catch Vertex video data because PrivateData.ResultURL never stores data: URIs;
instead, parse the raw Vertex response from task.Data as other video adaptors
do: unmarshal task.Data into the Vertex response struct (the same shape used by
ParseTaskResult), extract the base64 data: URL from the parsed object's Url
field, and call v.SetMetadata("url", extractedUrl). Update the adaptor.go logic
that currently checks GetResultURL() to instead inspect task.Data (and reference
types/functions such as task.Data, ParseTaskResult, GetResultURL, and
v.SetMetadata) so successful Vertex tasks expose the video data in metadata.
In `@relay/relay_task.go`:
- Around line 254-273: recalcQuotaFromRatios loses precision by trying to
recover the original base quota via repeated integer division (causing drift
compared to RelayTaskSubmit's sequential int truncations); instead add and use
an explicit pre-OtherRatios base quota stored on PriceData (e.g.,
PriceData.BaseQuota) so recalcQuotaFromRatios can use that exact integer base
rather than dividing back, and update RelayTaskSubmit to set PriceData.BaseQuota
when computing the original quota so all downstream calls
(recalcQuotaFromRatios) use the exact stored base quota.
In `@service/task_polling.go`:
- Around line 383-387: The code assigns taskResult.Reason directly into
task.FailReason which can be a data: URI; before assignment in the block where
task.FinishTime is set and logger.LogInfo is called, check the value of
taskResult.Reason and only assign it to task.FailReason if it does not start
with "data:" (use strings.HasPrefix(taskResult.Reason, "data:")), otherwise set
task.FailReason to a safe placeholder like "[redacted data]" or leave it
unchanged; ensure you import or reference strings and keep the existing
logger.LogInfo call using the sanitized task.FailReason.
- Around line 318-320: The debug log currently prints the full responseBody in
updateVideoSingleTask via logger.LogDebug, which can dump large base64 video
payloads; replace the direct string(responseBody) with a sanitized version using
the existing redactVideoResponseBody helper (or truncate to a safe length)
before logging so logs do not contain full binary/base64 data.
- Around line 326-340: The New API branch assigns unredacted t.Data to task.Data
while the adaptor branch applies redactVideoResponseBody; update the New API
branch (where responseItems / dto.TaskResponse[model.Task] is handled) to run
the parsed data through redactVideoResponseBody before storing it (i.e., set
task.Data = redactVideoResponseBody(t.Data) or otherwise redact the same
fields), ensuring the redaction function is applied consistently for both the
responseItems branch and the adaptor.ParseTaskResult branch so no base64/video
fields are stored unredacted.
---
Outside diff comments:
In `@relay/channel/task/hailuo/adaptor.go`:
- Around line 286-302: Remove the unused helper functions contains and
containsInt from adaptor.go: delete the functions named contains(slice []string,
item string) and containsInt(slice []int, item int) since they are not
referenced anywhere; if similar functionality is later required prefer using
slices.Contains from the standard library (Go 1.21+) instead of adding new
custom implementations.
In `@relay/relay_task.go`:
- Around line 281-301: In RelayTaskFetch, when fetchRespBuilders[relayMode] is
missing you assign taskResp but then continue and call respBuilder(c) which is
nil; change the !ok branch (fetchRespBuilders, respBuilder, taskResp) to return
immediately after setting taskResp (i.e., set taskResp =
service.TaskErrorWrapperLocal(...) and return) so the function does not call
respBuilder on a nil value and avoids the panic.
In `@web/src/components/table/task-logs/TaskLogsColumnDefs.jsx`:
- Around line 389-411: The comment promises a fallback for old records with a
URL in fail_reason but the code only checks record.result_url; update the logic
so resultUrl is computed as record.result_url || parsedUrlFromFailReason, where
parsedUrlFromFailReason extracts an http(s) URL from record.fail_reason using
the same regex; then change hasResultUrl to test that computed resultUrl and
continue to call openVideoModal(resultUrl) when present (and update/remove the
comment accordingly if you intend to drop backward compatibility).
---
Duplicate comments:
In `@controller/relay.go`:
- Around line 556-581: The current flow settles billing (service.SettleBilling)
before persisting the task (task.Insert), so a failed Insert leaves users
charged but the task untracked; move the persistence step before settling
billing: create the task via model.InitTask(...) and call task.Insert() prior to
service.SettleBilling(...) and service.LogTaskConsumption(...), preserving the
same task.PrivateData assignments (UpstreamTaskID, BillingSource,
SubscriptionId, TokenId, BillingContext, Quota, Data, Action) so the billing
uses the exact task info; alternatively, if reordering is not possible,
implement a compensating rollback by catching insertErr and invoking a
refund/rollback API (e.g., service.RefundBilling or a new rollback method) and
logging the failure so billing and task state remain consistent.
In `@model/task.go`:
- Around line 131-136: GetResultURL currently falls back to FailReason which can
be plain error text; change GetResultURL on Task to validate that the fallback
is a real URL (e.g., parse the string with net/url and accept only http/https
schemes) and return an empty string when validation fails so callers (such as
the task polling code that expects a URL) don't receive non-URL content; update
the method (and its comment) to document that it returns a non-empty URL only
when PrivateData.ResultURL is present or the fallback parses as a valid
http/https URL.
- Around line 139-142: GenerateTaskID currently discards the error from
common.GenerateRandomCharsKey(32) which can produce an empty key; change
GenerateTaskID to propagate the failure instead of ignoring it by updating its
signature to return (string, error), call common.GenerateRandomCharsKey and if
it returns an error return "", err, otherwise return "task_"+key, and update all
callers of GenerateTaskID to handle the returned error; alternatively, if
changing the signature is infeasible, handle the error inside GenerateTaskID by
retrying key generation a few times and returning a deterministic fallback (or
logging and returning an error) rather than silently returning "task_".
In `@relay/channel/task/sora/adaptor.go`:
- Around line 167-201: The multipart reconstruction currently ignores errors
from writer.WriteField, writer.CreateFormFile, and io.Copy which can produce
truncated upstream requests; update the block that handles multipart/form-data
(around ParseMultipartFormReusable, writer.WriteField, writer.CreateFormFile,
io.Copy, and writer.Close) to check and handle errors for each call: on any
error, ensure any opened file handles are closed, abort building the multipart
buffer, and return the original cached body (bytes.NewReader(cachedBody), nil)
so we send the safe fallback instead of a partial request; also propagate or log
the underlying error as appropriate for debugging.
In `@service/task_polling.go`:
- Around line 142-144: Replace the direct dereference of ch.BaseURL when calling
adaptor.FetchTask to avoid a nil pointer panic: use ch.GetBaseURL() instead of
*ch.BaseURL in the adaptor.FetchTask(...) call (the call site that passes ch.Key
and the map of "ids"), mirroring other safe call sites in this file; update the
argument so FetchTask receives the string returned by ch.GetBaseURL() and run
tests to ensure behavior is unchanged.
- Around line 221-234: The byte-sorting comparison currently applied to
oldData/newData is incorrect and unnecessary; replace the sort.Slice-based
byte-histogram equality with a direct bytes.Equal check of the marshalled
outputs from common.Marshal (i.e., compare oldData and newData with bytes.Equal
and return that result inverted as needed), remove the sort.Slice calls and the
"sort" import, and keep using the existing common.Marshal results to determine
equality.
- Around line 170-174: The loop over responseItems.Data can panic because taskM
may not contain responseItem.TaskID; before calling taskNeedsUpdate or accessing
task fields, check whether task := taskM[responseItem.TaskID] is present
(non-nil) and skip or log the unknown TaskID; update the loop in the same block
that references responseItem and task, guarding the task dereference and
returning/continuing early when missing so taskNeedsUpdate and subsequent code
never receive a nil task.
- Around line 165-168: The code checks responseItems.IsSuccess() but then does
return err even though err is nil after a successful Unmarshal, so replace the
nil return with a non-nil error (and keep the log) — for example, construct and
return an fmt.Errorf that includes context and the response body (e.g.,
fmt.Errorf("Suno response unsuccessful for channel %d: %s", channelId,
string(responseBody))); update the branch where responseItems.IsSuccess() is
false (the block referencing responseItems, responseBody, channelId and taskIds)
to return that constructed error so the caller sees the failure.
- Around line 56-76: The bulk update marks tasks with empty upstream IDs
(collected in nullTaskIds from task.GetUpstreamTaskID()) as FAILURE but never
refunds any pre-consumed quota; after calling
model.TaskBulkUpdateByID(nullTaskIds, ...), add a refund step that calls the
existing billing/refund API (e.g., a function like model.RefundQuotaForTaskIDs
or billing.RefundPreConsumedQuota) with nullTaskIds, handle and log errors (use
logger.LogError / LogInfo), and ensure the refund call is idempotent so repeated
processing of the same task IDs is safe; keep using taskM/taskChannelM to look
up any per-task metadata needed for the refund.
---
Nitpick comments:
In `@controller/task.go`:
- Around line 69-93: tasksToDto currently mutates the input *model.Task by
assigning task.Username inside the loop; instead avoid changing the original
model by creating a local copy or by supplying the username to the DTO
conversion. Update tasksToDto to, for each task where a username is found, make
a shallow copy (e.g. copy := *task) and set copy.Username = user.Username, then
call relay.TaskModel2Dto(©); or change relay.TaskModel2Dto to accept an
optional username parameter and pass it through—ensure you only modify the
copied instance so the original tasks slice remains unchanged.
In `@relay/channel/task/ali/adaptor.go`:
- Around line 328-341: The metadata is being unmarshalled directly into
AliVideoRequest (aliReq) which lets user metadata overwrite any field; instead
unmarshal metadataBytes into a constrained AliMetadata struct and then copy only
allowed fields (e.g., Prompt, Parameters, other explicitly permitted fields)
into aliReq, leaving model untouched; keep the existing model-check against
upstreamModel and return the same errors on marshal/unmarshal failures; if the
broad override was intentional, add a short comment above the common.Unmarshal
explaining that behavior and why it's safe.
In `@relay/channel/task/gemini/adaptor.go`:
- Around line 251-252: ParseTaskResult assigns ti.TaskID =
taskcommon.EncodeLocalTaskID(op.Name) but the polling loop in
updateVideoSingleTask never reads taskResult.TaskID, so this is dead data;
remove the unused assignment in ParseTaskResult (or alternatively, if intended
to be used, update updateVideoSingleTask to read taskResult.TaskID) — search for
the assignment to ti.TaskID in ParseTaskResult and either delete that line or
wire taskResult.TaskID into updateVideoSingleTask's polling logic so the value
is actually consumed.
In `@relay/channel/task/hailuo/adaptor.go`:
- Around line 162-164: Replace the direct call to req.UnmarshalMetadata with the
shared helper to match other adaptors: use
taskcommon.UnmarshalMetadata(req.Metadata, &videoRequest) instead of
req.UnmarshalMetadata(&videoRequest), handle the returned error the same way
(wrap with "unmarshal metadata to video request failed"), and keep the same
variables (req and videoRequest) so behavior remains identical to
vidu/kling/jimeng adaptors.
In `@relay/relay_task.go`:
- Around line 415-494: The function tryRealtimeFetch currently returns nil on
all error paths without logging; add diagnostic logs at key failure points to
improve observability: log errors from model.GetChannelById (include
task.ChannelId), when adaptor is nil (include channelModel.Type), on
adaptor.FetchTask failures (log error and response status/URL if present), on
io.ReadAll failures, and on adaptor.ParseTaskResult failures (include response
body or parse error); use common.SysLog or processLogger with context
(task.TaskID, task.GetUpstreamTaskID(), channelModel.Key) before each early
return so failures are visible while preserving the same fallback behavior.
- Around line 191-197: The loop that multiplies info.PriceData.Quota by each ra
in info.PriceData.OtherRatios currently skips only exact 1.0 via `ra != 1.0`,
which is brittle for computed floats; change the check to use an epsilon
comparison (e.g., math.Abs(ra-1.0) > epsilon) so near-1.0 ratios are treated as
1.0 and not applied. Update the same pattern used in recalcQuotaFromRatios to
use the same epsilon constant and reference the same symbols:
info.PriceData.OtherRatios, info.PriceData.Quota, and constant.TaskPricePatches.
In `@service/task_polling.go`:
- Around line 38-85: TaskPollingLoop currently spins forever with time.Sleep and
has no shutdown hook; change it to accept a context.Context (or use a passed-in
stop channel) and replace the for { time.Sleep(...) ... } with a select loop
that listens for ctx.Done() and a time.Ticker or time.After case so the
goroutine can exit immediately on cancellation; ensure you propagate the context
into calls like logger.LogError/logger.LogInfo, model.TaskBulkUpdateByID and
DispatchPlatformUpdate so ongoing work can be cancelled/observed and the
function returns when ctx.Done() is closed.
In `@web/src/components/table/task-logs/TaskLogsColumnDefs.jsx`:
- Around line 295-314: The render function for the column still names its first
param userId and does a redundant fallback using record.username || userId
despite dataIndex: 'username' supplying username as the first arg; rename the
first render parameter to username (or displayName) and remove the redundant
record.username || userId fallback so displayText is derived directly from
username (with the existing fallback of '?'), updating any usages inside render
(e.g., displayText, Avatar content, and stringToColor call) to use the new
parameter name and eliminate record.username references.
| // 使用预生成的公开 ID（如果有），否则新生成 | ||
| taskID := "" | ||
| if relayInfo.TaskRelayInfo != nil && relayInfo.TaskRelayInfo.PublicTaskID != "" { | ||
| taskID = relayInfo.TaskRelayInfo.PublicTaskID | ||
| } else { | ||
| taskID = GenerateTaskID() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the model/task.go file around lines 175-197 to understand the context
head -n 200 model/task.go | tail -n 50Repository: QuantumNous/new-api
Length of output: 1439
🏁 Script executed:
# Search for all callers of InitTask to understand usage patterns
rg -n 'InitTask\(' --type=go -B2 -A2Repository: QuantumNous/new-api
Length of output: 642
🏁 Script executed:
# Examine the context around the InitTask call in relay.go
sed -n '540,580p' controller/relay.goRepository: QuantumNous/new-api
Length of output: 1773
🏁 Script executed:
# Check the function signature and early returns to see if relayInfo is guaranteed non-nil
sed -n '1,570p' controller/relay.go | tail -n 100Repository: QuantumNous/new-api
Length of output: 3585
Nil-pointer dereference on relayInfo due to inconsistent guards in InitTask.
The function conditionally guards against relayInfo being nil at line 175 (if relayInfo != nil && relayInfo.ChannelMeta != nil), but unconditionally dereferences relayInfo at lines 189 and 197 (relayInfo.TaskRelayInfo, relayInfo.UserId). If relayInfo is nil, this will panic.
While the single caller in controller/relay.go:562 guarantees non-nil through upstream error handling, the function signature accepts *commonRelay.RelayInfo (nullable), and the defensive check at line 175 suggests nil should be handled. Either add a nil check at the function start to handle it safely, or remove the partial guard at line 175 and document that relayInfo must not be nil.
Suggested fix — add early nil guard
func InitTask(platform constant.TaskPlatform, relayInfo *commonRelay.RelayInfo) *Task {
+ if relayInfo == nil {
+ return &Task{
+ TaskID: GenerateTaskID(),
+ Platform: platform,
+ Status: TaskStatusNotStart,
+ Progress: "0%",
+ }
+ }
properties := Properties{}
privateData := TaskPrivateData{}
- if relayInfo != nil && relayInfo.ChannelMeta != nil {
+ if relayInfo.ChannelMeta != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/task.go` around lines 187 - 193, InitTask currently dereferences
relayInfo later (relayInfo.TaskRelayInfo, relayInfo.UserId) while only partially
guarding it earlier, causing a potential nil-pointer panic; add an early
nil-check at the start of InitTask that returns an error or sets sensible
defaults when relayInfo == nil (or alternatively document and enforce non-nil),
and update usages of TaskRelayInfo and UserId to be inside that guard (symbols:
InitTask, relayInfo, TaskRelayInfo, UserId, GenerateTaskID) so GenerateTaskID is
only used when needed and no nil deref can occur.
| if resultURL := task.GetResultURL(); strings.HasPrefix(resultURL, "data:") && len(resultURL) > 0 { | ||
| v.SetMetadata("url", resultURL) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the task_polling.go lines around 369-377 to understand the flow
rg -n 'strings\.HasPrefix.*data:' --type=go -B 3 -A 3Repository: QuantumNous/new-api
Length of output: 3297
🏁 Script executed:
# Find task_polling.go and check the specific flow
fd task_polling.go --type f | head -5Repository: QuantumNous/new-api
Length of output: 86
🏁 Script executed:
# Check GetResultURL() implementation in the task model
rg -n 'func.*GetResultURL' --type=go -A 10Repository: QuantumNous/new-api
Length of output: 535
🏁 Script executed:
# Check ParseTaskResult in vertex/adaptor.go
fd 'adaptor.go' relay/channel/task/vertex --type f | xargs cat -n | head -400Repository: QuantumNous/new-api
Length of output: 15162
🏁 Script executed:
# Check for data: URL handling across the codebase, especially in FailReason storage
rg -n 'FailReason.*data:' --type=go -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Verify that the actual base64 video data is stored in task.Data by checking how task.Data is populated
rg -n 'task\.Data\s*=' relay/relay_task.go --context=2Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check how other video task adaptors (Kling, etc.) handle ConvertToOpenAIVideo to see comparison patterns
fd 'adaptor.go' relay/channel/task --type f -x cat {} -n | grep -A 20 'ConvertToOpenAIVideo'Repository: QuantumNous/new-api
Length of output: 8054
🏁 Script executed:
# Check how task.Data is populated for Vertex to confirm it contains the raw Vertex API response
rg -n 'task\.Data.*=' relay/relay_task.go -A 2 -B 2 | head -30Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Verify learnings about data: URL filtering from FailReason in controller/task_video.go
fd 'task_video.go' controller --type f -x cat {} -n | grep -B 5 -A 5 'data:'Repository: QuantumNous/new-api
Length of output: 45
The condition at line 369-371 is dead code and will never set the video URL metadata for successful Vertex tasks.
For Vertex tasks, ParseTaskResult returns the video as a data: URI in taskResult.Url. The polling loop in task_polling.go (line 369-370) explicitly skips storing data: URIs in PrivateData.ResultURL. Since GetResultURL() reads from PrivateData.ResultURL (falling back to FailReason, which is also prevented from containing data: URIs), it will never return a value matching strings.HasPrefix(resultURL, "data:") for successful tasks.
To expose the base64 video data in the OpenAI response, follow the pattern used by other video adaptors (Kling, Jimeng, Doubao, etc.): unmarshal task.Data (the raw Vertex API response) and extract the video URL from the parsed response structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/channel/task/vertex/adaptor.go` around lines 369 - 371, The current
check using task.GetResultURL() will never catch Vertex video data because
PrivateData.ResultURL never stores data: URIs; instead, parse the raw Vertex
response from task.Data as other video adaptors do: unmarshal task.Data into the
Vertex response struct (the same shape used by ParseTaskResult), extract the
base64 data: URL from the parsed object's Url field, and call
v.SetMetadata("url", extractedUrl). Update the adaptor.go logic that currently
checks GetResultURL() to instead inspect task.Data (and reference
types/functions such as task.Data, ParseTaskResult, GetResultURL, and
v.SetMetadata) so successful Vertex tasks expose the video data in metadata.
| // recalcQuotaFromRatios 根据 adjustedRatios 重新计算 quota。 | ||
| // 公式: baseQuota × ∏(ratio) — 其中 baseQuota 是不含 OtherRatios 的基础额度。 | ||
| func recalcQuotaFromRatios(info *relaycommon.RelayInfo, ratios map[string]float64) int { | ||
| // 从 PriceData 获取不含 OtherRatios 的基础价格 | ||
| baseQuota := info.PriceData.Quota | ||
| // 先除掉原有的 OtherRatios 恢复基础额度 | ||
| for _, ra := range info.PriceData.OtherRatios { | ||
| if ra != 1.0 && ra > 0 { | ||
| baseQuota = int(float64(baseQuota) / ra) | ||
| } | ||
| } | ||
| // 应用新的 ratios | ||
| result := float64(baseQuota) | ||
| for _, ra := range ratios { | ||
| if ra != 1.0 { | ||
| result *= ra | ||
| } | ||
| } | ||
| return int(result) | ||
| } |
There was a problem hiding this comment.
Floating-point rounding in recalcQuotaFromRatios can drift from the originally computed quota.
The original quota in RelayTaskSubmit (line 194) is computed by sequentially multiplying and truncating to int. Here, you reverse that by dividing, but the intermediate int() truncations mean the round-trip isn't lossless. For example:
- Base=7, ratio1=1.5, ratio2=2.0 → step 1:
int(7*1.5)=10, step 2:int(10*2.0)=20 - Reverse:
int(20/2.0)=10,int(10/1.5)=6→ base recovered as 6, not 7
Consider storing the pre-OtherRatios base quota in PriceData to avoid the lossy round-trip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/relay_task.go` around lines 254 - 273, recalcQuotaFromRatios loses
precision by trying to recover the original base quota via repeated integer
division (causing drift compared to RelayTaskSubmit's sequential int
truncations); instead add and use an explicit pre-OtherRatios base quota stored
on PriceData (e.g., PriceData.BaseQuota) so recalcQuotaFromRatios can use that
exact integer base rather than dividing back, and update RelayTaskSubmit to set
PriceData.BaseQuota when computing the original quota so all downstream calls
(recalcQuotaFromRatios) use the exact stored base quota.
| } | ||
|
|
||
| logger.LogDebug(ctx, fmt.Sprintf("updateVideoSingleTask response: %s", string(responseBody))) |
There was a problem hiding this comment.
Debug log may dump entire base64-encoded video payloads.
string(responseBody) can be megabytes for Vertex/Gemini responses containing bytesBase64Encoded. This bloats logs and risks leaking sensitive binary data.
Consider using the redactVideoResponseBody helper (already defined in this file) before logging, or limit the logged length.
- logger.LogDebug(ctx, fmt.Sprintf("updateVideoSingleTask response: %s", string(responseBody)))
+ logger.LogDebug(ctx, fmt.Sprintf("updateVideoSingleTask response: %s", string(redactVideoResponseBody(responseBody))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/task_polling.go` around lines 318 - 320, The debug log currently
prints the full responseBody in updateVideoSingleTask via logger.LogDebug, which
can dump large base64 video payloads; replace the direct string(responseBody)
with a sanitized version using the existing redactVideoResponseBody helper (or
truncate to a safe length) before logging so logs do not contain full
binary/base64 data.
| var responseItems dto.TaskResponse[model.Task] | ||
| if err = common.Unmarshal(responseBody, &responseItems); err == nil && responseItems.IsSuccess() { | ||
| logger.LogDebug(ctx, fmt.Sprintf("updateVideoSingleTask parsed as new api response format: %+v", responseItems)) | ||
| t := responseItems.Data | ||
| taskResult.TaskID = t.TaskID | ||
| taskResult.Status = string(t.Status) | ||
| taskResult.Url = t.GetResultURL() | ||
| taskResult.Progress = t.Progress | ||
| taskResult.Reason = t.FailReason | ||
| task.Data = t.Data | ||
| } else if taskResult, err = adaptor.ParseTaskResult(responseBody); err != nil { | ||
| return fmt.Errorf("parseTaskResult failed for task %s: %w", taskId, err) | ||
| } else { | ||
| task.Data = redactVideoResponseBody(responseBody) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the redactVideoResponseBody function
rg -n 'func redactVideoResponseBody' --type goRepository: QuantumNous/new-api
Length of output: 141
🏁 Script executed:
# Check the structure of dto.TaskResponse and the Data field
fd 'task.go' controller/ --type go | head -20Repository: QuantumNous/new-api
Length of output: 232
🏁 Script executed:
# Look for all task.Data assignments in service/task_polling.go
rg -n 'task\.Data\s*=' service/task_polling.goRepository: QuantumNous/new-api
Length of output: 179
🏁 Script executed:
# Check the Task model definition to understand the Data field
rg -n 'type Task struct' model/ --type go -A 20 | grep -A 20 'type Task'Repository: QuantumNous/new-api
Length of output: 1897
🏁 Script executed:
# Get the full Task struct definition
cat -n model/task.go | sed -n '44,100p'Repository: QuantumNous/new-api
Length of output: 2766
🏁 Script executed:
# Read the redactVideoResponseBody function implementation
cat -n service/task_polling.go | sed -n '430,470p'Repository: QuantumNous/new-api
Length of output: 1388
🏁 Script executed:
# Check context around line 189 task.Data assignment
cat -n service/task_polling.go | sed -n '180,200p'Repository: QuantumNous/new-api
Length of output: 885
🏁 Script executed:
# Get full context around lines 326-340
cat -n service/task_polling.go | sed -n '320,345p'Repository: QuantumNous/new-api
Length of output: 1299
🏁 Script executed:
# Find dto.TaskResponse definition
fd 'dto.go\|task.go' --type f | xargs grep -l 'type TaskResponse' | head -5Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check the dto.TaskResponse structure
rg -n 'type TaskResponse' dto/ -A 10Repository: QuantumNous/new-api
Length of output: 466
🏁 Script executed:
# Check if there are any comments or documentation about why the New API format branch doesn't redact
cat -n service/task_polling.go | sed -n '310,360p'Repository: QuantumNous/new-api
Length of output: 2192
🏁 Script executed:
# Verify what fields the New API response might contain for video tasks
rg -n 'GetResultURL\|bytesBase64Encoded' --type go -B 3 -A 3 | head -40Repository: QuantumNous/new-api
Length of output: 45
New API format branch stores task.Data without redaction; apply same redaction to both branches.
Line 335 sets task.Data = t.Data from the parsed New API response, while line 339 applies redactVideoResponseBody() to the adaptor-parsed response. Both branches can contain video task responses with large base64 strings or bytesBase64Encoded fields. To prevent unredacted data from being stored in the database, either apply redactVideoResponseBody() consistently to both branches, or verify that the New API format response structure guarantees no problematic base64 fields in the Data field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/task_polling.go` around lines 326 - 340, The New API branch assigns
unredacted t.Data to task.Data while the adaptor branch applies
redactVideoResponseBody; update the New API branch (where responseItems /
dto.TaskResponse[model.Task] is handled) to run the parsed data through
redactVideoResponseBody before storing it (i.e., set task.Data =
redactVideoResponseBody(t.Data) or otherwise redact the same fields), ensuring
the redaction function is applied consistently for both the responseItems branch
and the adaptor.ParseTaskResult branch so no base64/video fields are stored
unredacted.
| if task.FinishTime == 0 { | ||
| task.FinishTime = now | ||
| } | ||
| task.FailReason = taskResult.Reason | ||
| logger.LogInfo(ctx, fmt.Sprintf("Task %s failed: %s", task.TaskID, task.FailReason)) |
There was a problem hiding this comment.
task.FailReason may receive a data: URI from the upstream.
Based on learnings, data: URLs (containing base64-encoded video data) should be prevented from being stored in FailReason. Add a guard before assigning taskResult.Reason.
Proposed fix
- task.FailReason = taskResult.Reason
+ if !strings.HasPrefix(taskResult.Reason, "data:") {
+ task.FailReason = taskResult.Reason
+ } else {
+ task.FailReason = "task failed (reason redacted: data URI)"
+ }Based on learnings: "data: URLs (containing base64 encoded video data) are prevented from being stored in task.FailReason by checking if the URL starts with 'data:' before assignment."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/task_polling.go` around lines 383 - 387, The code assigns
taskResult.Reason directly into task.FailReason which can be a data: URI; before
assignment in the block where task.FinishTime is set and logger.LogInfo is
called, check the value of taskResult.Reason and only assign it to
task.FailReason if it does not start with "data:" (use
strings.HasPrefix(taskResult.Reason, "data:")), otherwise set task.FailReason to
a safe placeholder like "[redacted data]" or leave it unchanged; ensure you
import or reference strings and keep the existing logger.LogInfo call using the
sanitized task.FailReason.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/src/i18n/locales/en.json`:
- Line 2508: Remove the duplicate JSON key "任务ID" and consolidate on the
existing "任务 ID" entry: delete the "任务ID" key from en.json and update any UI
code that calls t('任务ID') to call t('任务 ID') instead (search for usages of
t('任务ID') and replace them), ensuring only the single canonical key "任务 ID"
remains and translations stay consistent.
…ask-merge refactor: async task
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements