From 1b14c0e2c8208d27626d45294a79252bd8075586 Mon Sep 17 00:00:00 2001 From: "Orca (openab agent)" Date: Thu, 13 Aug 2026 23:43:53 +0800 Subject: [PATCH 1/2] fix(studio-cp): resolve short service name to full ECS name before ListTasks `observe_deployment` accepted either the display short name (`orca`) or the full ECS name (`oab-prod-orca`) when matching a service, then passed the caller's string verbatim to `instance_status`. ECS `ListTasks` filters by `service_name`, which only accepts the full name, so a short name 404s as `ServiceNotFoundException`. The roster / `get_agent_states` path collects short names, so it failed for every agent once credentials resolved to the right account (previously masked by an AccessDenied earlier in the chain). - studio-cp: centralise the short->full mapping (`canonical_service_name` + `find_service`) and query tasks by the resolved full name. - oabctl: harden `instance_status` to fail loud when handed a non-`oab-` name instead of surfacing an opaque ECS `ServiceNotFoundException`. - test: `service_selector_resolves_to_full_ecs_name` guards the short-name path. Co-Authored-By: Claude Opus 4.8 --- crates/oabctl/src/status.rs | 12 +++++++ crates/studio-cp/src/lib.rs | 69 ++++++++++++++++++++++++++++++++----- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/crates/oabctl/src/status.rs b/crates/oabctl/src/status.rs index cc62f4b..c58013b 100644 --- a/crates/oabctl/src/status.rs +++ b/crates/oabctl/src/status.rs @@ -151,6 +151,18 @@ pub async fn instance_status( cluster: &str, service: &str, ) -> Result> { + // ECS `ListTasks` filters by the FULL service name (`oab-{ns}-{name}`); a + // display short name (`orca`) silently 404s as `ServiceNotFoundException`. + // Fail loud at the boundary so the mistake is unambiguous rather than an + // opaque AWS error — callers resolve the full name in + // `studio_cp::observe_deployment` before reaching here. + if !service.starts_with("oab-") { + anyhow::bail!( + "instance_status: expected full ECS service name `oab--`, got `{service}` \ + — a short/display name never matches an ECS service_name filter" + ); + } + let ecs = aws_sdk_ecs::Client::new(aws_config); // List task ARNs for the service (paginated). diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index 8d69160..bf8b0f7 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -120,20 +120,45 @@ pub fn build_deployment(svc: &ServiceStatus, instances: &[InstanceStatus]) -> De } } +/// The canonical ECS service name for a Deployment: `oab-{namespace}-{name}`. +/// +/// ECS identity APIs (`ListTasks` / `DescribeServices` / `UpdateService`) key on +/// this full name — never the display short name (`orca`). Resolve to it before +/// any call that filters by `service_name`, or ECS 404s with +/// `ServiceNotFoundException`. +fn canonical_service_name(namespace: &str, name: &str) -> String { + format!("oab-{namespace}-{name}") +} + +/// Find the service a caller's `service` selector refers to, accepting **either** +/// the full ECS name `oab-{ns}-{name}` **or** the display short name `{name}`. +/// Centralising this is what lets [`observe_deployment`] map a short name back to +/// the full ECS name before it reaches a `service_name` filter. +fn find_service<'a>(service: &str, services: &'a [ServiceStatus]) -> Option<&'a ServiceStatus> { + services + .iter() + .find(|s| service == canonical_service_name(&s.namespace, &s.name) || service == s.name) +} + /// Observe one Deployment end-to-end: service-level counters + per-Instance -/// phases. `service` is the ECS service name (`oab-{namespace}-{name}`). +/// phases. `service` may be the full ECS name (`oab-{namespace}-{name}`) **or** +/// the display short name (`{name}`) — both resolve to the same Deployment. pub async fn observe_deployment( aws_config: &aws_config::SdkConfig, cluster: &str, service: &str, ) -> anyhow::Result> { - let svc = oabctl::service_status(aws_config, cluster) - .await? - .into_iter() - .find(|s| service == format!("oab-{}-{}", s.namespace, s.name) || service == s.name); - let Some(svc) = svc else { return Ok(None) }; - let instances = oabctl::instance_status(aws_config, cluster, service).await?; - Ok(Some(build_deployment(&svc, &instances))) + let services = oabctl::service_status(aws_config, cluster).await?; + let Some(svc) = find_service(service, &services) else { + return Ok(None); + }; + // `service` may be the display short name (e.g. `orca`); ECS `ListTasks` + // only accepts the full name, so query by the resolved canonical name rather + // than passing the caller's string straight through — passing a short name + // through here is exactly what surfaced as `ServiceNotFoundException`. + let full = canonical_service_name(&svc.namespace, &svc.name); + let instances = oabctl::instance_status(aws_config, cluster, &full).await?; + Ok(Some(build_deployment(svc, &instances))) } /// Observe recent ECS control-plane **events** for the cluster (optionally one @@ -466,6 +491,34 @@ mod tests { } } + fn svc_named(namespace: &str, name: &str) -> ServiceStatus { + ServiceStatus { + name: name.into(), + namespace: namespace.into(), + ..svc(1, 1) + } + } + + #[test] + fn service_selector_resolves_to_full_ecs_name() { + let services = vec![svc_named("prod", "orca"), svc_named("prod", "mira")]; + + // Regression: a display short name (`orca`) must resolve to the FULL ECS + // service name before it reaches `instance_status`/`ListTasks`, or ECS + // 404s with `ServiceNotFoundException`. Pre-fix, `observe_deployment` + // matched on the short name but then queried tasks with it verbatim. + let s = find_service("orca", &services).expect("short name matches"); + assert_eq!(canonical_service_name(&s.namespace, &s.name), "oab-prod-orca"); + + // The full name resolves to itself. + let s = find_service("oab-prod-mira", &services).expect("full name matches"); + assert_eq!(canonical_service_name(&s.namespace, &s.name), "oab-prod-mira"); + + // An unknown selector is a clean miss — the Deployment then reports + // not-found rather than issuing a doomed ECS query. + assert!(find_service("nope", &services).is_none()); + } + #[test] fn build_deployment_counts_ready_and_phases() { let insts = vec![ From 57c8048b584a88273039a1b8caf9f073c9b7f665 Mon Sep 17 00:00:00 2001 From: "Orca (openab agent)" Date: Thu, 13 Aug 2026 23:49:18 +0800 Subject: [PATCH 2/2] review: carry raw ECS service_name; test the observe_deployment call site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review (Jelly): - #3: add `ServiceStatus::service_name` carrying the raw ECS name verbatim, and query tasks by it instead of `format!`-rebuilding `oab-{ns}-{name}`. The rebuild is wrong for any service that doesn't fit the `oab--` shape (the parser falls back to `namespace = "?"`), so this is a correctness fix, not just cleanup. - #4: the resolver (`resolve_service`) now returns the matched service whose `service_name` is passed to `instance_status` with zero further transformation, so the test asserts the exact value the call site queries with — not just an isolated helper. Co-Authored-By: Claude Opus 4.8 --- crates/oabctl/src/status.rs | 7 ++++ crates/studio-cp/src/lib.rs | 68 +++++++++++++++++++------------------ 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/crates/oabctl/src/status.rs b/crates/oabctl/src/status.rs index c58013b..a4cdfa4 100644 --- a/crates/oabctl/src/status.rs +++ b/crates/oabctl/src/status.rs @@ -16,6 +16,12 @@ pub struct ServiceStatus { /// Agent name (the `{name}` in `oab-{namespace}-{name}`). pub name: String, pub namespace: String, + /// Full ECS service name exactly as ECS returned it (`oab-{namespace}-{name}`). + /// Carried verbatim so downstream ECS calls (`ListTasks`) query by the + /// authoritative name instead of rebuilding it from `namespace`/`name` — + /// rebuilding is wrong for any service that doesn't fit the `oab--` + /// shape (where the parser falls back to `namespace = "?"`). + pub service_name: String, pub cpu: String, pub memory: String, pub capacity: String, @@ -112,6 +118,7 @@ pub async fn service_status( out.push(ServiceStatus { name: agent_name, namespace, + service_name: svc_name.to_string(), cpu, memory, capacity, diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index bf8b0f7..013c462 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -120,24 +120,16 @@ pub fn build_deployment(svc: &ServiceStatus, instances: &[InstanceStatus]) -> De } } -/// The canonical ECS service name for a Deployment: `oab-{namespace}-{name}`. -/// -/// ECS identity APIs (`ListTasks` / `DescribeServices` / `UpdateService`) key on -/// this full name — never the display short name (`orca`). Resolve to it before -/// any call that filters by `service_name`, or ECS 404s with -/// `ServiceNotFoundException`. -fn canonical_service_name(namespace: &str, name: &str) -> String { - format!("oab-{namespace}-{name}") -} - -/// Find the service a caller's `service` selector refers to, accepting **either** -/// the full ECS name `oab-{ns}-{name}` **or** the display short name `{name}`. -/// Centralising this is what lets [`observe_deployment`] map a short name back to -/// the full ECS name before it reaches a `service_name` filter. -fn find_service<'a>(service: &str, services: &'a [ServiceStatus]) -> Option<&'a ServiceStatus> { +/// Resolve a caller's `service` selector to the matched service, accepting +/// **either** the full ECS name (`oab-{ns}-{name}`) **or** the display short +/// name (`{name}`). [`observe_deployment`] passes the resolved service's +/// `service_name` straight to `instance_status` with no further transformation, +/// so a test over this fully covers the "a short selector must query tasks by +/// the full ECS name" guarantee. +fn resolve_service<'a>(service: &str, services: &'a [ServiceStatus]) -> Option<&'a ServiceStatus> { services .iter() - .find(|s| service == canonical_service_name(&s.namespace, &s.name) || service == s.name) + .find(|s| service == s.service_name || service == s.name) } /// Observe one Deployment end-to-end: service-level counters + per-Instance @@ -149,15 +141,15 @@ pub async fn observe_deployment( service: &str, ) -> anyhow::Result> { let services = oabctl::service_status(aws_config, cluster).await?; - let Some(svc) = find_service(service, &services) else { + let Some(svc) = resolve_service(service, &services) else { return Ok(None); }; - // `service` may be the display short name (e.g. `orca`); ECS `ListTasks` - // only accepts the full name, so query by the resolved canonical name rather - // than passing the caller's string straight through — passing a short name - // through here is exactly what surfaced as `ServiceNotFoundException`. - let full = canonical_service_name(&svc.namespace, &svc.name); - let instances = oabctl::instance_status(aws_config, cluster, &full).await?; + // Query tasks by the authoritative ECS service name ECS handed back — never + // the caller's (possibly short) selector, and never a `format!`-rebuilt name + // (wrong for services that don't fit the `oab--` shape). A short + // name reaching `ListTasks` is exactly what surfaced as + // `ServiceNotFoundException`. + let instances = oabctl::instance_status(aws_config, cluster, &svc.service_name).await?; Ok(Some(build_deployment(svc, &instances))) } @@ -482,6 +474,7 @@ mod tests { ServiceStatus { name: "orca".into(), namespace: "prod".into(), + service_name: "oab-prod-orca".into(), cpu: "512".into(), memory: "1024".into(), capacity: "FARGATE".into(), @@ -493,6 +486,7 @@ mod tests { fn svc_named(namespace: &str, name: &str) -> ServiceStatus { ServiceStatus { + service_name: format!("oab-{namespace}-{name}"), name: name.into(), namespace: namespace.into(), ..svc(1, 1) @@ -500,23 +494,31 @@ mod tests { } #[test] - fn service_selector_resolves_to_full_ecs_name() { + fn observe_deployment_resolves_selector_to_full_ecs_service_name() { let services = vec![svc_named("prod", "orca"), svc_named("prod", "mira")]; - // Regression: a display short name (`orca`) must resolve to the FULL ECS - // service name before it reaches `instance_status`/`ListTasks`, or ECS - // 404s with `ServiceNotFoundException`. Pre-fix, `observe_deployment` - // matched on the short name but then queried tasks with it verbatim. - let s = find_service("orca", &services).expect("short name matches"); - assert_eq!(canonical_service_name(&s.namespace, &s.name), "oab-prod-orca"); + // `observe_deployment` passes the resolved service's `service_name` + // verbatim to `instance_status`/`ListTasks`. Regression: a display short + // name (`orca`) must resolve to the FULL ECS name, or ECS 404s with + // `ServiceNotFoundException` (pre-fix it queried with the short name). + assert_eq!( + resolve_service("orca", &services) + .expect("short name matches") + .service_name, + "oab-prod-orca" + ); // The full name resolves to itself. - let s = find_service("oab-prod-mira", &services).expect("full name matches"); - assert_eq!(canonical_service_name(&s.namespace, &s.name), "oab-prod-mira"); + assert_eq!( + resolve_service("oab-prod-mira", &services) + .expect("full name matches") + .service_name, + "oab-prod-mira" + ); // An unknown selector is a clean miss — the Deployment then reports // not-found rather than issuing a doomed ECS query. - assert!(find_service("nope", &services).is_none()); + assert!(resolve_service("nope", &services).is_none()); } #[test]