Repository navigation
fix(termination-watcher): look up the runner by exact name instead of paginating all runners - #5573
Conversation
… paginating all runners On every instance termination the watcher resolved the runner's GitHub registration by iterating every page of the owner's runner list and matching `name.includes(instanceId)`. The cost of that lookup grows with the number of registered runners (one request per 100), and on an ephemeral fleet the registration is normally already gone when the termination event arrives, so the common outcome is a full scan that finds nothing. The runner modules tag every instance with `ghr:runner_name_prefix`, and runners are registered as `<prefix><instance-id>`. Use that to call the list-runners API with its `name` filter, which returns the single matching runner (or none) in one request. Instances without the tag keep the previous paginated scan as a fallback. The behaviour is otherwise unchanged: a found runner is deleted, a busy runner (422) is retried through SQS, and a missing runner is logged.
There was a problem hiding this comment.
🟡 Changes recommended
The deregistration lookup can use an overridden tag prefix that differs from the JIT registration prefix, causing it to miss runners and skip deregistration.
1 open issue
What changed in this PR
This PR reduces termination-watcher API usage by replacing full runner-list scans with exact-name lookups when the instance has a runner-prefix tag, addressing item 1 of #5572.
Changes:
- Adds name-filtered lookups for organization and repository runners, retaining the scan fallback when the prefix tag is absent.
- Adds four tests covering successful lookups, missing runners, and non-exact responses.
- Documents lookup costs and the registration janitor’s reconciliation role.
| File | Description |
|---|---|
| modules/termination-watcher/README.md | Documents lookup behavior, API costs, and reconciliation. |
| lambdas/functions/termination-watcher/src/deregister.ts | Adds tag-derived exact-name lookup with a legacy scan fallback. |
| lambdas/functions/termination-watcher/src/deregister.test.ts | Tests exact-name lookup and response validation. |
Review effort: Balanced
| if (runnerNamePrefix !== undefined) { | ||
| runner = await findRunnerByName(installationOctokit, owner, `${runnerNamePrefix}${instanceId}`, runnerType); |
Brend-Smits
left a comment
Brend-Smits
left a comment
There was a problem hiding this comment.
I like this fix! However, CoPilot is right on this one, can you take another look at this please?
guicaulada
left a comment
guicaulada
left a comment
There was a problem hiding this comment.
Could we apply the bounded lookup to handleDeregisterRetry() as well? It still calls findRunnerByInstanceId() and scans the entire runner list. We confirmed production retry failures are 403 rate-limit errors from the same exhausted installation used by the initial watcher. Carrying the resolved runner ID (or a verified exact name) in DeregisterRetryMessage would let retries avoid another org-wide scan, while retaining compatibility for older queued messages. A regression test covering the SQS retry path would help ensure both paths benefit from this fix.
Description
findRunnerByInstanceId()in the termination watcher iterated every page of the owner's runner list (per_page: 100) and matchedname.includes(instanceId). On ephemeral fleets the registration is usually already gone when theshutting-downevent arrives, so the common outcome was a full scan that found nothing, at a cost of one request per 100 registered runners, every termination, charged to the installation's shared primary rate limit. Details and measurements in #5572.This PR resolves the runner by its exact name instead. The runner modules tag every instance with
ghr:runner_name_prefix(modules/runners/main.tf), and runners register as<prefix><instance-id>, so the watcher can call the list-runners API with itsnamefilter and get the single matching runner (or none) in one request, independent of fleet size.findRunnerByName()used when the tag is present;findRunnerByInstanceId()is kept as the fallback for instances without the tag (logged at debug).r.name === runnerName) rather than trusting the filter alone.modules/termination-watcher/README.md: short paragraph on how the lookup works and its cost, and a pointer toregistration-janitoras the reconciliation path.No Terraform changes; no change to inputs or behaviour for callers.
Test Plan
deregister.test.tscovering the exact-name path: org runner found and deleted with{ org, name, per_page: 1 }and nopaginate.iteratorcall; repo runner likewise; not-found makes exactly one request and no scan; a non-exact name in the response is ignored. Existing 16 deregister tests (fixtures without the prefix tag) now exercise the fallback path unchanged.yarn nx run-many --target=lint,test --projects=termination-watcher: lint clean, 9 test files / all tests passing (deregister.test.ts20 tests).yarn nx run @aws-github-runner/termination-watcher:build: builds.namequery parameter is typed onactions/list-self-hosted-runners-for-organd-for-repoin the pinned@octokit/openapi-types.Related Issues
Fixes #5572 (item 1). Items 2 and 3 in that issue (default for
enable_runner_deregistrationinmulti-runner, metric emission from a request hook) are left for follow-up discussion.