diff --git a/crates/app-tauri/src/commands.rs b/crates/app-tauri/src/commands.rs index 0a20663b..67203f6e 100644 --- a/crates/app-tauri/src/commands.rs +++ b/crates/app-tauri/src/commands.rs @@ -4447,8 +4447,7 @@ pub async fn spawn_background_command( /// `cancel_background_task` — request cancellation of a running background task. /// -/// The terminal `Cancelled` state is written by the completion sink; this returns -/// the task as currently persisted (absent if it no longer exists). +/// Returns the task as currently persisted after the cancel request. /// /// # Errors /// Returns an [`ErrorDto`] (`INVALID` for a malformed id, `PROCESS`/`STORE` on @@ -4457,13 +4456,13 @@ pub async fn spawn_background_command( pub async fn cancel_background_task( task_id: String, state: State<'_, AppState>, -) -> Result, ErrorDto> { +) -> Result { let id = parse_task_id(&task_id)?; state .cancel_background_task .execute(id) .await - .map(|out| out.task.map(BackgroundTaskDto::from)) + .map(|out| BackgroundTaskDto::from(out.task)) .map_err(ErrorDto::from) } diff --git a/crates/application/src/background/mod.rs b/crates/application/src/background/mod.rs index 7d93132b..3e786bea 100644 --- a/crates/application/src/background/mod.rs +++ b/crates/application/src/background/mod.rs @@ -209,33 +209,103 @@ impl SpawnBackgroundCommand { /// Output of [`CancelBackgroundTask`]. #[derive(Debug, Clone)] pub struct CancelBackgroundTaskOutput { - /// The task as currently persisted, if it still exists. The terminal - /// `Cancelled` state is written by the completion sink, not here. - pub task: Option, + /// The task as currently persisted after the cancel request. + pub task: BackgroundTask, } /// Requests cancellation of a running background task. pub struct CancelBackgroundTask { store: Arc, runner: Arc, + clock: Arc, + events: Option>, } impl CancelBackgroundTask { /// Wires the use case to its ports. #[must_use] - pub fn new(store: Arc, runner: Arc) -> Self { - Self { store, runner } + pub fn new( + store: Arc, + runner: Arc, + clock: Arc, + ) -> Self { + Self { + store, + runner, + clock, + events: None, + } } - /// Signals the runner to cancel `task_id` (idempotent). + /// Attaches the public event stream publisher. + #[must_use] + pub fn with_events(mut self, events: Arc) -> Self { + self.events = Some(events); + self + } + + /// Projects `Cancelling` immediately, then signals the runner to cancel `task_id`. /// /// # Errors - /// [`AppError`] if the runner or store fails. + /// [`AppError`] if the task is missing, terminal, or the runner/store fails. pub async fn execute(&self, task_id: TaskId) -> Result { - self.runner.cancel(task_id).await.map_err(map_port_err)?; - let task = self.store.get(task_id).await.map_err(map_port_err)?; + let task = self + .store + .get(task_id) + .await + .map_err(map_port_err)? + .ok_or_else(|| AppError::NotFound(format!("background task {task_id}")))?; + if task.is_terminal() { + return Err(AppError::Invalid(format!( + "background task {task_id} is already terminal ({:?})", + task.state + ))); + } + + let cancelling = if task.state == BackgroundTaskState::Cancelling { + task + } else { + let now = u64::try_from(self.clock.now_millis().max(0)).unwrap_or(0); + let cancelling = task + .transition(BackgroundTaskState::Cancelling, now) + .map_err(|e| AppError::Invalid(e.to_string()))?; + self.store.save(&cancelling).await.map_err(map_port_err)?; + self.publish_state_changed(&cancelling); + cancelling + }; + + if let Err(err) = self.runner.cancel(task_id).await { + let now = u64::try_from(self.clock.now_millis().max(0)).unwrap_or(0); + let failed = cancelling + .complete(BackgroundTaskResult::CancelFailed { + finished_at_ms: now, + error: format!("failed to cancel background task: {err}"), + }) + .map_err(|e| AppError::Invalid(e.to_string()))?; + self.store.save(&failed).await.map_err(map_port_err)?; + self.publish_state_changed(&failed); + return Err(map_port_err(err)); + } + + let task = self + .store + .get(task_id) + .await + .map_err(map_port_err)? + .unwrap_or(cancelling); Ok(CancelBackgroundTaskOutput { task }) } + + fn publish_state_changed(&self, task: &BackgroundTask) { + if let Some(events) = &self.events { + events.publish(DomainEvent::BackgroundTaskStateChanged { + project_id: task.project_id, + task_id: task.id, + owner_agent_id: task.owner_agent_id, + state: task.state, + }); + } + } } /// Read-only recall of the command invocation a background task was spawned with. diff --git a/crates/application/src/orchestrator/service.rs b/crates/application/src/orchestrator/service.rs index 5e79ff53..7b7b2c59 100644 --- a/crates/application/src/orchestrator/service.rs +++ b/crates/application/src/orchestrator/service.rs @@ -942,12 +942,14 @@ impl OrchestratorService { owner_agent_id, rendezvous, }, - BackgroundTaskResult::Failure { .. } => DomainEvent::BackgroundTaskFailed { - project_id: project.id, - task_id, - owner_agent_id, - rendezvous, - }, + BackgroundTaskResult::Failure { .. } | BackgroundTaskResult::CancelFailed { .. } => { + DomainEvent::BackgroundTaskFailed { + project_id: project.id, + task_id, + owner_agent_id, + rendezvous, + } + } BackgroundTaskResult::Cancelled { .. } | BackgroundTaskResult::Expired { .. } => { DomainEvent::BackgroundTaskCancelled { project_id: project.id, @@ -1333,9 +1335,11 @@ impl OrchestratorService { BackgroundTaskState::Queued => "Queued", BackgroundTaskState::Running => "Running", BackgroundTaskState::Waiting => "Waiting", + BackgroundTaskState::Cancelling => "Cancelling", BackgroundTaskState::Completed => "Completed", BackgroundTaskState::Failed => "Failed", BackgroundTaskState::Cancelled => "Cancelled", + BackgroundTaskState::CancelFailed => "CancelFailed", BackgroundTaskState::Expired => "Expired", }; Ok(OrchestratorOutcome { diff --git a/crates/application/src/orchestrator/wake.rs b/crates/application/src/orchestrator/wake.rs index 5aeb0d78..ef2da4eb 100644 --- a/crates/application/src/orchestrator/wake.rs +++ b/crates/application/src/orchestrator/wake.rs @@ -317,6 +317,10 @@ fn background_completion_prompt(task: &BackgroundTask) -> String { "La tâche de fond {} est terminée : annulée.\nRaison : {}", task.id, reason ), + Some(BackgroundTaskResult::CancelFailed { error, .. }) => format!( + "La tâche de fond {} est terminée : échec de l'annulation.\nErreur : {}", + task.id, error + ), Some(BackgroundTaskResult::Expired { reason, .. }) => format!( "La tâche de fond {} est terminée : expirée.\nRaison : {}", task.id, reason diff --git a/crates/application/src/plugin/mod.rs b/crates/application/src/plugin/mod.rs index ece68647..afc932c7 100644 --- a/crates/application/src/plugin/mod.rs +++ b/crates/application/src/plugin/mod.rs @@ -1940,9 +1940,11 @@ fn background_task_state_label(state: BackgroundTaskState) -> &'static str { BackgroundTaskState::Queued => "queued", BackgroundTaskState::Running => "running", BackgroundTaskState::Waiting => "waiting", + BackgroundTaskState::Cancelling => "cancelling", BackgroundTaskState::Completed => "completed", BackgroundTaskState::Failed => "failed", BackgroundTaskState::Cancelled => "cancelled", + BackgroundTaskState::CancelFailed => "cancelFailed", BackgroundTaskState::Expired => "expired", } } diff --git a/crates/application/src/workstate/mod.rs b/crates/application/src/workstate/mod.rs index a1ef5b37..cd7fada2 100644 --- a/crates/application/src/workstate/mod.rs +++ b/crates/application/src/workstate/mod.rs @@ -689,6 +689,9 @@ fn flatten_background_result( | Some(BackgroundTaskResult::Expired { reason, .. }) => { (None, Some(reason.clone()), None, None) } + Some(BackgroundTaskResult::CancelFailed { error, .. }) => { + (None, Some(error.clone()), None, None) + } None => (None, None, None, None), } } diff --git a/crates/application/tests/background_task_cancel.rs b/crates/application/tests/background_task_cancel.rs new file mode 100644 index 00000000..eb1e1cf6 --- /dev/null +++ b/crates/application/tests/background_task_cancel.rs @@ -0,0 +1,306 @@ +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; + +use async_trait::async_trait; + +use application::CancelBackgroundTask; +use domain::ports::{ + BackgroundCompletionStream, BackgroundTaskHandle, BackgroundTaskPortError, + BackgroundTaskRunner, BackgroundTaskSpec, BackgroundTaskStore, Clock, EventBus, EventStream, +}; +use domain::{ + AgentId, BackgroundTask, BackgroundTaskKind, BackgroundTaskResult, BackgroundTaskState, + BackgroundTaskWakePolicy, DomainEvent, ProjectId, TaskId, +}; +use uuid::Uuid; + +fn tid(n: u128) -> TaskId { + TaskId::from_uuid(Uuid::from_u128(n)) +} + +fn pid(n: u128) -> ProjectId { + ProjectId::from_uuid(Uuid::from_u128(n)) +} + +fn aid(n: u128) -> AgentId { + AgentId::from_uuid(Uuid::from_u128(n)) +} + +fn running_task(id: TaskId) -> BackgroundTask { + BackgroundTask::new( + id, + pid(1), + aid(2), + BackgroundTaskKind::Command { + label: "build".to_owned(), + }, + BackgroundTaskWakePolicy::WakeOwner, + 100, + None, + ) + .unwrap() + .transition(BackgroundTaskState::Running, 110) + .unwrap() +} + +#[derive(Default)] +struct FakeStore { + tasks: Mutex>, + saves: Mutex>, +} + +impl FakeStore { + fn insert(&self, task: BackgroundTask) { + self.tasks.lock().unwrap().insert(task.id, task); + } + + fn saved_states(&self) -> Vec { + self.saves.lock().unwrap().clone() + } +} + +#[async_trait] +impl BackgroundTaskStore for FakeStore { + async fn create(&self, task: &BackgroundTask) -> Result<(), BackgroundTaskPortError> { + self.insert(task.clone()); + Ok(()) + } + + async fn get(&self, id: TaskId) -> Result, BackgroundTaskPortError> { + Ok(self.tasks.lock().unwrap().get(&id).cloned()) + } + + async fn save(&self, task: &BackgroundTask) -> Result<(), BackgroundTaskPortError> { + self.saves.lock().unwrap().push(task.state); + self.insert(task.clone()); + Ok(()) + } + + async fn list_open_for_agent( + &self, + agent_id: AgentId, + ) -> Result, BackgroundTaskPortError> { + Ok(self + .tasks + .lock() + .unwrap() + .values() + .filter(|task| task.owner_agent_id == agent_id && !task.is_terminal()) + .cloned() + .collect()) + } + + async fn list_undelivered_completions( + &self, + ) -> Result, BackgroundTaskPortError> { + Ok(Vec::new()) + } + + async fn mark_completion_delivered( + &self, + _task_id: TaskId, + ) -> Result<(), BackgroundTaskPortError> { + Ok(()) + } +} + +struct FakeRunner { + result: Mutex>, + cancels: Mutex>, +} + +impl FakeRunner { + fn ok() -> Self { + Self { + result: Mutex::new(Ok(())), + cancels: Mutex::new(Vec::new()), + } + } + + fn failing(message: &str) -> Self { + Self { + result: Mutex::new(Err(BackgroundTaskPortError::Runner(message.to_owned()))), + cancels: Mutex::new(Vec::new()), + } + } + + fn cancels(&self) -> Vec { + self.cancels.lock().unwrap().clone() + } +} + +#[async_trait] +impl BackgroundTaskRunner for FakeRunner { + async fn spawn( + &self, + spec: BackgroundTaskSpec, + ) -> Result { + Ok(BackgroundTaskHandle { + task_id: spec.task_id, + }) + } + + async fn cancel(&self, task_id: TaskId) -> Result<(), BackgroundTaskPortError> { + self.cancels.lock().unwrap().push(task_id); + self.result.lock().unwrap().clone() + } + + fn subscribe_completions(&self) -> BackgroundCompletionStream { + Box::new(std::iter::empty()) + } +} + +#[derive(Default)] +struct SpyEvents { + events: Mutex>, +} + +impl SpyEvents { + fn events(&self) -> Vec { + self.events.lock().unwrap().clone() + } +} + +impl EventBus for SpyEvents { + fn publish(&self, event: DomainEvent) { + self.events.lock().unwrap().push(event); + } + + fn subscribe(&self) -> EventStream { + Box::new(std::iter::empty()) + } +} + +struct FixedClock(i64); + +impl Clock for FixedClock { + fn now_millis(&self) -> i64 { + self.0 + } +} + +fn usecase( + store: Arc, + runner: Arc, + events: Arc, +) -> CancelBackgroundTask { + CancelBackgroundTask::new(store, runner, Arc::new(FixedClock(200))).with_events(events) +} + +#[tokio::test] +async fn cancel_running_projects_cancelling_immediately_then_calls_runner() { + let store = Arc::new(FakeStore::default()); + let runner = Arc::new(FakeRunner::ok()); + let events = Arc::new(SpyEvents::default()); + store.insert(running_task(tid(10))); + + let out = usecase(Arc::clone(&store), Arc::clone(&runner), Arc::clone(&events)) + .execute(tid(10)) + .await + .unwrap(); + + assert_eq!(out.task.state, BackgroundTaskState::Cancelling); + assert_eq!(store.saved_states(), vec![BackgroundTaskState::Cancelling]); + assert_eq!(runner.cancels(), vec![tid(10)]); + assert_eq!( + events.events(), + vec![DomainEvent::BackgroundTaskStateChanged { + project_id: pid(1), + task_id: tid(10), + owner_agent_id: aid(2), + state: BackgroundTaskState::Cancelling, + }] + ); +} + +#[tokio::test] +async fn cancel_terminal_task_is_explicit_error_without_runner_call() { + let store = Arc::new(FakeStore::default()); + let runner = Arc::new(FakeRunner::ok()); + let events = Arc::new(SpyEvents::default()); + let done = running_task(tid(10)) + .complete(BackgroundTaskResult::Success { + finished_at_ms: 120, + exit_code: Some(0), + summary: "ok".to_owned(), + stdout_tail: None, + stderr_tail: None, + }) + .unwrap(); + store.insert(done); + + let err = usecase(Arc::clone(&store), Arc::clone(&runner), events) + .execute(tid(10)) + .await + .unwrap_err(); + + assert!(matches!(err, application::AppError::Invalid(_))); + assert!(runner.cancels().is_empty()); + assert!(store.saved_states().is_empty()); +} + +#[tokio::test] +async fn cancel_already_cancelling_is_idempotent_and_retries_runner_cancel() { + let store = Arc::new(FakeStore::default()); + let runner = Arc::new(FakeRunner::ok()); + let events = Arc::new(SpyEvents::default()); + store.insert( + running_task(tid(10)) + .transition(BackgroundTaskState::Cancelling, 120) + .unwrap(), + ); + + let out = usecase(Arc::clone(&store), Arc::clone(&runner), events) + .execute(tid(10)) + .await + .unwrap(); + + assert_eq!(out.task.state, BackgroundTaskState::Cancelling); + assert_eq!(runner.cancels(), vec![tid(10)]); + assert!(store.saved_states().is_empty()); +} + +#[tokio::test] +async fn cancel_kill_failure_persists_cancel_failed_and_returns_error() { + let store = Arc::new(FakeStore::default()); + let runner = Arc::new(FakeRunner::failing("kill denied")); + let events = Arc::new(SpyEvents::default()); + store.insert(running_task(tid(10))); + + let err = usecase(Arc::clone(&store), Arc::clone(&runner), Arc::clone(&events)) + .execute(tid(10)) + .await + .unwrap_err(); + + assert!(matches!(err, application::AppError::Process(_))); + assert_eq!( + store.saved_states(), + vec![ + BackgroundTaskState::Cancelling, + BackgroundTaskState::CancelFailed, + ] + ); + let persisted = store.get(tid(10)).await.unwrap().unwrap(); + assert_eq!(persisted.state, BackgroundTaskState::CancelFailed); + assert!(matches!( + persisted.result, + Some(BackgroundTaskResult::CancelFailed { .. }) + )); + assert_eq!( + events.events(), + vec![ + DomainEvent::BackgroundTaskStateChanged { + project_id: pid(1), + task_id: tid(10), + owner_agent_id: aid(2), + state: BackgroundTaskState::Cancelling, + }, + DomainEvent::BackgroundTaskStateChanged { + project_id: pid(1), + task_id: tid(10), + owner_agent_id: aid(2), + state: BackgroundTaskState::CancelFailed, + }, + ] + ); +} diff --git a/crates/application/tests/workstate.rs b/crates/application/tests/workstate.rs index c6bbc578..4032c7b0 100644 --- a/crates/application/tests/workstate.rs +++ b/crates/application/tests/workstate.rs @@ -568,9 +568,15 @@ fn background_task( .unwrap() .transition(BackgroundTaskState::Waiting, created_at_ms + 20) .unwrap(), + BackgroundTaskState::Cancelling => base + .transition(BackgroundTaskState::Running, created_at_ms + 10) + .unwrap() + .transition(BackgroundTaskState::Cancelling, created_at_ms + 20) + .unwrap(), BackgroundTaskState::Completed | BackgroundTaskState::Failed | BackgroundTaskState::Cancelled + | BackgroundTaskState::CancelFailed | BackgroundTaskState::Expired => { let result = match state { BackgroundTaskState::Completed => BackgroundTaskResult::Success { @@ -591,13 +597,18 @@ fn background_task( finished_at_ms: created_at_ms + 20, reason: "cancelled".to_owned(), }, + BackgroundTaskState::CancelFailed => BackgroundTaskResult::CancelFailed { + finished_at_ms: created_at_ms + 20, + error: "cancel failed".to_owned(), + }, BackgroundTaskState::Expired => BackgroundTaskResult::Expired { finished_at_ms: created_at_ms + 20, reason: "expired".to_owned(), }, BackgroundTaskState::Queued | BackgroundTaskState::Running - | BackgroundTaskState::Waiting => unreachable!(), + | BackgroundTaskState::Waiting + | BackgroundTaskState::Cancelling => unreachable!(), }; let running = base .transition(BackgroundTaskState::Running, created_at_ms + 10) diff --git a/crates/backend/src/dto.rs b/crates/backend/src/dto.rs index 89fd0473..3c55ba8d 100644 --- a/crates/backend/src/dto.rs +++ b/crates/backend/src/dto.rs @@ -5161,9 +5161,11 @@ fn background_state_label(state: BackgroundTaskState) -> &'static str { BackgroundTaskState::Queued => "queued", BackgroundTaskState::Running => "running", BackgroundTaskState::Waiting => "waiting", + BackgroundTaskState::Cancelling => "cancelling", BackgroundTaskState::Completed => "completed", BackgroundTaskState::Failed => "failed", BackgroundTaskState::Cancelled => "cancelled", + BackgroundTaskState::CancelFailed => "cancelFailed", BackgroundTaskState::Expired => "expired", } } @@ -5201,6 +5203,9 @@ impl From for BackgroundTaskDto { | Some(BackgroundTaskResult::Expired { reason, .. }) => { (None, Some(reason.clone()), None, None) } + Some(BackgroundTaskResult::CancelFailed { error, .. }) => { + (None, Some(error.clone()), None, None) + } None => (None, None, None, None), }; Self { diff --git a/crates/backend/src/lib.rs b/crates/backend/src/lib.rs index 666eca2c..809c9b53 100644 --- a/crates/backend/src/lib.rs +++ b/crates/backend/src/lib.rs @@ -1357,6 +1357,7 @@ fn background_task_tail_bytes(task: &BackgroundTask) -> Vec { .into_bytes(), Some(BackgroundTaskResult::Cancelled { reason, .. }) | Some(BackgroundTaskResult::Expired { reason, .. }) => reason.clone().into_bytes(), + Some(BackgroundTaskResult::CancelFailed { error, .. }) => error.clone().into_bytes(), None => Vec::new(), } } @@ -2452,10 +2453,14 @@ impl BackendCore { ) .with_events(Arc::clone(&events_port)), ); - let cancel_background_task = Arc::new(CancelBackgroundTask::new( - Arc::clone(&background_tasks_port), - Arc::clone(&background_runner_port), - )); + let cancel_background_task = Arc::new( + CancelBackgroundTask::new( + Arc::clone(&background_tasks_port), + Arc::clone(&background_runner_port), + Arc::clone(&clock) as Arc, + ) + .with_events(Arc::clone(&events_port)), + ); let retry_background_task = Arc::new(RetryBackgroundTask::new( Arc::clone(&background_tasks_port), Arc::clone(&background_runner) as Arc, diff --git a/crates/domain/src/background_task.rs b/crates/domain/src/background_task.rs index 9b48fff1..151f7a5d 100644 --- a/crates/domain/src/background_task.rs +++ b/crates/domain/src/background_task.rs @@ -28,12 +28,16 @@ pub enum BackgroundTaskState { Running, /// Suspended while waiting for an external event. Waiting, + /// Cancellation has been accepted and the runner is stopping the resource. + Cancelling, /// Finished successfully. Completed, /// Finished with an error. Failed, /// Cancelled before natural completion. Cancelled, + /// Cancellation was requested but the runner could not stop the resource. + CancelFailed, /// Expired after its deadline. Expired, } @@ -44,7 +48,7 @@ impl BackgroundTaskState { pub const fn is_terminal(self) -> bool { matches!( self, - Self::Completed | Self::Failed | Self::Cancelled | Self::Expired + Self::Completed | Self::Failed | Self::Cancelled | Self::CancelFailed | Self::Expired ) } } @@ -342,6 +346,13 @@ pub enum BackgroundTaskResult { /// Cancellation reason. reason: String, }, + /// Cancellation failed. + CancelFailed { + /// Completion timestamp, epoch milliseconds. + finished_at_ms: u64, + /// Human-readable error. + error: String, + }, /// Expired completion. Expired { /// Completion timestamp, epoch milliseconds. @@ -375,6 +386,7 @@ impl BackgroundTaskResult { Self::Success { finished_at_ms, .. } | Self::Failure { finished_at_ms, .. } | Self::Cancelled { finished_at_ms, .. } + | Self::CancelFailed { finished_at_ms, .. } | Self::Expired { finished_at_ms, .. } => *finished_at_ms, } } @@ -408,6 +420,9 @@ impl BackgroundTaskResult { Self::Cancelled { reason, .. } | Self::Expired { reason, .. } => { validate_text("reason", reason)?; } + Self::CancelFailed { error, .. } => { + validate_text("error", error)?; + } } Ok(()) } @@ -417,6 +432,7 @@ impl BackgroundTaskResult { Self::Success { .. } => BackgroundTaskState::Completed, Self::Failure { .. } => BackgroundTaskState::Failed, Self::Cancelled { .. } => BackgroundTaskState::Cancelled, + Self::CancelFailed { .. } => BackgroundTaskState::CancelFailed, Self::Expired { .. } => BackgroundTaskState::Expired, } } @@ -459,13 +475,22 @@ pub enum BackgroundTaskError { } fn is_transition_allowed(from: BackgroundTaskState, to: BackgroundTaskState) -> bool { - use BackgroundTaskState::{Cancelled, Completed, Expired, Failed, Queued, Running, Waiting}; + use BackgroundTaskState::{ + CancelFailed, Cancelled, Cancelling, Completed, Expired, Failed, Queued, Running, Waiting, + }; matches!( (from, to), - (Queued, Running | Cancelled | Expired) - | (Running, Waiting | Completed | Failed | Cancelled | Expired) - | (Waiting, Running | Completed | Failed | Cancelled | Expired) + ( + Queued, + Running | Cancelling | Cancelled | CancelFailed | Expired + ) | ( + Running, + Waiting | Cancelling | Completed | Failed | Cancelled | CancelFailed | Expired + ) | ( + Waiting, + Running | Cancelling | Completed | Failed | Cancelled | CancelFailed | Expired + ) | (Cancelling, Cancelled | CancelFailed | Expired) ) } diff --git a/crates/domain/tests/background_task.rs b/crates/domain/tests/background_task.rs index beac463c..94459473 100644 --- a/crates/domain/tests/background_task.rs +++ b/crates/domain/tests/background_task.rs @@ -134,6 +134,11 @@ fn allowed_non_terminal_transitions_update_timestamp() { .transition(BackgroundTaskState::Running, 140) .unwrap(); assert_eq!(running_again.state, BackgroundTaskState::Running); + + let cancelling = running_again + .transition(BackgroundTaskState::Cancelling, 150) + .unwrap(); + assert_eq!(cancelling.state, BackgroundTaskState::Cancelling); } #[test] @@ -245,6 +250,18 @@ fn result_variants_determine_terminal_state() { .state, BackgroundTaskState::Expired ); + assert_eq!( + running + .transition(BackgroundTaskState::Cancelling, 125) + .unwrap() + .complete(BackgroundTaskResult::CancelFailed { + finished_at_ms: 130, + error: "kill failed".into(), + }) + .unwrap() + .state, + BackgroundTaskState::CancelFailed + ); } #[test] diff --git a/crates/infrastructure/src/background_task/runner.rs b/crates/infrastructure/src/background_task/runner.rs index 6d6b368a..269392a4 100644 --- a/crates/infrastructure/src/background_task/runner.rs +++ b/crates/infrastructure/src/background_task/runner.rs @@ -139,11 +139,14 @@ impl CommandBackgroundRunner { .filter(|s| !s.is_empty()); // Reap the real exit status (works for a natural exit and a forced kill). - let exit_code = pty - .kill(&control.pty_handle) - .await - .ok() - .and_then(|status| status.code); + let exit_code = match outcome { + WaitOutcome::Cancelled => None, + WaitOutcome::Expired | WaitOutcome::Exited => pty + .kill(&control.pty_handle) + .await + .ok() + .and_then(|status| status.code), + }; let finished_at_ms = u64::try_from(clock.now_millis().max(0)).unwrap_or(0); let result = match outcome { @@ -281,8 +284,12 @@ impl BackgroundTaskRunner for CommandBackgroundRunner { .cloned(); // Unknown task ⇒ already terminal or never ours: cancellation is idempotent. if let Some(control) = control { + self.pty + .kill(&control.pty_handle) + .await + .map_err(|e| BackgroundTaskPortError::Runner(format!("pty kill failed: {e}")))?; control.cancelled.store(true, Ordering::SeqCst); - control.cancel.notify_one(); + control.cancel.notify_waiters(); } Ok(()) } diff --git a/crates/infrastructure/src/store/background_task.rs b/crates/infrastructure/src/store/background_task.rs index e4706c52..6787ef85 100644 --- a/crates/infrastructure/src/store/background_task.rs +++ b/crates/infrastructure/src/store/background_task.rs @@ -108,7 +108,8 @@ impl FsBackgroundTaskStore { /// Reconciles persisted tasks against live runtime handles at boot. /// /// B2 rule: `Running`/`Waiting` tasks missing from `live_task_ids` are marked - /// `Failed` with a synthetic restart-loss failure and become delivery-pending. + /// `Failed`; `Cancelling` tasks missing from `live_task_ids` are marked + /// `CancelFailed`. Both become delivery-pending. /// Already-terminal undelivered tasks are reported as delivery-pending. `Queued` /// tasks are left open. /// @@ -127,18 +128,13 @@ impl FsBackgroundTaskStore { for (project_id, doc) in &mut state.docs { for task in &mut doc.tasks { match task.state { - BackgroundTaskState::Running | BackgroundTaskState::Waiting + BackgroundTaskState::Running + | BackgroundTaskState::Waiting + | BackgroundTaskState::Cancelling if !live.contains(&task.id) => { let finished_at_ms = now_ms.max(task.updated_at_ms); - let result = BackgroundTaskResult::Failure { - finished_at_ms, - exit_code: None, - error: "background task lost its runtime handle during IdeA restart" - .into(), - stdout_tail: None, - stderr_tail: None, - }; + let result = lost_runtime_result(task.state, finished_at_ms); *task = task.complete(result).map_err(invalid_task)?; report.failed_task_ids.push(task.id); report.delivery_pending_task_ids.push(task.id); @@ -158,18 +154,13 @@ impl FsBackgroundTaskStore { for task in &mut doc.tasks { if matches!( task.state, - BackgroundTaskState::Running | BackgroundTaskState::Waiting + BackgroundTaskState::Running + | BackgroundTaskState::Waiting + | BackgroundTaskState::Cancelling ) && !live.contains(&task.id) { let finished_at_ms = now_ms.max(task.updated_at_ms); - let result = BackgroundTaskResult::Failure { - finished_at_ms, - exit_code: None, - error: "background task lost its runtime handle during IdeA restart" - .into(), - stdout_tail: None, - stderr_tail: None, - }; + let result = lost_runtime_result(task.state, finished_at_ms); *task = task.complete(result).map_err(invalid_task)?; } } @@ -444,3 +435,21 @@ fn io_error(error: std::io::Error) -> BackgroundTaskPortError { fn invalid_task(error: domain::BackgroundTaskError) -> BackgroundTaskPortError { BackgroundTaskPortError::Invalid(error.to_string()) } + +fn lost_runtime_result(state: BackgroundTaskState, finished_at_ms: u64) -> BackgroundTaskResult { + if state == BackgroundTaskState::Cancelling { + BackgroundTaskResult::CancelFailed { + finished_at_ms, + error: "background task cancellation lost its runtime handle during IdeA restart" + .into(), + } + } else { + BackgroundTaskResult::Failure { + finished_at_ms, + exit_code: None, + error: "background task lost its runtime handle during IdeA restart".into(), + stdout_tail: None, + stderr_tail: None, + } + } +} diff --git a/crates/infrastructure/tests/background_task_runner.rs b/crates/infrastructure/tests/background_task_runner.rs index 32bd3871..627e47d3 100644 --- a/crates/infrastructure/tests/background_task_runner.rs +++ b/crates/infrastructure/tests/background_task_runner.rs @@ -33,6 +33,7 @@ struct FakePtyState { handle: Option, status: Option, kills: usize, + fail_kill: bool, scrollback: Vec, } @@ -57,6 +58,10 @@ impl FakePty { self.state.lock().unwrap().kills } + fn fail_kill(&self) { + self.state.lock().unwrap().fail_kill = true; + } + fn subscribe_count(&self) -> usize { self.subscribe_count.load(Ordering::SeqCst) } @@ -105,6 +110,9 @@ impl PtyPort for FakePty { async fn kill(&self, _handle: &PtyHandle) -> Result { let mut state = self.state.lock().unwrap(); state.kills += 1; + if state.fail_kill { + return Err(PtyError::Io("kill denied".to_owned())); + } let status = state.status.get_or_insert(ExitStatus { code: Some(130) }); Ok(*status) } @@ -314,6 +322,31 @@ async fn deadline_expires_and_kills_when_wait_never_resolves() { assert_eq!(pty.subscribe_count(), 0); } +#[tokio::test] +async fn cancel_surfaces_pty_kill_failure_without_completion() { + let pty = Arc::new(FakePty::default()); + pty.fail_kill(); + let clock = Arc::new(FakeClock::default()); + let (runner, _completions) = runner_with(Arc::clone(&pty), clock); + let task_id = TaskId::from_uuid(id(16)); + + runner + .spawn(spawn_spec(task_id, None)) + .await + .expect("spawn succeeds"); + + let err = runner + .cancel(task_id) + .await + .expect_err("kill failure surfaces"); + assert!(matches!(err, domain::BackgroundTaskPortError::Runner(_))); + assert_eq!(pty.kill_count(), 1); + assert!( + runner.pty_handle_for(task_id).is_some(), + "failed cancel keeps the live handle available for diagnostics/retry" + ); +} + #[tokio::test] async fn cancel_wins_even_if_output_is_still_available() { let pty = Arc::new(FakePty::default()); diff --git a/crates/infrastructure/tests/background_task_store.rs b/crates/infrastructure/tests/background_task_store.rs index 85a2ba14..a09867a2 100644 --- a/crates/infrastructure/tests/background_task_store.rs +++ b/crates/infrastructure/tests/background_task_store.rs @@ -86,6 +86,12 @@ fn running_task(id: u128, project: u128, owner: u128, now_ms: u64) -> Background .unwrap() } +fn cancelling_task(id: u128, project: u128, owner: u128, now_ms: u64) -> BackgroundTask { + running_task(id, project, owner, now_ms) + .transition(BackgroundTaskState::Cancelling, now_ms + 2) + .unwrap() +} + fn completed_task(id: u128, project: u128, owner: u128, now_ms: u64) -> BackgroundTask { running_task(id, project, owner, now_ms) .complete(BackgroundTaskResult::Success { @@ -337,6 +343,31 @@ async fn reconcile_marks_running_without_live_handle_failed_and_pending() { ); } +#[tokio::test] +async fn reconcile_marks_cancelling_without_live_handle_cancel_failed_and_pending() { + let tmp = TempDir::new(); + let store = FsBackgroundTaskStore::new(&tmp.project_path()); + let lost = cancelling_task(1, 10, 100, 1_000); + + store.create(&lost).await.unwrap(); + + let report = store.reconcile_boot(&[], 2_000).await.unwrap(); + + assert_eq!(report.failed_task_ids, vec![lost.id]); + assert_eq!(report.delivery_pending_task_ids, vec![lost.id]); + + let lost_after = store.get(lost.id).await.unwrap().unwrap(); + assert_eq!(lost_after.state, BackgroundTaskState::CancelFailed); + assert!(lost_after.has_pending_completion_delivery()); + assert!(matches!( + lost_after.result, + Some(BackgroundTaskResult::CancelFailed { + finished_at_ms: 2_000, + .. + }) + )); +} + #[tokio::test] async fn reconcile_uses_monotonic_timestamp_when_clock_is_behind() { let tmp = TempDir::new(); diff --git a/crates/web-server/src/lib.rs b/crates/web-server/src/lib.rs index aec765ec..ad2a7362 100644 --- a/crates/web-server/src/lib.rs +++ b/crates/web-server/src/lib.rs @@ -2955,7 +2955,7 @@ async fn invoke_cancel_background_task( .execute(task_id) .await .map_err(ErrorDto::from)?; - serde_json::to_value(output.task.map(BackgroundTaskDto::from)).map_err(serialization_error) + serde_json::to_value(BackgroundTaskDto::from(output.task)).map_err(serialization_error) } async fn invoke_retry_background_task( diff --git a/frontend/src/adapters/workStateNormalization.ts b/frontend/src/adapters/workStateNormalization.ts index 9ae54422..a47655a9 100644 --- a/frontend/src/adapters/workStateNormalization.ts +++ b/frontend/src/adapters/workStateNormalization.ts @@ -86,7 +86,11 @@ function normalizeBackgroundStatus( const raw = typeof value === "string" ? value.toLowerCase() : ""; if (raw === "completed" || raw === "success" || raw === "succeeded") return "completed"; if (raw === "failed" || raw === "failure") return "failed"; + if (raw === "cancelfailed" || raw === "cancel_failed" || raw === "cancel-failed") { + return "cancelFailed"; + } if (raw === "cancelled" || raw === "canceled" || raw === "expired") return "cancelled"; + if (raw === "cancelling" || raw === "canceling") return "cancelling"; if (raw === "running" || raw === "started") return "running"; if (raw === "delivered") return "delivered"; // Backend `queued`/`waiting` collapse to the cancellable "pending" bucket. diff --git a/frontend/src/domain/index.ts b/frontend/src/domain/index.ts index 06536921..a7d78f04 100644 --- a/frontend/src/domain/index.ts +++ b/frontend/src/domain/index.ts @@ -612,7 +612,15 @@ export interface BackgroundCompletion { ownerAgentId: string; projectId: string; kind: string; - status: "running" | "completed" | "failed" | "cancelled" | "pending" | "delivered"; + status: + | "running" + | "cancelling" + | "completed" + | "failed" + | "cancelFailed" + | "cancelled" + | "pending" + | "delivered"; exitCode: number | null; /** Human-readable summary / error / reason of a terminal result (ticket #5). */ summary: string | null; diff --git a/frontend/src/features/projects/projectActivity.ts b/frontend/src/features/projects/projectActivity.ts index fcf7db3f..3444f1b4 100644 --- a/frontend/src/features/projects/projectActivity.ts +++ b/frontend/src/features/projects/projectActivity.ts @@ -3,7 +3,7 @@ import type { BackgroundCompletion, ProjectWorkState } from "@/domain"; export type ProjectActivityStatus = "none" | "idle" | "busy"; const ACTIVE_BACKGROUND_STATUSES: ReadonlySet = - new Set(["pending", "running"]); + new Set(["pending", "running", "cancelling"]); export function projectActivityStatus( state: ProjectWorkState | null | undefined, diff --git a/frontend/src/features/web/WebWorkspace.tsx b/frontend/src/features/web/WebWorkspace.tsx index e72fb514..ed70ff31 100644 --- a/frontend/src/features/web/WebWorkspace.tsx +++ b/frontend/src/features/web/WebWorkspace.tsx @@ -507,17 +507,23 @@ function AgentLiveRow({ const TASK_STATUS_LABEL: Record = { running: "Running", + cancelling: "Cancelling", completed: "Completed", failed: "Failed", + cancelFailed: "Cancel failed", cancelled: "Cancelled", pending: "Pending", delivered: "Delivered", }; function taskStatusClass(status: BackgroundCompletion["status"]): string { - if (status === "running" || status === "pending") return "bg-warning/15 text-warning"; + if (status === "running" || status === "pending" || status === "cancelling") { + return "bg-warning/15 text-warning"; + } if (status === "completed" || status === "delivered") return "bg-success/10 text-success"; - if (status === "failed" || status === "cancelled") return "bg-danger/10 text-danger"; + if (status === "failed" || status === "cancelFailed" || status === "cancelled") { + return "bg-danger/10 text-danger"; + } return "bg-raised text-muted"; } diff --git a/frontend/src/features/workstate/ProjectWorkStatePanel.tsx b/frontend/src/features/workstate/ProjectWorkStatePanel.tsx index 2d8e4ea7..c36e490a 100644 --- a/frontend/src/features/workstate/ProjectWorkStatePanel.tsx +++ b/frontend/src/features/workstate/ProjectWorkStatePanel.tsx @@ -92,10 +92,14 @@ function formatTaskStatus(status: BackgroundCompletion["status"]): string { switch (status) { case "running": return "Running"; + case "cancelling": + return "Cancelling"; case "completed": return "Completed"; case "failed": return "Failed"; + case "cancelFailed": + return "Cancel failed"; case "cancelled": return "Cancelled"; case "delivered": @@ -107,16 +111,33 @@ function formatTaskStatus(status: BackgroundCompletion["status"]): string { /** Terminal (finished) task states that carry a result summary (ticket #5). */ function isTerminalTaskStatus(status: BackgroundCompletion["status"]): boolean { - return status === "completed" || status === "failed" || status === "cancelled"; + return ( + status === "completed" || + status === "failed" || + status === "cancelFailed" || + status === "cancelled" + ); } function taskStatusClass(status: BackgroundCompletion["status"]): string { - if (status === "running" || status === "pending") return "bg-warning/15 text-warning"; + if (status === "running" || status === "pending" || status === "cancelling") { + return "bg-warning/15 text-warning"; + } if (status === "completed" || status === "delivered") return "bg-success/10 text-success"; - if (status === "failed" || status === "cancelled") return "bg-danger/10 text-danger"; + if (status === "failed" || status === "cancelFailed" || status === "cancelled") { + return "bg-danger/10 text-danger"; + } return "bg-raised text-muted"; } +function canCancelTask(status: BackgroundCompletion["status"]): boolean { + return status === "running" || status === "pending" || status === "cancelling"; +} + +function canRetryTask(status: BackgroundCompletion["status"]): boolean { + return status === "failed" || status === "cancelFailed" || status === "cancelled"; +} + function requesterLabel(ticket: AgentTicketState): string { if (ticket.source.kind === "human") return "Human"; const id = shortTicket(ticket.source.agentId); @@ -346,8 +367,8 @@ function BackgroundTaskRow({ const attachmentRef = useRef(null); const attachSeqRef = useRef(0); const hasOutput = Boolean(task.stdoutTail || task.stderrTail); - const canCancel = task.status === "running" || task.status === "pending"; - const canRetry = task.status === "failed" || task.status === "cancelled"; + const canCancel = canCancelTask(task.status); + const canRetry = canRetryTask(task.status); const canAttachLive = canCancel; function detachLive(): void { diff --git a/frontend/src/features/workstate/workstate.test.tsx b/frontend/src/features/workstate/workstate.test.tsx index 8663ebf7..030e095a 100644 --- a/frontend/src/features/workstate/workstate.test.tsx +++ b/frontend/src/features/workstate/workstate.test.tsx @@ -224,6 +224,21 @@ describe("ProjectWorkStatePanel", () => { createdAtMs: 16, updatedAtMs: 17, }, + { + taskId: "task-cancelling-1", + kind: "command", + state: "cancelling", + createdAtMs: 18, + updatedAtMs: 19, + }, + { + taskId: "task-cancel-failed-1", + kind: "command", + state: "cancelFailed", + summary: "kill denied", + createdAtMs: 20, + updatedAtMs: 21, + }, ], }, ], @@ -237,6 +252,8 @@ describe("ProjectWorkStatePanel", () => { ["task-queued-1", "pending"], ["task-failed-1", "failed"], ["task-expired-1", "cancelled"], + ["task-cancelling-1", "cancelling"], + ["task-cancel-failed-1", "cancelFailed"], ]); // Optional fields omitted by the backend normalize to null (not undefined/NaN). expect(tasks[0]?.exitCode).toBeNull(); @@ -249,25 +266,44 @@ describe("ProjectWorkStatePanel", () => { expect(tasks[1]?.targetAgentId).toBe("agent-bg"); expect(tasks[1]?.conversationId).toBe("conversation-rdv"); // updatedAtMs (backend-emitted) is carried through for chronological ordering. - expect(tasks.map((t) => t.updatedAtMs)).toEqual([11, 13, 15, 17]); + expect(tasks.map((t) => t.updatedAtMs)).toEqual([11, 13, 15, 17, 19, 21]); renderPanel(workState); const list = await screen.findByLabelText("Runner background tasks"); const cancelButtons = within(list).getAllByRole("button", { name: "Cancel" }); const retryButtons = within(list).getAllByRole("button", { name: "Retry" }); - // waiting + queued are cancellable; failed + expired are retryable. - expect(cancelButtons.filter((b) => !(b as HTMLButtonElement).disabled)).toHaveLength(2); - expect(retryButtons.filter((b) => !(b as HTMLButtonElement).disabled)).toHaveLength(2); + // waiting + queued + cancelling are cancellable; failed + expired + cancelFailed are retryable. + expect(cancelButtons.filter((b) => !(b as HTMLButtonElement).disabled)).toHaveLength(3); + expect(retryButtons.filter((b) => !(b as HTMLButtonElement).disabled)).toHaveLength(3); + expect(list.textContent).toContain("Cancelling"); + expect(list.textContent).toContain("Cancel failed"); + const rows = within(list).getAllByRole("listitem"); + const rowTexts = rows.map((row) => row.textContent ?? ""); + expect(rowTexts).toHaveLength(6); + const cancelFailedRow = rows[0] as HTMLElement; + const cancellingRow = rows[1] as HTMLElement; + expect(within(cancellingRow).getByText("Cancelling")).toBeTruthy(); + expect( + (within(cancellingRow).getByRole("button", { + name: "Cancel", + }) as HTMLButtonElement).disabled, + ).toBe(false); + expect(within(cancelFailedRow).getByText("Cancel failed")).toBeTruthy(); + expect( + (within(cancelFailedRow).getByRole("button", { + name: "Retry", + }) as HTMLButtonElement).disabled, + ).toBe(false); // Rows are ordered most-recently-updated first (updatedAtMs desc), taskId as // the deterministic tiebreak — locks the fix for the dead finishedAtMs sort. - const text = list.textContent ?? ""; - const order = ["task-exp", "task-fai", "task-que", "task-wai"].map((id) => - text.indexOf(id), - ); - expect(order).toEqual([...order].sort((a, b) => a - b)); - expect(order.every((i) => i >= 0)).toBe(true); + expect(rowTexts[0]).toContain("Cancel failed"); + expect(rowTexts[1]).toContain("Cancelling"); + expect(rowTexts[2]).toContain("Cancelled"); + expect(rowTexts[3]).toContain("Failed"); + expect(rowTexts[4]).toContain("Pending"); + expect(rowTexts[5]).toContain("Pending"); }); it("shows a terminal task summary and hides it when absent or non-terminal (#5)", async () => { diff --git a/frontend/src/plugins/runtime/services.ts b/frontend/src/plugins/runtime/services.ts index a0dea166..9ec77841 100644 --- a/frontend/src/plugins/runtime/services.ts +++ b/frontend/src/plugins/runtime/services.ts @@ -142,7 +142,15 @@ export interface BackgroundTaskStatus { ownerAgentId: string; projectId: string; kind: string; - status: "pending" | "running" | "completed" | "failed" | "cancelled" | "delivered"; + status: + | "pending" + | "running" + | "cancelling" + | "completed" + | "failed" + | "cancelFailed" + | "cancelled" + | "delivered"; exitCode: number | null; summary: string | null; stdoutTail: string | null; @@ -178,7 +186,16 @@ export interface CommandTaskStatus { ownerAgentId: string; projectId: string; kind: string; - state: "queued" | "running" | "waiting" | "completed" | "failed" | "cancelled" | "expired"; + state: + | "queued" + | "running" + | "waiting" + | "cancelling" + | "completed" + | "failed" + | "cancelFailed" + | "cancelled" + | "expired"; exitCode: number | null; summary: string | null; stdoutTail: string | null;