fix(read-model): UNKNOWN health with no defined check is Running, not Unhealthy

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(

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<String, bool> = std::collections::HashMap::new();
+
     let mut out = Vec::new();
     for chunk in task_arns.chunks(100) {
         let resp = ecs
@@ -200,13 +209,18 @@ 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(),
                 health_status: task
                     .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<String, bool>,
+) -> 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
+}

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!(