You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(scale-down): clean EC2 runners incrementally and isolate failures - #5463
Scale-down waits for the complete EC2 inventory before removing active runners. With large fleets, listing can fail or time out before any cleanup happens. Orphan cleanup can also terminate a runner whose registration ID tag is missing without checking GitHub, or stop processing other runners after one failure.
Process EC2 inventory in bounded pages of up to 100 instances and clean each page before requesting the next. Look up registrations by valid GitHub runner ID or exact full runner name instead of listing the entire organization/repository. Verify orphan registrations, remove false-positive orphan markers, preserve unverifiable runners, and isolate failures per runner. Existing busy checks, bypass-removal, boot grace, and idle detection remain in place.
Cleanup is stateless. Every invocation starts from current inventory; successfully terminated instances are no longer listed on subsequent runs. Pagination tokens exist only within the invocation. A deadline guard stops starting new work with ten seconds remaining, and a later listing failure retains earlier cleanup. Busy, retained, or failed items can be checked again on the next invocation.
The idle allowance is shared across pages within an invocation and recalculated on the next run. Eviction ordering applies within each page; strict global oldest/newest ordering requires collecting the whole fleet. Repeated scheduled scans reconcile pagination shifts as instances disappear. Custom compute providers without the optional paging capability retain their existing list-based path, including a complete owner-listing fallback when GitHub identity fields are absent. Only successful complete listings are cached.
Both Terraform scale-down paths supply the configured runner-name prefix. EC2 trusts an instance prefix tag only when it matches that configuration. Missing or mismatched tags without a valid GitHub ID cause cleanup to retain the instance; affected instances need their tags or registration IDs corrected before automatic cleanup. An exact-name miss without an ID also retains the instance because a custom script may have registered a different name. Unverifiable identity is reported at error level before unnecessary authentication when possible.
Validation: 395 control-plane tests, 312 compute-provider tests, and seven mocked Terraform tests pass, covering cleanup before later-page failure, fresh scans of reduced inventory, cross-page idle allowance, deadline handling, direct GitHub ID lookup, and isolated failures. Runtime TypeScript, ESLint, Prettier, Lambda bundle build, and diff checks pass. The compute-provider package's separate TypeScript check has existing Vitest configuration typing errors, reproduced with unchanged compute-provider code. No AWS deployment or live termination was performed.
Operational logs report per-page counts and final scan totals, successful termination counts, scan completeness, deadline exits, and orphan termination. These observations replace the old full-fleet before/after count on the EC2 path; dashboard migration and the additional per-instance GitHub API cost are documented. EC2 paging honors request cancellation before sending a request.
✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.
Scanned Files
None
guicaulada
changed the title
fix(scale-down): verify untagged orphans and isolate cleanup failures
fix(scale-down): process EC2 pages incrementally and resume cleanup
Sep 23, 2026
guicaulada
changed the title
fix(scale-down): process EC2 pages incrementally and resume cleanup
fix(scale-down): clean EC2 runners incrementally and isolate failures
Sep 23, 2026
The reason will be displayed to describe this comment to others. Learn more.
Tested on a sandbox deployment (multi-runner example, non-ephemeral Linux config). Normal scale-down worked. Removing the registration-ID tag exercised the exact-name lookup, which de-registered and terminated correctly. Setting a mismatched prefix tag kept the instance with a warning, as described. A few observations:
Fleet-count logs are gone on the EC2 path. Found: 'N' active EC2 runners before/after clean-up. is now only logged on the legacy list path, so EC2 deployments no longer emit it (confirmed after deploy). Anyone with log-based dashboards or alerts on this line will lose them. Could we log a total at the end of the paged run (or per page)?
Silent exits. When the time limit is hit, the run returns without any log. On the name-lookup path, an orphan is terminated without an info-level log, because the old Terminating orphan runner message was removed. An info log for each would help operators.
GitHub API volume. Before, there was one paginated list per owner, cached for the run. Now there is one call per instance, before any busy check. On large fleets with a 1–5 minute schedule this uses the app's rate limit much faster. It may be worth mentioning in docs/rate-limits-and-tuning.md.
MaxResults: 10 means about 100 DescribeInstances calls per run for 1,000 instances. These share EC2's throttling limit with scale-up and pool. Would a larger page (around 100) or a setting be better?
Exact-name lookup is stricter than the old endsWith(instanceId) match. A runner registered under a custom name (custom start script) that also has no ID tag is now not found. It gets marked orphan and then terminated, and the orphan path does no busy check. Please document the naming requirement, or avoid treating "not found by name" as proof of absence.
Kept instances only produce a warning. Instances that can't be verified stay up indefinitely with a warning every run. An error-level log or a metric would make this cost leak visible. Also, the GitHub client and installation token are created before the identity check that throws; checking first saves the calls.
Nit: listPage skips the runWithRequestSignal pre-check that the other EC2 calls use.
The SSM housekeeper accumulates every parameter page before deleting
anything. A later listing failure prevents all cleanup, and an omitted
`Parameters` array on the first page silently discards later candidates.
Delete eligible parameters as each page arrives, including pages after
empty responses. Isolate deletion failures per parameter, preserve the
existing age and dry-run protections, and stop starting new work with
ten seconds remaining. A later listing failure leaves earlier deletions
completed.
Cleanup is stateless: every invocation starts a fresh scan of current
parameters. Successfully deleted parameters disappear from subsequent
listings, leaving the remaining work for later scheduled runs.
Pagination tokens exist only within an invocation.
Validation: all 115 storage-provider tests pass, including empty pages,
deletion before later-page failure, fresh scans after partial cleanup,
and continuing past a failed deletion. Runtime TypeScript, ESLint,
Prettier, Lambda bundle build, and diff checks pass. No AWS deployment
or live parameter deletion was performed.
Independent of native SSM expiration-policy work in #5265. Companion
PRs: #5463 and #5465.
Thanks for testing this in a sandbox and writing up the observations. I addressed all seven in f3c63c5 and rebased onto current main:
Added info-level per-page inventory counts and a final scan summary with pages, scanned runners, successful terminations, completeness, and deadline status. These are scan observations rather than an atomic fleet census; the docs explain how to migrate dashboards from the old before/after lines. A later listing failure still emits the partial summary.
Added info logs for deadline exits and orphan termination.
Documented the per-instance GitHub API cost and scheduling tradeoff in docs/rate-limits-and-tuning.md, including an example of 1,000 runners on a five-minute schedule.
Increased the EC2 inventory page size from 10 to 100 while retaining per-runner processing and deadline checks.
Changed the safety behavior: an exact-name miss without a registration ID no longer establishes absence on the paged path. The instance is retained even if already marked orphaned. This protects custom-name runners, including busy registrations we cannot identify. The naming/ID requirement and remediation are documented; this deliberately also retains genuinely absent registrations when their identity cannot be proved safely.
Unverifiable identities now emit an error-level UNVERIFIABLE_RUNNER code suitable for alerting. Missing identity is checked before creating GitHub authentication/clients.
Wrapped listPage in runWithRequestSignal and added a regression proving an already-aborted request never sends DescribeInstances.
The deadline guard runs only before each runner. If GitHub verification starts with 11 seconds remaining and returns with 9, active cleanup still enters removeRunner, while orphan cleanup still starts termination or tag removal at lines 455–462. Further awaits inside removeRunner can also exhaust the reserve before deregistration or EC2 termination starts, risking a timeout with cleanup only partly completed. Recheck the budget after verification in both paths, and pass remainingTime into removal so subsequent requests and mutations also respect the guard. Add regressions where identity and busy-state lookups cross the threshold; a check only after the initial lookup does not cover the whole sequence.
Addressed the latest Copilot review summary finding. Scale-down now rechecks its deadline after identity verification in both active and orphan paths, and between client creation, busy-state checks, idle confirmation, identity persistence, deregistration, and termination.
An orphan found by name is treated as healthy merely because its registration exists. If it is offline and busy, this removes the orphan marker, although the ID-based path treats that same state as orphaned. Later active sweeps retain it because it remains busy, leaving the instance running. Recheck the matched registration IDs using the existing last-chance check, and unmark only if a matching registration is healthy. Add a regression for an offline, busy orphan without an ID tag.
Capture eviction strategy once across paginated scans
Each page calls evaluateAndRemoveRunners, which recalculates getEvictionStrategy. The cron predicate in scale-down-config.ts:16–22 matches only within five seconds of the next occurrence, so a scan can start with newest_first and switch to the default oldest_first on later pages. This changes which runners are retained or removed within those pages, independently of the documented loss of global ordering. Capture the strategy once per invocation alongside the shared idle allowance and reuse it across pages. Add a regression where page processing crosses the cron window.
Addressed both additional findings from the Copilot review summary in f45ff75: name-matched orphans now receive a fresh registration health check before being unmarked, and paginated scans capture the eviction strategy once per invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brend-Smits
left a comment
Scale-down waits for the complete EC2 inventory before removing active runners. With large fleets, listing can fail or time out before any cleanup happens. Orphan cleanup can also terminate a runner whose registration ID tag is missing without checking GitHub, or stop processing other runners after one failure.
Process EC2 inventory in bounded pages of up to 100 instances and clean each page before requesting the next. Look up registrations by valid GitHub runner ID or exact full runner name instead of listing the entire organization/repository. Verify orphan registrations, remove false-positive orphan markers, preserve unverifiable runners, and isolate failures per runner. Existing busy checks, bypass-removal, boot grace, and idle detection remain in place.
Cleanup is stateless. Every invocation starts from current inventory; successfully terminated instances are no longer listed on subsequent runs. Pagination tokens exist only within the invocation. A deadline guard stops starting new work with ten seconds remaining, and a later listing failure retains earlier cleanup. Busy, retained, or failed items can be checked again on the next invocation.
The idle allowance is shared across pages within an invocation and recalculated on the next run. Eviction ordering applies within each page; strict global oldest/newest ordering requires collecting the whole fleet. Repeated scheduled scans reconcile pagination shifts as instances disappear. Custom compute providers without the optional paging capability retain their existing list-based path, including a complete owner-listing fallback when GitHub identity fields are absent. Only successful complete listings are cached.
Both Terraform scale-down paths supply the configured runner-name prefix. EC2 trusts an instance prefix tag only when it matches that configuration. Missing or mismatched tags without a valid GitHub ID cause cleanup to retain the instance; affected instances need their tags or registration IDs corrected before automatic cleanup. An exact-name miss without an ID also retains the instance because a custom script may have registered a different name. Unverifiable identity is reported at error level before unnecessary authentication when possible.
Validation: 395 control-plane tests, 312 compute-provider tests, and seven mocked Terraform tests pass, covering cleanup before later-page failure, fresh scans of reduced inventory, cross-page idle allowance, deadline handling, direct GitHub ID lookup, and isolated failures. Runtime TypeScript, ESLint, Prettier, Lambda bundle build, and diff checks pass. The compute-provider package's separate TypeScript check has existing Vitest configuration typing errors, reproduced with unchanged compute-provider code. No AWS deployment or live termination was performed.
Independent companion PRs: #5464 (SSM cleanup) and #5465 (opt-in registration reconciliation).
Operational logs report per-page counts and final scan totals, successful termination counts, scan completeness, deadline exits, and orphan termination. These observations replace the old full-fleet before/after count on the EC2 path; dashboard migration and the additional per-instance GitHub API cost are documented. EC2 paging honors request cancellation before sending a request.