fix(offline-download): preserve canceled task state - #3007
Conversation
- Return context.Canceled after successful provider cleanup so tache records cancellation instead of success. - Update the task detail status and add regression coverage for canceled cleanup. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
- Prioritize canceled contexts after terminal status updates. - Run Transmission cleanup with a non-cancelable context. - Add regression coverage for completion and cancellation races. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
jyxjjj
left a comment
jyxjjj
left a comment
There was a problem hiding this comment.
Update() performs the transfer before Run() checks whether the task has been canceled.
When Status() reports Completed: true, Update() immediately calls t.Transfer(). If cancellation happens between the status update and this point, the transfer side effect can still run, and Run() only notices the canceled context afterward.
The new regression test uses Thunder, where Transfer() returns without performing a transfer when TempDir == DstDirPath, so it does not cover this race.
Consider checking t.Ctx().Err() inside the info.Completed branch before calling Transfer(), and add a test that exercises an actual transfer path.
已追加commit |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ff94e677c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return err | ||
| } | ||
| err = t.client.TorrentRemove(task.Ctx(), transmissionrpc.TorrentRemovePayload{ | ||
| err = t.client.TorrentRemove(context.WithoutCancel(task.Ctx()), transmissionrpc.TorrentRemovePayload{ |
There was a problem hiding this comment.
When the Transmission RPC endpoint accepts a request but stops responding, this context can never terminate: context.WithoutCancel removes both cancellation and any deadline from the task context, while Init constructs the RPC client with the default client configuration. Consequently, canceling the download can leave it permanently in the canceling state and occupy an offline-download worker indefinitely; use a detached context with a finite cleanup timeout instead.
Useful? React with 👍 / 👎.
| if err := t.tool.Remove(t); err != nil { | ||
| return err |
There was a problem hiding this comment.
When Remove returns an error—deterministically for 123 Open, whose implementation returns errs.NotSupport, and transiently for other providers—this helper returns that ordinary error after setting the status to canceled. The task manager can therefore classify or retry the task as failed rather than canceled, leaving its state inconsistent with the new status and defeating the state-preservation change; retain the cleanup error while ensuring the returned error still wraps context.Canceled.
Useful? React with 👍 / 👎.
- Check the task context before transferring completed downloads. - Cover cancellation between status reporting and transfer dispatch. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
- Interrupt completion waits and preserve cancellation across cleanup and AddURL failures. - Bound Transmission cleanup and guard direct transfer dispatch. - Add cancellation-state and cleanup-timeout regression coverage. Co-authored-by: Codex <267193182+codex@users.noreply.github.com>
2ff94e6 to
f172010
Compare
|
已修复有关问题 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Dismissed: superseded by a re-issued review in the standard format.
pikachuren
left a comment
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @laorentou666 提交！
🤖 AI 自动审核声明：本评审报告由 AI 自动生成，当前使用 Claude Opus 5 模型进行分析。
🎯 结论
🔄 Request Changes — 修的是真 bug 且方向正确，但改动范围超出标题、清理逻辑只覆盖了一个 provider，建议调整后再合并
📖 概要
fix(offline-download): preserve canceled task state · 修复离线下载任务被取消后仍被记为「成功」的问题。
核心改动：取消路径改为返回非 nil 错误以命中 tache 的取消终态判定，并为 Transmission 的清理请求 detach 掉已取消的 ctx；同时顺带重构了 provider 分支逻辑。
🧭 整体方案
技术路线是「让 Run() 在取消时返回错误」：tache 的 Worker.Execute 只有在 task.Run() 返回非 nil error 时才会进入 onError，而 onError 用 isCanceled(task.Ctx()) 判定终态、needRetry 也只对 StateErrored/StateFailed 重试。旧代码收到 CtxDone 后 Remove 成功就 return nil，于是被记成 StateSucceeded——这个根因分析是准确的，方案方向正确。附带一点：既然终态由 ctx 决定，返回任意非 nil 错误即可，不一定要构造 context.Canceled，cancellationError 或许可以更简单些~
📊 变更统计
4 个文件（+484 / -50 行） | 功能 ⭐⭐⭐⭐ | 最小改动 ⭐⭐ | 前向兼容 ⭐⭐⭐ | 方案设计 ⭐⭐⭐
🚨 关键问题
P0（阻塞合并）：
⚠️ internal/offline_download/tool/download.go— 取消时的Remove清理只对 Transmission 做了 ctx detach，其它 provider（aria2 / qBittorrent / 115 等）在取消路径上仍使用已取消的 ctx 调用Remove，HTTP 请求会立刻返回context canceled，远端任务留成孤儿。请问是否考虑在DownloadTask.cancelDownload这一层统一处理（调用Remove前统一换成context.WithoutCancel+ 超时），而不是逐个 driver 打补丁呢？目前只改 Transmission 容易让人误以为问题已整体解决~
P1（建议修复）：
⚠️ 改动范围超出标题，是否考虑拆成两个 PR？第 4 个 commit 把一长串if t.tool.Name() == ...重写成了switch，并把 qBittorrent/Transmission 的做种等待收拢进waitAndRemove。重构本身质量不错（逐个分支核对过，行为等价，115 Open/123 Open归组也没问题），但与取消语义修复混在一起会放大 review 成本和回归风险。建议重构单独成 PR，本 PR 保持最小 diff。⚠️ 集成测试TestDownloadTaskManagerKeepsCanceledStateWhenCleanupFails存在 flake 风险：它拉起真实 tache manager，靠两个 1 秒超时的 channel 同步，CI 高负载时容易假失败。更需要注意的是observedDownloadTask.SetState中的close(t.canceled)——onError会先后两次调用SetState(StateCanceled)（一次在isCanceled分支，一次在needRetry为 false 之后），目前靠failedHookCalled标志位恰好只关闭一次，这个不变量依赖 tache 内部调用顺序，上游一改就会 panic。是否可以用sync.Once保护 close，或者把它降级为直接断言Run()返回值的单元测试呢？
P2（可选）：
- 💡
cancelDownload中t.Status被赋值了两次：先设为"offline download canceled"，紧接着cancellationError又设一遍。删掉前一行即可~ - 💡 行为变更建议写进 PR 描述：旧代码在 qBittorrent/Transmission 做种等待期间被取消最终是
return nil（成功），新代码走cancelDownload后会变成 canceled。我认为这个改变是对的，但它是用户可见的状态变化，值得在描述里点明。 - 💡
stderrors.Join(ctxErr, extra)的错误消息是换行拼接的，展示到任务列表会是多行。功能上没问题，想确认一下 UI 侧若只渲染单行，用户看到的会不会是被截断的信息？ - 💡
transmissionCleanupTimeout = 15 * time.Second是硬编码常量，而项目里其它超时（做种时长等）都走了setting。这里是否也考虑做成可配置？这条更适合听维护者的意见。
📂 逐文件分析
internal/offline_download/tool/download.go
改动意图：让取消路径返回错误以保留 canceled 状态，并重构 provider 分支。
代码逻辑：cancelDownload 设置状态并构造 cancellationError；原先散落的 if t.tool.Name() == ... 判断被收敛为 switch，做种等待与清理合并进 waitAndRemove。
问题分析：取消语义的修复正确；但 ① 清理阶段未统一 detach ctx（见 P0）；② t.Status 重复赋值（见 P2）；③ 重构与修复耦合在同一 PR（见 P1）。
详细建议：建议在调用各 provider 的 Remove 前统一处理 ctx，这样一次覆盖所有 provider：
// 统一在 cancelDownload/waitAndRemove 中 detach，替代逐 driver 修改
cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), cleanupTimeout)
defer cancel()
if err := t.tool.Remove(cleanupCtx, t); err != nil {
// 清理失败不改变 canceled 终态，仅附加到错误信息
}drivers/.../transmission/client.go
改动意图：为 Transmission 的清理请求 detach 已取消的 ctx。
问题分析：本身写法正确，但作为单点修复不完整——如果按上面的建议在 download.go 层统一处理，这里的改动或许就可以回退了。
详细建议：确认统一方案后，此文件建议保持原样以缩小 diff。
测试文件
问题分析：见 P1 第二条，主要是同步方式与 close(t.canceled) 的一次性保证。
详细建议：sync.Once 保护 close，或改为直接断言 Run() 返回 error 且 ctx 已取消的纯单元测试，避免依赖 tache 内部调用次数。
其余文件无重大问题。
✅ 待处理清单
- [P0] 在
cancelDownload层统一为Remove提供 detach 后的 ctx，覆盖 aria2 / qBittorrent / 115 等全部 provider - [P1] 将
switch重构与waitAndRemove收敛拆分为独立 PR，本 PR 保持最小 diff - [P1] 用
sync.Once保护close(t.canceled)，或将集成测试降级为单元测试 - [P2] 删除
cancelDownload中重复的t.Status赋值 - [P2] 在 PR 描述中说明做种等待期取消的状态变化（success → canceled）
- [P2] 确认多行错误消息在任务列表 UI 中的展示效果
- [P2] 与维护者确认
transmissionCleanupTimeout是否需要可配置
🎯 结论：🔄 Request Changes — 根因与方向都对，但取消清理需覆盖全部 provider，且建议把重构拆出去以缩小回归面。
3 participants
Summary / 摘要
修复离线下载任务取消时的状态和清理问题：
context.Canceled返回逻辑。/ 此 PR 包含破坏性变更。
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Testing / 测试
go test ./...go test -race ./internal/offline_download/toolgo test -vet=off ./internal/offline_download/...Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Documentation / 文档
Tests / 测试
Translation / 翻译
Review assistance / 审查辅助
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。