fix: remove caller worker revision fence
This commit is contained in:
@@ -125,7 +125,6 @@ pub trait EmbeddedWorkerMutationDispatcher: Send + Sync {
|
|||||||
proof: InProcessWorkerMutationProof,
|
proof: InProcessWorkerMutationProof,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, RuntimeWorkerMutationForwardError>;
|
) -> Result<WorkspaceResponse, RuntimeWorkerMutationForwardError>;
|
||||||
}
|
}
|
||||||
@@ -187,7 +186,6 @@ impl RuntimeWorkerMutationForwarder {
|
|||||||
&self,
|
&self,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, RuntimeWorkerMutationForwardError> {
|
) -> Result<WorkspaceResponse, RuntimeWorkerMutationForwardError> {
|
||||||
let proof = self.authority.issue_worker_remove(
|
let proof = self.authority.issue_worker_remove(
|
||||||
@@ -206,7 +204,6 @@ impl RuntimeWorkerMutationForwarder {
|
|||||||
token,
|
token,
|
||||||
target_runtime_id: target_runtime_id.to_string(),
|
target_runtime_id: target_runtime_id.to_string(),
|
||||||
target_worker_id: target_worker_id.to_string(),
|
target_worker_id: target_worker_id.to_string(),
|
||||||
expected_worker_revision: expected_worker_revision.to_string(),
|
|
||||||
reason: reason.to_string(),
|
reason: reason.to_string(),
|
||||||
}),
|
}),
|
||||||
(
|
(
|
||||||
@@ -216,7 +213,6 @@ impl RuntimeWorkerMutationForwarder {
|
|||||||
claims,
|
claims,
|
||||||
target_runtime_id,
|
target_runtime_id,
|
||||||
target_worker_id,
|
target_worker_id,
|
||||||
expected_worker_revision,
|
|
||||||
reason,
|
reason,
|
||||||
),
|
),
|
||||||
_ => Err(RuntimeWorkerMutationForwardError::AuthorityTransportMismatch),
|
_ => Err(RuntimeWorkerMutationForwardError::AuthorityTransportMismatch),
|
||||||
@@ -230,7 +226,6 @@ struct RemoteWorkerRemoveHttpRequest {
|
|||||||
token: String,
|
token: String,
|
||||||
target_runtime_id: String,
|
target_runtime_id: String,
|
||||||
target_worker_id: String,
|
target_worker_id: String,
|
||||||
expected_worker_revision: String,
|
|
||||||
reason: String,
|
reason: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -267,7 +262,6 @@ fn execute_remote_worker_remove_http_blocking(
|
|||||||
let body = serde_json::json!({
|
let body = serde_json::json!({
|
||||||
"target_runtime_id": request.target_runtime_id,
|
"target_runtime_id": request.target_runtime_id,
|
||||||
"target_worker_id": request.target_worker_id,
|
"target_worker_id": request.target_worker_id,
|
||||||
"expected_worker_revision": request.expected_worker_revision,
|
|
||||||
"reason": request.reason,
|
"reason": request.reason,
|
||||||
});
|
});
|
||||||
let client = reqwest::blocking::Client::new();
|
let client = reqwest::blocking::Client::new();
|
||||||
@@ -474,7 +468,6 @@ impl WorkspaceClient for RuntimeOwnedWorkspaceClient {
|
|||||||
&self,
|
&self,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
||||||
self.worker_remove
|
self.worker_remove
|
||||||
@@ -484,12 +477,7 @@ impl WorkspaceClient for RuntimeOwnedWorkspaceClient {
|
|||||||
"Runtime-owned WorkerRemove forwarding is unavailable".to_string(),
|
"Runtime-owned WorkerRemove forwarding is unavailable".to_string(),
|
||||||
)
|
)
|
||||||
})?
|
})?
|
||||||
.execute_worker_remove(
|
.execute_worker_remove(target_runtime_id, target_worker_id, reason)
|
||||||
target_runtime_id,
|
|
||||||
target_worker_id,
|
|
||||||
expected_worker_revision,
|
|
||||||
reason,
|
|
||||||
)
|
|
||||||
.map_err(|error| WorkspaceClientError::Request(error.to_string()))
|
.map_err(|error| WorkspaceClientError::Request(error.to_string()))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -915,12 +903,7 @@ mod tests {
|
|||||||
format!("http://{address}"),
|
format!("http://{address}"),
|
||||||
);
|
);
|
||||||
let response = forwarder
|
let response = forwarder
|
||||||
.execute_worker_remove(
|
.execute_worker_remove("runtime-target", "worker-target", "retire obsolete Worker")
|
||||||
"runtime-target",
|
|
||||||
"worker-target",
|
|
||||||
"revision-7",
|
|
||||||
"retire obsolete Worker",
|
|
||||||
)
|
|
||||||
.unwrap();
|
.unwrap();
|
||||||
assert_eq!(response.status, 204);
|
assert_eq!(response.status, 204);
|
||||||
server.join().unwrap();
|
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.starts_with("POST /api/w/workspace-a/workers/remove HTTP/1.1"));
|
||||||
assert!(request.contains("\"target_runtime_id\":\"runtime-target\""));
|
assert!(request.contains("\"target_runtime_id\":\"runtime-target\""));
|
||||||
assert!(request.contains("\"target_worker_id\":\"worker-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\""));
|
assert!(request.contains("\"reason\":\"retire obsolete Worker\""));
|
||||||
let token = request
|
let token = request
|
||||||
.lines()
|
.lines()
|
||||||
@@ -962,7 +945,7 @@ mod tests {
|
|||||||
|
|
||||||
#[derive(Default)]
|
#[derive(Default)]
|
||||||
struct RecordingDispatcher {
|
struct RecordingDispatcher {
|
||||||
seen: Mutex<Option<(WorkerMutationSourceClaims, String, String, String, String)>>,
|
seen: Mutex<Option<(WorkerMutationSourceClaims, String, String, String)>>,
|
||||||
}
|
}
|
||||||
impl EmbeddedWorkerMutationDispatcher for RecordingDispatcher {
|
impl EmbeddedWorkerMutationDispatcher for RecordingDispatcher {
|
||||||
fn execute_worker_remove(
|
fn execute_worker_remove(
|
||||||
@@ -970,14 +953,12 @@ mod tests {
|
|||||||
proof: InProcessWorkerMutationProof,
|
proof: InProcessWorkerMutationProof,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, RuntimeWorkerMutationForwardError> {
|
) -> Result<WorkspaceResponse, RuntimeWorkerMutationForwardError> {
|
||||||
*self.seen.lock().unwrap() = Some((
|
*self.seen.lock().unwrap() = Some((
|
||||||
proof.into_claims(),
|
proof.into_claims(),
|
||||||
target_runtime_id.to_string(),
|
target_runtime_id.to_string(),
|
||||||
target_worker_id.to_string(),
|
target_worker_id.to_string(),
|
||||||
expected_worker_revision.to_string(),
|
|
||||||
reason.to_string(),
|
reason.to_string(),
|
||||||
));
|
));
|
||||||
Ok(WorkspaceResponse {
|
Ok(WorkspaceResponse {
|
||||||
@@ -996,15 +977,10 @@ mod tests {
|
|||||||
dispatcher.clone(),
|
dispatcher.clone(),
|
||||||
);
|
);
|
||||||
let response = forwarder
|
let response = forwarder
|
||||||
.execute_worker_remove(
|
.execute_worker_remove("runtime-target", "worker-target", "retire obsolete Worker")
|
||||||
"runtime-target",
|
|
||||||
"worker-target",
|
|
||||||
"revision-7",
|
|
||||||
"retire obsolete Worker",
|
|
||||||
)
|
|
||||||
.unwrap();
|
.unwrap();
|
||||||
assert_eq!(response.status, 202);
|
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();
|
dispatcher.seen.lock().unwrap().take().unwrap();
|
||||||
assert_eq!(claims.iss, "runtime-embedded");
|
assert_eq!(claims.iss, "runtime-embedded");
|
||||||
assert_eq!(claims.worker_id, "worker-source");
|
assert_eq!(claims.worker_id, "worker-source");
|
||||||
@@ -1012,7 +988,6 @@ mod tests {
|
|||||||
assert_eq!(claims.target_worker_id, "worker-target");
|
assert_eq!(claims.target_worker_id, "worker-target");
|
||||||
assert_eq!(target_runtime_id, "runtime-target");
|
assert_eq!(target_runtime_id, "runtime-target");
|
||||||
assert_eq!(target_worker_id, "worker-target");
|
assert_eq!(target_worker_id, "worker-target");
|
||||||
assert_eq!(expected_revision, "revision-7");
|
|
||||||
assert_eq!(reason, "retire obsolete Worker");
|
assert_eq!(reason, "retire obsolete Worker");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -48,7 +48,6 @@ pub trait WorkerControlService: Send + Sync {
|
|||||||
&self,
|
&self,
|
||||||
runtime_id: &str,
|
runtime_id: &str,
|
||||||
worker_id: &str,
|
worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, WorkspaceClientError>;
|
) -> Result<WorkspaceResponse, WorkspaceClientError>;
|
||||||
async fn execute_runtime(
|
async fn execute_runtime(
|
||||||
@@ -165,11 +164,10 @@ impl WorkerControlService for WorkspaceWorkerControlService {
|
|||||||
&self,
|
&self,
|
||||||
runtime_id: &str,
|
runtime_id: &str,
|
||||||
worker_id: &str,
|
worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
||||||
self.client
|
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(
|
async fn execute_runtime(
|
||||||
@@ -530,7 +528,6 @@ struct WorkerStopInput {
|
|||||||
#[serde(deny_unknown_fields)]
|
#[serde(deny_unknown_fields)]
|
||||||
struct WorkerRemoveInput {
|
struct WorkerRemoveInput {
|
||||||
subject: WorkerSubjectInput,
|
subject: WorkerSubjectInput,
|
||||||
expected_worker_revision: String,
|
|
||||||
reason: String,
|
reason: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -592,7 +589,7 @@ impl WorkerOperation {
|
|||||||
"Restore a stopped Backend/Runtime Worker session in the current Workspace."
|
"Restore a stopped Backend/Runtime Worker session in the current Workspace."
|
||||||
}
|
}
|
||||||
Self::Remove => {
|
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 => {
|
WorkerOperation::Remove => {
|
||||||
let input = parse::<WorkerRemoveInput>(input_json, "WorkerRemove")?;
|
let input = parse::<WorkerRemoveInput>(input_json, "WorkerRemove")?;
|
||||||
let (runtime_id, worker_id) = runtime_subject_ids(&input.subject, self.operation)?;
|
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")?;
|
let reason = non_empty(input.reason, "reason")?;
|
||||||
if reason.len() > 512 {
|
if reason.len() > 512 {
|
||||||
return Err(ToolError::ExecutionFailed(
|
return Err(ToolError::ExecutionFailed(
|
||||||
@@ -751,12 +746,7 @@ impl Tool for WorkspaceWorkerTool {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
self.control
|
self.control
|
||||||
.remove_runtime_worker(
|
.remove_runtime_worker(&runtime_id, &worker_id, &reason)
|
||||||
&runtime_id,
|
|
||||||
&worker_id,
|
|
||||||
&expected_worker_revision,
|
|
||||||
&reason,
|
|
||||||
)
|
|
||||||
.map_err(control_tool_error)?
|
.map_err(control_tool_error)?
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
@@ -912,7 +902,7 @@ mod tests {
|
|||||||
#[derive(Debug, Default)]
|
#[derive(Debug, Default)]
|
||||||
struct RecordingWorkspaceClient {
|
struct RecordingWorkspaceClient {
|
||||||
requests: Mutex<Vec<WorkspaceRequest>>,
|
requests: Mutex<Vec<WorkspaceRequest>>,
|
||||||
removals: Mutex<Vec<(String, String, String, String)>>,
|
removals: Mutex<Vec<(String, String, String)>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl WorkspaceClient for RecordingWorkspaceClient {
|
impl WorkspaceClient for RecordingWorkspaceClient {
|
||||||
@@ -943,13 +933,11 @@ mod tests {
|
|||||||
&self,
|
&self,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
||||||
self.removals.lock().unwrap().push((
|
self.removals.lock().unwrap().push((
|
||||||
target_runtime_id.to_string(),
|
target_runtime_id.to_string(),
|
||||||
target_worker_id.to_string(),
|
target_worker_id.to_string(),
|
||||||
expected_worker_revision.to_string(),
|
|
||||||
reason.to_string(),
|
reason.to_string(),
|
||||||
));
|
));
|
||||||
Ok(WorkspaceResponse {
|
Ok(WorkspaceResponse {
|
||||||
@@ -1160,7 +1148,7 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[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 client = Arc::new(RecordingWorkspaceClient::default());
|
||||||
let tool = WorkspaceWorkerTool {
|
let tool = WorkspaceWorkerTool {
|
||||||
operation: WorkerOperation::Remove,
|
operation: WorkerOperation::Remove,
|
||||||
@@ -1173,7 +1161,6 @@ mod tests {
|
|||||||
"runtime_id": "runtime-1",
|
"runtime_id": "runtime-1",
|
||||||
"worker_id": "worker-7",
|
"worker_id": "worker-7",
|
||||||
},
|
},
|
||||||
"expected_worker_revision": "2026-08-11T20:00:00Z",
|
|
||||||
"reason": " retire completed Worker "
|
"reason": " retire completed Worker "
|
||||||
})
|
})
|
||||||
.to_string(),
|
.to_string(),
|
||||||
@@ -1186,7 +1173,6 @@ mod tests {
|
|||||||
[(
|
[(
|
||||||
"runtime-1".to_string(),
|
"runtime-1".to_string(),
|
||||||
"worker-7".to_string(),
|
"worker-7".to_string(),
|
||||||
"2026-08-11T20:00:00Z".to_string(),
|
|
||||||
"retire completed Worker".to_string(),
|
"retire completed Worker".to_string(),
|
||||||
)]
|
)]
|
||||||
);
|
);
|
||||||
@@ -1194,15 +1180,18 @@ mod tests {
|
|||||||
let schema = serde_json::to_value(schemars::schema_for!(WorkerRemoveInput))
|
let schema = serde_json::to_value(schemars::schema_for!(WorkerRemoveInput))
|
||||||
.unwrap()
|
.unwrap()
|
||||||
.to_string();
|
.to_string();
|
||||||
for field in [
|
for field in ["runtime_id", "worker_id", "reason"] {
|
||||||
"runtime_id",
|
|
||||||
"worker_id",
|
|
||||||
"expected_worker_revision",
|
|
||||||
"reason",
|
|
||||||
] {
|
|
||||||
assert!(schema.contains(field));
|
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}");
|
assert!(!schema.contains(forbidden), "schema leaked {forbidden}");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1223,7 +1212,6 @@ mod tests {
|
|||||||
"runtime_id": "runtime-1",
|
"runtime_id": "runtime-1",
|
||||||
"worker_id": "worker-7",
|
"worker_id": "worker-7",
|
||||||
},
|
},
|
||||||
"expected_worker_revision": "revision-1",
|
|
||||||
"reason": reason,
|
"reason": reason,
|
||||||
})
|
})
|
||||||
.to_string(),
|
.to_string(),
|
||||||
|
|||||||
@@ -272,7 +272,6 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync {
|
|||||||
&self,
|
&self,
|
||||||
_target_runtime_id: &str,
|
_target_runtime_id: &str,
|
||||||
_target_worker_id: &str,
|
_target_worker_id: &str,
|
||||||
_expected_worker_revision: &str,
|
|
||||||
_reason: &str,
|
_reason: &str,
|
||||||
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
) -> Result<WorkspaceResponse, WorkspaceClientError> {
|
||||||
Err(WorkspaceClientError::Unavailable(
|
Err(WorkspaceClientError::Unavailable(
|
||||||
|
|||||||
@@ -59,7 +59,6 @@ pub struct WorkerRetentionPolicyUpdate {
|
|||||||
pub struct WorkerRemovalPlanRequest {
|
pub struct WorkerRemovalPlanRequest {
|
||||||
pub workspace_id: String,
|
pub workspace_id: String,
|
||||||
pub worker: RuntimeWorkerRef,
|
pub worker: RuntimeWorkerRef,
|
||||||
pub expected_worker_revision: String,
|
|
||||||
pub reason: String,
|
pub reason: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -154,8 +153,6 @@ pub enum WorkerRetentionError {
|
|||||||
WorkerNotFound,
|
WorkerNotFound,
|
||||||
#[error("Worker belongs to a different Workspace")]
|
#[error("Worker belongs to a different Workspace")]
|
||||||
CrossWorkspace,
|
CrossWorkspace,
|
||||||
#[error("Worker revision changed: expected {expected}, current {actual}")]
|
|
||||||
WorkerRevisionConflict { expected: String, actual: String },
|
|
||||||
#[error("Worker removal is blocked: {0:?}")]
|
#[error("Worker removal is blocked: {0:?}")]
|
||||||
Blocked(Vec<WorkerRemovalBlocker>),
|
Blocked(Vec<WorkerRemovalBlocker>),
|
||||||
#[error("Worker removal plan {plan_id} is stale: {reason}")]
|
#[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()}));
|
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();
|
let mut blockers=Vec::new();
|
||||||
if worker.retention_state=="pinned" { blockers.push(WorkerRemovalBlocker::Hold); }
|
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()? {
|
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});
|
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 plan_id=stable("wrp",&fp); let operation_id=stable("wro",&fp);
|
||||||
let archive_id=(policy.session_disposition==SessionDisposition::Archive).then(||stable("wra",&fp));
|
let archive_id=(policy.session_disposition==SessionDisposition::Archive).then(||stable("wra",&fp));
|
||||||
let state=if blockers.is_empty(){WorkerRemovalPlanState::Planned}else{WorkerRemovalPlanState::Blocked};
|
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()))?;
|
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)));}
|
if plan.input_fingerprint!=fp{return Err(StoreError::InvalidInput(format!("fingerprint:{}",plan.operation_id)));}
|
||||||
tx.commit()?; Ok(plan)
|
tx.commit()?; Ok(plan)
|
||||||
@@ -422,27 +418,22 @@ impl SqliteWorkspaceStore {
|
|||||||
&self,
|
&self,
|
||||||
workspace_id: &str,
|
workspace_id: &str,
|
||||||
worker: &RuntimeWorkerRef,
|
worker: &RuntimeWorkerRef,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
|
||||||
) -> Result<Option<PreparedWorkerRemoval>, WorkerRetentionError> {
|
) -> Result<Option<PreparedWorkerRemoval>, WorkerRetentionError> {
|
||||||
bounded("workspace", workspace_id, 160)?;
|
bounded("workspace", workspace_id, 160)?;
|
||||||
bounded("revision", expected_worker_revision, 256)?;
|
|
||||||
bounded("reason", reason, 512)?;
|
|
||||||
let plan = self.with_conn(|conn| {
|
let plan = self.with_conn(|conn| {
|
||||||
conn.query_row(
|
conn.query_row(
|
||||||
"SELECT plan_id FROM worker_removal_operations
|
"SELECT plan_id FROM worker_removal_operations
|
||||||
WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3
|
WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3
|
||||||
AND worker_revision=?4 AND reason=?5
|
AND state IN ('planned','executing','failed','succeeded')
|
||||||
AND state IN ('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,
|
ORDER BY CASE state WHEN 'succeeded' THEN 0 ELSE 1 END,
|
||||||
created_at DESC LIMIT 1",
|
created_at DESC LIMIT 1",
|
||||||
params![
|
params![workspace_id, worker.runtime_id, worker.worker_id],
|
||||||
workspace_id,
|
|
||||||
worker.runtime_id,
|
|
||||||
worker.worker_id,
|
|
||||||
expected_worker_revision,
|
|
||||||
reason,
|
|
||||||
],
|
|
||||||
|row| row.get::<_, String>(0),
|
|row| row.get::<_, String>(0),
|
||||||
)
|
)
|
||||||
.optional()
|
.optional()
|
||||||
@@ -848,6 +839,7 @@ fn stale_error(plan: &WorkerRemovalPlan, reason: &str) -> StoreError {
|
|||||||
}
|
}
|
||||||
fn fingerprint(
|
fn fingerprint(
|
||||||
r: &WorkerRemovalPlanRequest,
|
r: &WorkerRemovalPlanRequest,
|
||||||
|
worker_revision: &str,
|
||||||
i: &WorkerRetentionInventory,
|
i: &WorkerRetentionInventory,
|
||||||
p: &WorkerRetentionPolicy,
|
p: &WorkerRetentionPolicy,
|
||||||
b: &[WorkerRemovalBlocker],
|
b: &[WorkerRemovalBlocker],
|
||||||
@@ -856,7 +848,7 @@ fn fingerprint(
|
|||||||
r.workspace_id,
|
r.workspace_id,
|
||||||
r.worker.runtime_id,
|
r.worker.runtime_id,
|
||||||
r.worker.worker_id,
|
r.worker.worker_id,
|
||||||
r.expected_worker_revision,
|
worker_revision,
|
||||||
i.run_generation,
|
i.run_generation,
|
||||||
i.session_id,
|
i.session_id,
|
||||||
i.segment_ids,
|
i.segment_ids,
|
||||||
@@ -887,7 +879,6 @@ fn validate_plan(
|
|||||||
i: &WorkerRetentionInventory,
|
i: &WorkerRetentionInventory,
|
||||||
) -> Result<(), WorkerRetentionError> {
|
) -> Result<(), WorkerRetentionError> {
|
||||||
bounded("workspace", &r.workspace_id, 160)?;
|
bounded("workspace", &r.workspace_id, 160)?;
|
||||||
bounded("revision", &r.expected_worker_revision, 256)?;
|
|
||||||
bounded("reason", &r.reason, 2000)?;
|
bounded("reason", &r.reason, 2000)?;
|
||||||
if i.workspace_id != r.workspace_id
|
if i.workspace_id != r.workspace_id
|
||||||
|| i.runtime_id != r.worker.runtime_id
|
|| i.runtime_id != r.worker.runtime_id
|
||||||
@@ -947,13 +938,6 @@ fn map_error(e: StoreError) -> WorkerRetentionError {
|
|||||||
if m == "worker-missing" {
|
if m == "worker-missing" {
|
||||||
return WorkerRetentionError::WorkerNotFound;
|
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:") {
|
if let Some(x) = m.strip_prefix("fingerprint:") {
|
||||||
return WorkerRetentionError::OperationFingerprintConflict {
|
return WorkerRetentionError::OperationFingerprintConflict {
|
||||||
operation_id: x.into(),
|
operation_id: x.into(),
|
||||||
@@ -1106,7 +1090,6 @@ mod tests {
|
|||||||
runtime_id: "r".into(),
|
runtime_id: "r".into(),
|
||||||
worker_id: worker_id().to_string(),
|
worker_id: worker_id().to_string(),
|
||||||
},
|
},
|
||||||
expected_worker_revision: "rev1".into(),
|
|
||||||
reason: "cleanup".into(),
|
reason: "cleanup".into(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1141,6 +1124,7 @@ mod tests {
|
|||||||
let a = s.plan_worker_removal(&req(), &inv()).unwrap();
|
let a = s.plan_worker_removal(&req(), &inv()).unwrap();
|
||||||
let b = 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.plan_id, b.plan_id);
|
||||||
|
assert_eq!(a.worker_revision, "rev1");
|
||||||
s.with_conn(|c| {
|
s.with_conn(|c| {
|
||||||
c.execute(
|
c.execute(
|
||||||
"UPDATE worker_registry SET retention_state='pinned' WHERE workspace_id='w'",
|
"UPDATE worker_registry SET retention_state='pinned' WHERE workspace_id='w'",
|
||||||
@@ -1149,9 +1133,7 @@ mod tests {
|
|||||||
Ok(())
|
Ok(())
|
||||||
})
|
})
|
||||||
.unwrap();
|
.unwrap();
|
||||||
let mut q = req();
|
let p = s.plan_worker_removal(&req(), &inv()).unwrap();
|
||||||
q.expected_worker_revision = "rev1".into();
|
|
||||||
let p = s.plan_worker_removal(&q, &inv()).unwrap();
|
|
||||||
assert_eq!(p.blockers, vec![WorkerRemovalBlocker::Hold]);
|
assert_eq!(p.blockers, vec![WorkerRemovalBlocker::Hold]);
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
s.begin_worker_removal("w", &p.plan_id, &p.input_fingerprint),
|
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]
|
#[test]
|
||||||
fn succeeded_worker_removal_recovers_after_registry_purge() {
|
fn succeeded_worker_removal_recovers_after_registry_purge() {
|
||||||
let s = setup();
|
let s = setup();
|
||||||
@@ -1623,18 +1676,13 @@ mod tests {
|
|||||||
.is_none()
|
.is_none()
|
||||||
);
|
);
|
||||||
let recovered = s
|
let recovered = s
|
||||||
.recover_worker_removal_execution(
|
.recover_worker_removal_execution("w", &request.worker)
|
||||||
"w",
|
|
||||||
&request.worker,
|
|
||||||
&request.expected_worker_revision,
|
|
||||||
&request.reason,
|
|
||||||
)
|
|
||||||
.unwrap()
|
.unwrap()
|
||||||
.unwrap();
|
.unwrap();
|
||||||
assert_eq!(recovered.plan.state, WorkerRemovalPlanState::Succeeded);
|
assert_eq!(recovered.plan.state, WorkerRemovalPlanState::Succeeded);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
recovered.runtime_request.expected_worker_revision,
|
recovered.runtime_request.expected_worker_revision,
|
||||||
request.expected_worker_revision
|
plan.worker_revision
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1653,12 +1701,7 @@ mod tests {
|
|||||||
)
|
)
|
||||||
.unwrap();
|
.unwrap();
|
||||||
let recovered = s
|
let recovered = s
|
||||||
.recover_worker_removal_execution(
|
.recover_worker_removal_execution("w", &request.worker)
|
||||||
"w",
|
|
||||||
&request.worker,
|
|
||||||
&request.expected_worker_revision,
|
|
||||||
&request.reason,
|
|
||||||
)
|
|
||||||
.unwrap()
|
.unwrap()
|
||||||
.unwrap();
|
.unwrap();
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
@@ -330,7 +330,6 @@ impl WorkspaceWorkerRemoveExecutor {
|
|||||||
source: crate::worker_source::VerifiedWorkerMutationSource,
|
source: crate::worker_source::VerifiedWorkerMutationSource,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> std::result::Result<worker::WorkspaceResponse, String> {
|
) -> std::result::Result<worker::WorkspaceResponse, String> {
|
||||||
let reason = reason.trim();
|
let reason = reason.trim();
|
||||||
@@ -428,73 +427,63 @@ impl WorkspaceWorkerRemoveExecutor {
|
|||||||
|
|
||||||
let prepared = self
|
let prepared = self
|
||||||
.store
|
.store
|
||||||
.recover_worker_removal_execution(
|
.recover_worker_removal_execution(&self.workspace_id, &target)
|
||||||
&self.workspace_id,
|
|
||||||
&target,
|
|
||||||
expected_worker_revision,
|
|
||||||
reason,
|
|
||||||
)
|
|
||||||
.map_err(|_| "Worker removal recovery authority is unavailable".to_string())?;
|
.map_err(|_| "Worker removal recovery authority is unavailable".to_string())?;
|
||||||
if let Some(prepared) = prepared {
|
if let Some(prepared) = prepared {
|
||||||
if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Succeeded {
|
if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Succeeded {
|
||||||
return Ok(worker_remove_success_response(&target));
|
return Ok(worker_remove_success_response(&target));
|
||||||
}
|
}
|
||||||
let must_close_session =
|
let prepared = if matches!(
|
||||||
prepared.prior_failure_category.as_deref() == Some("workdir_session_close_failed");
|
prepared.plan.state,
|
||||||
let must_release_attachment = must_close_session
|
crate::retention::WorkerRemovalPlanState::Planned
|
||||||
|| prepared.prior_failure_category.as_deref()
|
| crate::retention::WorkerRemovalPlanState::Failed
|
||||||
== Some("workdir_attachment_release_failed");
|
) {
|
||||||
let prepared =
|
match self.store.prepare_worker_removal_execution(
|
||||||
if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Failed {
|
&self.workspace_id,
|
||||||
match self.store.prepare_worker_removal_execution(
|
&prepared.plan.plan_id,
|
||||||
&self.workspace_id,
|
&prepared.plan.input_fingerprint,
|
||||||
&prepared.plan.plan_id,
|
) {
|
||||||
&prepared.plan.input_fingerprint,
|
Ok(prepared) => prepared,
|
||||||
) {
|
Err(error) => return Ok(worker_retention_error_response(error)),
|
||||||
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);
|
|
||||||
}
|
}
|
||||||
}
|
} else {
|
||||||
if must_release_attachment
|
prepared
|
||||||
&& self
|
};
|
||||||
.store
|
let session = {
|
||||||
.detach_worker_workdir(
|
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,
|
&self.workspace_id,
|
||||||
&target,
|
&prepared.plan.operation_id,
|
||||||
None,
|
&prepared.plan.input_fingerprint,
|
||||||
&Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true),
|
"workdir_session_close_failed",
|
||||||
)
|
);
|
||||||
.is_err()
|
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(
|
let _ = self.store.fail_worker_removal(
|
||||||
&self.workspace_id,
|
&self.workspace_id,
|
||||||
@@ -551,7 +540,6 @@ impl WorkspaceWorkerRemoveExecutor {
|
|||||||
let request = crate::retention::WorkerRemovalPlanRequest {
|
let request = crate::retention::WorkerRemovalPlanRequest {
|
||||||
workspace_id: self.workspace_id.clone(),
|
workspace_id: self.workspace_id.clone(),
|
||||||
worker: target.clone(),
|
worker: target.clone(),
|
||||||
expected_worker_revision: expected_worker_revision.to_string(),
|
|
||||||
reason: reason.to_string(),
|
reason: reason.to_string(),
|
||||||
};
|
};
|
||||||
let plan = match self.store.plan_worker_removal(&request, &inventory) {
|
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,
|
source: crate::worker_source::VerifiedWorkerMutationSource,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> std::result::Result<worker::WorkspaceResponse, String> {
|
) -> std::result::Result<worker::WorkspaceResponse, String> {
|
||||||
let executor = self.clone();
|
let executor = self.clone();
|
||||||
let target_runtime_id = target_runtime_id.to_string();
|
let target_runtime_id = target_runtime_id.to_string();
|
||||||
let target_worker_id = target_worker_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();
|
let reason = reason.to_string();
|
||||||
std::thread::spawn(move || {
|
std::thread::spawn(move || {
|
||||||
tokio::runtime::Builder::new_current_thread()
|
tokio::runtime::Builder::new_current_thread()
|
||||||
@@ -673,7 +659,6 @@ impl crate::worker_source::VerifiedWorkerRemoveExecutor for WorkspaceWorkerRemov
|
|||||||
source,
|
source,
|
||||||
&target_runtime_id,
|
&target_runtime_id,
|
||||||
&target_worker_id,
|
&target_worker_id,
|
||||||
&expected_worker_revision,
|
|
||||||
&reason,
|
&reason,
|
||||||
))
|
))
|
||||||
})
|
})
|
||||||
@@ -6145,14 +6130,13 @@ fn worker_retention_error_response(
|
|||||||
"worker_not_found",
|
"worker_not_found",
|
||||||
"Worker was not found in this Workspace",
|
"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::StalePlan { .. }
|
||||||
| crate::retention::WorkerRetentionError::OperationFingerprintConflict { .. } => {
|
| crate::retention::WorkerRetentionError::OperationFingerprintConflict { .. } => {
|
||||||
worker_remove_error_response(
|
worker_remove_error_response(
|
||||||
StatusCode::CONFLICT,
|
StatusCode::CONFLICT,
|
||||||
"worker_revision_conflict",
|
"worker_removal_conflict",
|
||||||
"Worker removal state changed; reread the Worker and retry",
|
"Worker removal state changed; retry the operation",
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
crate::retention::WorkerRetentionError::Blocked(_) => worker_remove_error_response(
|
crate::retention::WorkerRetentionError::Blocked(_) => worker_remove_error_response(
|
||||||
@@ -6179,7 +6163,6 @@ fn worker_retention_error_response(
|
|||||||
struct WorkerRemoveBoundaryRequest {
|
struct WorkerRemoveBoundaryRequest {
|
||||||
target_runtime_id: String,
|
target_runtime_id: String,
|
||||||
target_worker_id: String,
|
target_worker_id: String,
|
||||||
expected_worker_revision: String,
|
|
||||||
reason: String,
|
reason: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -6217,7 +6200,6 @@ async fn scoped_worker_remove_source_boundary(
|
|||||||
source,
|
source,
|
||||||
&request.target_runtime_id,
|
&request.target_runtime_id,
|
||||||
&request.target_worker_id,
|
&request.target_worker_id,
|
||||||
&request.expected_worker_revision,
|
|
||||||
&request.reason,
|
&request.reason,
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
@@ -15674,7 +15656,7 @@ mod tests {
|
|||||||
|
|
||||||
let temp = tempfile::tempdir().unwrap();
|
let temp = tempfile::tempdir().unwrap();
|
||||||
let app = build_router(test_api(temp.path()).await);
|
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
|
let browser = app
|
||||||
.clone()
|
.clone()
|
||||||
.oneshot(
|
.oneshot(
|
||||||
@@ -15791,7 +15773,6 @@ mod tests {
|
|||||||
fresh_proof,
|
fresh_proof,
|
||||||
"runtime-target",
|
"runtime-target",
|
||||||
"target-worker",
|
"target-worker",
|
||||||
"revision-1",
|
|
||||||
"retire target Worker",
|
"retire target Worker",
|
||||||
)
|
)
|
||||||
.unwrap_err();
|
.unwrap_err();
|
||||||
@@ -15799,7 +15780,7 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[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 temp = tempfile::tempdir().unwrap();
|
||||||
let api = test_api(temp.path()).await;
|
let api = test_api(temp.path()).await;
|
||||||
let Json(orchestrator) = scoped_start_workspace_orchestrator(
|
let Json(orchestrator) = scoped_start_workspace_orchestrator(
|
||||||
@@ -15824,7 +15805,6 @@ mod tests {
|
|||||||
verified_source(),
|
verified_source(),
|
||||||
&source.runtime_id,
|
&source.runtime_id,
|
||||||
&source.worker_id,
|
&source.worker_id,
|
||||||
"irrelevant",
|
|
||||||
"must reject self",
|
"must reject self",
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
@@ -15866,37 +15846,12 @@ mod tests {
|
|||||||
verified_source(),
|
verified_source(),
|
||||||
&target.runtime_id,
|
&target.runtime_id,
|
||||||
&target.worker_id,
|
&target.worker_id,
|
||||||
"irrelevant",
|
|
||||||
"must reject a live Worker",
|
"must reject a live Worker",
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
.unwrap();
|
.unwrap();
|
||||||
assert_eq!(running_response.status, StatusCode::CONFLICT.as_u16());
|
assert_eq!(running_response.status, StatusCode::CONFLICT.as_u16());
|
||||||
assert!(running_response.body.contains("worker_not_stopped"));
|
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]
|
#[tokio::test]
|
||||||
@@ -15970,7 +15925,7 @@ mod tests {
|
|||||||
)
|
)
|
||||||
.unwrap();
|
.unwrap();
|
||||||
let summary = api.runtime.worker(&target).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");
|
seed_worker_control_grant(&api, &source, &target, "embedded-valid-proof");
|
||||||
|
|
||||||
let response = WorkspaceWorkerRemoveExecutor::new(&api)
|
let response = WorkspaceWorkerRemoveExecutor::new(&api)
|
||||||
@@ -15984,7 +15939,6 @@ mod tests {
|
|||||||
},
|
},
|
||||||
&target.runtime_id,
|
&target.runtime_id,
|
||||||
&target.worker_id,
|
&target.worker_id,
|
||||||
&record.updated_at,
|
|
||||||
"retire completed Worker",
|
"retire completed Worker",
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
@@ -16182,7 +16136,7 @@ mod tests {
|
|||||||
route_token,
|
route_token,
|
||||||
)
|
)
|
||||||
.body(Body::from(
|
.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(),
|
.unwrap(),
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -619,13 +619,11 @@ pub trait ControlPlaneStore: Send + Sync {
|
|||||||
&self,
|
&self,
|
||||||
workspace_id: &str,
|
workspace_id: &str,
|
||||||
worker: &RuntimeWorkerRef,
|
worker: &RuntimeWorkerRef,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
|
||||||
) -> std::result::Result<
|
) -> std::result::Result<
|
||||||
Option<crate::retention::PreparedWorkerRemoval>,
|
Option<crate::retention::PreparedWorkerRemoval>,
|
||||||
crate::retention::WorkerRetentionError,
|
crate::retention::WorkerRetentionError,
|
||||||
> {
|
> {
|
||||||
let _ = (workspace_id, worker, expected_worker_revision, reason);
|
let _ = (workspace_id, worker);
|
||||||
Ok(None)
|
Ok(None)
|
||||||
}
|
}
|
||||||
fn fail_worker_removal(
|
fn fail_worker_removal(
|
||||||
@@ -1431,19 +1429,11 @@ impl ControlPlaneStore for SqliteWorkspaceStore {
|
|||||||
&self,
|
&self,
|
||||||
workspace_id: &str,
|
workspace_id: &str,
|
||||||
worker: &RuntimeWorkerRef,
|
worker: &RuntimeWorkerRef,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
|
||||||
) -> std::result::Result<
|
) -> std::result::Result<
|
||||||
Option<crate::retention::PreparedWorkerRemoval>,
|
Option<crate::retention::PreparedWorkerRemoval>,
|
||||||
crate::retention::WorkerRetentionError,
|
crate::retention::WorkerRetentionError,
|
||||||
> {
|
> {
|
||||||
SqliteWorkspaceStore::recover_worker_removal_execution(
|
SqliteWorkspaceStore::recover_worker_removal_execution(self, workspace_id, worker)
|
||||||
self,
|
|
||||||
workspace_id,
|
|
||||||
worker,
|
|
||||||
expected_worker_revision,
|
|
||||||
reason,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn fail_worker_removal(
|
fn fail_worker_removal(
|
||||||
|
|||||||
@@ -175,7 +175,6 @@ pub(crate) trait VerifiedWorkerRemoveExecutor: Send + Sync {
|
|||||||
source: VerifiedWorkerMutationSource,
|
source: VerifiedWorkerMutationSource,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<worker::WorkspaceResponse, String>;
|
) -> Result<worker::WorkspaceResponse, String>;
|
||||||
}
|
}
|
||||||
@@ -217,7 +216,6 @@ impl worker_runtime::worker_source::EmbeddedWorkerMutationDispatcher
|
|||||||
proof: InProcessWorkerMutationProof,
|
proof: InProcessWorkerMutationProof,
|
||||||
target_runtime_id: &str,
|
target_runtime_id: &str,
|
||||||
target_worker_id: &str,
|
target_worker_id: &str,
|
||||||
expected_worker_revision: &str,
|
|
||||||
reason: &str,
|
reason: &str,
|
||||||
) -> Result<
|
) -> Result<
|
||||||
worker::WorkspaceResponse,
|
worker::WorkspaceResponse,
|
||||||
@@ -241,13 +239,7 @@ impl worker_runtime::worker_source::EmbeddedWorkerMutationDispatcher
|
|||||||
)
|
)
|
||||||
})?;
|
})?;
|
||||||
executor
|
executor
|
||||||
.execute(
|
.execute(source, target_runtime_id, target_worker_id, reason)
|
||||||
source,
|
|
||||||
target_runtime_id,
|
|
||||||
target_worker_id,
|
|
||||||
expected_worker_revision,
|
|
||||||
reason,
|
|
||||||
)
|
|
||||||
.map_err(worker_runtime::worker_source::RuntimeWorkerMutationForwardError::Embedded)
|
.map_err(worker_runtime::worker_source::RuntimeWorkerMutationForwardError::Embedded)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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.
|
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.
|
||||||
|
|||||||
Reference in New Issue
Block a user