fix(ecs): correct UpdateService/CreateService validation and ListTasks filtering (bug-hunt) (#2310)

vieiralucas · web-flow · commit 8d505bbea213 · 2026-07-16T22:18:33.000-03:00
diff --git a/crates/fakecloud-ecs/src/helpers.rs b/crates/fakecloud-ecs/src/helpers.rs
@@ -764,6 +764,13 @@ pub(crate) fn service_name_from_ref(input: &str) -> String {
     input.to_string()
 }
 
+/// Extract the trailing ID segment from an ARN or path-style ref, e.g. a
+/// container-instance ARN `arn:aws:ecs:...:container-instance/<cluster>/<id>`
+/// yields `<id>`. A bare ID is returned unchanged.
+pub(crate) fn id_from_ref(input: &str) -> String {
+    input.rsplit('/').next().unwrap_or(input).to_string()
+}
+
 pub(crate) fn service_not_found(name: &str) -> AwsServiceError {
     AwsServiceError::aws_error(
         StatusCode::BAD_REQUEST,
@@ -772,11 +779,21 @@ pub(crate) fn service_not_found(name: &str) -> AwsServiceError {
     )
 }
 
-pub(crate) fn service_already_exists(name: &str) -> AwsServiceError {
+pub(crate) fn service_already_exists() -> AwsServiceError {
+    // AWS returns InvalidParameterException (not ServiceNotActiveException)
+    // when a CreateService call collides with an existing active service.
+    AwsServiceError::aws_error(
+        StatusCode::BAD_REQUEST,
+        "InvalidParameterException",
+        "Creation of service was not idempotent.",
+    )
+}
+
+pub(crate) fn service_not_active(name: &str) -> AwsServiceError {
     AwsServiceError::aws_error(
         StatusCode::BAD_REQUEST,
         "ServiceNotActiveException",
-        format!("The service {name} already exists"),
+        format!("Service {name} is not ACTIVE."),
     )
 }
 
diff --git a/crates/fakecloud-ecs/src/service_services_resource.rs b/crates/fakecloud-ecs/src/service_services_resource.rs
@@ -150,7 +150,7 @@ impl EcsService {
             let key = EcsState::service_key(&cluster_name, &service_name);
             if let Some(existing) = state.services.get(&key) {
                 if existing.status != "INACTIVE" {
-                    return Err(service_already_exists(&service_name));
+                    return Err(service_already_exists());
                 }
             }
             let is_code_deploy = deployment_controller == "CODE_DEPLOY";
@@ -350,6 +350,16 @@ impl EcsService {
         let cluster_ref = opt_str(&body, "cluster");
         let cluster_name = EcsState::resolve_cluster_name(cluster_ref);
         let new_desired = body.get("desiredCount").and_then(|v| v.as_i64());
+        // AWS rejects a negative desiredCount with InvalidParameterException
+        // rather than silently clamping it to 0 (which would scale the service
+        // to zero and cause a silent outage). Matches CreateService's message.
+        if let Some(n) = new_desired {
+            if n < 0 {
+                return Err(invalid_parameter(format!(
+                    "desiredCount cannot be negative. desiredCount={n}"
+                )));
+            }
+        }
         let new_td_ref = opt_str(&body, "taskDefinition");
         let update_lifecycle_hooks: Vec<Value> = body
             .get("deploymentConfiguration")
@@ -371,8 +381,15 @@ impl EcsService {
                 .get_mut(&account)
                 .ok_or_else(|| service_not_found(&service_name))?;
             let key = EcsState::service_key(&cluster_name, &service_name);
-            if !state.services.contains_key(&key) {
-                return Err(service_not_found(&service_name));
+            // A deleted service transitions to DRAINING then INACTIVE but stays
+            // describable. UpdateService must not respawn one: AWS returns
+            // ServiceNotActiveException for any non-ACTIVE service.
+            match state.services.get(&key) {
+                None => return Err(service_not_found(&service_name)),
+                Some(svc) if svc.status != "ACTIVE" => {
+                    return Err(service_not_active(&service_name));
+                }
+                Some(_) => {}
             }
 
             // Resolve new task definition (may stay on current one).
@@ -486,7 +503,8 @@ impl EcsService {
                 }
 
                 if let Some(n) = new_desired {
-                    let n = n.max(0) as i32;
+                    // Negatives are already rejected above; n is >= 0 here.
+                    let n = n as i32;
                     svc.desired_count = n;
                     if svc.deployment_controller != "CODE_DEPLOY" {
                         if let Some(d) = svc.deployments.iter_mut().find(|d| d.status == "PRIMARY")
@@ -1073,12 +1091,14 @@ impl EcsService {
         let cluster_name = EcsState::resolve_cluster_name(cluster_ref);
         let launch_type = opt_str(&body, "launchType");
         let scheduling = opt_str(&body, "schedulingStrategy");
+        // The ListServices model defaults maxResults to 10 (unlike sibling ECS
+        // list ops which default to 100).
         let max_results = body
             .get("maxResults")
             .and_then(|v| v.as_i64())
             .filter(|n| (1..=100).contains(n))
             .map(|n| n as usize)
-            .unwrap_or(100);
+            .unwrap_or(10);
         let next_token = opt_str(&body, "nextToken").unwrap_or("");
 
         let account = request.account_id.clone();
diff --git a/crates/fakecloud-ecs/src/service_tasks.rs b/crates/fakecloud-ecs/src/service_tasks.rs
@@ -546,6 +546,13 @@ impl EcsService {
         let family = opt_str(&body, "family");
         let status_filter = opt_str(&body, "desiredStatus").or(Some("RUNNING"));
         let started_by = opt_str(&body, "startedBy");
+        // A service's tasks carry group `service:<name>`. The serviceName
+        // filter may arrive as a bare name or a full service ARN.
+        let service_group =
+            opt_str(&body, "serviceName").map(|s| format!("service:{}", service_name_from_ref(s)));
+        // containerInstance may be a full ARN or a bare instance ID; compare on
+        // the trailing ID segment so both forms match.
+        let container_instance = opt_str(&body, "containerInstance").map(id_from_ref);
         let max_results = body
             .get("maxResults")
             .and_then(|v| v.as_i64())
@@ -564,6 +571,20 @@ impl EcsService {
                 .filter(|t| family.is_none_or(|f| t.family == f))
                 .filter(|t| status_filter.is_none_or(|s| t.desired_status == s))
                 .filter(|t| started_by.is_none_or(|s| t.started_by.as_deref() == Some(s)))
+                .filter(|t| {
+                    service_group
+                        .as_deref()
+                        .is_none_or(|g| t.group.as_deref() == Some(g))
+                })
+                .filter(|t| {
+                    container_instance.as_deref().is_none_or(|ci| {
+                        t.container_instance_arn
+                            .as_deref()
+                            .map(id_from_ref)
+                            .as_deref()
+                            == Some(ci)
+                    })
+                })
                 .map(|t| t.task_arn.clone())
                 .collect(),
             None => Vec::new(),
@@ -591,7 +612,7 @@ mod multi_container_tests {
     use std::collections::HashMap;
     use std::sync::Arc;
 
-    fn fresh_service() -> EcsService {
+    pub(super) fn fresh_service() -> EcsService {
         let accounts: MultiAccountState<EcsState> =
             MultiAccountState::new("000000000000", "us-east-1", "http://localhost:4566");
         let state = Arc::new(RwLock::new(accounts));
@@ -606,7 +627,7 @@ mod multi_container_tests {
         svc
     }
 
-    fn make_request(action: &str, body: Value) -> AwsRequest {
+    pub(super) fn make_request(action: &str, body: Value) -> AwsRequest {
         let body_bytes = Bytes::from(serde_json::to_vec(&body).unwrap());
         AwsRequest {
             service: "ecs".into(),
@@ -687,7 +708,7 @@ mod multi_container_tests {
         );
     }
 
-    fn insert_task(svc: &EcsService, id: &str, cluster: &str) {
+    pub(super) fn insert_task(svc: &EcsService, id: &str, cluster: &str) {
         let state = svc.state.clone();
         let mut accounts = state.write();
         let acct = accounts.get_or_create("000000000000");
@@ -1294,3 +1315,136 @@ mod port_mapping_tests {
         assert_eq!(nb, &serde_json::Value::Array(bindings));
     }
 }
+
+#[cfg(test)]
+mod service_validation_tests {
+    use super::multi_container_tests::*;
+    use super::*;
+    use serde_json::{json, Value};
+
+    /// Register a task def + create an ACTIVE service named `web` with
+    /// desiredCount 0, so the update/create validation tests have a target.
+    fn service_ready(svc: &EcsService) {
+        svc.register_task_definition(&make_request(
+            "RegisterTaskDefinition",
+            json!({"family": "web", "containerDefinitions": [{"name": "app", "image": "alpine"}]}),
+        ))
+        .unwrap();
+        svc.create_service(&make_request(
+            "CreateService",
+            json!({"serviceName": "web", "taskDefinition": "web", "desiredCount": 0}),
+        ))
+        .unwrap();
+    }
+
+    #[test]
+    fn update_service_negative_desired_count_is_invalid_parameter() {
+        let svc = fresh_service();
+        service_ready(&svc);
+        // A negative desiredCount previously clamped to 0 and silently scaled
+        // the service down. AWS rejects it instead.
+        let err = match svc.update_service(&make_request(
+            "UpdateService",
+            json!({"service": "web", "desiredCount": -1}),
+        )) {
+            Ok(_) => panic!("expected InvalidParameterException for negative desiredCount"),
+            Err(e) => e,
+        };
+        assert_eq!(err.code(), "InvalidParameterException");
+    }
+
+    #[test]
+    fn update_service_on_inactive_service_is_service_not_active() {
+        let svc = fresh_service();
+        service_ready(&svc);
+        // Simulate a deleted service left at INACTIVE (still describable).
+        {
+            let mut accounts = svc.state.write();
+            let state = accounts.get_mut("000000000000").unwrap();
+            let key = EcsState::service_key("default", "web");
+            state.services.get_mut(&key).unwrap().status = "INACTIVE".into();
+        }
+        let err = match svc.update_service(&make_request(
+            "UpdateService",
+            json!({"service": "web", "desiredCount": 2}),
+        )) {
+            Ok(_) => panic!("expected ServiceNotActiveException for a non-ACTIVE service"),
+            Err(e) => e,
+        };
+        assert_eq!(err.code(), "ServiceNotActiveException");
+    }
+
+    #[test]
+    fn create_service_duplicate_name_is_invalid_parameter_not_idempotent() {
+        let svc = fresh_service();
+        service_ready(&svc);
+        let err = match svc.create_service(&make_request(
+            "CreateService",
+            json!({"serviceName": "web", "taskDefinition": "web", "desiredCount": 0}),
+        )) {
+            Ok(_) => panic!("expected InvalidParameterException for a duplicate service name"),
+            Err(e) => e,
+        };
+        assert_eq!(err.code(), "InvalidParameterException");
+        assert_eq!(err.message(), "Creation of service was not idempotent.");
+    }
+
+    #[test]
+    fn list_tasks_filters_by_service_name_and_container_instance() {
+        let svc = fresh_service();
+        // Two service-owned tasks (group service:web) on distinct container
+        // instances, plus one unrelated task (group service:api).
+        insert_task(&svc, "web1", "default");
+        insert_task(&svc, "web2", "default");
+        insert_task(&svc, "api1", "default");
+        {
+            let mut accounts = svc.state.write();
+            let state = accounts.get_mut("000000000000").unwrap();
+            let ci_a = "arn:aws:ecs:us-east-1:000000000000:container-instance/default/aaaaaaaa"
+                .to_string();
+            let ci_b = "arn:aws:ecs:us-east-1:000000000000:container-instance/default/bbbbbbbb"
+                .to_string();
+            let t = state.tasks.get_mut("web1").unwrap();
+            t.group = Some("service:web".into());
+            t.container_instance_arn = Some(ci_a);
+            let t = state.tasks.get_mut("web2").unwrap();
+            t.group = Some("service:web".into());
+            t.container_instance_arn = Some(ci_b);
+            let t = state.tasks.get_mut("api1").unwrap();
+            t.group = Some("service:api".into());
+        }
+
+        // serviceName filter returns only the two web tasks.
+        let resp = svc
+            .list_tasks(&make_request("ListTasks", json!({"serviceName": "web"})))
+            .unwrap();
+        let body: Value = serde_json::from_slice(resp.body.expect_bytes()).unwrap();
+        let arns = body["taskArns"].as_array().unwrap();
+        assert_eq!(arns.len(), 2);
+        assert!(arns.iter().all(|a| a.as_str().unwrap().contains("/web")));
+
+        // containerInstance filter (bare ID) narrows to the single task on it.
+        let resp = svc
+            .list_tasks(&make_request(
+                "ListTasks",
+                json!({"containerInstance": "aaaaaaaa"}),
+            ))
+            .unwrap();
+        let body: Value = serde_json::from_slice(resp.body.expect_bytes()).unwrap();
+        let arns = body["taskArns"].as_array().unwrap();
+        assert_eq!(arns.len(), 1);
+        assert!(arns[0].as_str().unwrap().ends_with("/web1"));
+
+        // containerInstance filter also accepts a full ARN.
+        let resp = svc
+            .list_tasks(&make_request(
+                "ListTasks",
+                json!({"containerInstance": "arn:aws:ecs:us-east-1:000000000000:container-instance/default/bbbbbbbb"}),
+            ))
+            .unwrap();
+        let body: Value = serde_json::from_slice(resp.body.expect_bytes()).unwrap();
+        let arns = body["taskArns"].as_array().unwrap();
+        assert_eq!(arns.len(), 1);
+        assert!(arns[0].as_str().unwrap().ends_with("/web2"));
+    }
+}
