fix(fork): keep terminal task memory consistent when artifact cleanup fails - #47
Conversation
|
Thanks @asto18089 for taking the time to contribute. This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered. Please read |
…a drop Follow-up from the pinvou-agent#453 review of the v0.9.12 re-fork: - task_manager: delete_terminal_task removed artifacts before the task record, so a mid-failure left a live task with destroyed artifacts; reverse the order so a failure only leaves orphan files. Also document that legacy v4 fields survive reads but are dropped on re-save. (The one-shot misfire fix and the search-backend configuration hint from the original PR are already on main as baa87f4 and 1fafee7.) Signed-off-by: asto18089 <44870036+asto18089@users.noreply.github.com>
e655c3a to
2725259
Compare
h3c-hexin
left a comment
h3c-hexin
left a comment
There was a problem hiding this comment.
正式评审：Request changes
评审基于精确 head 272525912acf5e45f1479cd60f770777a6c9c889，相对 r1 基线 1fafee7e26b60a59457a43bce50c63aa2ad9dbaf。当前实际 diff 只有 crates/tui/src/task_manager.rs 一个文件。
Major（阻塞）：artifact 删除失败后，磁盘与内存任务状态发生分叉
delete_terminal_task() 现在先在 1606 删除任务 JSON，再在 1614–1620 删除 artifact；但 artifact 删除错误通过 ? 直接返回，state.tasks.remove() 与 queue 更新要到 1623–1625 才执行。
因此只要 remove_dir_all() 失败（例如路径形态异常、Windows 文件占用或 I/O 错误），结果并非注释所说的“只留下 orphan files”，而是同时出现：
- 磁盘任务记录已经不存在；
- 当前进程的
state.tasks仍把该任务当作存在； - API 返回错误；
- 同一状态在重启前后不同：重启后该内存 ghost 消失，但 artifact orphan 仍在；
- 经
automation_manager::delete_terminal_run()调用时，automation run 也因该错误暂不删除。
这是数据完整性失败路径，不能只靠正常路径用例推断。现有 forkguard_terminal_task_delete_refuses_active_and_is_idempotent 只覆盖 active 拒绝、完整成功及第二次返回 false；旧顺序和新顺序都能通过，它没有证明本 PR 唯一行为修改的失败语义。
请让内存状态与已经完成的 durable record 删除保持一致，或引入可持久化的 tombstone/retry 机制；同时补一个强制 artifact 删除失败的回归测试，至少钉住当前进程、重试/重启后的可见状态和残留文件契约。若设计选择接受 orphan，也应让实现与注释真实地只留下 orphan，而不是额外留下未持久化的 live task。
合并前元数据与 required gate
当前标题、正文和 Testing 仍声称本 PR 包含 one-shot automation 与 Web search guidance，但最终 head 只有上述 task-manager 提交；那两项已经位于 r1 基线，不在本 PR diff 中。请把标题、Changes、Testing 改成当前真实范围，避免 release/review 记录失真。
此外 required PR closes an issue / link 检查仍为 FAILURE；请按仓库约定补充 issue 引用或 No-Issue:。
本地验证
cargo fmt --all -- --check：通过- 精确重编译该 head 后，
forkguard_：31 passed - 删除正常路径定向用例：1 passed
git diff --check：通过
上述绿灯不覆盖本评审指出的 artifact 删除失败分支。
Follow-up from the PR Hmbown#47 review: - task_manager: delete_terminal_task removed the durable record first but only dropped the in-memory entry after artifact cleanup, so a failing remove_dir_all returned an error while the disk no longer knew about a task that state.tasks still served. Commit the in-memory removal (task entry, queue, persisted queue) immediately after the record is gone, so a cleanup failure leaves only orphan files and the retry path (including automation_manager::delete_terminal_run) becomes an idempotent no-op that can finish its own cleanup. - Pin the failure semantics with a regression test that forces artifact deletion to fail and asserts in-process visibility, retry idempotence, restart state, and the orphan-file contract; it fails against the previous order at the in-process ghost assertion. Signed-off-by: asto18089 <44870036+asto18089@users.noreply.github.com>
asto18089
changed the title
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
感谢评审。新 head：b69be2913（基于 272525912，未改写历史）。三条意见逐一处置如下：
Major（artifact 删除失败后磁盘与内存状态分叉）— 成立，已修复。
delete_terminal_task 现在在 durable record 删除成功后、artifact 清理之前，先提交内存侧的一致状态（state.tasks.remove + queue 清理 + persist_queue_locked）。remove_dir_all 失败时仍向调用方返回错误，但结果与注释声明一致——只留下 orphan files，不再留下磁盘已不存在的 live ghost task；重启前后可见状态一致。重试路径随之自愈：automation_manager::delete_terminal_run 重试时 delete_terminal_task 返回幂等的 Ok(false)，automation run 自身得以继续删除。选择了"内存与已完成删除保持一致"方案，未引入 tombstone（当前调用链下重试已可收敛，无需持久化重试记录）。
回归测试 forkguard_terminal_task_delete_artifact_failure_leaves_no_memory_ghost：在 artifact 路径放常规文件强制 remove_dir_all 失败（ENOTDIR，不依赖权限位，root CI 下同样成立），钉住进程内可见性（get_task 失败）、重试幂等（第二次 Ok(false)）、重启后状态一致、orphan 文件残留契约。已验证该测试在旧顺序下于内存 ghost 断言处失败（FAILED. 0 passed; 1 failed），新顺序下通过。
元数据失真 — 成立，已更正。 确认 pinvou3-clean tip 即 1fafee7e2（含 baa87f4de one-shot 修复），两者已不在本 PR diff 中。标题已改为 fix(fork): keep terminal task memory consistent when artifact cleanup fails，正文 Summary/Changes/Testing 已重写为当前真实范围，并注明两项的落点。
link 检查 — 已修。 正文已加 No-Issue: 行说明这是对已合并 #44 评审发现的跟进、无独立 issue，检查应随本次 body 编辑转绿。
本地验证：cargo test -p codewhale-tui --lib -- automation_manager:: task_manager:: tools::web:: → 173 passed / 0 failed；cargo test -p codewhale-tui --lib forkguard → 32 passed / 0 failed（含新测试）；cargo fmt --all -- --check、git diff --check 通过。
|
@h3c-hexin 三条评审意见已全部处置（详见上一条评论），新 head |
h3c-hexin
left a comment
h3c-hexin
left a comment
There was a problem hiding this comment.
R2 正式复审：Approve
复审基于精确 head b69be29139757f4c54adc1039691e3e3f334d4fb。
原 Major 已解决：durable task record 删除后，内存 task/queue 状态在 artifact 清理前立即同步；artifact 删除失败时不再留下当前进程可见的 ghost task。新增回归测试使用常规文件占据 artifact 目录路径，稳定覆盖错误返回、进程内状态、幂等重试、重启状态与 orphan 契约。实现与注释、自动化调用方的重试语义一致。
PR 标题、正文、Testing 与最终 diff 已对齐，最新 link 与 DCO 检查均成功。未发现新增阻塞问题。
本地在该 head 上精确重编译并验证：
cargo fmt --all -- --check：通过- 新增 artifact 失败路径测试：1 passed
forkguard_：32 passedgit diff --check：通过
Signed-off-by: h3c-hexin <13790929+h3c-hexin@users.noreply.github.com>
|
已补充修复提交 原测试的 drop(manager) 仅释放测试持有的 Arc，后台 worker 仍持有强引用，不能表示旧 manager 已停止。现在先 shutdown，再在 5 秒超时内等待 Weak::strong_count() 降到 0，确认旧 worker 已释放 manager 后才重新打开存储。超时会直接使测试失败。 本地重新编译后的删除相关测试：2 passed / 0 failed（包括 artifact 失败路径与正常删除/幂等）；cargo fmt --all -- --check、git diff --check 通过。按 scoped AGENTS.md 选择针对性验证，未重新运行完整 workspace 测试或跨平台验证。等待新提交的 GitHub 检查。 |
Signed-off-by: h3c-hexin <13790929+h3c-hexin@users.noreply.github.com>
|
门禁修复 #49 已通过完整验收并正常合并，落点 新工作流实际生成受保护分支要求的 本 PR 已通过普通 merge commit 无冲突同步门禁修复，未改写作者历史。最新 head 的完整 CI 已触发，待真实门禁通过后再正常合并。 |
|
最终验收 PASS：集成 head 已通过正常 squash merge 合并到 |
2 participants
Summary
Follow-up fix for the review findings on the merged v0.9.12 re-fork (#44), raised during the parent-side review of Pinvou/pinvou-agent#453 and addressed per the PR #47 review (h3c-hexin).
The one-shot misfire fix (
baa87f4de) and the search-backend guidance (1fafee7e2) that this PR originally carried were merged topinvou3-cleanseparately, so they are no longer part of this PR's diff; everything below describes the actual remaining change only.Changes
task_manager: keep in-memory state consistent with the durable record when artifact cleanup fails.delete_terminal_tasknow deletes the task record first, then immediately commits the matching in-memory removal (task entry, queue, persisted queue) before deleting artifacts. Aremove_dir_allfailure therefore leaves only orphan files — exactly what the in-function comment declares — instead of also leaving a live ghost task that the disk no longer knows about and whose visibility differed before and after a restart. The artifact error is still reported to the caller; because the in-memory ghost is gone, a retry (including viaautomation_manager::delete_terminal_run) is an idempotentOk(false)that lets the caller finish its own run deletion.Testing
cargo test -p codewhale-tui --lib -- automation_manager:: task_manager:: tools::web::→test result: ok. 173 passed; 0 failedcargo test -p codewhale-tui --lib forkguard→test result: ok. 32 passed; 0 failedforkguard_terminal_task_delete_artifact_failure_leaves_no_memory_ghostforcesremove_dir_allto fail (a regular file placed at the artifact path, so no reliance on permission bits) and pins in-process visibility, retry idempotence, restart state, and the orphan-file contract. It fails without the production change (FAILED. 0 passed; 1 failed, at the in-process ghost assertion) and passes with it.cargo fmt --all -- --checkandgit diff --check: pass.Notes
No-Issue: follow-up to review findings on merged #44; no dedicated issue tracks it.