Skip to content

Clean up ECS task definitions even when the error-check lookup fails - #78

Merged
rnathuji merged 1 commit into
mainfrom
SHRMP-340-cleanup-on-lookup-failure
Oct 14, 2026
Merged

rnathuji merged 1 commit into
mainfrom
SHRMP-340-cleanup-on-lookup-failure

Conversation

@rnathuji

@rnathuji rnathuji commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Jira: SHRMP-340. Follow-up to #76 (SHRMP-339).

What and why

checkForAWSErroredJobs() only garbage-collected orphan ECS task definitions on its success paths. If getAllTasksWithJobId or describeECSTasks rejected, the function threw before cleanup ran, and orphan task definitions were left behind for that poll cycle.

The lookup and per-task reporting now live in a private reportErroredTasks() helper. checkForAWSErroredJobs() runs cleanup on both paths:

  • Lookup fails: cleanup still runs, best-effort. Any cleanup error is logged with console.error, and the original error is rethrown so createResilientCycle logs the real cause.
  • Lookup succeeds: unchanged. Cleanup runs once, and a cleanup failure still propagates as before.

Missing env vars (ECS_CLUSTER, MANAGEMENT_APP_*) still fail before any AWS calls, and no cleanup runs. Cleanup needs the management app tag, so it can't run without them.

Reviewer focus

  • Catch-and-rethrow rather than finally. A plain finally { await cleanupTaskDefs(...) } would let a cleanup error replace the lookup error, which would hide the root cause. The .catch keeps cleanup best-effort on the failure path only.
  • The early return for "no tasks" no longer calls cleanup itself. Cleanup now happens once in the caller.

Testing

  • pnpm run checks passes (typecheck, Prettier, ESLint).
  • pnpm test passes: 162 tests, 100% coverage, including src/lib/aws-check-jobs.ts.
  • New tests in aws-check-jobs.test.ts:
    • getAllTasksWithJobId rejects: same error rethrown, cleanup runs, describeECSTasks not called
    • describeECSTasks rejects: same error rethrown, cleanup runs
    • lookup and cleanup both reject: the original lookup error wins, and the cleanup error is logged
    • lookup succeeds and cleanup rejects: the cleanup error propagates (unchanged behaviour)
    • a required env var is missing: rejects with no lookup and no cleanup calls
    • the early-return test now asserts cleanup runs exactly once
  • Not run locally: the Trivy and SonarQube CI steps. They will run on this PR.

No merge preconditions or manual steps.

🤖 Generated with Claude Code

checkForAWSErroredJobs() only garbage-collected orphan task definitions on
its success paths, so a rejection from getAllTasksWithJobId or
describeECSTasks skipped cleanup for that poll cycle. The lookup and
per-task reporting now live in a reportErroredTasks() helper, and cleanup
runs on both the success and failure paths.

On failure we catch, run cleanup best-effort (logging any cleanup error
with console.error), and rethrow the original error. A plain `finally`
would let a cleanup error replace the lookup error, hiding the real cause
from createResilientCycle. On the success path a cleanup failure still
propagates, as before. The env-var checks stay outside the try/catch,
since cleanup needs the management app tag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rnathuji
rnathuji requested a review from a team October 13, 2026 17:57

@chrisbendel chrisbendel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - checked that running cleanup on the failure path can't delete anything extra, since orphan status only depends on the BMA ready list and not on the report step.

@rnathuji
rnathuji merged commit dead267 into main Oct 14, 2026
5 checks passed
@rnathuji
rnathuji deleted the SHRMP-340-cleanup-on-lookup-failure branch October 14, 2026 15:41
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants