From 272525912acf5e45f1479cd60f770777a6c9c889 Mon Sep 17 00:00:00 2001 From: asto18089 <44870036+asto18089@users.noreply.github.com> Date: Wed, 9 Sep 2026 12:45:28 +0800 Subject: [PATCH 1/3] fix(fork): delete task record before artifacts, document legacy schema 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 baa87f4de and 1fafee7e2.) Signed-off-by: asto18089 <44870036+asto18089@users.noreply.github.com> --- crates/tui/src/task_manager.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/crates/tui/src/task_manager.rs b/crates/tui/src/task_manager.rs index 37b1871479..6b6e5d5b39 100644 --- a/crates/tui/src/task_manager.rs +++ b/crates/tui/src/task_manager.rs @@ -43,6 +43,8 @@ const CURRENT_TASK_SCHEMA_VERSION: u32 = 3; // Pinvou v0.9.0 persisted an additive v4 schema. Its additional fields are // serde-defaulted or safely ignored by this reader, so accept exactly that // historical version without widening compatibility to unknown future data. +// Note the fields are only preserved on read: TaskRecord has no catch-all, so +// a load→persist cycle writes v3 and drops them. const PINVOU_LEGACY_TASK_SCHEMA_VERSION: u32 = 4; const fn default_task_schema_version() -> u32 { @@ -1598,14 +1600,9 @@ impl TaskManager { let task_path = self.tasks_dir.join(format!("{task_id}.json")); let artifacts_path = self.artifacts_dir.join(task_id); - if artifacts_path.exists() { - fs::remove_dir_all(&artifacts_path).with_context(|| { - format!( - "Failed to delete task artifacts {}", - artifacts_path.display() - ) - })?; - } + // Delete the record before the artifacts: failing after the record is + // gone only leaves orphan files, while failing after the artifacts are + // gone would leave a live task whose artifacts were destroyed. match fs::remove_file(&task_path) { Ok(()) => {} Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} @@ -1614,6 +1611,14 @@ impl TaskManager { .with_context(|| format!("Failed to delete task {}", task_path.display())); } } + if artifacts_path.exists() { + fs::remove_dir_all(&artifacts_path).with_context(|| { + format!( + "Failed to delete task artifacts {}", + artifacts_path.display() + ) + })?; + } state.tasks.remove(task_id); state.queue.retain(|queued_id| queued_id != task_id); From b69be29139757f4c54adc1039691e3e3f334d4fb Mon Sep 17 00:00:00 2001 From: asto18089 <44870036+asto18089@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:07:47 +0800 Subject: [PATCH 2/3] fix(fork): keep task memory consistent when artifact cleanup fails Follow-up from the PR #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> --- crates/tui/src/task_manager.rs | 66 ++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 3 deletions(-) diff --git a/crates/tui/src/task_manager.rs b/crates/tui/src/task_manager.rs index 6b6e5d5b39..c051a53b40 100644 --- a/crates/tui/src/task_manager.rs +++ b/crates/tui/src/task_manager.rs @@ -1611,6 +1611,12 @@ impl TaskManager { .with_context(|| format!("Failed to delete task {}", task_path.display())); } } + // The durable record is gone, so the in-memory state must follow before + // artifact cleanup: a cleanup failure may leave orphan files, but never + // a live task that the disk no longer knows about. + state.tasks.remove(task_id); + state.queue.retain(|queued_id| queued_id != task_id); + self.persist_queue_locked(&state.queue)?; if artifacts_path.exists() { fs::remove_dir_all(&artifacts_path).with_context(|| { format!( @@ -1620,9 +1626,6 @@ impl TaskManager { })?; } - state.tasks.remove(task_id); - state.queue.retain(|queued_id| queued_id != task_id); - self.persist_queue_locked(&state.queue)?; Ok(true) } @@ -3073,6 +3076,63 @@ mod tests { Ok(()) } + #[tokio::test] + async fn forkguard_terminal_task_delete_artifact_failure_leaves_no_memory_ghost() -> Result<()> + { + let root = tempfile::tempdir()?; + let manager = TaskManager::start_with_executor( + test_config(root.path().to_path_buf()), + Arc::new(MockExecutor), + ) + .await?; + let mut task = sample_task_record(); + task.id = "task_feedfacefeedface".to_string(); + task.status = TaskStatus::Completed; + task.ended_at = Some(Utc::now()); + { + let mut state = manager.state.lock().await; + state.tasks.insert(task.id.clone(), task.clone()); + } + manager.persist_task_locked(&task)?; + // A regular file where the artifact directory belongs fails + // remove_dir_all on every platform without relying on permission bits + // (which a root CI runner would bypass anyway). + fs::create_dir_all(&manager.artifacts_dir)?; + let orphan_path = manager.artifacts_dir.join(&task.id); + fs::write(&orphan_path, "not a directory")?; + + let error = manager + .delete_terminal_task(&task.id) + .await + .expect_err("artifact cleanup failure must still be reported"); + assert!( + error + .to_string() + .contains("Failed to delete task artifacts"), + "{error:#}" + ); + + // The durable record is gone, so the failure may only leave orphan + // files: the in-process ghost must be gone too, and the retry must be + // an idempotent no-op instead of a second error. + assert!(!manager.tasks_dir.join(format!("{}.json", task.id)).exists()); + assert!(manager.get_task(&task.id).await.is_err()); + assert!(!manager.delete_terminal_task(&task.id).await?); + assert!(orphan_path.is_file()); + + // A restart must observe the same state the failed call left behind. + drop(manager); + let restarted = TaskManager::start_with_executor( + test_config(root.path().to_path_buf()), + Arc::new(MockExecutor), + ) + .await?; + assert!(restarted.get_task(&task.id).await.is_err()); + assert!(orphan_path.is_file()); + restarted.shutdown(); + Ok(()) + } + #[tokio::test] async fn preallocated_task_ids_are_validated_and_collision_safe() -> Result<()> { let root = std::env::temp_dir().join(format!("deepseek-task-test-{}", Uuid::new_v4())); From f9778a4e23dc8fd0d04dab65284b872939d435da Mon Sep 17 00:00:00 2001 From: h3c-hexin <13790929+h3c-hexin@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:32:41 +0800 Subject: [PATCH 3/3] test(tasks): stop workers before reopening the task store Signed-off-by: h3c-hexin <13790929+h3c-hexin@users.noreply.github.com> --- crates/tui/src/task_manager.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/tui/src/task_manager.rs b/crates/tui/src/task_manager.rs index c051a53b40..a6183df55c 100644 --- a/crates/tui/src/task_manager.rs +++ b/crates/tui/src/task_manager.rs @@ -3120,8 +3120,18 @@ mod tests { assert!(!manager.delete_terminal_task(&task.id).await?); assert!(orphan_path.is_file()); - // A restart must observe the same state the failed call left behind. + // Stop the old workers before reopening the store. Dropping our Arc + // alone leaves the workers' strong references alive. + let stopped = Arc::downgrade(&manager); + manager.shutdown(); drop(manager); + tokio::time::timeout(Duration::from_secs(5), async { + while stopped.strong_count() != 0 { + tokio::task::yield_now().await; + } + }) + .await + .expect("old task manager workers must exit before reopening the store"); let restarted = TaskManager::start_with_executor( test_config(root.path().to_path_buf()), Arc::new(MockExecutor),