fix(iam): retry ConcurrentModification, and make terminate idempotent

[scttfrdmn]

Fixes #648. Both halves of the report, each verified in code before implementing.

Primary: concurrent launches died on IAM ConcurrentModification

Your diagnosis was exactly right, including the file:line. Confirmed:

• grep -rn ConcurrentModification --include='*.go' . → no matches
• retryIAM (pkg/aws/iam.go:74) hits if !isThrottle(err) { return err } and gives up on the first attempt, never sleeping
• isThrottle only matches Throttling/RequestLimitExceeded; isAlreadyExists only EntityAlreadyExists/LimitExceeded — so ConcurrentModification fell through both
• CreateOrGetInstanceProfile does pass Tags: c.buildIAMTags(config.Tags), which is what makes the implicit tagging operation serialise

Fix is the predicate, as you suggested, keeping the existing 500 ms × attempt backoff. AccessDenied and friends still fail fast on attempt 1 rather than waiting out five sleeps — there's a test for that, since "retry more" is easy to overdo.

The adjacent spot was worth more than the one-liner

You flagged SetupSporedIAMRole (:1077) as the same concern implemented a second time. It is, and that's why #64's fix never reached it: three bare calls (CreateRole :1101, CreateInstanceProfile :1134, AddRoleToInstanceProfile :1145) tolerating only EntityAlreadyExists/LimitExceeded by string match.

All three now route through retryIAM. Behaviour is preserved, not changed — both of those codes are already success to retryIAM via isAlreadyExists, so the hand-rolled tolerance was doing exactly what the wrapper does. Note CreateRole and CreateInstanceProfile there also pass Tags, so they were equally exposed.

TestEveryTaggedIAMCreateIsRetried now fails if any CreateRole/CreateInstanceProfile/AddRoleToInstanceProfile sits outside a retryIAM closure — so the next one inherits the fix rather than needing its own.

Secondary: and it was worse than reported

You identified not-found. There are three states that already satisfy terminate's goal of "this instance is not running", and all three exited 1:

state	was	now
never existed	exit 1, no instance found with name: …	exit 0
already terminated	exit 1	exit 0
already shutting down	exit 1	exit 0

The latter two are if anything more common on your path: on_complete: terminate means a successful task's instance is usually already gone by the time an executor cleans up. So the misleading "may still be running and billing" would fire on the happy path too.

This is a behaviour change — a script relying on non-zero to detect a missing instance now sees success. Called out in the changelog. I think it's right: not-found is success for an idempotent delete, which is the convention the rest of the codebase follows (EventBridge DeleteRule likewise returns success for a missing rule).

Scoped tightly. resolveInstance is shared by ~10 commands — connect, dns, config, extend, task, service, plugin — where a non-existent instance genuinely is an error. The resolver's semantics are untouched; only terminate opts in, through an ErrInstanceNotFound sentinel rather than string matching, and TestOnlyTerminateOptsIntoNotFoundSuccess fails if another command starts doing the same.

The sentinel is a typed error rather than a %w wrap: wrapping appended the sentinel's text and produced no instance found with name: x: instance not found. The type keeps the CLI message clean while errors.Is still works.

On your offer to move the message to nf-spawn: the wrapping sentence is theirs, but the non-zero exit that triggers it is spawn's, so fixing it here is the right half.

Verification

• All three gates confirmed to fail when inverted: reverting the predicate, un-wrapping one SetupSporedIAMRole call, and making terminate error on already-terminated.
• gofmt, go vet, full go test -short ./..., make check-docs, and semgrep scan --config=auto --error . (0 findings) run locally before pushing.
• The full-backoff test (5 attempts = 7.5 s of real sleeping) is skipped under -short, which is what CI runs; the 1.5 s test that proves the retry actually happens is not skipped.
• My own changelog gate caught a duplicate ### Fixed group, for the third time this session.

Not reproduced against real AWS

Forcing a genuine ConcurrentModification needs two launches racing on a fresh profile name, and the window closes after the first success — so a real-AWS attempt would be a coin flip that proves nothing when it passes. The unit tests drive the predicate through smithy.GenericAPIError with IAM's verbatim message instead. Happy to attempt a live race if you'd rather have that too.

[codecov]

Codecov Report

❌ Patch coverage is 21.95122% with 32 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/aws/iam.go	22.58%	24 Missing ⚠️
cmd/terminate.go	0.00%	7 Missing ⚠️
cmd/utils.go	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[scttfrdmn]

CI caught me widening this too far — narrowed, assertion kept

Pushed 5b966e8. test/e2e/tier0_output_matrix_test.go:58 asserts spawn terminate i-doesnotexist -y exits non-zero, and my first version broke it by making every not-found resolution succeed.

The test was right, so I kept it and narrowed the fix rather than relaxing the assertion:

case	behaviour
unknown name	exit 0 — the case reported here
already terminated	exit 0
already shutting down	exit 0
unknown instance ID	still a non-zero error

The distinction is principled rather than test-driven convenience. An instance ID is opaque and AWS-assigned — you don't guess or reuse one — so an ID resolving to nothing means the caller is referring to something that never existed in this account, overwhelmingly a typo. Exiting 0 there would let a user or script believe they stopped a still-billing instance, and cost control is this project's existential concern.

A name is the opposite: a handle the caller chose, whose absence after cleanup is the expected steady state. Your reported case used a name (nf-03461ad6fc36), so this still fixes it.

ErrInstanceNotFound is therefore set only on the name path. TestUnknownInstanceIDStillFails pins both halves and was confirmed to fail in both directions — making the ID path a notFoundError, and making the name path a plain error.

Full e2e tier0 now passes locally (125s) as well as the unit suite.

Also fixed a bug in my own new test: it located the return statement with strings.LastIndex and panicked on -1. It scans statements now instead of doing index arithmetic.