fix(scale-down): clean EC2 runners incrementally and isolate failures - #5463
guicaulada wants to merge 1 commit into
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
guicaulada
changed the title
a180a12 to
df635b1
Compare
guicaulada
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Runner-name derivation can target the wrong JIT registration, and existing custom providers can silently lose scale-down behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds incremental EC2 scale-down processing with bounded pagination, direct GitHub runner lookup, deadline handling, and per-runner failure isolation.
Changes:
- Processes EC2 runners page-by-page while sharing idle allowance.
- Verifies runners by GitHub ID or exact name before cleanup.
- Updates tests and scale-down documentation.
| File | Description |
|---|---|
modules/runners/scale-down-state-diagram.md |
Documents incremental cleanup flow. |
modules/orchestration-providers/webhook/scale-down-state-diagram.md |
Mirrors updated scale-down documentation. |
lambdas/libs/compute-providers/registry.test.ts |
Relaxes capability registry assertion. |
lambdas/libs/compute-providers/core/index.ts |
Adds runner names and optional paging. |
lambdas/libs/compute-providers/aws/ec2/src/runners.ts |
Implements bounded EC2 pages and runner names. |
lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts |
Tests paging and name extraction. |
lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.ts |
Exposes EC2 paging capability. |
lambdas/functions/control-plane/src/test/compute-provider-contracts/scale-down.ts |
Updates orphan cleanup contract. |
lambdas/functions/control-plane/src/scale-runners/scale-down.ts |
Implements incremental, isolated cleanup. |
lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts |
Covers paging, deadlines, and failures. |
lambdas/functions/control-plane/src/scale-runners/scale-down-contract.test.ts |
Adds GitHub authentication mocks. |
lambdas/functions/control-plane/src/lambda.ts |
Supplies Lambda remaining execution time. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Without either identity we cannot establish absence with a bounded query. | ||
| // Do not let a legacy/incomplete record force an organization-wide inventory | ||
| // before other instances can be cleaned up. | ||
| throw new Error(`Runner '${runner.id}' has no GitHub ID or complete runner name; skipping unverifiable cleanup`); |
There was a problem hiding this comment.
Fixed in 2663e6c. List-only providers without either GitHub identity field now retain the complete owner-listing fallback, cached per runner type and owner only after a successful listing. Paged providers continue using bounded identity lookups and retain unverifiable records. Added organization/repository compatibility regressions and documented the provider contract. All 391 control-plane tests pass.
| githubRunnerName: i.Tags?.some((tag) => tag.Key === 'ghr:runner_name_prefix') | ||
| ? `${i.Tags.find((tag) => tag.Key === 'ghr:runner_name_prefix')?.Value ?? ''}${i.InstanceId}` | ||
| : undefined, |
There was a problem hiding this comment.
Fixed in 2663e6c by supplying RUNNER_NAME_PREFIX to scale-down from the same Terraform configuration as scale-up, in both legacy and provider-based modules. EC2 derives a lookup name only when the instance tag matches that trusted configuration. A missing/mismatched tag without a valid GitHub ID remains unverifiable and cannot be marked orphaned or terminated; this also protects existing instances with overridden tags. Added prefix-mismatch/missing-configuration tests, repeated-sweep protection, and Terraform wiring assertions. All 311 provider tests and four Terraform tests pass. The provider documentation explains that affected old instances need their tags or GitHub registration IDs corrected before automatic cleanup.
df635b1 to
2663e6c
Compare
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 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.
Validation: 391 control-plane tests, 311 compute-provider tests, and four 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).