feat: paginate ECS listings and close prune deletion safety gaps - #547
feat: paginate ECS listings and close prune deletion safety gaps#547mhmdio wants to merge 8 commits into
Conversation
`prune` could delete task definition revisions that were serving live traffic, and several ECS listings were silently truncated. This fixes those, plus a CI gate that could never fail. Critical * Paginate ListServices. It returns only 10 items when maxResults is omitted, and the code never paginated. Consequences: prune's in-use protection covered only the first 10 services in a cluster, so a revision used by service #11+ was offered for deletion and deleted with no error; rollback could not reach service #11+; and cluster service counts were wrong. DescribeServices is separately capped at 10 per call, so it is now batched. * Fail closed when the in-use lookup fails. It previously warned and continued with an empty in-use set, presenting revisions that were in use as safe to delete. * Enforce the keep-latest-5 promise in every selection option. The four age-based bulk options filtered on isProtected only, so a family whose last deploy was months ago offered revisions 2-5 -- exactly the ones a rollback would target. The bucket maths is now one pure function, computeDeletionBuckets, and every option derives from it. Correctness and cost * Paginate ListClusters, ListTasks, ListContainerInstances and ListTaskDefinitions; batch DescribeTasks at its 100-task cap. Cluster counts and the rollback revision list are no longer truncated at the first page. * Replace the hand-rolled family scan with ListTaskDefinitionFamilies. The previous version listed every task definition in the account, then issued up to 100 describeTaskDefinition calls per family; counts are now exact rather than extrapolated from a biased sample of the newest 100 revisions, with bounded concurrency to avoid throttling. * Pass the credential provider to the ECS client instead of pre-resolved credentials, so the SDK can refresh them. A long prune could outlive a short-lived SSO session. * Honour AWS_PROFILE, AWS_REGION, AWS_DEFAULT_REGION, AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE. * Skip [sso-session ...] and [services ...] sections when parsing ~/.aws/config; they were offered as selectable profiles. The `profile ` strip is now anchored. * Report containers added or removed between revisions in the rollback diff, not just images that changed in place. Image tag parsing no longer reads a registry port (registry:5000/app) as the tag, and understands digest pins. * Re-sync the profile cache when it is empty instead of wedging on "Invalid AWS profile" for a full sync interval. * Raise a clear error when a task reports no containers, rather than rendering an empty prompt. CLI * Add --version. It was never registered. * Add --command to pick the shell, replacing the hardcoded /bin/sh. * Print the banner only to a TTY, so `config show | jq` works and it no longer precedes --help output. * `doctor` exits non-zero when a check fails, and no longer dumps "Command failed: ..." before its own friendly message. CI, packaging and tests * `npm run format -- --check` expanded to `prettier --write ... --check`, which rewrites files and exits 0 -- the gate could never fail. Added a real format:check script and applied the formatting drift that had accumulated behind it. * release.yml: npm ci instead of npm install, format:check instead of a rewrite, npm cache, and a concurrency group. * test.yml: drop unused pull-requests:write, add a concurrency group. * Move npm-check-updates to devDependencies; it was shipping to every install. Drop inquirer (second prompt library, now unused) and the unwired eslint prettier plugins. Remove .npmignore, superseded by the files field. * Replace the old test, which exercised the conf package rather than this project, with 29 tests over the prune selection logic and the AWS helpers. Module-level side effects are now behind an entrypoint guard so the file can be imported. Note: a large part of the index.js diff is the accumulated Prettier formatting the broken CI gate had been hiding.
…ages npm install on macOS drops optional @emnapi/* and typescript entries that Linux runners resolve, which breaks npm ci. Regenerated with --package-lock-only so the tree stays platform-independent.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dcd0d816c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function cancelOperation() { | ||
| cancelOperation(); |
There was a problem hiding this comment.
When a prompt wired to cancelOperation is cancelled (for example, by pressing Ctrl+C during configuration or prune selection), this function calls itself indefinitely and eventually throws RangeError: Maximum call stack size exceeded; the surrounding command then reports a failure and exits non-zero instead of cancelling cleanly. Restore the intended log-and-exit behavior rather than recursively invoking the handler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ec62198 — the handler called itself instead of logging and exiting, so every cancellation path threw RangeError: Maximum call stack size exceeded and exited non-zero. Restored the log-and-exit body and added a regression test that fails against the recursive version.
| /** | ||
| * Shared `prompts` onCancel handler: report and exit without a stack trace. | ||
| */ | ||
| function cancelOperation() { |
There was a problem hiding this comment.
is this intentional? seems not to match the comment
There was a problem hiding this comment.
Good catch — not intentional, it was a real bug. Fixed in ec62198.
The body called itself instead of logging and exiting, so every cancellation path (Ctrl+C at any prompt, config set, config cleanup, all the prune selection prompts, both prune confirmations) blew the stack with RangeError: Maximum call stack size exceeded. The surrounding command then reported a failure and exited non-zero instead of cancelling cleanly.
Cause: I consolidated the repeated
logger.info(chalk.dim("Operation cancelled"));
process.exit(0);blocks into this helper with a bulk find-and-replace. The pattern it matched was also the helper own body, which I had just written with those exact two statements — so it replaced itself. I checked the call sites and not the definition, and lint cannot see it since the code is valid.
Added a test that stubs process.exit and asserts no recursion; verified it fails with Maximum call stack size exceeded against the old version. Also confirmed end to end: Ctrl+C at config set now prints "Operation cancelled" and exits 0 with stored config untouched.
The entrypoint guard compared process.argv[1] to import.meta.url directly. npm installs the bin as a symlink (bin/taskonaut -> lib/node_modules/@schematichq/taskonaut/index.js) and Node resolves symlinks when building import.meta.url, so argv[1] was the link path and the target never matched. isMainModule() returned false for every global install, program.parse() was skipped, and the CLI exited 0 having done nothing. Resolve argv[1] with realpathSync before comparing. The guard exists so the module can be imported by tests without executing; added a test that runs the CLI directly, through a real symlink, and as an import, so the symlink path cannot regress unnoticed. Verified it fails against the unfixed guard.
cancelOperation was the shared prompts onCancel handler, but its body called itself instead of logging and exiting. Every cancellation path -- Ctrl+C at any prompt, config set, config cleanup, prune family/method/ range/manual selection, the cluster usage check, and both prune confirmations -- blew the stack with RangeError: Maximum call stack size exceeded, so the surrounding command reported a failure and exited non-zero instead of cancelling cleanly. Introduced by the bulk edit that consolidated the repeated log-and-exit blocks into this helper: the pattern it matched was also the helper's own body, which had just been written with those exact two statements. Restores the log-and-exit body and adds a test that fails with RangeError against the recursive version. Verified end to end: Ctrl+C at `config set` now prints "Operation cancelled" and exits 0, leaving stored config untouched.
Resolves package.json / package-lock.json conflicts with six dependency bumps that landed on main. Resolution: * inquirer: dropped. main bumped it 14.0.2 -> 14.1.0, but this branch removes the package entirely in favour of a single prompt library. * npm-check-updates: kept in devDependencies only, as on this branch. * eslint-config-prettier / eslint-plugin-prettier: stay removed; they were never wired into eslint.config.js. * Took main's bumps for everything this branch still ships: @aws-sdk/client-ecs 3.1113.0, @aws-sdk/credential-providers 3.1114.0, conventional-changelog-conventionalcommits 10.4.0. Lockfile regenerated from main's with --package-lock-only so optional platform packages are not pruned. Re-verified against the bumped SDK that the limits this branch relies on still hold: DescribeServices caps at 10 services per call, and ListServices returns up to 10 results when maxResults is omitted.
Four review findings, all reproduced before fixing. INACTIVE revisions were invisible to the analysis (P1) ListTaskDefinitions lists only ACTIVE revisions when status is omitted, and analyzeTaskDefinitionRevisions omitted it. Inactive-only families reported "No revisions found", mixed families dropped every INACTIVE revision, and the recommended "All INACTIVE revisions beyond latest 5" bucket was therefore always empty -- while the family picker advertised exact inactive counts, so the two directly contradicted each other. Both statuses are now fetched and merged newest-first. The latest-5 keep window spans both statuses, so it means the newest 5 overall. Verified against prod: schematic-api-prod-orbit now analyses as 124 revisions (92 ACTIVE, 32 INACTIVE), matching `aws ecs list-task-definitions --status ACTIVE|INACTIVE` exactly. The 32 INACTIVE revisions were previously invisible. Live revisions outside the primary deployment were unprotected (P1) listServices kept only service.taskDefinition, which names the PRIMARY deployment. During a rolling deployment the previous revision is still running under an ACTIVE deployment, and CodeDeploy / EXTERNAL deployments place revisions on task sets. Standalone tasks from run-task or a scheduled task reference revisions with no service at all. All of these could be offered for deletion while live -- and the risk window is a deploy, exactly when a rollback target matters most. Protection now covers every deployment, every task set, and running standalone tasks, via collectInUseTaskDefinitions. Stale profile cache rejected valid profiles (P2) The cache is reused for an hour, so a profile added since the last sync -- or selected through AWS_PROFILE / a changed AWS_CONFIG_FILE -- was rejected with "Invalid AWS profile" before fromIni was attempted. doctor read the raw cache with no sync at all, so it failed on a fresh install; since doctor now exits non-zero, that broke using it to gate setup. Both paths re-sync once on a miss. config show was not machine-readable (P2) Suppressing the banner was not sufficient: the command still wrote headings, the path and "Values:" to stdout, so `config show | jq` failed with "Invalid numeric literal". Added --json, which writes only JSON. Default human output is unchanged. Tests 56 total, up from 33. Each fix has a test that fails when the fix is reverted -- verified by reverting all four at once, which fails 11 tests across all four areas. Coverage is at the call sites, not only the pure helpers, since that is where both P1 bugs lived.
|
All four findings fixed in 0b306ce. Every one reproduced before fixing — the review was accurate on all counts, including the two line-level P1s. 1. INACTIVE revisions omitted (P1) — confirmed via the SDK docs: "By default, only Verified against prod: 2. Live revisions unprotected (P1) — 3. Stale cache / doctor (P2) — both halves reproduced. A profile added after the last sync was rejected with 4. Tests: 33 → 56. Each fix has a test that fails when the fix is reverted — verified by reverting all four at once, which fails 11 tests across all four areas. Coverage is at the call sites rather than only the pure helpers, since that is where both P1 bugs actually lived. That includes a test that runs Two notes for the record: #1 and #2 were pre-existing rather than regressions, but #1 made the PR internally inconsistent and #2 undercut its stated purpose, so both belong here. #3 and #4 were genuinely introduced or worsened by this PR. CI green on 0b306ce, |
paginate() looped with `do { ... } while (nextToken)`. nextToken is
undefined before the first request, so when the very first page was
throttled the retry ran, hit the `while`, and exited -- returning an
empty array instead of the caller's data. A rate-limited account saw
"no revisions found" rather than an error, and the prune analysis
treated the family as empty. The loop now runs on an explicit flag,
resets the retry budget after each successful page, and re-requests the
same page rather than skipping it.
The per-call throttling checks are consolidated into isThrottlingError,
and fetchAllTaskDefinitions is now a thin wrapper over paginate rather
than a second paginator that had none of this handling.
analyzeTaskDefinitionRevisions took `latest` from revisions[0], the
first revision it could successfully describe. A failed describe on the
newest revision silently promoted the second-newest, so the "latest is
always protected" guarantee applied to the wrong revision. It now comes
from the listing. The analysis also returns `skipped` and `listed`, and
both the analysis and the prune summary say when revisions could not be
read instead of quietly reporting smaller counts.
Also:
- listTaskDefinitionFamilies records families it cannot read and reports
them after the spinner stops, instead of writing over a live spinner,
and no longer spreads a whole revision list into Math.max.
- The prune warning for a skipped usage check said only the latest
revision would be protected; the latest 5 are kept regardless, and the
real risk is that in-use revisions go undetected.
- README no longer claims revisions inside the latest 5 can be manually
selected, which contradicted the rest of the document.
Tests: 208 across 15 suites, 83% statements / 84% lines, enforced by a
coverage threshold in CI. New suites cover pagination and throttling,
cluster/service/task listing and batching, task definition families,
the deregister and delete phases, the interactive pickers, exec session
argv and exit codes, and the prune and rollback commands end to end --
including the type-to-confirm gate, the final confirmation, and the
fail-closed behaviour when the in-use lookup errors. The suite uses
test doubles only and needs no AWS credentials.
…ests The subprocess tests redirected HOME, but on Linux the stored configuration lives under XDG_CONFIG_HOME when it is set, so they read and rewrote the config of whoever ran them -- and the assertion that the config stays inside the isolated home failed on CI for that reason. Both HOME and XDG_CONFIG_HOME are now redirected, which also makes the profile-cache tests independent of each other and of the machine.
2 participants
Summary
prunecould delete task definition revisions that were serving live traffic. This PR fixes that and the surrounding class of bugs — silently truncated ECS listings — plus a CI gate that could never fail.Found during a full review of the repo. Every AWS API limit cited below was verified against the vendored
@aws-sdk/client-ecstype docs, not from memory.Critical
1.
ListServicesreturns 10 items by default and was never paginatedindex.jscalledecs.listServices({ cluster })with nomaxResultsand nonextTokenloop. UnlikeListClusters(default 100), the SDK docs are explicit:Three consequences, worst first:
prunesilently lost in-use protection.analyzeTaskDefinitionRevisionsbuilds itsinUseRevisionsset from this call. On a cluster with more than 10 services, revisions used by service Bump @aws-sdk/credential-providers from 3.715.0 to 3.726.0 #11+ were never markedisInUse, soisProtectedstayed false, so they were offered for deletion and deleted. No error, no warning — and the user was shown a confident "Protected revisions (cannot be deleted)" list that was simply incomplete.rollbackcould not reach service Bump @aws-sdk/credential-providers from 3.715.0 to 3.726.0 #11+. The picker only ever listed the first 10.Services: 10for a cluster with 40.DescribeServicesis separately capped ("You may specify up to 10 services to describe in a single operation"). The old code passed the whole array and only got away with it because the list was already truncated to 10. It is now batched.2. The in-use lookup failed open
Any failure — throttling, permissions, the batch limit above — left
inUseRevisionsempty and pruning continued. For a destructive operation the safety check now aborts with an explicit message rather than warning and proceeding.3. Age-based bulk options ignored the keep-latest-5 promise
The command announces "keeping latest 5 and protecting in-use revisions", but the four age options filtered on
isProtectedonly, which is justisLatest || isInUse. On a family whose last deploy was months ago, "older than 30 days" selected revisions 2–5 — exactly the ones you would roll back to.The bucket maths is now one pure function,
computeDeletionBuckets, and every option derives from a singledeletablelist. No option can reach past the promise the command prints.4.
ListTaskDefinitionsomits INACTIVE revisions unless you askstatuswas never set, and the API returns only ACTIVE revisions by default. So the pruning analysis could not see INACTIVE revisions at all: the "All INACTIVE revisions" bulk option — the one the command recommends — was always empty, and a family with only INACTIVE revisions reported "No revisions found". Both statuses are now listed and merged in revision order, so the keep-latest-5 window spans them as a user would expect.5. A throttled first page returned an empty list instead of retrying
paginatelooped withdo { ... } while (nextToken).nextTokenisundefinedbefore the first request, so when the very first page was throttled the retry ran, hit thewhile, and exited — handing the caller an empty array. A rate-limited account saw "no revisions found" rather than an error, and the prune analysis treated the family as empty.The loop now runs on an explicit flag, resets its retry budget after each successful page, and re-requests the same page rather than skipping it. The per-call throttling checks are consolidated into one predicate, and
fetchAllTaskDefinitionsis a thin wrapper overpaginateinstead of a second paginator with none of this handling.Verified against a probe that throttles only the first ACTIVE page: before, 1 call / 0 revisions /
latest: 0; after, 2 calls / 1 revision /latest: 10.6. A failed
describecould move which revision counts as "latest"analyzeTaskDefinitionRevisionstooklatestfromrevisions[0]— the first revision it could successfully describe. A failed describe on the newest revision silently promoted the second-newest, so the "latest is always protected" guarantee applied to the wrong revision. It now comes from the listing.Describe failures are still skipped rather than fatal, but a destructive command must not quietly analyse fewer revisions than it listed: the analysis returns
skippedandlisted, and both the analysis and the prune summary now say so on screen.Correctness and cost
ListClusters,ListTasks,ListContainerInstances,ListTaskDefinitions.DescribeTasksbatched at its 100-task cap. Cluster counts and the rollback revision list are no longer truncated at the first page.listTaskDefinitionFamiliesreplaced with theListTaskDefinitionFamiliesAPI. The old version listed every task definition in the account, grouped them client-side, then issued up to 100describeTaskDefinitioncalls per family — roughly 5,000 API calls to render one picker for 50 families. It also extrapolated ACTIVE/INACTIVE counts from the newest 100 revisions, which are disproportionately ACTIVE, so the numbers were biased. Counts are now exact and need no describe calls (the revision number is in the ARN, status is the filter), with bounded concurrency to avoid throttling.await fromIni({ profile })()resolved once and handed the client a static object with no refresh path. A longpruneruns for minutes with deliberate backoff and could outlive a short-lived SSO session. The provider is now passed through so the SDK refreshes.AWS_PROFILE,AWS_REGION,AWS_DEFAULT_REGION,AWS_CONFIG_FILE,AWS_SHARED_CREDENTIALS_FILE.profile.replace("profile ", "")was unanchored and only handled that one prefix, so[sso-session my-sso]and[services ...]sections showed up as selectable profiles. Now anchored, with non-profile sections and stray top-level keys skipped.registry:5000/appas tag5000/app, and understands digest pins.Invalid AWS profilefor a full hour.listTaskDefinitionFamiliesrecords families it cannot read and reports them after the spinner stops, instead of writing over a live spinner, and no longer spreads a whole revision list intoMath.max.CLI
--version— it was never registered;taskonaut --versionerrored on a package published at 1.10.13.--commandto choose the shell, replacing the hardcoded/bin/sh.taskonaut config show | jqworks and it no longer precedes--help.doctorexits non-zero when a check fails, so it can gate a setup script. It also no longer dumpsCommand failed: session-manager-plugin --versionin front of its own friendly message.CI, packaging, tests
The formatting gate could never fail.
npm run format -- --checkexpanded toprettier --write '**/*.{js,json,md}' --check, which writes the files and exits 0 — verified locally.index.jsandREADME.mdonmainhad real drift (324 insertions / 150 deletions) behind a green check. Added a realformat:checkscript and applied the accumulated drift.release.yml:npm ciinstead ofnpm install(a publish job should build the tree the PR checks validated),format:checkinstead of a rewrite whose output was discarded, npm cache, concurrency group.test.yml: dropped unusedpull-requests: write, added a concurrency group.npm-check-updatesmoved todevDependencies— a maintenance tool was shipping to everynpm i -g. Droppedinquirer(second prompt library, now unused) and theeslint-plugin-prettier/eslint-config-prettierpair that was installed but never wired intoeslint.config.js. Removed.npmignore, superseded by thefilesfield. Fixed the ESLint global-ignores object.Tests: the old suite constructed its own
Confinstance and asserted thatconfstores and returns values — a test of the library, not this project, with the 2,629 lines of application code at ~0% coverage. It is replaced by 208 tests across 15 suites, 83% statements / 84% lines, enforced by a coverage threshold injest.config.jsthat CI now runs. Module-level side effects sit behind an entrypoint guard so the file can be imported, and the suite talks to test doubles only — no AWS credentials, no network.What the suite covers:
nextToken;maxResults: 100is requested; a throttled first page is retried; the same page is re-requested rather than skipped; the retry budget resets after a success; non-throttling errors are not retriedDescribeServices≤ 10 per call,DescribeTasks≤ 100,DeleteTaskDefinitions≤ 10; family detail fan-out stays within its concurrency boundUpdateServicecall; confirming updates to the chosen revision; the diff reports containers added and removedaws ecs execute-commandargv, exit-code propagation, the missing-CLI message, and that per-session signal handlers are removed--help, unknown option and argument handling,config show/--json/path,config set,config cleanup, anddoctor's exit status per failed checkVerification
Every fix in this PR was also verified by reverting it and confirming the suite goes red. Reverting the four most recent fixes produced 7 failures across 4 suites.
Notes for the reviewer
index.jsdiff is the Prettier formatting the broken CI gate had been hiding. Reviewing with?w=1or ignoring whitespace-only hunks helps.npm auditreports vulnerabilities inundiciundernpm-check-updates' bundled npm. Moving it todevDependenciestakes it out of the published package; the advisories themselves are for Dependabot.