feat(aws): ECS SDK-compat service (#159) + EC2 managed-resource visibility (#300)

[Satyam-Trivedi-ZS]

Summary

Adds two related AWS features (ECS Managed Instances are a primary producer of managed EC2 instances, so #300 composes naturally with #159):

• #159 — AWS ECS SDK-compat handler: a complete new ECS service so real aws-sdk-go-v2/service/ecs clients work against cloudemu.
• #300 — EC2 managed-resource visibility: Operator.Managed + IncludeManagedResources on DescribeInstances, with a hidden-by-default account setting.

Both are implemented end-to-end across the established layers and verified with real-SDK round-trip + live-socket e2e.

Closes #159
Closes #300

---

#159 — ECS SDK-compat service

New service (AWS JSON 1.1, X-Amz-Target: AmazonEC2ContainerServiceV20141113.*) across all four layers — services/ecs/driver (interface) → providers/aws/ecs (in-memory Mock) → services/ecs (portable API with the cross-cutting do() wrapper) → server/aws/ecs (restJson/AwsJson handler) — and registered in server/aws/aws.go + providers/aws/aws.go.

All 19 operations from the ticket:

Family	Operations
Clusters	CreateCluster, ListClusters, DescribeClusters, DeleteCluster
Task definitions	RegisterTaskDefinition, ListTaskDefinitions, DescribeTaskDefinition, DeregisterTaskDefinition
Tasks	RunTask, StopTask, ListTasks, DescribeTasks
Services	CreateService, UpdateService, ListServices, DescribeServices, DeleteService
Container instances	ListContainerInstances, DescribeContainerInstances

Behavior (AWS-faithful):

• Resources complete synchronously — RunTask returns RUNNING tasks, CreateService reaches desiredCount, task-definition revisions auto-increment per family.
• DescribeTaskDefinition resolves family (→ latest ACTIVE), family:revision, or full ARN; a missing definition errors (ClientException), not a failures[] entry.
• The four batch Describe* ops and RunTask return partial success — unresolved ids land in failures[]{arn, reason: "MISSING"} rather than failing the whole call.
• DescribeClusters honors the include parameter — tags/settings are only returned when TAGS/SETTINGS is requested (matches real ECS).
• DeregisterTaskDefinition/DeleteService/DeleteCluster soft-transition to INACTIVE; duplicate CreateService in a cluster is rejected.
• Cluster defaults to "default" when omitted. ARNs via idgen.

#300 — EC2 managed-resource visibility

• Instance model: added an Operator block (Managed, Principal) to the compute Instance; instances can be seeded managed at launch (InstanceConfig.Managed/Principal) or via SetManaged(id, principal). aws:-prefixed system tags (e.g. aws:ec2:managed-launch) round-trip unchanged.

• Account setting: cloud.EC2.SetManagedResourceVisibility("visible"|"hidden").

• DescribeInstances honors IncludeManagedResources:

  visibility	IncludeManagedResources	managed instance in result?
  hidden	unset / false	no
  hidden	true	yes
  visible	any	yes

  The rule applies uniformly to list, filtered, and explicit-InstanceIds requests; non-managed instances are always returned.

• Works on both surfaces — the typed Go API and the query-protocol SDK server. The response Operator block (<operator><managed>/<principal>/<hiddenByDefault></operator>) matches the ec2 SDK deserializer, so a real aws-sdk-go-v2 client sees Instance.Operator.Managed/Principal.

• The compute driver's DescribeInstances gained a trailing variadic options param (existing 3-arg callers compile unchanged); all implementers (GCP/Azure/chaos/ssm) were updated.

Dependencies

• Add github.com/aws/aws-sdk-go-v2/service/ecs v1.89.1.
• Bump github.com/aws/aws-sdk-go-v2/service/ec2 to v1.317.1 (adds the DescribeInstances IncludeManagedResources request parameter).

Testing

• go build ./..., go vet, gofmt — clean. golangci-lint — zero new findings (the 7 pre-existing findings live in untouched server/aws/ec2 files).
• go test ./... — 210 packages, 0 failures. go test -race on the ecs/ec2 packages — clean.
• Real-SDK end-to-end against awsserver.New(awsserver.DriversFrom(cloud)) over a live socket:
  • ECS — 29 checks covering all 19 ops + edge cases (default cluster, revision bump, latest-vs-pinned task-def, deregister→INACTIVE, duplicate-service conflict, partial failures across clusters/tasks/services/instances, container-instance round-trip, include=TAGS gating).
  • EC2 — the full 6-case visibility matrix incl. explicit-ID paths and Operator/system-tag round-trip.
• docs/services.md updated: new Container Orchestration service (§24), an EC2 managed-resource visibility subsection with Go-API + SDK examples, and the service/op-count tables.

[NitinKumar004]

Review — ECS SDK-compat (#159) + EC2 managed-resource visibility (#300)

Deep end-to-end pass (5-agent fan-out + direct verification): architecture conformance, all edge cases, blast radius/side-effects, and a full "what's left for real ECS parity" gap analysis. Build/tests pass.

✅ Architecture conformance — PASS

The ECS service is idiomatic and matches the bedrock reference: all 4 layers (driver → memstore-backed provider Mock → portable services/ecs/ecs.go with a real do() wrapper, incl. a doBatch[T] generic that threads the partial-success shape → AWS JSON 1.1 handler dispatched on X-Amz-Target prefix AmazonEC2ContainerServiceV20141113.). idgen for every ARN (correct long-ARN shapes), SortedValues() for list determinism, registration wired in both factories.

✅ Blast radius — clean (no regressions)

• The compute-driver DescribeInstances gained a variadic opts ...DescribeInstancesOptions, so all existing 3-arg callers compile and behave identically; the chaos wrapper and portable layer forward opts... (not dropped).
• Managed instances are visible by default — hiding needs explicit opt-in on both axes (Managed=true AND SetManagedResourceVisibility("hidden")), and non-managed instances are provably never filtered. So no existing launch-then-describe flow changes.
• ECS X-Amz-Target prefix is disjoint from every other JSON handler (no shadowing); go mod verify clean; factory wiring additive/nil-safe.

🔴 HIGH — real correctness bugs (ECS)

1. Concurrency + aliasing data corruption. ECS omits both halves of the repo's copy-on-write discipline (bedrock does both). Mutators write through the stored pointer with no lock — StopTask (tasks.go:80-87, incl. t.Containers[i].LastStatus), DeleteCluster (clusters.go:86), DeregisterTaskDefinition (taskdefs.go:103), UpdateService/DeleteService (services.go:67,126) — while Describe/List return *stored shallow copies that alias Containers/Tags/Settings/ContainerDefinitions. Concrete: RunTask/DescribeTasks hand back a task whose Containers aliases the store; a later StopTask flips LastStatus in that shared backing array, retroactively mutating every previously-returned snapshot — plus a genuine -race data race. There is no concurrency test, so the reported "-race clean" is vacuous for this class. Fix: copy-on-write on mutate + deep-copy slice fields on read (mirror cloneGuardrail).
2. DeleteCluster has no cascade guard. It just flips Status=INACTIVE regardless of active services / running tasks / registered container instances (clusters.go:78). Real ECS refuses with ClusterContainsServicesException / ClusterContainsTasksException / ClusterContainsContainerInstancesException — none of which exist in errors.go. So a non-empty cluster deletes and orphans its services.
3. DeleteService ignores desiredCount and has no force. AWS rejects deleting a service scaled >0 without force (InvalidParameterException); the mock always deletes, and force isn't even representable (absent from the driver, portable, and wire request). The SDK round-trip test cements the wrong behavior.

🟠 MEDIUM (ECS correctness/fidelity)

• No referential integrity on writes: RunTask/CreateService/DeleteService/ListTasks don't validate the cluster exists (should be ClusterNotFoundException); CreateService doesn't validate the task-def exists (should be ClientException). You can run a task into a ghost cluster.
• RunTask with a missing task-def returns failures[]{MISSING} — real ECS throws ClientException synchronously (failures[] is for placement/capacity only). The batch Describe* partial-success is correct; RunTask isn't.
• CreateService blocks reusing an INACTIVE (deleted) service name (Has is true for the lingering INACTIVE record) — AWS only blocks while ACTIVE, so delete-then-recreate fails.
• Cluster counters are always 0 — RunningTasksCount/PendingTasksCount/ActiveServicesCount/RegisteredContainerInstancesCount are never updated, so DescribeClusters reports every cluster idle regardless of load (and makes a counter-based cascade check impossible).
• ListTaskDefinitions mis-orders at ≥10 revisions — SortedValues() sorts the family:revision store key with sort.Strings, so web:10 < web:2 (verified: [web:1 web:10 web:11 web:2 web:3]). The sort ASC/DESC param is also not decoded.
• Error mapping loses typed exceptions — all NotFound → generic ClientException; SDK callers matching errors.As(&types.ClusterNotFoundException{}) / ServiceNotFoundException won't match. Notable for a "SDK-compat" PR.
• Create TOCTOU — CreateCluster/CreateService use Has-then-Set instead of memstore.SetIfAbsent; concurrent same-name creates both pass the guard.

🟠 MEDIUM — EC2 visibility cross-subsystem inconsistencies (silent, undocumented)

"Hidden" is only a DescribeInstances-layer filter; the instance stays fully live elsewhere, and internal callers that route through DescribeInstances without passing opts inherit the default-hide:

• CloudWatch metrics leak the hidden instance (metrics emitted on launch/lifecycle with the real InstanceId dimension) — the sharpest leak: hidden in Describe, visible in monitoring.
• Mutable by ID — Start/Stop/Reboot/Terminate resolve by Get with no visibility check.
• resourcediscovery/RE2, topology CanConnect, and SSM RunCommand all drop hidden managed instances (each calls DescribeInstances(ctx, …, ) with no opts) → topology reports a running instance as NotFound; SSM rejects it as InvalidInstanceId. None documented.
• LOW: managedResourceVisibility is unsynchronized shared state (-race if toggled concurrently; contradicts the "all mocks use RWMutex" rule); SetManagedResourceVisibility accepts any string (a typo silently means "visible"); <hiddenByDefault> XML is a cloudemu invention the real SDK ignores (the "matches the deserializer" comment is inaccurate); explicit-ID describe of a hidden instance returns empty rather than InvalidInstanceID.NotFound.

---

📋 Gap analysis — what's left for 100% real-AWS ECS (the headline ask)

The PR models cluster/task-def/task/service/container-instance identity and lifecycle well, but almost none of the scheduling, networking, or task-runtime semantics that make ECS ECS. Most advanced fields aren't even decoded off the wire (absent), and the few that are decoded are echoed but never acted on (accepted-but-ignored → misleading).

1. Launch types — Fargate vs EC2 (the core gap): launchType is a pass-through label — FARGATE / EC2 / EXTERNAL all take the identical path and spring to RUNNING. No validation against the task-def's requiresCompatibilities. No EC2 placement at all: tasks are never placed onto a container instance, no CPU/memory reservation/decrement, no bin-pack/spread, no "no capacity → PENDING/RESOURCE failure" — you can RunTask launchType=EC2 into a cluster with zero container instances and get RUNNING. capacityProviderStrategy / capacity providers entirely absent (no FARGATE_SPOT, no ASG providers, no PutClusterCapacityProviders). Fargate: platformVersion not modeled; networkConfiguration.awsvpcConfiguration (subnets/SGs) not decoded and not required (real Fargate errors without it); no ENI attachments[]/private IP on tasks; no CPU/memory-combo validation.
2. Networking modes: networkMode stored opaque — awsvpc/bridge/host/none indistinguishable; networkConfiguration absent everywhere.
3. Load balancer / service discovery: loadBalancers and serviceRegistries not decoded on CreateService — silently dropped (no target registration, no health-check grace).
4. Service scheduling/deployments: CreateService fakes instant convergence (RunningCount=DesiredCount, no actual tasks launched, no service→task linkage). No deployments[]/events[] (the steady-state signal callers poll), no deploymentController/deploymentConfiguration, no circuit-breaker/rollback, DAEMON vs REPLICA not acted on, no task sets, no Application Auto Scaling. UpdateService supports only taskDefinition+desiredCount.
5. Task-definition runtime surface (silently dropped on register): ContainerDefinition models 8 basic fields; absent are healthCheck, dependsOn/ordering, essential-exit semantics, stopTimeout, secrets/environmentFiles, volumes/mountPoints (EFS/bind/docker), ephemeralStorage, logConfiguration (awslogs), resourceRequirements (GPU), ulimits/linuxParameters, pidMode/ipcMode, runtimePlatform, proxyConfiguration; taskRoleArn/executionRoleArn stored but never enforced. No RegisterTaskDefinition validation (empty containers registers fine).
6. Missing operations: Tag/Untag/ListTagsForResource; account settings (Put/List/Delete); RegisterContainerInstance / DeregisterContainerInstance / UpdateContainerInstancesState (DRAINING); capacity-provider ops; task-set ops; ExecuteCommand (ECS Exec); UpdateCluster/UpdateClusterSettings; Put/Delete/ListAttributes; ListTaskDefinitionFamilies; ListServicesByNamespace.
7. Container instances are test-only: the sole way one exists is the Go helper SeedContainerInstance (callers: tests only) — there's no RegisterContainerInstance API, so over the real SDK List/DescribeContainerInstances is always empty. And despite the "#300 produces managed EC2 instances" framing, ECS and EC2 are not wired — SeedContainerInstance takes an opaque EC2InstanceID string, never creates an EC2 instance or calls SetManaged, so #159 and #300 don't actually compose.

Docs (services.md §24) acknowledge only "resources complete synchronously"; everything above is a silent gap. Suggest documenting the accepted-but-ignored fields (launchType, loadBalancers, essential, task/execution roles) so callers aren't misled, and prioritizing: EC2 placement+capacity → launch-type branching+Fargate networkConfiguration → real service deployment model → LB/serviceRegistries → task-def runtime fields → the missing ops.

Bottom line

Architecture and blast radius are solid — this is a well-structured, idiomatic scaffold. But before it's "faithful" it needs the HIGH fixes (the copy-on-write/aliasing corruption with a -race test, DeleteCluster cascade guards, DeleteService force/desiredCount), the MEDIUM referential-integrity + typed-error + revision-ordering items, and a decision on the EC2 hidden-instance cross-subsystem consistency. The parity gap is large and mostly about scheduling/runtime semantics (Fargate-vs-EC2 placement first) — reasonable to land in waves, but the currently accepted-but-ignored fields should be documented so they don't read as working.

[Satyam-Trivedi-ZS]

Thanks @NitinKumar004 — exceptionally thorough review. I took the whole thing, including the parity gap, and reworked ECS in four waves (pushed in 36f9f1d). Verified each item against the code, and re-ran the full suite + go test -race + 66 real-SDK end-to-end checks (ECS core 25, ECS services 33, EC2 visibility + ECS↔EC2 compose 8) driving the genuine aws-sdk-go-v2 clients over a live socket. Point by point:

🔴 HIGH — all fixed

1. Concurrency + aliasing corruption — fixed both halves of the discipline: every mutator (StopTask/DeleteCluster/DeregisterTaskDefinition/Update+DeleteService/UpdateContainerInstancesState) is now copy-on-write (clone → mutate → Set), and every read path deep-copies slice/map fields via new clone.go helpers (cloneTask/cloneCluster/cloneService/cloneTaskDef/...). Added -race concurrency tests (mutator hammered vs Get/List from N goroutines) — the earlier "-race clean" is no longer vacuous.
2. DeleteCluster cascade guard — now refuses a non-empty cluster with ClusterContainsServicesException / ClusterContainsTasksException / ClusterContainsContainerInstancesException (added to the error mapping), backed by real cluster counters (see MEDIUM below). e2e exercises all three branches.
3. DeleteService force + desiredCount — force added to driver/portable/wire; deleting a service with running/desired > 0 without force → InvalidParameterException; force=true deletes and stops its tasks.

🟠 MEDIUM (ECS) — all fixed

• Referential integrity — RunTask/CreateService/DeleteService/ListTasks validate the cluster (ClusterNotFoundException; default treated as implicit); CreateService validates the task-def (ClientException).
• RunTask missing task-def — now a synchronous ClientException, not failures[] (failures[] reserved for placement/capacity).
• INACTIVE service-name reuse — only an ACTIVE service blocks the name; delete-then-recreate works.
• Cluster counters — RunningTasksCount/PendingTasksCount/ActiveServicesCount/RegisteredContainerInstancesCount computed on read.
• ListTaskDefinitions ordering — sorts by family then numeric revision (web:2 before web:10); sort ASC/DESC honored.
• Typed error mapping — per-context exceptions so errors.As(&types.ClusterNotFoundException{}) / ServiceNotFoundException / ClusterContains* / InvalidParameterException / ClientException all match.
• Create TOCTOU — CreateCluster/CreateService use SetIfAbsent.

🟠 MEDIUM (EC2 #300 cross-subsystem) — all fixed

• CloudWatch metric leak — managed instances no longer emit instance-dimensioned metrics.
• resourcediscovery / topology / SSM — now pass IncludeManagedResources: true, so a managed instance is discoverable/connectable/targetable (no more topology-NotFound / SSM-InvalidInstanceId).
• Unsynchronized visibility — RWMutex-guarded; invalid value rejected (SetManagedResourceVisibility errors on anything but visible/hidden); explicit-ID describe of a hidden instance → InvalidInstanceID.NotFound; the <hiddenByDefault> element is a real SDK OperatorResponse field (confirmed in the deserializer), comment corrected.

📋 Parity gap — built out (the headline ask)

Rather than defer, I implemented the scheduling/runtime surface across waves 2–4:

• EC2 vs Fargate placement + capacity — container instances carry CPU/memory; EC2 RunTask/services are first-fit placed (reserve on run, release on stop); no capacity → failures[] (AGENT/RESOURCE) or service PENDING. launchType validated vs requiresCompatibilities. Fargate requires networkConfiguration.awsvpcConfiguration + awsvpc cpu/memory and synthesizes an ENI attachment + platformVersion.
• Service deployment model — real desiredCount task convergence + service→task linkage, deployments[]/events[], PRIMARY/ACTIVE promotion, DAEMON vs REPLICA, deploymentController/configuration, loadBalancers/serviceRegistries round-trip, full UpdateService.
• Task-def runtime surface — portMappings/environment/secrets/healthCheck/logConfiguration/mountPoints/ulimits/resourceRequirements/volumes/ephemeralStorage/runtimePlatform/proxyConfiguration/... round-trip; register validation (essential defaults true, empty/duplicate names rejected).
• Missing ops added (19 → 37): Tag/Untag/ListTagsForResource, account settings (Put/+Default/List/Delete), RegisterContainerInstance / DeregisterContainerInstance / UpdateContainerInstancesState (DRAINING), UpdateCluster / UpdateClusterSettings / PutClusterCapacityProviders, Put/Delete/ListAttributes, ListTaskDefinitionFamilies, ExecuteCommand.
• feat: AWS ECS SDK-compat handler #159 now composes with feat: emulate AWS managed-resource visibility on EC2 DescribeInstances (Operator.Managed + IncludeManagedResources) #300 — RegisterContainerInstance provisions a real managed EC2 instance (Operator.Managed=true, principal ecs.amazonaws.com, aws:ec2:managed-launch tag) that's subject to managed-resource visibility. The e2e proves it end-to-end over the real SDK: register a container instance → it appears in DescribeInstances(IncludeManagedResources=true), hidden by default.

Remaining scope intentionally not modeled (documented in docs/services.md §24 as accepted simplifications): capacity-provider resource resolution (FARGATE_SPOT/ASG scaling), real LB target registration/health checks, deployment circuit-breaker/rollback simulation, and Service Connect — accept-and-echo, not simulated. Happy to take these in a follow-up if you'd like any prioritized.

docs/services.md §24 rewritten to describe placement/Fargate/deployment/compose behavior and the accepted-but-not-simulated fields, so nothing reads as working that isn't. golangci-lint reports 0 new findings; go test ./... = 210 pkgs / 0 failures; -race clean.

[thzgajendra]

Deep re-review — ECS SDK-compat (#159) + EC2 managed-visibility (#300)

Full 8-lens pass (concurrency/COW, lifecycle & referential integrity, placement/capacity & Fargate, service deployment model, EC2 visibility + cross-subsystem, architecture/layering, tests, docs), verifying against source and the vendored SDK. The rework in 36f9f1d is real and high-quality — every HIGH/MEDIUM from @NitinKumar004's review is genuinely fixed, and this is real logic, not accept-and-echo. Verified:

• Copy-on-write + deep-copy-on-read is correct and regression-guarded. clone.go deep-copies every nested slice/map; all mutators clone→mutate→Set under lock; all reads return clones. I empirically reverted clone-on-read in a scratch copy and go test -race ./providers/aws/ecs/... fired DATA RACE; restored it → clean. The concurrency tests (concurrency_race_test.go, 8 goroutines × 50 iters, mutators vs Describe/List) are genuine, not vacuous — they'd fail if the fix were reverted.
• All 9 lifecycle/referential fixes hold: DeleteCluster cascade guard (3 real ClusterContains* exceptions), DeleteService force+desiredCount, cluster/task-def validation (no more ghost-cluster runs), RunTask-missing-taskdef→ClientException, INACTIVE name reuse, live cluster counters, numeric revision ordering, context-specific typed errors (errors.As matches), SetIfAbsent TOCTOU.
• Placement/capacity accounting is correct and concurrency-safe (reserve on run, release on stop, no leak/double-release, first-fit with correct AGENT/RESOURCE failure reasons). CodeQL slice-allocation alert is genuinely resolved — count is bounded to [1,10] before any make(), which uses the constant.
• EC2 #300 cross-subsystem leaks all closed: visibility matrix correct for list/filtered/explicit-ID (explicit hidden ID → InvalidInstanceID.NotFound), CloudWatch metric leak fixed, resourcediscovery/topology/SSM pass IncludeManagedResources=true (correct — internal control-plane must see the real fleet), RWMutex + invalid-value rejection. OperatorResponse.HiddenByDefault IS a real aws-sdk-go-v2/service/ec2 v1.317.1 field (verified in the vendored deserializer) — the earlier doubt was unfounded; the author is right. Variadic driver change is non-breaking (all implementers forward/accept opts). #159↔#300 genuinely composes: RegisterContainerInstance provisions a real managed EC2 instance, proven end-to-end over the SDK.
• Architecture is textbook-conformant to the bedrock/rds reference (4 layers, do()/doBatch[T], idgen ARNs, disjoint X-Amz-Target). go test ./... and -race pass.

New findings from this pass (none block the core value)

1. [Medium] Fargate cpu/memory combo validation is missing — contradicts the stated claim. validateLaunch (providers/aws/ecs/tasks.go) only checks td.CPU/Memory != ""; there is no valid-pairing check, so cpu=256, memory=1000000 (or cpu=999) is accepted and springs to RUNNING. Real Fargate rejects with "No Fargate configuration exists for given values." The PR/docs claim a "valid awsvpc cpu/memory combo" — either add the combo table or soften the claim so a real-SDK caller isn't misled into thinking the emulator catches bad pairings.

2. [Medium] docs/services.md §24 under-discloses accept-and-echo fields. The code is honest (these are pure store-and-echo), but §24 doesn't say so for capacityProviderStrategy (silently falls through to EC2 placement), loadBalancers/serviceRegistries (no target registration/health checks), deployment circuit-breaker/rollback, and Service Connect. The repo already has the convention for this — see the RDS "accepted but not [simulated]" note at services.md:1206. Also the "full container/task runtime surface" wording (§24) risks over-reading: task-def fields (secrets/healthCheck/logConfiguration/volumes/…) are round-tripped on the definition but not reflected onto launched containers (containersFor copies only name/image/status). Suggest "accepted and round-tripped on the task definition (not executed)."

3. [Low–med] Deployment ACTIVE entries are never pruned. redeployService demotes prior deployments to ACTIVE (counts zeroed) and prepends the new PRIMARY, but never removes drained ones — so deployments[] grows unbounded across repeated UpdateService calls. Real ECS drops a deployment once drained; a caller polling for eventual single-PRIMARY steady state will see accumulating stale entries.

4. [Low–med] DAEMON silently overwrites a caller-supplied desiredCount (desiredForStrategy sets it to the placeable-instance count). Real ECS rejects CreateService with desiredCount set under DAEMON (InvalidParameterException). Accepting-and-overwriting could mislead.

5. [Low] Lost-update on concurrent same-key RMW. UpdateService/mutateCluster/TagResource do Get→(unlock)→clone→mutate→Set — two concurrent writers to the same key silently drop one update (last-writer-wins). Not a -race data race (each mutates its own clone), but a real correctness race. The store already exposes an atomic Update(key, fn) that would close it; it's unused. Fine for a mock, worth a note.

6. [Low] Miscellaneous fidelity/robustness: DeregisterContainerInstance force=true leaves its tasks RUNNING on a deleted instance (real ECS stops them); UpdateContainerInstancesState(DRAINING) blocks new placement but doesn't drain/reschedule existing tasks; launchManagedInstance silently falls back to a synthesized i-… string if the launcher errors (a future misconfig would regress compose to the opaque-string behavior invisibly — surface the error instead); serviceFromInput/applyServiceRefs store the caller's LoadBalancers/ServiceRegistries slices without copying (input-aliasing on write); no test covers RunTask count<=0 (defaults to 1) even though the >10 cap is tested.

Bottom line: correct, well-tested, idiomatic, and honestly built out far beyond the original scaffold. I'd resolve #1 (claim-vs-code) and #2 (doc disclosure) before merge; #3–#6 are good follow-up candidates. Deferring the merge decision to you.

[thzgajendra]

Thanks for the thorough 8-lens re-review. Addressed in 37cf9f5:

#1 — Fargate cpu/memory combo (fixed). validateLaunch now validates the task-level cpu/memory against the supported Fargate configuration table (fargateMemRanges), not just presence. An unsupported pairing (e.g. cpu=512, memory=512) is rejected with the AWS "No Fargate configuration exists for given values" error; the .25 vCPU tier's non-uniform set (512/1024/2048) is modeled explicitly so 1536 is correctly rejected. Test: TestRunTaskFargateInvalidCPUMemoryCombo.

#2 — docs §24 disclosure (fixed). §24 now lists the accepted but not simulated fields — capacityProviderStrategy (placement still falls through by launch type), loadBalancers/serviceRegistries (no target registration/health checks/Service Connect), and deployment circuit-breaker/rollback — and clarifies that the task-def runtime surface is round-tripped on the definition, not reflected onto launched containers (which carry name/image/status). The Fargate combo validation is now called out as real.

#3 — deployment accumulation (fixed). redeployService drains the superseded deployment (synchronously) and replaces the slice with just the new PRIMARY, so deployments[] no longer grows across repeated UpdateService. Test: TestUpdateServiceDeploymentsDoNotAccumulate.

#4 — DAEMON desiredCount (fixed). CreateService rejects a caller-supplied desiredCount on a DAEMON service with InvalidParameterException instead of overwriting it. Test: TestCreateServiceDaemonRejectsDesiredCount.

#6 — misc:

• Input-aliasing (fixed): serviceFromInput/applyServiceRefs now clone LoadBalancers/ServiceRegistries/CapacityProviderStrategy (and the network/deployment configs), so the stored record never aliases the caller's slices.
• launchManagedInstance silent fallback (fixed): it now returns an error; RegisterContainerInstance surfaces a launcher failure as ServerException (only the no-launcher-wired case synthesizes an id).
• Force-deregister leaving tasks RUNNING (fixed): DeregisterContainerInstance(force=true) stops the tasks placed on the instance. Test: TestForceDeregisterStopsTasks.
• RunTask count<=0 (test added): TestRunTaskDefaultsCountToOne.

Addressed as documented limitations (per your "fine for a mock" note):

• Add dead-letter queues, cost simulation, and serverless triggers #5 lost-update on concurrent same-key RMW — UpdateService/mutateCluster/TagResource are last-writer-wins (each mutates its own clone, so no -race data race). The synchronous single-caller SDK flows don't hit it; left as a noted limitation rather than threading every RMW through store.Update.
• Simplify README and add new features docs #6 DRAINING not rescheduling — UpdateContainerInstancesState(DRAINING) blocks new placement but doesn't drain/reschedule existing tasks; faithful rescheduling needs a reconciliation loop that the synchronous model doesn't have.

Build, go test -race ./providers/aws/ecs/..., and golangci-lint are green.

[Satyam-Trivedi-ZS]

Thanks @thzgajendra — that's an unusually thorough pass, and the empirical clone-on-read revert to confirm the race guard is exactly the kind of verification I'd hoped for. All six findings are now resolved across two commits (37cf9f5 addressed 1–4 and most of 6; f32bf93 closes 5 and the two remaining items). Point by point:

1 — Fargate cpu/memory combo validation [Fixed — 37cf9f5]
validateLaunch now runs a Fargate combo check (validateFargateCPUMemory / fargateComboValid in providers/aws/ecs/tasks.go). Invalid pairings (cpu=256, memory=1000000, cpu=999, …) are rejected with the real message "No Fargate configuration exists for given cpu (%s) and memory (%s) values" before the task springs to RUNNING, so the claim and the code now agree.

2 — docs/services.md §24 accept-and-echo disclosure [Fixed — 37cf9f5]
§24 now carries an explicit "Accepted but not simulated (stored and round-tripped so SDK calls succeed, but with no behavioral effect)" note covering capacityProviderStrategy, loadBalancers/serviceRegistries (no target-group registration/health checks/Service Connect), and the deployment circuit-breaker/rollback — matching the RDS convention you pointed to. The task-def wording is softened to "accepted and round-tripped on the task definition, not reflected onto launched containers, which carry only name/image/status."

3 — Deployment ACTIVE entries never pruned [Fixed — 37cf9f5]
redeployService drains the superseded deployment synchronously and then replaces the slice with the single new PRIMARY (svc.Deployments = []driver.Deployment{dep}) rather than prepending. Repeated UpdateService calls now converge to a single-PRIMARY steady state instead of accumulating stale entries.

4 — DAEMON silently overwrites caller desiredCount [Fixed — 37cf9f5]
CreateService now rejects a caller-supplied desiredCount under DAEMON with InvalidParameterException ("desiredCount must not be specified for a DAEMON service") instead of overriding it, matching real ECS.

5 — Lost-update on concurrent same-key RMW [Fixed — f32bf93]
TagResource/UntagResource and mutateCluster now route the read-modify-write through the store's atomic Update(key, fn) (with SetIfAbsent seeding the tag entry), so the whole RMW runs under the store lock — two concurrent writers to the same key can no longer drop one another's changes.

One deliberate exception: UpdateService still does clone→mutate→Set. Its RMW wraps cross-store orchestration (drainService/converge mutate the tasks store and take the placement lock), so folding it into a single services.Update closure would hold the service-store lock across those and introduce a lock-ordering hazard for negligible benefit in a mock. Flagging it here rather than forcing it under one lock.

6 — Miscellaneous fidelity/robustness [Fixed — 37cf9f5 + f32bf93]

• DeregisterContainerInstance force=true now stops the instance's tasks (stopTasksOnInstance → STOPPED, TerminationNotice) instead of stranding them RUNNING.
• launchManagedInstance surfaces a wired-launcher error (wrapped as ServerException) instead of silently falling back to a synthesized i-…, so a future misconfig can't regress the compose invisibly.
• serviceFromInput/applyServiceRefs now copy the caller's LoadBalancers/ServiceRegistries (and capacity-provider) slices, closing the input-aliasing-on-write.
• Added a count<=0 → single task test (review_fixes_test.go, #6d).

Left as a documented follow-up (not this PR): UpdateContainerInstancesState(DRAINING) blocks new placement but does not actively drain/reschedule the instance's existing tasks — real drain-and-reschedule is a scheduler feature better done on its own.

Bonus (f32bf93): wire error messages were leaking the internal "<Code>: " prefix (e.g. "InvalidArgument: No Fargate configuration…") because the server used err.Error(). Added wireMessage() to unwrap *cerrors.Error and surface the bare Message, so SDK-surfaced exceptions read like real AWS.

go build ./..., go vet, gofmt, go test ./..., go test -race ./providers/aws/ecs/..., and golangci-lint are all green. Thanks again for the depth here.

[NitinKumar004]

Deep review — AWS ECS SDK-compat (#159) + EC2 managed-resource visibility (#300)

Reviewed end-to-end in an isolated worktree (gate matrix, static/coverage layer, architecture-pillar fit, and a multi-lens corner-case pass with adversarial verification), then re-verified against the latest fix commits.

Gates (verified): build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓ — CI-blocking gates pass; the ec2 bump is necessary (adds IncludeManagedResources), deps are tidy with no version skew, and golangci-lint is clean on changed files.

Architecture: strong, and the recent fixes are clean. The DescribeInstances variadic ripple is fully sound (chaos/ssm/topology/discovery forward opts, gce/vm accept-and-ignore, rds-driver is a separate interface). EC2 visibility matrix, Operator XML, and system-tag round-trip are correct. ECS wire shape is correct (37 ops routed + exercised, typed exceptions + failures[] modeled, include gating implemented, epoch timestamps), read-path aliasing is clone-on-read, revision auto-increment is race-safe, the DeleteCluster cascade guard is correct, factory/Matches don't collide with ECR, and docs op-counts are exact 3-way. The latest fix commits (deployment accumulation → single PRIMARY, DAEMON+desiredCount rejected, Fargate cpu/memory validation, atomic tag/cluster RMW, force-deregister without deadlock, wire error typing) were verified and introduce no new bugs.

The items below are correctness/fidelity polish.

Medium

• INACTIVE (deregistered) task def still launches tasks/services — resolveTaskDef returns an explicit family:revision/ARN with no ACTIVE check. Deregister("web:3") then RunTask(taskDefinition="web:3") launches RUNNING; real ECS rejects it. (inline)
• INACTIVE (deleted) cluster still accepts RunTask/CreateService — clusterExists is presence-only; DeleteCluster marks INACTIVE but leaves the tombstone. DeleteCluster("prod") then RunTask(cluster="prod") stores a RUNNING task on a dead cluster (real ECS: ClusterNotFoundException); the tombstone also blocks recreating the name. (inline)
• UpdateContainerInstancesState capacity lost-update race — it does copy-on-write without placeMu, while reserve/release/deregister all take it. A concurrent RunTask (capacity reserve) + UpdateContainerInstancesState(DRAINING) silently reverts the reservation; invisible to -race and untested. (inline)
• SetManaged is a test-only helper in production code — zero non-test callers (the production managed-launch path uses LaunchManaged). Violates the project's "no test-only helpers in production" rule. (inline)
• ListServices returns INACTIVE (deleted) service tombstones — no status filter; real ECS lists only ACTIVE/DRAINING. Create → delete → list still shows the deleted service. (inline)
• The two headline "must-verify" behaviors are unasserted — server/aws/ecs/sdk_roundtrip_test.go has 0 ClientException refs and no include=[TAGS] gating assertion, so the typed-exception-vs-failures[] distinction and the include gating are never checked; PutAccountSettingDefault is untested. Coverage is low on services/ecs (55.6%) and the server handlers. If either headline behavior regresses, the suite stays green.

Low

• UpdateCluster stores Configuration json.RawMessage without cloning — write-side aliasing (read paths are safe). (inline)
• Nested ContainerDefinition slices (Environment/PortMappings/Secrets) still alias on register (the top-level slice is now cloned).
• SetManaged mutates inst.Operator without m.mu; UpdateService doesn't reject DAEMON+desiredCount (CreateService does).
• Intentional parity gaps (ECS→CloudWatch metrics, ECS cost rate) — consistent with the EKS precedent; worth a tracked follow-up rather than a blocker.

Verdict: comment. Gate-green, architecturally clean, deps tidy, docs accurate, and the recent fixes are solid; the remaining items are fidelity/correctness polish. I'd prioritize the INACTIVE-resource guards and the SetManaged rule violation.

[NitinKumar004]

[Medium] INACTIVE task def still launches. resolveTaskDef returns an explicit family:revision/ARN via a bare Get with no ACTIVE check, so RunTask/CreateService/UpdateService accept a deregistered definition. → Deregister("web:3") then RunTask(taskDefinition="web:3") launches RUNNING; real ECS rejects running new tasks from an INACTIVE task def. (Bare-family lookups are fine via latestActive.)

[Satyam-Trivedi-ZS]

Fixed in f27e99a. Added resolveLaunchableTaskDef (resolves and requires ACTIVE); RunTask/CreateService/UpdateService route launches through it, so a deregistered family:revision is now rejected with a ClientException (InvalidParameter — it exists, just isn't runnable). DescribeTaskDefinition/DeregisterTaskDefinition/tag resolution still use resolveTaskDef and keep seeing INACTIVE defs. Tests: TestRunTaskRejectsInactiveTaskDef + Create/Update variants.

[NitinKumar004]

[Medium] INACTIVE (deleted) cluster still accepts work. clusterExists is presence-only; DeleteCluster marks the cluster INACTIVE but leaves the tombstone in the store, and RunTask/CreateService gate only on clusterExists. → DeleteCluster("prod") then RunTask(cluster="prod") stores a RUNNING task on a dead cluster (real ECS: ClusterNotFoundException); the tombstone also makes CreateCluster("prod") fail AlreadyExists, so the name can never be recreated. Add a status check (reject non-ACTIVE clusters).

[Satyam-Trivedi-ZS]

Fixed in f27e99a. Added clusterActive (present and ACTIVE); RunTask/CreateService gate on it, so work against a deleted cluster now returns ClusterNotFoundException instead of landing on the tombstone. And CreateCluster reuses a deleted name — only an ACTIVE same-name cluster is an AlreadyExists conflict; an INACTIVE tombstone is overwritten — under a new clusterMu compare-and-set. Tests: TestRunTaskRejectsDeletedCluster, TestCreateClusterReusesDeletedName.

[NitinKumar004]

[Medium] Capacity lost-update race. UpdateContainerInstancesState mutates a clone and Sets it back without placeMu, while reserve (capacity.go), release/StopTask, and DeregisterContainerInstance all take placeMu for their instance RMW. → A concurrent RunTask (reserve: decrement RemainingCPU, RunningTasksCount++) + UpdateContainerInstancesState(DRAINING) (reads the pre-decrement copy) → whichever Set lands last wins, silently reverting the reservation. memstore.Set is per-key locked so -race won't flag it; it's a logical lost update and concurrency_race_test.go doesn't exercise this combo. Take placeMu here too.

[Satyam-Trivedi-ZS]

Fixed in f27e99a. UpdateContainerInstancesState now holds placeMu for the whole resolve→clone→Set window, serializing it with reserve/release/DeregisterContainerInstance so a concurrent RunTask reservation can no longer be silently reverted. Resolving under the lock also means the clone reflects the freshest capacity counts.

[NitinKumar004]

[Medium] Test-only helper in production code. SetManaged has zero non-test callers — the production managed-launch path is LaunchManaged (consumed by providers/aws/ecs/containerinstances.go). This violates the project's "no test-only helpers in production" rule (dead exported API a maintainer may mistake for a real flow). Move the marking into the test package, or fold it into RunInstances(Managed:true).

[Satyam-Trivedi-ZS]

Fixed in f27e99a. Removed SetManaged — zero non-test callers; the production managed-launch path is LaunchManaged → RunInstances(Managed:true, Principal:…). Its dedicated TestSetManaged went with it; TestManagedResourceVisibility already covers hidden/revealed/opt-in/explicit-id via the real path. This also removes the m.mu-less inst.Operator mutation you flagged under Low.

[NitinKumar004]

[Medium] ListServices returns INACTIVE tombstones. No status filter, so a deleted service (markServiceDeleted sets Status=INACTIVE but keeps the record) still appears. → Create → DeleteService → ListServices still lists the deleted service; real ECS ListServices returns only ACTIVE/DRAINING. Filter out INACTIVE.

[Satyam-Trivedi-ZS]

Fixed in f27e99a. ListServices now skips INACTIVE tombstones (returns only live ACTIVE/DRAINING services); DescribeServices still resolves a deleted service by name. Test: TestListServicesExcludesInactive (asserts a deleted svc is absent from List but present in Describe).

[NitinKumar004]

[Low] Write-side aliasing on UpdateCluster. c.Configuration = in.Configuration stores the caller's json.RawMessage without cloning, so a caller mutating the byte slice it passed corrupts the store (read paths are safe via cloneCluster/cloneRaw). Clone it like the other reference-typed fields.

[Satyam-Trivedi-ZS]

Fixed in f27e99a. c.Configuration = cloneRaw(in.Configuration) so the store no longer aliases the caller's byte slice. Test: TestUpdateClusterDoesNotAliasConfiguration (mutates the input after the call, asserts the store is unchanged).

[Satyam-Trivedi-ZS]

Thanks @NitinKumar004 — the isolated-worktree pass and the adversarial corner-case verification are much appreciated. Every Medium and every actionable Low is now fixed in f27e99a (each inline thread has the specifics); summarizing the items that weren't inline:

Medium

Headline behaviors were unasserted (test coverage). Added the missing server-roundtrip assertions in server/aws/ecs/sdk_roundtrip_test.go:

• TestSDKClientException — RunTask against an unresolvable task definition now asserts a typed *ecstypes.ClientException is returned (not a failures[] entry), pinning the typed-exception-vs-failures[] distinction.
• TestSDKDescribeIncludeTagsGating — asserts DescribeClusters withholds tags without include, and returns them with Include: [TAGS] (ClusterFieldTags). This is the op that actually implements the gating (server/aws/ecs/clusters.go).
• TestSDKPutAccountSettingDefaultRoundtrip — covers the Default variant over the wire (the existing test only exercised PutAccountSetting).

Plus provider-level guards in providers/aws/ecs/nitin_review_test.go for every behavior change (INACTIVE task-def/cluster rejection, name reuse, ListServices filtering, DAEMON+desiredCount, and the two write-side aliasing fixes).

Low

• Nested ContainerDefinition slices aliased on register — fixed. RegisterTaskDefinition now deep-clones via cloneContainerDefs for the container defs and stores a full cloneTaskDef clone, so the task-level reference fields (Volumes, PlacementConstraints, InferenceAccelerators, EphemeralStorage, RuntimePlatform, ProxyConfiguration) no longer alias the caller's input either. Covered by TestRegisterTaskDefinitionDoesNotAliasInput (mutates the caller's Environment/PortMappings after register, asserts the store is untouched).
• SetManaged without m.mu — resolved by removing SetManaged entirely (see the inline thread; it was test-only).
• UpdateService didn't reject DAEMON+desiredCount — fixed; it now returns InvalidParameterException just like CreateService. Test: TestUpdateServiceDaemonRejectsDesiredCount.

Deferred (tracked follow-up, not this PR)

• Intentional parity gaps — ECS→CloudWatch metrics and an ECS cost rate. Agreed these are follow-ups consistent with the EKS precedent, not blockers; leaving them out of this PR as you suggested.
• UpdateContainerInstancesState(DRAINING) doesn't drain/reschedule existing tasks (from @thzgajendra's pass) — it blocks new placement but faithful drain-and-reschedule needs a reconciliation loop the synchronous model doesn't have; better as its own change.

Gates re-verified locally: build ✓ vet ✓ gofmt ✓ go test ./... ✓ go test -race ./providers/aws/ecs/… ./providers/aws/ec2/… ./server/aws/ecs/… ✓ golangci-lint ✓ go mod tidy ✓. Thanks again for the depth.

[NitinKumar004]

Re-review — all findings addressed ✅

Verified the fix commit (f27e99a) against my earlier review. Every finding is resolved, correctly and without over-correcting, with tests added and gates green (build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean on the changed files).

• INACTIVE task def launches → resolveActiveTaskDef rejects a deregistered def on RunTask/CreateService as a typed ClientException; DescribeTaskDefinition still resolves INACTIVE (no over-correction).
• INACTIVE cluster accepts work → new clusterActive guard on the launch paths only; ListTasks/StopTask still work on a deleting cluster.
• UpdateContainerInstancesState capacity race → now holds placeMu for the whole read-modify-write.
• SetManaged test-only helper → removed.
• Unasserted ClientException / include-gating → now asserted in the SDK round-trip tests.
• ListServices INACTIVE tombstones → filtered out (DescribeServices still returns them).
• UpdateCluster config aliasing → cloneRaw.

No regressions; the INACTIVE guards are scoped to launch paths so read/describe/list/stop on INACTIVE resources still behave correctly. LGTM.

Residual (non-blocking): the portable services/ecs and server/aws/ec2 packages remain under the 90% coverage pillar, and the intentional ECS→CloudWatch metrics / cost-rate gaps are still there (consistent with EKS — worth a tracked follow-up).