Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.
Where
.github/workflows/destroy-fargate-dev.yml:63-66 - aws s3 rm ...tflock with || true, run before terraform init, on every invocation
.github/workflows/destroy-fargate-dev.yml:70-78 - the ECR delete loop; the query at :73 uses contains(repositoryName,'cudly-dev')
- Contrast:
.github/workflows/deploy-aws-fargate.yml:117-118 gates its equivalent behind the operator input clear_stale_lock (:40-44)
- Contrast:
.github/workflows/cleanup-staging.yml:85-91 adds an explicit case "$REPO" in cudly-staging*) allow-list
What
Two independent defects in one workflow.
-
Unconditional state-lock delete. The aws s3 rm at :65 runs on every invocation, before terraform init, with || true so it cannot fail. It is not gated behind an operator input the way the Fargate deploy workflow gates its equivalent. There is no concurrency: group on this workflow either, so nothing prevents two invocations overlapping.
-
Substring ECR matching followed by force-delete. The query is contains(repositoryName,'cudly-dev'), a substring match, and the loop then calls delete-repository --force. cleanup-staging.yml:85-91 added an explicit case allow-list for precisely this reason, with the in-file rationale calling out:
in particular the bare 'cudly' prod-adjacent repo
The dev workflow never received that guard.
Failure scenario
- The workflow is dispatched while another run (or a deploy) holds the
github-fargate-dev lock. The unconditional delete frees the lock out from under the run that is still applying, both proceed, and the state is corrupted by concurrent writes.
- Any ECR repository whose name merely contains
cudly-dev - for example backup-cudly-dev or cudly-dev-prod-mirror - is force-deleted along with every image it holds, in whatever account AWS_ROLE_TO_ASSUME points at. There is no confirmation step between the query and the delete.
Fix direction
-
Gate the lock delete behind an explicit clear_stale_lock-style workflow_dispatch input, mirroring deploy-aws-fargate.yml:40-44,117-118, and drop the || true so a failed delete is visible.
-
Add a concurrency: group keyed on the state key (not on github.ref - the state key is ref-independent).
-
Delete only the exact repository names this workflow owns. Enumerate them (the same names the dev Fargate deploy pushes to) and compare with ==.
Not starts_with('cudly-dev'), and not the case "$REPO" in cudly-dev*) allow-list cleanup-staging.yml uses: both still match cudly-dev-prod-mirror, which is the counterexample in the failure scenario above. A prefix guard reads as a fix for a substring bug while leaving the same class open, and copying the staging workflow's guard would spread that shape rather than contain it. An exact list is also shorter than either.
Remedy simplified (2026-08-03): starts_with plus a prefix allow-list was replaced with an exact-name list. The original remedy did not close its own failure scenario, and the repo has shipped substring-for-equality six times; an exact comparison is both smaller and the only one that holds.
Related
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.Where
.github/workflows/destroy-fargate-dev.yml:63-66-aws s3 rm ...tflockwith|| true, run beforeterraform init, on every invocation.github/workflows/destroy-fargate-dev.yml:70-78- the ECR delete loop; the query at:73usescontains(repositoryName,'cudly-dev').github/workflows/deploy-aws-fargate.yml:117-118gates its equivalent behind the operator inputclear_stale_lock(:40-44).github/workflows/cleanup-staging.yml:85-91adds an explicitcase "$REPO" in cudly-staging*)allow-listWhat
Two independent defects in one workflow.
Unconditional state-lock delete. The
aws s3 rmat:65runs on every invocation, beforeterraform init, with|| trueso it cannot fail. It is not gated behind an operator input the way the Fargate deploy workflow gates its equivalent. There is noconcurrency:group on this workflow either, so nothing prevents two invocations overlapping.Substring ECR matching followed by force-delete. The query is
contains(repositoryName,'cudly-dev'), a substring match, and the loop then callsdelete-repository --force.cleanup-staging.yml:85-91added an explicitcaseallow-list for precisely this reason, with the in-file rationale calling out:The dev workflow never received that guard.
Failure scenario
github-fargate-devlock. The unconditional delete frees the lock out from under the run that is still applying, both proceed, and the state is corrupted by concurrent writes.cudly-dev- for examplebackup-cudly-devorcudly-dev-prod-mirror- is force-deleted along with every image it holds, in whatever accountAWS_ROLE_TO_ASSUMEpoints at. There is no confirmation step between the query and the delete.Fix direction
Gate the lock delete behind an explicit
clear_stale_lock-styleworkflow_dispatchinput, mirroringdeploy-aws-fargate.yml:40-44,117-118, and drop the|| trueso a failed delete is visible.Add a
concurrency:group keyed on the state key (not ongithub.ref- the state key is ref-independent).Delete only the exact repository names this workflow owns. Enumerate them (the same names the dev Fargate deploy pushes to) and compare with
==.Not
starts_with('cudly-dev'), and not thecase "$REPO" in cudly-dev*)allow-listcleanup-staging.ymluses: both still matchcudly-dev-prod-mirror, which is the counterexample in the failure scenario above. A prefix guard reads as a fix for a substring bug while leaving the same class open, and copying the staging workflow's guard would spread that shape rather than contain it. An exact list is also shorter than either.Remedy simplified (2026-08-03):
starts_withplus a prefix allow-list was replaced with an exact-name list. The original remedy did not close its own failure scenario, and the repo has shipped substring-for-equality six times; an exact comparison is both smaller and the only one that holds.Related
deploy-aws-lambda.yml; fixing both together is one coherent change.cleanup-staging.ymlbut notdestroy-fargate-dev.yml.concurrency:group is also tracked as part of the broader no-concurrency issue from this review.