From 8bedfcda8467dd901674bd9804e375817ff4f702 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 21 Aug 2026 07:11:45 +0900 Subject: [PATCH 1/5] fix: remove caller worker revision fence --- crates/worker-runtime/src/worker_source.rs | 37 +--- .../src/feature/builtin/manage_worker.rs | 42 ++--- crates/worker/src/worker.rs | 1 - crates/workspace-server/src/retention.rs | 133 ++++++++++----- crates/workspace-server/src/server.rs | 160 +++++++----------- crates/workspace-server/src/store.rs | 14 +- crates/workspace-server/src/worker_source.rs | 10 +- resources/prompts/role/orchestrator.md | 2 +- 8 files changed, 170 insertions(+), 229 deletions(-) diff --git a/crates/worker-runtime/src/worker_source.rs b/crates/worker-runtime/src/worker_source.rs index 8acde9f1..fbf6f328 100644 --- a/crates/worker-runtime/src/worker_source.rs +++ b/crates/worker-runtime/src/worker_source.rs @@ -125,7 +125,6 @@ pub trait EmbeddedWorkerMutationDispatcher: Send + Sync { proof: InProcessWorkerMutationProof, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result; } @@ -187,7 +186,6 @@ impl RuntimeWorkerMutationForwarder { &self, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { let proof = self.authority.issue_worker_remove( @@ -206,7 +204,6 @@ impl RuntimeWorkerMutationForwarder { token, target_runtime_id: target_runtime_id.to_string(), target_worker_id: target_worker_id.to_string(), - expected_worker_revision: expected_worker_revision.to_string(), reason: reason.to_string(), }), ( @@ -216,7 +213,6 @@ impl RuntimeWorkerMutationForwarder { claims, target_runtime_id, target_worker_id, - expected_worker_revision, reason, ), _ => Err(RuntimeWorkerMutationForwardError::AuthorityTransportMismatch), @@ -230,7 +226,6 @@ struct RemoteWorkerRemoveHttpRequest { token: String, target_runtime_id: String, target_worker_id: String, - expected_worker_revision: String, reason: String, } @@ -267,7 +262,6 @@ fn execute_remote_worker_remove_http_blocking( let body = serde_json::json!({ "target_runtime_id": request.target_runtime_id, "target_worker_id": request.target_worker_id, - "expected_worker_revision": request.expected_worker_revision, "reason": request.reason, }); let client = reqwest::blocking::Client::new(); @@ -474,7 +468,6 @@ impl WorkspaceClient for RuntimeOwnedWorkspaceClient { &self, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { self.worker_remove @@ -484,12 +477,7 @@ impl WorkspaceClient for RuntimeOwnedWorkspaceClient { "Runtime-owned WorkerRemove forwarding is unavailable".to_string(), ) })? - .execute_worker_remove( - target_runtime_id, - target_worker_id, - expected_worker_revision, - reason, - ) + .execute_worker_remove(target_runtime_id, target_worker_id, reason) .map_err(|error| WorkspaceClientError::Request(error.to_string())) } } @@ -915,12 +903,7 @@ mod tests { format!("http://{address}"), ); let response = forwarder - .execute_worker_remove( - "runtime-target", - "worker-target", - "revision-7", - "retire obsolete Worker", - ) + .execute_worker_remove("runtime-target", "worker-target", "retire obsolete Worker") .unwrap(); assert_eq!(response.status, 204); server.join().unwrap(); @@ -929,7 +912,7 @@ mod tests { assert!(request.starts_with("POST /api/w/workspace-a/workers/remove HTTP/1.1")); assert!(request.contains("\"target_runtime_id\":\"runtime-target\"")); assert!(request.contains("\"target_worker_id\":\"worker-target\"")); - assert!(request.contains("\"expected_worker_revision\":\"revision-7\"")); + assert!(!request.contains("expected_worker_revision")); assert!(request.contains("\"reason\":\"retire obsolete Worker\"")); let token = request .lines() @@ -962,7 +945,7 @@ mod tests { #[derive(Default)] struct RecordingDispatcher { - seen: Mutex>, + seen: Mutex>, } impl EmbeddedWorkerMutationDispatcher for RecordingDispatcher { fn execute_worker_remove( @@ -970,14 +953,12 @@ mod tests { proof: InProcessWorkerMutationProof, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { *self.seen.lock().unwrap() = Some(( proof.into_claims(), target_runtime_id.to_string(), target_worker_id.to_string(), - expected_worker_revision.to_string(), reason.to_string(), )); Ok(WorkspaceResponse { @@ -996,15 +977,10 @@ mod tests { dispatcher.clone(), ); let response = forwarder - .execute_worker_remove( - "runtime-target", - "worker-target", - "revision-7", - "retire obsolete Worker", - ) + .execute_worker_remove("runtime-target", "worker-target", "retire obsolete Worker") .unwrap(); assert_eq!(response.status, 202); - let (claims, target_runtime_id, target_worker_id, expected_revision, reason) = + let (claims, target_runtime_id, target_worker_id, reason) = dispatcher.seen.lock().unwrap().take().unwrap(); assert_eq!(claims.iss, "runtime-embedded"); assert_eq!(claims.worker_id, "worker-source"); @@ -1012,7 +988,6 @@ mod tests { assert_eq!(claims.target_worker_id, "worker-target"); assert_eq!(target_runtime_id, "runtime-target"); assert_eq!(target_worker_id, "worker-target"); - assert_eq!(expected_revision, "revision-7"); assert_eq!(reason, "retire obsolete Worker"); } diff --git a/crates/worker/src/feature/builtin/manage_worker.rs b/crates/worker/src/feature/builtin/manage_worker.rs index 54b97524..837e40e0 100644 --- a/crates/worker/src/feature/builtin/manage_worker.rs +++ b/crates/worker/src/feature/builtin/manage_worker.rs @@ -48,7 +48,6 @@ pub trait WorkerControlService: Send + Sync { &self, runtime_id: &str, worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result; async fn execute_runtime( @@ -165,11 +164,10 @@ impl WorkerControlService for WorkspaceWorkerControlService { &self, runtime_id: &str, worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { self.client - .execute_worker_remove(runtime_id, worker_id, expected_worker_revision, reason) + .execute_worker_remove(runtime_id, worker_id, reason) } async fn execute_runtime( @@ -530,7 +528,6 @@ struct WorkerStopInput { #[serde(deny_unknown_fields)] struct WorkerRemoveInput { subject: WorkerSubjectInput, - expected_worker_revision: String, reason: String, } @@ -592,7 +589,7 @@ impl WorkerOperation { "Restore a stopped Backend/Runtime Worker session in the current Workspace." } Self::Remove => { - "Remove an eligible stopped, unassigned, non-internal Worker. Supply the current Worker revision and a bounded reason; Backend validation and retention are authoritative." + "Remove an eligible stopped, unassigned, non-internal Worker. Supply a bounded reason; Backend validation and retention are authoritative." } } } @@ -742,8 +739,6 @@ impl Tool for WorkspaceWorkerTool { WorkerOperation::Remove => { let input = parse::(input_json, "WorkerRemove")?; let (runtime_id, worker_id) = runtime_subject_ids(&input.subject, self.operation)?; - let expected_worker_revision = - non_empty(input.expected_worker_revision, "expected_worker_revision")?; let reason = non_empty(input.reason, "reason")?; if reason.len() > 512 { return Err(ToolError::ExecutionFailed( @@ -751,12 +746,7 @@ impl Tool for WorkspaceWorkerTool { )); } self.control - .remove_runtime_worker( - &runtime_id, - &worker_id, - &expected_worker_revision, - &reason, - ) + .remove_runtime_worker(&runtime_id, &worker_id, &reason) .map_err(control_tool_error)? } }; @@ -912,7 +902,7 @@ mod tests { #[derive(Debug, Default)] struct RecordingWorkspaceClient { requests: Mutex>, - removals: Mutex>, + removals: Mutex>, } impl WorkspaceClient for RecordingWorkspaceClient { @@ -943,13 +933,11 @@ mod tests { &self, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { self.removals.lock().unwrap().push(( target_runtime_id.to_string(), target_worker_id.to_string(), - expected_worker_revision.to_string(), reason.to_string(), )); Ok(WorkspaceResponse { @@ -1160,7 +1148,7 @@ mod tests { } #[tokio::test] - async fn worker_remove_forwards_only_target_revision_and_bounded_reason() { + async fn worker_remove_forwards_only_target_and_bounded_reason() { let client = Arc::new(RecordingWorkspaceClient::default()); let tool = WorkspaceWorkerTool { operation: WorkerOperation::Remove, @@ -1173,7 +1161,6 @@ mod tests { "runtime_id": "runtime-1", "worker_id": "worker-7", }, - "expected_worker_revision": "2026-08-11T20:00:00Z", "reason": " retire completed Worker " }) .to_string(), @@ -1186,7 +1173,6 @@ mod tests { [( "runtime-1".to_string(), "worker-7".to_string(), - "2026-08-11T20:00:00Z".to_string(), "retire completed Worker".to_string(), )] ); @@ -1194,15 +1180,18 @@ mod tests { let schema = serde_json::to_value(schemars::schema_for!(WorkerRemoveInput)) .unwrap() .to_string(); - for field in [ - "runtime_id", - "worker_id", - "expected_worker_revision", - "reason", - ] { + for field in ["runtime_id", "worker_id", "reason"] { assert!(schema.contains(field)); } - for forbidden in ["proof", "actor", "workspace_id", "policy", "plan", "stage"] { + for forbidden in [ + "expected_worker_revision", + "proof", + "actor", + "workspace_id", + "policy", + "plan", + "stage", + ] { assert!(!schema.contains(forbidden), "schema leaked {forbidden}"); } } @@ -1223,7 +1212,6 @@ mod tests { "runtime_id": "runtime-1", "worker_id": "worker-7", }, - "expected_worker_revision": "revision-1", "reason": reason, }) .to_string(), diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index dd4ec9f2..18c81401 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -272,7 +272,6 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync { &self, _target_runtime_id: &str, _target_worker_id: &str, - _expected_worker_revision: &str, _reason: &str, ) -> Result { Err(WorkspaceClientError::Unavailable( diff --git a/crates/workspace-server/src/retention.rs b/crates/workspace-server/src/retention.rs index 3b34a6db..ff48cc77 100644 --- a/crates/workspace-server/src/retention.rs +++ b/crates/workspace-server/src/retention.rs @@ -59,7 +59,6 @@ pub struct WorkerRetentionPolicyUpdate { pub struct WorkerRemovalPlanRequest { pub workspace_id: String, pub worker: RuntimeWorkerRef, - pub expected_worker_revision: String, pub reason: String, } @@ -154,8 +153,6 @@ pub enum WorkerRetentionError { WorkerNotFound, #[error("Worker belongs to a different Workspace")] CrossWorkspace, - #[error("Worker revision changed: expected {expected}, current {actual}")] - WorkerRevisionConflict { expected: String, actual: String }, #[error("Worker removal is blocked: {0:?}")] Blocked(Vec), #[error("Worker removal plan {plan_id} is stale: {reason}")] @@ -308,17 +305,16 @@ impl SqliteWorkspaceStore { return Err(StoreError::InvalidInput(if other{"cross-workspace".into()}else{"worker-missing".into()})); } }; - if worker.updated_at!=req.expected_worker_revision { return Err(StoreError::InvalidInput(format!("worker-conflict:{}:{}",req.expected_worker_revision,worker.updated_at))); } let mut blockers=Vec::new(); if worker.retention_state=="pinned" { blockers.push(WorkerRemovalBlocker::Hold); } if let Some((assignment_id,ticket_id))=tx.query_row("SELECT a.assignment_id,a.ticket_id FROM ticket_current_worker_assignments c JOIN ticket_worker_assignments a ON a.workspace_id=c.workspace_id AND a.ticket_id=c.ticket_id AND a.assignment_id=c.assignment_id WHERE a.workspace_id=?1 AND a.runtime_id=?2 AND a.worker_id=?3",params![req.workspace_id,req.worker.runtime_id,req.worker.worker_id],|r|Ok((r.get(0)?,r.get(1)?))).optional()? { blockers.push(WorkerRemovalBlocker::CurrentAssignment{assignment_id,ticket_id}); } - let fp=fingerprint(req,inv,&policy,&blockers)?; + let fp=fingerprint(req,&worker.updated_at,inv,&policy,&blockers)?; let plan_id=stable("wrp",&fp); let operation_id=stable("wro",&fp); let archive_id=(policy.session_disposition==SessionDisposition::Archive).then(||stable("wra",&fp)); let state=if blockers.is_empty(){WorkerRemovalPlanState::Planned}else{WorkerRemovalPlanState::Blocked}; - tx.execute("INSERT OR IGNORE INTO worker_removal_operations(operation_id,plan_id,input_fingerprint,workspace_id,runtime_id,worker_id,worker_revision,run_generation,policy_id,policy_revision,session_disposition,metadata_disposition,archive_retention_kind,archive_retention_seconds,diagnostics_disposition,diagnostics_retention_seconds,archive_id,blockers_json,state,reason,created_at,updated_at) VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17,?18,?19,?20,?21,?21)",params![operation_id,plan_id,fp,req.workspace_id,req.worker.runtime_id,req.worker.worker_id,req.expected_worker_revision,inv.run_generation,policy.policy_id,policy.revision,sess(policy.session_disposition),meta(policy.metadata_disposition),archive_kind(policy.archive_retention),archive_seconds(policy.archive_retention),diag(policy.diagnostics_disposition),policy.diagnostics_retention_seconds,archive_id,serde_json::to_string(&blockers).map_err(|e|StoreError::InvalidInput(e.to_string()))?,state_s(state),req.reason,now])?; + tx.execute("INSERT OR IGNORE INTO worker_removal_operations(operation_id,plan_id,input_fingerprint,workspace_id,runtime_id,worker_id,worker_revision,run_generation,policy_id,policy_revision,session_disposition,metadata_disposition,archive_retention_kind,archive_retention_seconds,diagnostics_disposition,diagnostics_retention_seconds,archive_id,blockers_json,state,reason,created_at,updated_at) VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17,?18,?19,?20,?21,?21)",params![operation_id,plan_id,fp,req.workspace_id,req.worker.runtime_id,req.worker.worker_id,worker.updated_at,inv.run_generation,policy.policy_id,policy.revision,sess(policy.session_disposition),meta(policy.metadata_disposition),archive_kind(policy.archive_retention),archive_seconds(policy.archive_retention),diag(policy.diagnostics_disposition),policy.diagnostics_retention_seconds,archive_id,serde_json::to_string(&blockers).map_err(|e|StoreError::InvalidInput(e.to_string()))?,state_s(state),req.reason,now])?; let plan=load_plan(&tx,&plan_id)?.ok_or_else(||StoreError::InvalidInput("plan missing".into()))?; if plan.input_fingerprint!=fp{return Err(StoreError::InvalidInput(format!("fingerprint:{}",plan.operation_id)));} tx.commit()?; Ok(plan) @@ -422,27 +418,22 @@ impl SqliteWorkspaceStore { &self, workspace_id: &str, worker: &RuntimeWorkerRef, - expected_worker_revision: &str, - reason: &str, ) -> Result, WorkerRetentionError> { bounded("workspace", workspace_id, 160)?; - bounded("revision", expected_worker_revision, 256)?; - bounded("reason", reason, 512)?; let plan = self.with_conn(|conn| { conn.query_row( "SELECT plan_id FROM worker_removal_operations WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3 - AND worker_revision=?4 AND reason=?5 - AND state IN ('executing','failed','succeeded') + AND state IN ('planned','executing','failed','succeeded') + AND ( + state='succeeded' OR worker_revision=( + SELECT updated_at FROM worker_registry + WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3 + ) + ) ORDER BY CASE state WHEN 'succeeded' THEN 0 ELSE 1 END, created_at DESC LIMIT 1", - params![ - workspace_id, - worker.runtime_id, - worker.worker_id, - expected_worker_revision, - reason, - ], + params![workspace_id, worker.runtime_id, worker.worker_id], |row| row.get::<_, String>(0), ) .optional() @@ -848,6 +839,7 @@ fn stale_error(plan: &WorkerRemovalPlan, reason: &str) -> StoreError { } fn fingerprint( r: &WorkerRemovalPlanRequest, + worker_revision: &str, i: &WorkerRetentionInventory, p: &WorkerRetentionPolicy, b: &[WorkerRemovalBlocker], @@ -856,7 +848,7 @@ fn fingerprint( r.workspace_id, r.worker.runtime_id, r.worker.worker_id, - r.expected_worker_revision, + worker_revision, i.run_generation, i.session_id, i.segment_ids, @@ -887,7 +879,6 @@ fn validate_plan( i: &WorkerRetentionInventory, ) -> Result<(), WorkerRetentionError> { bounded("workspace", &r.workspace_id, 160)?; - bounded("revision", &r.expected_worker_revision, 256)?; bounded("reason", &r.reason, 2000)?; if i.workspace_id != r.workspace_id || i.runtime_id != r.worker.runtime_id @@ -947,13 +938,6 @@ fn map_error(e: StoreError) -> WorkerRetentionError { if m == "worker-missing" { return WorkerRetentionError::WorkerNotFound; } - if let Some(x) = m.strip_prefix("worker-conflict:") { - let mut s = x.splitn(2, ':'); - return WorkerRetentionError::WorkerRevisionConflict { - expected: s.next().unwrap_or_default().into(), - actual: s.next().unwrap_or_default().into(), - }; - } if let Some(x) = m.strip_prefix("fingerprint:") { return WorkerRetentionError::OperationFingerprintConflict { operation_id: x.into(), @@ -1106,7 +1090,6 @@ mod tests { runtime_id: "r".into(), worker_id: worker_id().to_string(), }, - expected_worker_revision: "rev1".into(), reason: "cleanup".into(), } } @@ -1141,6 +1124,7 @@ mod tests { let a = s.plan_worker_removal(&req(), &inv()).unwrap(); let b = s.plan_worker_removal(&req(), &inv()).unwrap(); assert_eq!(a.plan_id, b.plan_id); + assert_eq!(a.worker_revision, "rev1"); s.with_conn(|c| { c.execute( "UPDATE worker_registry SET retention_state='pinned' WHERE workspace_id='w'", @@ -1149,9 +1133,7 @@ mod tests { Ok(()) }) .unwrap(); - let mut q = req(); - q.expected_worker_revision = "rev1".into(); - let p = s.plan_worker_removal(&q, &inv()).unwrap(); + let p = s.plan_worker_removal(&req(), &inv()).unwrap(); assert_eq!(p.blockers, vec![WorkerRemovalBlocker::Hold]); assert!(matches!( s.begin_worker_removal("w", &p.plan_id, &p.input_fingerprint), @@ -1571,6 +1553,77 @@ mod tests { ); } + #[test] + fn worker_removal_recovery_is_target_keyed_and_preserves_original_reason() { + let store = setup(); + let request = req(); + let plan = store.plan_worker_removal(&request, &inv()).unwrap(); + let recovered_planned = store + .recover_worker_removal_execution("w", &request.worker) + .unwrap() + .unwrap(); + assert_eq!( + recovered_planned.plan.state, + WorkerRemovalPlanState::Planned + ); + assert_eq!(recovered_planned.plan.plan_id, plan.plan_id); + store + .prepare_worker_removal_execution("w", &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + store + .fail_worker_removal( + "w", + &plan.operation_id, + &plan.input_fingerprint, + "runtime_remove_failed", + ) + .unwrap(); + + let recovered = store + .recover_worker_removal_execution("w", &request.worker) + .unwrap() + .unwrap(); + assert_eq!(recovered.plan.plan_id, plan.plan_id); + assert_eq!(recovered.plan.reason, request.reason); + } + + #[test] + fn stale_failed_removal_is_not_recovered_after_worker_authority_changes() { + let store = setup(); + let request = req(); + let plan = store.plan_worker_removal(&request, &inv()).unwrap(); + store + .prepare_worker_removal_execution("w", &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + store + .fail_worker_removal( + "w", + &plan.operation_id, + &plan.input_fingerprint, + "runtime_remove_failed", + ) + .unwrap(); + store + .with_conn(|conn| { + conn.execute( + "UPDATE worker_registry SET updated_at='rev2' WHERE workspace_id='w' AND runtime_id='r' AND worker_id=?1", + [worker_id().to_string()], + )?; + Ok(()) + }) + .unwrap(); + + assert!( + store + .recover_worker_removal_execution("w", &request.worker) + .unwrap() + .is_none() + ); + let replacement = store.plan_worker_removal(&request, &inv()).unwrap(); + assert_eq!(replacement.worker_revision, "rev2"); + assert_ne!(replacement.plan_id, plan.plan_id); + } + #[test] fn succeeded_worker_removal_recovers_after_registry_purge() { let s = setup(); @@ -1623,18 +1676,13 @@ mod tests { .is_none() ); let recovered = s - .recover_worker_removal_execution( - "w", - &request.worker, - &request.expected_worker_revision, - &request.reason, - ) + .recover_worker_removal_execution("w", &request.worker) .unwrap() .unwrap(); assert_eq!(recovered.plan.state, WorkerRemovalPlanState::Succeeded); assert_eq!( recovered.runtime_request.expected_worker_revision, - request.expected_worker_revision + plan.worker_revision ); } @@ -1653,12 +1701,7 @@ mod tests { ) .unwrap(); let recovered = s - .recover_worker_removal_execution( - "w", - &request.worker, - &request.expected_worker_revision, - &request.reason, - ) + .recover_worker_removal_execution("w", &request.worker) .unwrap() .unwrap(); assert_eq!( diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index ca162ab3..61a11ce1 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -330,7 +330,6 @@ impl WorkspaceWorkerRemoveExecutor { source: crate::worker_source::VerifiedWorkerMutationSource, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> std::result::Result { let reason = reason.trim(); @@ -428,73 +427,63 @@ impl WorkspaceWorkerRemoveExecutor { let prepared = self .store - .recover_worker_removal_execution( - &self.workspace_id, - &target, - expected_worker_revision, - reason, - ) + .recover_worker_removal_execution(&self.workspace_id, &target) .map_err(|_| "Worker removal recovery authority is unavailable".to_string())?; if let Some(prepared) = prepared { if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Succeeded { return Ok(worker_remove_success_response(&target)); } - let must_close_session = - prepared.prior_failure_category.as_deref() == Some("workdir_session_close_failed"); - let must_release_attachment = must_close_session - || prepared.prior_failure_category.as_deref() - == Some("workdir_attachment_release_failed"); - let prepared = - if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Failed { - match self.store.prepare_worker_removal_execution( - &self.workspace_id, - &prepared.plan.plan_id, - &prepared.plan.input_fingerprint, - ) { - Ok(prepared) => prepared, - Err(error) => return Ok(worker_retention_error_response(error)), - } - } else { - prepared - }; - if must_close_session { - let session = { - self.workdir_sessions - .lock() - .map_err(|_| "Workdir session registry was poisoned".to_string())? - .get(&target) - .cloned() - }; - if let Some(session) = session { - if session.close().await.is_err() { - let _ = self.store.fail_worker_removal( - &self.workspace_id, - &prepared.plan.operation_id, - &prepared.plan.input_fingerprint, - "workdir_session_close_failed", - ); - return Ok(worker_remove_error_response( - StatusCode::SERVICE_UNAVAILABLE, - "attachment_close_failed", - "Worker Workdir session could not be closed; removal can be retried", - )); - } - self.workdir_sessions - .lock() - .map_err(|_| "Workdir session registry was poisoned".to_string())? - .remove(&target); + let prepared = if matches!( + prepared.plan.state, + crate::retention::WorkerRemovalPlanState::Planned + | crate::retention::WorkerRemovalPlanState::Failed + ) { + match self.store.prepare_worker_removal_execution( + &self.workspace_id, + &prepared.plan.plan_id, + &prepared.plan.input_fingerprint, + ) { + Ok(prepared) => prepared, + Err(error) => return Ok(worker_retention_error_response(error)), } - } - if must_release_attachment - && self - .store - .detach_worker_workdir( + } else { + prepared + }; + let session = { + self.workdir_sessions + .lock() + .map_err(|_| "Workdir session registry was poisoned".to_string())? + .get(&target) + .cloned() + }; + if let Some(session) = session { + if session.close().await.is_err() { + let _ = self.store.fail_worker_removal( &self.workspace_id, - &target, - None, - &Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), - ) - .is_err() + &prepared.plan.operation_id, + &prepared.plan.input_fingerprint, + "workdir_session_close_failed", + ); + return Ok(worker_remove_error_response( + StatusCode::SERVICE_UNAVAILABLE, + "attachment_close_failed", + "Worker Workdir session could not be closed; removal can be retried", + )); + } + self.workdir_sessions + .lock() + .map_err(|_| "Workdir session registry was poisoned".to_string())? + .remove(&target); + } + if self + .store + .detach_worker_workdir( + &self.workspace_id, + &target, + None, + &Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + ) + .is_err() { let _ = self.store.fail_worker_removal( &self.workspace_id, @@ -551,7 +540,6 @@ impl WorkspaceWorkerRemoveExecutor { let request = crate::retention::WorkerRemovalPlanRequest { workspace_id: self.workspace_id.clone(), worker: target.clone(), - expected_worker_revision: expected_worker_revision.to_string(), reason: reason.to_string(), }; let plan = match self.store.plan_worker_removal(&request, &inventory) { @@ -656,13 +644,11 @@ impl crate::worker_source::VerifiedWorkerRemoveExecutor for WorkspaceWorkerRemov source: crate::worker_source::VerifiedWorkerMutationSource, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> std::result::Result { let executor = self.clone(); let target_runtime_id = target_runtime_id.to_string(); let target_worker_id = target_worker_id.to_string(); - let expected_worker_revision = expected_worker_revision.to_string(); let reason = reason.to_string(); std::thread::spawn(move || { tokio::runtime::Builder::new_current_thread() @@ -673,7 +659,6 @@ impl crate::worker_source::VerifiedWorkerRemoveExecutor for WorkspaceWorkerRemov source, &target_runtime_id, &target_worker_id, - &expected_worker_revision, &reason, )) }) @@ -6145,14 +6130,13 @@ fn worker_retention_error_response( "worker_not_found", "Worker was not found in this Workspace", ), - crate::retention::WorkerRetentionError::WorkerRevisionConflict { .. } - | crate::retention::WorkerRetentionError::PolicyRevisionConflict { .. } + crate::retention::WorkerRetentionError::PolicyRevisionConflict { .. } | crate::retention::WorkerRetentionError::StalePlan { .. } | crate::retention::WorkerRetentionError::OperationFingerprintConflict { .. } => { worker_remove_error_response( StatusCode::CONFLICT, - "worker_revision_conflict", - "Worker removal state changed; reread the Worker and retry", + "worker_removal_conflict", + "Worker removal state changed; retry the operation", ) } crate::retention::WorkerRetentionError::Blocked(_) => worker_remove_error_response( @@ -6179,7 +6163,6 @@ fn worker_retention_error_response( struct WorkerRemoveBoundaryRequest { target_runtime_id: String, target_worker_id: String, - expected_worker_revision: String, reason: String, } @@ -6217,7 +6200,6 @@ async fn scoped_worker_remove_source_boundary( source, &request.target_runtime_id, &request.target_worker_id, - &request.expected_worker_revision, &request.reason, ) .await @@ -15674,7 +15656,7 @@ mod tests { let temp = tempfile::tempdir().unwrap(); let app = build_router(test_api(temp.path()).await); - let body = r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","expected_worker_revision":"revision-1","reason":"retire target Worker"}"#; + let body = r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","reason":"retire target Worker"}"#; let browser = app .clone() .oneshot( @@ -15791,7 +15773,6 @@ mod tests { fresh_proof, "runtime-target", "target-worker", - "revision-1", "retire target Worker", ) .unwrap_err(); @@ -15799,7 +15780,7 @@ mod tests { } #[tokio::test] - async fn worker_remove_rejects_self_running_and_stale_revision_at_caller_boundary() { + async fn worker_remove_rejects_self_and_running_at_caller_boundary() { let temp = tempfile::tempdir().unwrap(); let api = test_api(temp.path()).await; let Json(orchestrator) = scoped_start_workspace_orchestrator( @@ -15824,7 +15805,6 @@ mod tests { verified_source(), &source.runtime_id, &source.worker_id, - "irrelevant", "must reject self", ) .await @@ -15866,37 +15846,12 @@ mod tests { verified_source(), &target.runtime_id, &target.worker_id, - "irrelevant", "must reject a live Worker", ) .await .unwrap(); assert_eq!(running_response.status, StatusCode::CONFLICT.as_u16()); assert!(running_response.body.contains("worker_not_stopped")); - - api.runtime - .stop_worker( - &target, - WorkerLifecycleRequest { - reason: Some("prepare stale revision guard".to_string()), - ticket_assignment: None, - }, - ) - .unwrap(); - let summary = api.runtime.worker(&target).unwrap(); - let record = sync_worker_observation(&api, &summary).unwrap(); - let stale_response = executor - .execute_async( - verified_source(), - &target.runtime_id, - &target.worker_id, - &format!("{}-stale", record.updated_at), - "must reject stale revision", - ) - .await - .unwrap(); - assert_eq!(stale_response.status, StatusCode::CONFLICT.as_u16()); - assert!(stale_response.body.contains("worker_revision_conflict")); } #[tokio::test] @@ -15970,7 +15925,7 @@ mod tests { ) .unwrap(); let summary = api.runtime.worker(&target).unwrap(); - let record = sync_worker_observation(&api, &summary).unwrap(); + sync_worker_observation(&api, &summary).unwrap(); seed_worker_control_grant(&api, &source, &target, "embedded-valid-proof"); let response = WorkspaceWorkerRemoveExecutor::new(&api) @@ -15984,7 +15939,6 @@ mod tests { }, &target.runtime_id, &target.worker_id, - &record.updated_at, "retire completed Worker", ) .await @@ -16182,7 +16136,7 @@ mod tests { route_token, ) .body(Body::from( - r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","expected_worker_revision":"revision-1","reason":"retire target Worker"}"#, + r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","reason":"retire target Worker"}"#, )) .unwrap(), ) diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index 6fd3e48b..aaae4cb3 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -619,13 +619,11 @@ pub trait ControlPlaneStore: Send + Sync { &self, workspace_id: &str, worker: &RuntimeWorkerRef, - expected_worker_revision: &str, - reason: &str, ) -> std::result::Result< Option, crate::retention::WorkerRetentionError, > { - let _ = (workspace_id, worker, expected_worker_revision, reason); + let _ = (workspace_id, worker); Ok(None) } fn fail_worker_removal( @@ -1431,19 +1429,11 @@ impl ControlPlaneStore for SqliteWorkspaceStore { &self, workspace_id: &str, worker: &RuntimeWorkerRef, - expected_worker_revision: &str, - reason: &str, ) -> std::result::Result< Option, crate::retention::WorkerRetentionError, > { - SqliteWorkspaceStore::recover_worker_removal_execution( - self, - workspace_id, - worker, - expected_worker_revision, - reason, - ) + SqliteWorkspaceStore::recover_worker_removal_execution(self, workspace_id, worker) } fn fail_worker_removal( diff --git a/crates/workspace-server/src/worker_source.rs b/crates/workspace-server/src/worker_source.rs index 4d3a98da..08504cca 100644 --- a/crates/workspace-server/src/worker_source.rs +++ b/crates/workspace-server/src/worker_source.rs @@ -175,7 +175,6 @@ pub(crate) trait VerifiedWorkerRemoveExecutor: Send + Sync { source: VerifiedWorkerMutationSource, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result; } @@ -217,7 +216,6 @@ impl worker_runtime::worker_source::EmbeddedWorkerMutationDispatcher proof: InProcessWorkerMutationProof, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result< worker::WorkspaceResponse, @@ -241,13 +239,7 @@ impl worker_runtime::worker_source::EmbeddedWorkerMutationDispatcher ) })?; executor - .execute( - source, - target_runtime_id, - target_worker_id, - expected_worker_revision, - reason, - ) + .execute(source, target_runtime_id, target_worker_id, reason) .map_err(worker_runtime::worker_source::RuntimeWorkerMutationForwardError::Embedded) } } diff --git a/resources/prompts/role/orchestrator.md b/resources/prompts/role/orchestrator.md index 5a05800a..273298f6 100644 --- a/resources/prompts/role/orchestrator.md +++ b/resources/prompts/role/orchestrator.md @@ -22,4 +22,4 @@ Do not create or delegate an implementation worktree/branch until the Ticket rec Workspace roots, cwd, profile selector, and launch-prompt configuration are control-plane/environment facts rather than user instructions. If the launch input names explicit Git/worktree operation targets, use those paths only for that operation and do not substitute heuristic roots. -Use `WorkerRemove` only for a terminal or authoritatively reassigned non-internal Coder after implementation, review, fix, merge/commit, and report handoffs are complete. Do not remove a Coder merely because one turn completed or it is temporarily idle; retain it while review or request-changes work can still return. The Worker must already be stopped, must not be restoring, must have no current Ticket assignment, pending notification, Reviewer handoff, legal hold, or pin, and must not be this Orchestrator. Immediately before removal, reread authoritative Ticket state, assignment, thread/review evidence, and the target Worker with `WorkerShow`; pass the exact current `updated_at` value as `expected_worker_revision` with a concise reason. After removal, reread the Worker catalog and attachment state. Treat revision, assignment, running/restoring, retention-policy, attachment-close, and attachment-release conflicts as authoritative failures: do not guess policy or retry with stale input. `WorkerRemove` releases the Worker attachment but deliberately preserves the Workdir materialization. +Use `WorkerRemove` only for a terminal or authoritatively reassigned non-internal Coder after implementation, review, fix, merge/commit, and report handoffs are complete. Do not remove a Coder merely because one turn completed or it is temporarily idle; retain it while review or request-changes work can still return. The Worker must already be stopped, must not be restoring, must have no current Ticket assignment, pending notification, Reviewer handoff, legal hold, or pin, and must not be this Orchestrator. Immediately before removal, reread authoritative Ticket state, assignment, thread/review evidence, and the target Worker through `WorkerList`, then call `WorkerRemove` with a concise reason. Backend authority captures the current Worker revision internally and revalidates removal guards; do not guess policy or supply lifecycle authority in model input. After removal, reread the Worker catalog and attachment state. Treat assignment, running/restoring, retention-policy, attachment-close, and attachment-release conflicts as authoritative failures. `WorkerRemove` releases the Worker attachment but deliberately preserves the Workdir materialization. From 18112d29a6f89a34d38b8476186f729c379135a8 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 21 Aug 2026 07:12:04 +0900 Subject: [PATCH 2/5] feat: project SubWorker activity in sidebar --- crates/protocol/src/subscription.rs | 3 + crates/worker-runtime/src/runtime.rs | 333 +++++++++++++++++- web/workspace/src/lib/generated/protocol.ts | 2 +- .../src/lib/workspace/console/Spinner.svelte | 2 +- .../sidebar/WorkersNavSection.svelte | 9 +- .../src/lib/workspace/sidebar/sidebar.css | 27 +- .../sidebar/worker-subscription.test.ts | 1 + .../workspace/sidebar/worker-subscription.ts | 2 + .../tests/console-spinner-source.test.ts | 56 +++ 9 files changed, 409 insertions(+), 26 deletions(-) diff --git a/crates/protocol/src/subscription.rs b/crates/protocol/src/subscription.rs index 71f8cf2d..053f7810 100644 --- a/crates/protocol/src/subscription.rs +++ b/crates/protocol/src/subscription.rs @@ -554,6 +554,8 @@ pub struct SubscriptionWorker { /// Producer-owned monotonic revision for this Worker subject. pub subject_revision: u64, pub state: SubscriptionWorkerState, + #[serde(default)] + pub has_running_internal_workers: bool, #[serde(default, skip_serializing_if = "Option::is_none")] pub workspace_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -796,6 +798,7 @@ mod tests { runtime_id: None, subject_revision: 0, state: SubscriptionWorkerState::Idle, + has_running_internal_workers: false, workspace_id: Some("workspace-1".to_string()), display_name: Some(format!("Worker {value}")), profile: Some("builtin:coder".to_string()), diff --git a/crates/worker-runtime/src/runtime.rs b/crates/worker-runtime/src/runtime.rs index b97c5b58..441e72b0 100644 --- a/crates/worker-runtime/src/runtime.rs +++ b/crates/worker-runtime/src/runtime.rs @@ -335,6 +335,7 @@ impl Runtime { for (worker_id, worker) in &mut state.workers { if worker.status.is_active() { worker.status = WorkerStatus::Stopped; + worker.internal_workers.clear(); stopped.push(*worker_id); } } @@ -574,6 +575,7 @@ impl Runtime { run_generation: 1, working_directory: None, execution_handle: None, + internal_workers: BTreeMap::new(), }; state.workers.insert(worker_id, record); state.persist_runtime_snapshot()?; @@ -1471,7 +1473,8 @@ impl Runtime { let mut state = self.lock()?; state.ensure_worker_ref(worker_ref)?; let status_changed = state.project_protocol_event_to_status(worker_ref, &payload); - if status_changed { + let activity_changed = state.project_internal_worker_activity(worker_ref, &payload); + if status_changed || activity_changed { state.publish_worker_upsert(worker_ref.worker_id)?; } let event = state.push_worker_observation_event(worker_ref.clone(), payload); @@ -1528,6 +1531,7 @@ impl Runtime { let worker = state.worker_mut(worker_ref)?; worker.status = status; worker.execution_handle = None; + worker.internal_workers.clear(); let status = worker.status; state.publish_worker_upsert(worker_ref.worker_id)?; state.persist_runtime_snapshot()?; @@ -1940,6 +1944,7 @@ impl RuntimeState { run_generation: worker.run_generation, working_directory: worker.working_directory, execution_handle: None, + internal_workers: BTreeMap::new(), }, ); } @@ -2260,6 +2265,10 @@ impl RuntimeState { .copied() .unwrap_or(0), state: subscription_worker_state(worker.status), + has_running_internal_workers: worker + .internal_workers + .values() + .any(|worker| worker.status == protocol::WorkerStatus::Running), workspace_id: worker.workspace_id.clone(), display_name: worker.request.display_name.clone(), profile, @@ -2402,6 +2411,7 @@ impl RuntimeState { let worker = self.worker_mut(worker_ref)?; worker.execution_handle = None; worker.status = WorkerStatus::Stopped; + worker.internal_workers.clear(); self.publish_worker_upsert(worker_ref.worker_id)?; self.persist_runtime_snapshot()?; Ok(()) @@ -2455,7 +2465,134 @@ impl RuntimeState { event } - #[cfg(feature = "ws-server")] + fn internal_worker_snapshot_statuses( + statuses: &mut BTreeMap, + snapshot: &protocol::InternalWorkerSnapshot, + ) { + statuses.insert( + snapshot.worker.session_id.clone(), + InternalWorkerActivity { + status: snapshot.status, + parent_session_id: snapshot.worker.parent_session_id.clone(), + }, + ); + for child in &snapshot.internal_workers { + Self::internal_worker_snapshot_statuses(statuses, child); + } + } + + fn remove_internal_worker_subtree( + statuses: &mut BTreeMap, + root_session_id: &str, + ) { + let mut removed = vec![root_session_id.to_string()]; + while let Some(parent_session_id) = removed.pop() { + let children = statuses + .iter() + .filter_map(|(session_id, worker)| { + (worker.parent_session_id.as_deref() == Some(parent_session_id.as_str())) + .then(|| session_id.clone()) + }) + .collect::>(); + statuses.remove(&parent_session_id); + removed.extend(children); + } + } + + fn project_internal_worker_event( + statuses: &mut BTreeMap, + worker: &protocol::InternalWorkerRef, + event: &protocol::Event, + ) { + match event { + protocol::Event::Snapshot { + status, + internal_workers, + .. + } => { + Self::remove_internal_worker_subtree(statuses, &worker.session_id); + statuses.insert( + worker.session_id.clone(), + InternalWorkerActivity { + status: *status, + parent_session_id: worker.parent_session_id.clone(), + }, + ); + for child in internal_workers { + Self::internal_worker_snapshot_statuses(statuses, child); + } + } + protocol::Event::InternalWorker { + worker: nested_worker, + event, + .. + } => Self::project_internal_worker_event(statuses, nested_worker, event), + protocol::Event::Status { status } => { + statuses.insert( + worker.session_id.clone(), + InternalWorkerActivity { + status: *status, + parent_session_id: worker.parent_session_id.clone(), + }, + ); + } + protocol::Event::RunEnd { result } => { + let status = match result { + protocol::RunResult::Paused => protocol::WorkerStatus::Paused, + protocol::RunResult::Finished + | protocol::RunResult::LimitReached + | protocol::RunResult::RolledBack => protocol::WorkerStatus::Idle, + }; + statuses.insert( + worker.session_id.clone(), + InternalWorkerActivity { + status, + parent_session_id: worker.parent_session_id.clone(), + }, + ); + } + _ => {} + } + } + + fn update_internal_worker_activity( + statuses: &mut BTreeMap, + event: &protocol::Event, + ) -> bool { + let was_running = statuses + .values() + .any(|worker| worker.status == protocol::WorkerStatus::Running); + match event { + protocol::Event::Snapshot { + internal_workers, .. + } => { + statuses.clear(); + for child in internal_workers { + Self::internal_worker_snapshot_statuses(statuses, child); + } + } + protocol::Event::InternalWorker { worker, event, .. } => { + Self::project_internal_worker_event(statuses, worker, event); + } + _ => {} + } + let is_running = statuses + .values() + .any(|worker| worker.status == protocol::WorkerStatus::Running); + was_running != is_running + } + + fn project_internal_worker_activity( + &mut self, + worker_ref: &WorkerRef, + event: &protocol::Event, + ) -> bool { + let Some(worker) = self.workers.get_mut(&worker_ref.worker_id) else { + return false; + }; + Self::update_internal_worker_activity(&mut worker.internal_workers, event) + } + fn project_protocol_event_to_status( &mut self, worker_ref: &WorkerRef, @@ -2498,6 +2635,12 @@ impl RuntimeState { } } +#[derive(Debug, Clone)] +struct InternalWorkerActivity { + status: protocol::WorkerStatus, + parent_session_id: Option, +} + #[derive(Debug)] struct WorkerRecord { worker_ref: WorkerRef, @@ -2508,6 +2651,7 @@ struct WorkerRecord { run_generation: u64, working_directory: Option, execution_handle: Option, + internal_workers: BTreeMap, } impl WorkerRecord { @@ -2730,6 +2874,126 @@ mod tests { use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; + fn internal_worker_ref( + session_id: &str, + parent_session_id: Option<&str>, + ) -> protocol::InternalWorkerRef { + protocol::InternalWorkerRef { + session_id: session_id.to_string(), + parent_session_id: parent_session_id.map(str::to_string), + name: session_id.to_string(), + kind: protocol::InternalWorkerKind::SubWorker, + } + } + + fn internal_worker_status_event( + worker: protocol::InternalWorkerRef, + status: protocol::WorkerStatus, + ) -> protocol::Event { + protocol::Event::InternalWorker { + worker, + revision: 1, + event: Box::new(protocol::Event::Status { status }), + } + } + + #[test] + fn internal_worker_activity_tracks_running_children_independently() { + let mut activity = BTreeMap::new(); + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-a", None), + protocol::WorkerStatus::Running, + ), + )); + assert!(!RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-b", None), + protocol::WorkerStatus::Running, + ), + )); + assert!(!RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-a", None), + protocol::WorkerStatus::Idle, + ), + )); + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-b", None), + protocol::WorkerStatus::Idle, + ), + )); + } + + #[test] + fn nested_internal_worker_activity_reaches_the_parent_projection() { + let mut activity = BTreeMap::new(); + let direct_child = internal_worker_ref("child", None); + let nested_running = protocol::Event::InternalWorker { + worker: direct_child.clone(), + revision: 1, + event: Box::new(internal_worker_status_event( + internal_worker_ref("grandchild", Some("child")), + protocol::WorkerStatus::Running, + )), + }; + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &nested_running, + )); + + let nested_idle = protocol::Event::InternalWorker { + worker: direct_child, + revision: 2, + event: Box::new(internal_worker_status_event( + internal_worker_ref("grandchild", Some("child")), + protocol::WorkerStatus::Idle, + )), + }; + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &nested_idle, + )); + } + + #[test] + fn parent_snapshot_replaces_stale_internal_worker_activity() { + let mut activity = BTreeMap::new(); + RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-a", None), + protocol::WorkerStatus::Running, + ), + ); + let snapshot = protocol::Event::Snapshot { + entries: Vec::new(), + greeting: protocol::Greeting { + worker_name: "parent".to_string(), + cwd: "/tmp".to_string(), + provider: "test".to_string(), + model: "test".to_string(), + scope_summary: String::new(), + tools: Vec::new(), + context_window: 0, + context_tokens: 0, + }, + status: protocol::WorkerStatus::Idle, + in_flight: protocol::InFlightSnapshot::default(), + internal_workers: Vec::new(), + }; + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &snapshot, + )); + assert!(activity.is_empty()); + } + #[test] fn runtime_identity_binding_is_immutable_and_host_owned() { let runtime = Runtime::new_memory(); @@ -3083,16 +3347,75 @@ mod tests { payload => panic!("unexpected subscription payload: {payload:?}"), } - runtime.stop_runtime().unwrap(); + runtime + .observe_worker_event( + &created.worker_ref, + internal_worker_status_event( + internal_worker_ref("child-live", None), + protocol::WorkerStatus::Running, + ), + ) + .unwrap(); let update = receive_subscription_update(&mut subscription).unwrap(); assert_eq!(update.subject_revision, 2); match update.payload { SubscriptionEventPayload::WorkerUpserted { worker } => { - assert_eq!(worker.worker_id.as_str(), created.worker_id.to_string()); - assert_eq!(worker.state, SubscriptionWorkerState::Stopped); + assert_eq!(worker.state, SubscriptionWorkerState::Idle); + assert!(worker.has_running_internal_workers); } payload => panic!("unexpected subscription payload: {payload:?}"), } + + runtime + .observe_worker_event( + &created.worker_ref, + internal_worker_status_event( + internal_worker_ref("child-live", None), + protocol::WorkerStatus::Idle, + ), + ) + .unwrap(); + let update = receive_subscription_update(&mut subscription).unwrap(); + assert_eq!(update.subject_revision, 3); + match update.payload { + SubscriptionEventPayload::WorkerUpserted { worker } => { + assert_eq!(worker.state, SubscriptionWorkerState::Idle); + assert!(!worker.has_running_internal_workers); + } + payload => panic!("unexpected subscription payload: {payload:?}"), + } + + runtime + .observe_worker_event( + &created.worker_ref, + internal_worker_status_event( + internal_worker_ref("child-live", None), + protocol::WorkerStatus::Running, + ), + ) + .unwrap(); + let update = receive_subscription_update(&mut subscription).unwrap(); + assert_eq!(update.subject_revision, 4); + match update.payload { + SubscriptionEventPayload::WorkerUpserted { worker } => { + assert_eq!(worker.state, SubscriptionWorkerState::Idle); + assert!(worker.has_running_internal_workers); + } + payload => panic!("unexpected subscription payload: {payload:?}"), + } + + runtime.stop_worker(&created.worker_ref, None).unwrap(); + let update = receive_subscription_update(&mut subscription).unwrap(); + assert_eq!(update.subject_revision, 5); + match update.payload { + SubscriptionEventPayload::WorkerUpserted { worker } => { + assert_eq!(worker.worker_id.as_str(), created.worker_id.to_string()); + assert_eq!(worker.state, SubscriptionWorkerState::Stopped); + assert!(!worker.has_running_internal_workers); + } + payload => panic!("unexpected subscription payload: {payload:?}"), + } + runtime.stop_runtime().unwrap(); } #[test] diff --git a/web/workspace/src/lib/generated/protocol.ts b/web/workspace/src/lib/generated/protocol.ts index 471f4dab..25fc8cf2 100644 --- a/web/workspace/src/lib/generated/protocol.ts +++ b/web/workspace/src/lib/generated/protocol.ts @@ -128,7 +128,7 @@ runtime_id?: string | null, /** * Producer-owned monotonic revision for this Worker subject. */ -subject_revision: number, state: SubscriptionWorkerState, workspace_id?: string | null, display_name?: string | null, profile?: string | null, repository_id?: string | null, working_directory_id?: SubscriptionWorkdirId | null, }; +subject_revision: number, state: SubscriptionWorkerState, has_running_internal_workers: boolean, workspace_id?: string | null, display_name?: string | null, profile?: string | null, repository_id?: string | null, working_directory_id?: SubscriptionWorkdirId | null, }; export type SubscriptionWorkdir = { working_directory_id: SubscriptionWorkdirId, repository_id: string, state: string, primary_worker_id?: SubscriptionWorkerId | null, }; diff --git a/web/workspace/src/lib/workspace/console/Spinner.svelte b/web/workspace/src/lib/workspace/console/Spinner.svelte index 620e9b71..21b90518 100644 --- a/web/workspace/src/lib/workspace/console/Spinner.svelte +++ b/web/workspace/src/lib/workspace/console/Spinner.svelte @@ -29,7 +29,7 @@ diff --git a/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte b/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte index 584add88..b2fffcd6 100644 --- a/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte +++ b/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte @@ -1,4 +1,5 @@ -{#if mode === "mini" && tasks.length > 0} +{#if mode === "mini" && (tasks.length > 0 || workerViews.length > 1)}
{#each activeTasks as task (task.taskid)}
@@ -38,8 +52,25 @@ {task.subject.split("\n", 1)[0]}
{/each} -
- {counts.total} task(s) — pending: {counts.pending}, inprogress: {counts.inprogress}, completed: {counts.completed}, deleted: {counts.deleted} +
+ + {counts.total} task(s) — pending: {counts.pending}, inprogress: {counts.inprogress}, completed: {counts.completed}, deleted: {counts.deleted} + + {#if workerViews.length > 1} + + + {#each workerViews as view, index (view.sessionId ?? "main")} + {#if index > 0}{/if} + + {/each} + + + {/if}
{:else if mode === "pane"} @@ -95,12 +126,69 @@ } .task-mini-row, - .task-heading { + .task-heading, + .task-summary-row { display: flex; min-width: 0; gap: 0.5rem; } + .task-summary-row { + align-items: baseline; + } + + .task-summary { + flex: 1 1 auto; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } + + .worker-view-tabs { + display: flex; + flex: 0 1 auto; + min-width: 0; + max-width: 60%; + margin-left: auto; + overflow-x: auto; + color: var(--text-muted); + scrollbar-width: none; + white-space: nowrap; + } + + .worker-view-tabs::-webkit-scrollbar { + display: none; + } + + .worker-view-tabs button { + flex: 0 0 auto; + min-width: 0; + margin: 0; + padding: 0; + border: 0; + background: transparent; + color: inherit; + font: inherit; + line-height: inherit; + cursor: pointer; + } + + .worker-view-tabs button:hover, + .worker-view-tabs button:focus-visible { + color: var(--text); + } + + .worker-view-tabs button:focus-visible { + outline: 1px solid currentcolor; + outline-offset: 2px; + } + + .worker-view-tabs button.active { + color: var(--accent); + font-weight: 700; + } + .task-mark, .task-id { flex: 0 0 auto; diff --git a/web/workspace/src/lib/workspace/console/model.test.ts b/web/workspace/src/lib/workspace/console/model.test.ts index 450bdc74..ccda222c 100644 --- a/web/workspace/src/lib/workspace/console/model.test.ts +++ b/web/workspace/src/lib/workspace/console/model.test.ts @@ -2,11 +2,14 @@ import type { Event } from "$lib/generated/protocol"; import { type ConsoleEventInput, type ConsoleLine, + consoleWorkerViews, createConsoleProjector, isConsoleProjectionEvent, projectConsole, projectConsoleLines, projectOverviewLines, + resolveConsoleViewScrollTop, + resolveConsoleWorkerView, segmentsToText, selectConsoleTimelineLines, workerConsoleHref, @@ -1309,6 +1312,96 @@ Deno.test("Internal Worker output stays separate and revision-fenced", () => { }, }]); assertEquals(projection.internalWorkers[0].console.lines.length, 1); + + const views = consoleWorkerViews(projection); + assertEquals(views.map((view) => [view.sessionId, view.label]), [ + [null, "main"], + ["child-session", "research"], + ]); + assertEquals( + resolveConsoleWorkerView(projection, "child-session").console.lines[0].body, + "child output", + ); + assertEquals(resolveConsoleWorkerView(projection, "missing").sessionId, null); +}); + +Deno.test("console Worker views expose only direct Internal Workers", () => { + const projector = createConsoleProjector(); + projector.append([{ + eventId: "nested", + event: { + event: "internal_worker", + data: { + worker: { + session_id: "child-session", + name: "research", + parent_session_id: "parent-session", + kind: "sub_worker", + }, + revision: 1, + event: { + event: "internal_worker", + data: { + worker: { + session_id: "grandchild-session", + name: "nested", + parent_session_id: "child-session", + kind: "sub_worker", + }, + revision: 1, + event: { event: "status", data: { status: "running" } }, + }, + }, + }, + }, + }]); + + projector.append([{ + eventId: "peer", + event: { + event: "internal_worker", + data: { + worker: { + session_id: "peer-other", + name: "research", + parent_session_id: "parent-session", + kind: "sub_worker", + }, + revision: 1, + event: { event: "status", data: { status: "idle" } }, + }, + }, + }]); + + const projection = projector.snapshot(); + const views = consoleWorkerViews(projection); + assertEquals(views.map((view) => view.sessionId), [ + null, + "child-session", + "peer-other", + ]); + assertEquals(views[1].label, "research · ession"); + assertEquals(views[2].label, "research · -other"); + assertEquals( + resolveConsoleWorkerView(projection, "grandchild-session").sessionId, + null, + ); +}); + +Deno.test("console Worker view scroll restores manual offsets and auto-follow", () => { + assertEquals(resolveConsoleViewScrollTop(undefined, 1000, 200), 1000); + assertEquals( + resolveConsoleViewScrollTop({ top: 100, autoFollow: true }, 1000, 200), + 1000, + ); + assertEquals( + resolveConsoleViewScrollTop({ top: 300, autoFollow: false }, 1000, 200), + 300, + ); + assertEquals( + resolveConsoleViewScrollTop({ top: 900, autoFollow: false }, 1000, 200), + 800, + ); }); Deno.test("parent snapshot authoritatively replaces Internal Worker projections", () => { diff --git a/web/workspace/src/lib/workspace/console/model.ts b/web/workspace/src/lib/workspace/console/model.ts index ea383d76..18d0550b 100644 --- a/web/workspace/src/lib/workspace/console/model.ts +++ b/web/workspace/src/lib/workspace/console/model.ts @@ -87,18 +87,57 @@ export type InternalWorkerProjection = { console: ConsoleProjection; }; -export type FlattenedInternalWorkerProjection = InternalWorkerProjection & { - depth: number; +export type ConsoleViewScroll = { + top: number; + autoFollow: boolean; }; -export function flattenInternalWorkers( - workers: InternalWorkerProjection[], - depth = 0, -): FlattenedInternalWorkerProjection[] { - return workers.flatMap((worker) => [ - { ...worker, depth }, - ...flattenInternalWorkers(worker.console.internalWorkers, depth + 1), - ]); +export function resolveConsoleViewScrollTop( + state: ConsoleViewScroll | undefined, + scrollHeight: number, + clientHeight: number, +): number { + if (!state || state.autoFollow) return scrollHeight; + return Math.min(state.top, Math.max(0, scrollHeight - clientHeight)); +} + +export type ConsoleWorkerView = { + sessionId: string | null; + label: string; + console: ConsoleProjection; +}; + +export function consoleWorkerViews( + projection: ConsoleProjection, +): ConsoleWorkerView[] { + const labels = projection.internalWorkers.map((worker) => + worker.worker.name || "subworker" + ); + const labelCounts = new Map(); + for (const label of labels) { + labelCounts.set(label, (labelCounts.get(label) ?? 0) + 1); + } + return [ + { sessionId: null, label: "main", console: projection }, + ...projection.internalWorkers.map((worker, index) => { + const label = labels[index] ?? "subworker"; + return { + sessionId: worker.worker.session_id, + label: labelCounts.get(label) === 1 + ? label + : `${label} · ${worker.worker.session_id.slice(-6)}`, + console: worker.console, + }; + }), + ]; +} + +export function resolveConsoleWorkerView( + projection: ConsoleProjection, + selectedSessionId: string | null, +): ConsoleWorkerView { + const views = consoleWorkerViews(projection); + return views.find((view) => view.sessionId === selectedSessionId) ?? views[0]; } export type ConsoleProjection = { diff --git a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts index 67c9ef34..fa30d021 100644 --- a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts +++ b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts @@ -797,7 +797,7 @@ Deno.test("Web Console renders the client-projected Worker task store", async () assert( consolePage.includes("ConsoleTasks") && - consolePage.includes("consoleProjection.tasks") && + consolePage.includes("selectedConsoleProjection.tasks") && consolePage.includes("taskPaneOpen"), "Console should expose the projected task store through its existing client model", ); @@ -818,3 +818,44 @@ Deno.test("Web Console renders the client-projected Worker task store", async () "Task projection should replay the protocol client-side without adding a task API", ); }); + +Deno.test("Web Console switches main and direct SubWorker views from the Tasks row", async () => { + const consolePage = await Deno.readTextFile( + new URL( + "./../../../routes/w/[workspaceId]/runtimes/[runtimeId]/workers/[workerId]/console/+page.svelte", + import.meta.url, + ), + ); + const tasksComponent = await Deno.readTextFile( + new URL("./ConsoleTasks.svelte", import.meta.url), + ); + const consoleModel = await Deno.readTextFile( + new URL("./model.ts", import.meta.url), + ); + + assert( + consolePage.includes("selectedWorkerViewSessionId") && + consolePage.includes("selectConsoleWorkerView") && + consolePage.includes("selectedConsoleProjection.lines") && + consolePage.includes("selectedConsoleProjection.tasks") && + consolePage.includes("onSelectWorkerView") && + consolePage.includes("selectConsoleWorkerView(resolvedSessionId, false)") && + consolePage.includes("consoleWorkerViewSelectionIsResolved") && + !consolePage.includes("internal-worker-pane") && + !consolePage.includes("flattenInternalWorkers"), + "Console should render one selected transcript/task projection without appending Internal Worker panes", + ); + assert( + tasksComponent.includes('role="group"') && + tasksComponent.includes("aria-pressed") && + tasksComponent.includes("onclick") && + tasksComponent.includes("tasks.length > 0 || workerViews.length > 1"), + "Tasks summary should expose a clickable and accessible Worker view selector even with zero tasks", + ); + assert( + consoleModel.includes("consoleWorkerViews") && + consoleModel.includes("projection.internalWorkers.map") && + consoleModel.includes("resolveConsoleWorkerView"), + "Worker view selection should use direct Internal Worker session identities with main fallback", + ); +}); diff --git a/web/workspace/src/routes/w/[workspaceId]/runtimes/[runtimeId]/workers/[workerId]/console/+page.svelte b/web/workspace/src/routes/w/[workspaceId]/runtimes/[runtimeId]/workers/[workerId]/console/+page.svelte index 1eea394c..d2a75564 100644 --- a/web/workspace/src/routes/w/[workspaceId]/runtimes/[runtimeId]/workers/[workerId]/console/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/runtimes/[runtimeId]/workers/[workerId]/console/+page.svelte @@ -19,15 +19,18 @@ import { fitTextarea } from "$lib/workspace/console/textarea-fit"; import { resolveWorkerControlShortcut } from "$lib/workspace/console/worker-control-shortcuts"; import { + consoleWorkerViews, createConsoleProjector, - flattenInternalWorkers, isConsoleProjectionEvent, projectConsoleLines, + resolveConsoleViewScrollTop, + resolveConsoleWorkerView, selectConsoleTimelineLines, type ConsoleEventInput, type ConsoleLine, type ConsoleProjection, type ConsoleViewMode, + type ConsoleViewScroll, } from "$lib/workspace/console/model"; import type { Event as ProtocolEvent, Method as ProtocolMethod, RewindTarget, Segment } from "$lib/generated/protocol"; import { workspaceApiPath } from "$lib/workspace/api/http"; @@ -122,6 +125,8 @@ let streamDiagnostics = $state([]); let workerDetailsOpen = $state(false); let taskPaneOpen = $state(false); + let selectedWorkerViewSessionId = $state(null); + let workerViewSelectionGeneration = 0; let timelineOpen = $state(false); let consoleViewMode = $state("overview"); let consoleBodyElement: HTMLElement | null = null; @@ -129,6 +134,7 @@ let timelineRailDragCleanup: (() => void) | null = null; let autoFollowConsole = $state(true); let consoleScroll = $state({ top: 0, height: 1, client: 1 }); + const consoleViewScroll = new Map(); const eventObservedAtById = new Map(); let nextEventObservedAtVersion = 0; let eventObservedAtVersion = $state(0); @@ -156,13 +162,18 @@ const consoleTarget = $derived({ workspaceId, runtimeId, workerId }); + const workerViews = $derived(consoleWorkerViews(consoleProjection)); + const selectedWorkerView = $derived( + resolveConsoleWorkerView( + consoleProjection, + selectedWorkerViewSessionId, + ), + ); + const selectedConsoleProjection = $derived(selectedWorkerView.console); const lines = $derived( - projectConsoleLines(consoleProjection.lines, consoleViewMode), - ); - const tasks = $derived(consoleProjection.tasks); - const internalWorkers = $derived( - flattenInternalWorkers(consoleProjection.internalWorkers), + projectConsoleLines(selectedConsoleProjection.lines, consoleViewMode), ); + const tasks = $derived(selectedConsoleProjection.tasks); const timelineLayout = $derived( buildTimelineLayout(lines, eventObservedAtVersion, consoleScroll), ); @@ -1085,6 +1096,47 @@ : value.replaceAll('"', '\\"'); } + function consoleWorkerViewKey(sessionId: string | null): string { + return sessionId === null ? "main" : `internal:${sessionId}`; + } + + function consoleWorkerViewSelectionIsResolved(): boolean { + return selectedWorkerViewSessionId === selectedWorkerView.sessionId; + } + + function rememberConsoleWorkerViewScroll() { + if (!consoleBodyElement || !consoleWorkerViewSelectionIsResolved()) return; + consoleViewScroll.set(consoleWorkerViewKey(selectedWorkerView.sessionId), { + top: consoleBodyElement.scrollTop, + autoFollow: autoFollowConsole, + }); + } + + async function selectConsoleWorkerView( + sessionId: string | null, + rememberCurrent = true, + ) { + if (sessionId === selectedWorkerViewSessionId) return; + const generation = ++workerViewSelectionGeneration; + if (rememberCurrent) rememberConsoleWorkerViewScroll(); + const target = workerViews.find((view) => view.sessionId === sessionId) ?? + workerViews[0]; + const targetScroll = consoleViewScroll.get( + consoleWorkerViewKey(target.sessionId), + ); + autoFollowConsole = targetScroll?.autoFollow ?? true; + selectedWorkerViewSessionId = target.sessionId; + await tick(); + if (generation !== workerViewSelectionGeneration) return; + if (!consoleBodyElement) return; + consoleBodyElement.scrollTop = resolveConsoleViewScrollTop( + targetScroll, + consoleBodyElement.scrollHeight, + consoleBodyElement.clientHeight, + ); + updateConsoleScrollMetrics(); + } + function updateConsoleScrollMetrics() { if (!consoleBodyElement) { return; @@ -1104,21 +1156,30 @@ } function handleConsoleScroll() { - if (!consoleBodyElement) { + if (!consoleWorkerViewSelectionIsResolved() || !consoleBodyElement) { return; } autoFollowConsole = isNearConsoleBottom(consoleBodyElement); updateConsoleScrollMetrics(); + rememberConsoleWorkerViewScroll(); } async function scrollConsoleToBottom() { + if (!consoleWorkerViewSelectionIsResolved()) return; + const sessionId = selectedWorkerView.sessionId; await tick(); - if (!consoleBodyElement) { + if ( + !consoleBodyElement || + !autoFollowConsole || + !consoleWorkerViewSelectionIsResolved() || + selectedWorkerView.sessionId !== sessionId + ) { return; } consoleBodyElement.scrollTop = consoleBodyElement.scrollHeight; updateConsoleScrollMetrics(); autoFollowConsole = true; + rememberConsoleWorkerViewScroll(); } const scrollFollowKey = $derived( @@ -1132,10 +1193,32 @@ $effect(() => { scrollFollowKey; + if (!consoleWorkerViewSelectionIsResolved()) return; if (autoFollowConsole) { void scrollConsoleToBottom(); } else { - tick().then(updateConsoleScrollMetrics); + const sessionId = selectedWorkerView.sessionId; + tick().then(() => { + if ( + consoleWorkerViewSelectionIsResolved() && + selectedWorkerView.sessionId === sessionId + ) { + updateConsoleScrollMetrics(); + } + }); + } + }); + + $effect(() => { + const activeViewKeys = new Set( + workerViews.map((view) => consoleWorkerViewKey(view.sessionId)), + ); + for (const key of consoleViewScroll.keys()) { + if (!activeViewKeys.has(key)) consoleViewScroll.delete(key); + } + const resolvedSessionId = selectedWorkerView.sessionId; + if (resolvedSessionId !== selectedWorkerViewSessionId) { + void selectConsoleWorkerView(resolvedSessionId, false); } }); @@ -1147,6 +1230,10 @@ const target = consoleTarget; const targetWorker = data.worker; const targetWorkerError = data.workerError; + workerViewSelectionGeneration += 1; + selectedWorkerViewSessionId = null; + consoleViewScroll.clear(); + autoFollowConsole = true; resetObservedEvents(); taskPaneOpen = false; worker = targetWorker; @@ -1282,7 +1369,10 @@ bind:this={consoleBodyElement} onscroll={handleConsoleScroll} > -
+
{#if workerError}

{workerError}

{/if} @@ -1297,28 +1387,6 @@ {/if}
- - {#each internalWorkers as internal (internal.worker.session_id)} -
-
- {internal.worker.name} - {internal.console.status ?? "unknown"} -
- {#if internal.console.lines.length === 0} -

No output yet.

- {:else} -
    - {#each projectConsoleLines(internal.console.lines, consoleViewMode) as item (item.id)} - - {/each} -
- {/if} -
- {/each} {/if} - + ({ + sessionId, + label, + }))} + selectedWorkerViewSessionId={selectedWorkerView.sessionId} + onSelectWorkerView={(sessionId) => { + void selectConsoleWorkerView(sessionId); + }} + />
@@ -1869,23 +1948,6 @@ margin-right: auto; } - .internal-worker-pane { - margin: 0.75rem 0 0 calc((var(--internal-worker-depth) + 1) * 1rem); - border-left: 3px solid var(--color-border-strong, currentColor); - } - - .internal-worker-header { - display: flex; - justify-content: space-between; - gap: 1rem; - margin-bottom: 0.5rem; - font-family: var(--font-mono); - } - - .internal-worker-header span { - color: var(--color-text-muted); - } - @media (max-width: 960px) { .console-history.with-task-pane { grid-template-columns: minmax(0, 1fr); From b49abce7985105b2564a0bb319489898765a0ee7 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 21 Aug 2026 07:13:46 +0900 Subject: [PATCH 5/5] fix: locate multiline Console Tasks source --- web/workspace/tests/console-spinner-source.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web/workspace/tests/console-spinner-source.test.ts b/web/workspace/tests/console-spinner-source.test.ts index a4392077..424f74d6 100644 --- a/web/workspace/tests/console-spinner-source.test.ts +++ b/web/workspace/tests/console-spinner-source.test.ts @@ -98,10 +98,12 @@ Deno.test("running status is Composer-side above mini Tasks", async () => { ), ); const status = page.indexOf("= 0, "WorkerRunStatus should be rendered"); + assert(miniMode >= 0 && tasks >= 0, "mini Tasks should be rendered"); assert(status < tasks, "WorkerRunStatus should be above mini Tasks"); assert(tasks < composer, "mini Tasks should remain above Composer"); assert(