Fix task polling result URLs and reset locking - #5976
Conversation
WalkthroughAdmin subscription reset transactions now use a shared lockForUpdate helper instead of a no-op GORM v1 query option, ensuring actual row locking. Task polling parses upstream responses as dto.TaskDto to preserve result_url, with a fallback to FailReason and corresponding test updates. ChangesSubscription Reset Row Locking
Estimated code review effort: 2 (Simple) | ~10 minutes Task Polling result_url Preservation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Poller as UpdateVideoTasks
participant Adaptor as TaskAdaptor
participant Upstream as Upstream API
participant DB as Task Storage
Poller->>Adaptor: FetchTask(taskID)
Adaptor->>Upstream: request task status
Upstream-->>Adaptor: TaskResponse[TaskDto] with ResultURL
Adaptor-->>Poller: response body
Poller->>Poller: unmarshal into dto.TaskDto
alt ResultURL present
Poller->>Poller: taskResult.Url = trimmed ResultURL
else ResultURL empty and status success
Poller->>Poller: taskResult.Url = FailReason
end
Poller->>DB: persist updated task status and Url
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
service/task_polling_test.go (1)
135-178: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the FailReason fallback branch.
The new test only exercises the case where
ResultURLis populated. It doesn't cover theResultURL == "" && Status == Successfallback toFailReasonthat was introduced inservice/task_polling.go(Lines 478-480), leaving that branch of the fix untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/task_polling_test.go` around lines 135 - 178, Add a test case in TestUpdateVideoTasksPreservesNewAPITaskDtoResultURL coverage for the TaskDto success path where ResultURL is empty and FailReason is populated. Use the existing UpdateVideoTasks flow with taskPollingFetchAdaptor and TaskDto so the saved model.Task.PrivateData.ResultURL is asserted to fall back to FailReason when Status is TaskStatusSuccess and ResultURL is blank.service/task_polling.go (1)
471-486: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM on parsing as
dto.TaskResponse[dto.TaskDto]; verify the FailReason→Url fallback and consider deduplicating withTask.GetResultURL.The new branch replicates the same "ResultURL, else FailReason" compatibility fallback already encapsulated in
model.Task.GetResultURL()(per the retrieved context snippet), but re-implements it inline instead of reusing shared logic. This duplicates the compatibility rule in two places, and if it changes in one but not the other, the codebase will silently diverge.Separately, when
ResultURLis empty and status isSuccess,taskResult.Urlis set straight fromFailReasonwithout any validation that it actually looks like a URL/proxy string. SincetaskResult.Urlis later stored verbatim intotask.PrivateData.ResultURL(Line 540) when non-empty, an upstream that reportsSuccesswith an ordinary (non-URL)fail_reasonstring would get that text persisted as the task's result URL.♻️ Suggested consolidation
- taskResult.Url = strings.TrimSpace(t.ResultURL) - if taskResult.Url == "" && model.TaskStatus(t.Status) == model.TaskStatusSuccess { - taskResult.Url = strings.TrimSpace(t.FailReason) - } + if model.TaskStatus(t.Status) == model.TaskStatusSuccess { + // Mirrors model.Task.GetResultURL() historical compatibility fallback. + taskResult.Url = strings.TrimSpace(t.ResultURL) + if taskResult.Url == "" { + taskResult.Url = strings.TrimSpace(t.FailReason) + } + }Based on learnings, this same file was previously flagged for keeping auth/context derivation consistent across call sites (see
service/task_polling.golearning on reusingPrivateData.Key); the same principle of not re-deriving shared logic applies here to theResultURL/FailReasonfallback semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/task_polling.go` around lines 471 - 486, The `updateVideoSingleTask` parsing path is duplicating the `ResultURL`/`FailReason` compatibility fallback instead of reusing the shared `model.Task.GetResultURL()` logic. Replace the inline `FailReason` assignment with a call to the existing `Task.GetResultURL` behavior (or equivalent shared helper) so the fallback rule stays consistent in one place. Also ensure `taskResult.Url` only accepts a valid URL/proxy string before persisting it, rather than storing any non-empty `FailReason` verbatim. Keep the new `dto.TaskResponse[dto.TaskDto]` branch aligned with the same result-url semantics used elsewhere in the file.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@service/task_polling_test.go`:
- Around line 135-178: Add a test case in
TestUpdateVideoTasksPreservesNewAPITaskDtoResultURL coverage for the TaskDto
success path where ResultURL is empty and FailReason is populated. Use the
existing UpdateVideoTasks flow with taskPollingFetchAdaptor and TaskDto so the
saved model.Task.PrivateData.ResultURL is asserted to fall back to FailReason
when Status is TaskStatusSuccess and ResultURL is blank.
In `@service/task_polling.go`:
- Around line 471-486: The `updateVideoSingleTask` parsing path is duplicating
the `ResultURL`/`FailReason` compatibility fallback instead of reusing the
shared `model.Task.GetResultURL()` logic. Replace the inline `FailReason`
assignment with a call to the existing `Task.GetResultURL` behavior (or
equivalent shared helper) so the fallback rule stays consistent in one place.
Also ensure `taskResult.Url` only accepts a valid URL/proxy string before
persisting it, rather than storing any non-empty `FailReason` verbatim. Keep the
new `dto.TaskResponse[dto.TaskDto]` branch aligned with the same result-url
semantics used elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 017db407-d561-4e83-8a90-1371e16deb71
📒 Files selected for processing (3)
model/subscription.goservice/task_polling.goservice/task_polling_test.go
|
PR还是拆开俩吧，for update那个可以合并 |
2 participants
Important
📝 变更描述 / Description
修复两个异步任务和订阅重置相关问题：
dto.TaskResponse[dto.TaskDto]，读取顶层result_url并保存到任务私有结果 URL。这样 New API 作为上游异步任务服务时，成功任务不会因为按model.Task解析而丢失结果地址。lockForUpdate(tx)helper。GORM v2 会忽略旧的Set("gorm:query_option", "FOR UPDATE")，该 helper 能在支持的数据库上生成行锁，并在 SQLite 下保持兼容。补充了一个回归测试，覆盖 New API
TaskDto.result_url轮询结果保存流程。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
TaskDto.result_url解析，也没有覆盖订阅重置行锁问题。Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
通过：
补充验证：
go test ./model ./service结果：
./model通过；./service中现有的 channel affinity usage cache 用例失败，失败项为TestObserveChannelAffinityUsageCacheByRelayFormat_MixedMode和TestObserveChannelAffinityUsageCacheByRelayFormat_UnsupportedModeKeepsEmpty。这两个失败与本 PR 修改的任务轮询和订阅重置代码无关。Summary by CodeRabbit