From 55fa445243cebd0798c1f1736c1dc62713d0f65f Mon Sep 17 00:00:00 2001 From: "Orca (ecs-claude)" Date: Fri, 14 Aug 2026 00:51:40 +0800 Subject: [PATCH] fix(read-model): UNKNOWN health with no defined check is Running, not Unhealthy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every OAB service with no ECS container health check showed all instances red "Unhealthy" (ready 0/desired) even while RUNNING and CONNECTED — because ECS reports healthStatus=UNKNOWN forever when no check is defined, and the driver faulted on any non-HEALTHY status. Distinguish the two meanings of UNKNOWN: - no health check defined -> "no probe", benign -> Running (a leased, running instance is Ok). - a *defined* check reporting UNKNOWN -> lost signal / node lost -> Unhealthy (the fencing the model intended is preserved). - agent-lifecycle: EcsTask gains health_check_defined; UNKNOWN faults only when a check is defined (else Ok, still gated by lease). Two new driver tests. - oabctl: InstanceStatus carries health_check_defined, resolved via DescribeTaskDefinition (cached per task-def ARN; best-effort false on error so a permission gap reads benign, not red). - studio-cp: instance_phase plumbs the flag; regression tests for the exact orca/mira case (RUNNING + UNKNOWN + no check -> Running). Verified: agent-lifecycle tests run green (incl. the two new cases + the node-lost fence). studio-cp/oabctl test execution + clippy left to CI. Co-Authored-By: Claude Opus 4.8 --- crates/agent-lifecycle/src/ecs.rs | 61 ++++++++++++++++++++++++++++++- crates/oabctl/src/status.rs | 41 +++++++++++++++++++++ crates/studio-cp/src/lib.rs | 22 +++++++++++ 3 files changed, 122 insertions(+), 2 deletions(-) diff --git a/crates/agent-lifecycle/src/ecs.rs b/crates/agent-lifecycle/src/ecs.rs index 0f85f3b..853b959 100644 --- a/crates/agent-lifecycle/src/ecs.rs +++ b/crates/agent-lifecycle/src/ecs.rs @@ -33,6 +33,11 @@ pub struct EcsTask { /// ECS `desiredStatus == STOPPED`. pub desired_status_stopped: bool, pub health: EcsHealth, + /// Whether the task definition declares a container health check. When it + /// does not, ECS reports `healthStatus = UNKNOWN` forever — that is "no + /// probe", not a fault — so an UNKNOWN with no check must not read as + /// Unhealthy. A defined check reporting UNKNOWN is a lost signal (fault). + pub health_check_defined: bool, /// CP-issued lease still valid (heartbeat authorized). pub lease_valid: bool, /// CP/director cordon: `false` ⇒ not admitting new work (→ Paused). @@ -57,10 +62,14 @@ impl RuntimeDriver for EcsDriver { // `identity_verified` latches once `lastStatus` has ever reached RUNNING. let identity_verified = verified_before || task.last_status == EcsLastStatus::Running; - // Faulted on an unhealthy check, an unknown/unobservable status - // (node lost), or a lost lease. + // Healthy (with a valid lease) is a clean Ok. UNHEALTHY, a lost lease, + // or a UNKNOWN from a *defined* health check (lost signal / node lost) + // all fault. But when NO health check is defined, ECS reports UNKNOWN + // forever — that is "no probe", not a fault — so a running, leased + // instance stays Ok instead of reading as Unhealthy. let health = match task.health { EcsHealth::Healthy if task.lease_valid => Health::Ok, + EcsHealth::Unknown if task.lease_valid && !task.health_check_defined => Health::Ok, _ => Health::Faulted, }; @@ -84,11 +93,24 @@ mod tests { h: EcsHealth, lease: bool, accepting: bool, + ) -> EcsTask { + // Default: no health check defined (the common case for OAB agents). + task_hc(ls, stopped, h, lease, accepting, false) + } + + fn task_hc( + ls: EcsLastStatus, + stopped: bool, + h: EcsHealth, + lease: bool, + accepting: bool, + health_check_defined: bool, ) -> EcsTask { EcsTask { last_status: ls, desired_status_stopped: stopped, health: h, + health_check_defined, lease_valid: lease, accepting_work: accepting, } @@ -156,6 +178,41 @@ mod tests { assert_eq!(d.classify(), AgentState::Unhealthy); } + #[test] + fn running_unknown_without_health_check_is_running() { + // No health check defined ⇒ ECS reports healthStatus UNKNOWN forever; a + // running, leased instance is Running, NOT Unhealthy (the reported bug). + let d = EcsDriver.project( + &task_hc( + EcsLastStatus::Running, + false, + EcsHealth::Unknown, + true, + true, + false, // no health check defined + ), + true, + ); + assert_eq!(d.classify(), AgentState::Running); + } + + #[test] + fn running_unknown_with_defined_health_check_is_unhealthy() { + // A *defined* check reporting UNKNOWN (lost signal) still fences. + let d = EcsDriver.project( + &task_hc( + EcsLastStatus::Running, + false, + EcsHealth::Unknown, + true, + false, + true, // health check defined, signal unknown + ), + true, + ); + assert_eq!(d.classify(), AgentState::Unhealthy); + } + #[test] fn desired_stopped_is_stopping_while_observable() { let d = EcsDriver.project( diff --git a/crates/oabctl/src/status.rs b/crates/oabctl/src/status.rs index a4cdfa4..9130b6c 100644 --- a/crates/oabctl/src/status.rs +++ b/crates/oabctl/src/status.rs @@ -143,6 +143,11 @@ pub struct InstanceStatus { pub last_status: String, /// ECS container `healthStatus`: HEALTHY/UNHEALTHY/UNKNOWN. pub health_status: String, + /// Whether the task definition declares a container health check. Lets the + /// read model tell "no probe" (UNKNOWN forever, benign) apart from a defined + /// check whose signal is UNKNOWN (lost). Best-effort: `false` if the task + /// definition can't be read. + pub health_check_defined: bool, /// `desiredStatus == STOPPED`. pub desired_stopped: bool, /// ECS `stopCode` when stopped (else `None`). @@ -190,6 +195,10 @@ pub async fn instance_status( } } + // Whether a task definition declares a container health check, cached by ARN + // so a service's tasks (which share a task def) cost one DescribeTaskDefinition. + let mut hc_cache: std::collections::HashMap = std::collections::HashMap::new(); + let mut out = Vec::new(); for chunk in task_arns.chunks(100) { let resp = ecs @@ -200,6 +209,10 @@ pub async fn instance_status( .await .context("failed to describe ECS tasks")?; for task in resp.tasks() { + let health_check_defined = match task.task_definition_arn() { + Some(arn) => task_def_has_health_check(&ecs, arn, &mut hc_cache).await, + None => false, + }; out.push(InstanceStatus { id: task.task_arn().unwrap_or("-").to_string(), last_status: task.last_status().unwrap_or("UNKNOWN").to_string(), @@ -207,6 +220,7 @@ pub async fn instance_status( .health_status() .map(|h| h.as_str().to_string()) .unwrap_or_else(|| "UNKNOWN".to_string()), + health_check_defined, desired_stopped: task.desired_status() == Some("STOPPED"), stop_code: task.stop_code().map(|c| c.as_str().to_string()), }); @@ -215,3 +229,30 @@ pub async fn instance_status( Ok(out) } + +/// Does this task definition declare a container health check? Cached by ARN. +/// Best-effort: on any read error (e.g. missing `ecs:DescribeTaskDefinition`), +/// returns `false` so an UNKNOWN health reads as benign (no probe) rather than +/// faulting the instance on a permission gap. +async fn task_def_has_health_check( + ecs: &aws_sdk_ecs::Client, + arn: &str, + cache: &mut std::collections::HashMap, +) -> bool { + if let Some(v) = cache.get(arn) { + return *v; + } + let defined = match ecs.describe_task_definition().task_definition(arn).send().await { + Ok(resp) => resp + .task_definition() + .map(|td| { + td.container_definitions() + .iter() + .any(|c| c.health_check().is_some()) + }) + .unwrap_or(false), + Err(_) => false, + }; + cache.insert(arn.to_string(), defined); + defined +} diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index 9792269..975d13e 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -61,6 +61,7 @@ pub fn instance_phase(inst: &InstanceStatus, verified_before: bool) -> AgentStat last_status, desired_status_stopped: inst.desired_stopped, health, + health_check_defined: inst.health_check_defined, lease_valid: true, // CP-level, not ECS-observable accepting_work: true, // CP-level admission, not ECS-observable }; @@ -480,6 +481,7 @@ mod tests { id: "arn".into(), last_status: last.into(), health_status: health.into(), + health_check_defined: false, // default: no ECS health check (common for OAB agents) desired_stopped: stopped, stop_code: None, } @@ -509,6 +511,26 @@ mod tests { ); } + #[test] + fn running_unknown_without_health_check_maps_to_running() { + // The reported bug: orca/mira run fine but define no ECS health check, so + // healthStatus is UNKNOWN forever — that must read as Running, not Unhealthy. + assert_eq!( + instance_phase(&inst("RUNNING", "UNKNOWN", false), true), + AgentState::Running + ); + } + + #[test] + fn running_unknown_with_defined_health_check_maps_to_unhealthy() { + // A *defined* check reporting UNKNOWN (lost signal) still fences. + let i = InstanceStatus { + health_check_defined: true, + ..inst("RUNNING", "UNKNOWN", false) + }; + assert_eq!(instance_phase(&i, true), AgentState::Unhealthy); + } + #[test] fn desired_stopped_maps_to_stopping() { assert_eq!(