Skip to content

feat: add stateless registration cleanup with multi-App support - #5465

Merged
guicaulada merged 8 commits into
mainfrom
gc/feat/registration-janitor
Oct 27, 2026
Merged

guicaulada merged 8 commits into
mainfrom
gc/feat/registration-janitor

Conversation

@guicaulada

@guicaulada guicaulada commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Missed deregistration events can leave offline GitHub runners behind after their compute resources disappear. The termination watcher handles termination events and busy-runner retries, but cannot discover historical registrations or missed events. Scale-down also cannot discover this backlog because it starts from existing compute instances. This scheduled reconciliation complements both paths.

Add an opt-in modules/registration-janitor module deploying a separate scheduled Lambda from the existing termination-watcher archive with GitHub App/GHES authentication. Discovery processes each runner-group page immediately, verifies candidates individually, and queues work before fetching later pages. Candidate errors are isolated; unavailable pages are skipped, with advancement to another group after three consecutive page failures. Retained instances and failed checks do not consume the queued-candidate limit.

Scanning is stateless. Every invocation starts from current GitHub inventory; registrations deleted by earlier cleanup are no longer listed. Pagination is local to each invocation. A deadline guard stops starting new work with ten seconds remaining. Busy, retained, or failed registrations may be checked again, and subsequent scans reconcile shifting page boundaries.

Scope is an organization, explicit runner groups, an exact account-exclusive runner-name prefix, and every AWS region using that prefix. Any non-terminated EC2 state protects the registration. Offline, non-busy registrations with absent instances enter SQS for a second observation at least 15 minutes later. These messages carry candidate identity and observation time for deletion safety. Confirmation searches group membership page by page, then rechecks EC2 absence and current GitHub identity/offline/busy state before deletion. Incomplete or failed lookups never establish absence. Confirmation failures retry independently through SQS and its dead-letter queue; duplicate deletion treats 404 as success. Scope changes invalidate candidates, and candidates expire after 24 hours. Dry-run is the default and suppresses queueing and deletion.

The account-exclusive prefix and complete region list are required configuration because tags cannot prove ownership after an instance disappears. This implementation handles organization EC2 runners in one account and multiple regions. Repository runners and cross-account inventory remain outside its scope. Delayed observations reduce races but are not an atomic GitHub/EC2 transaction.

Additional GitHub Apps are supported through the existing credential manifest format. Discovery selects an App for each group page, and confirmation selects one per candidate. Selection prefers observed remaining quota, skips throttled Apps for one minute when another is available, and tries another configured App after authentication failure. App JWT and installation-token authentication use the same credential and resolve access for the requested owner. Terraform passes the optional manifest and grants read access to its configured credential parameters. Single-App configurations remain supported. Existing termination-watcher behavior is unchanged by this PR.

The GitHub cleanup flow uses a compute-provider interface for runner-name interpretation, backing-resource lookup, and inventory scope. EC2 is the first bundled provider; it performs per-candidate lookups across the configured regions. Providers must throw on incomplete or failed lookups rather than report absence. Provider type and normalized scope are included in candidate validation. Additional providers can be registered without changing GitHub discovery or confirmation; this Terraform module currently supplies EC2 configuration and permissions.

Validation: 109 termination-watcher package tests, 315 compute-provider tests, and four mocked Terraform tests pass. A non-EC2 test provider exercises discovery and confirmation without AWS inventory calls. Regressions cover fresh discovery after earlier deletion, queueing before later-page failure, independent failures, candidate-budget fairness, paged membership verification, deadline handling, paired App authentication, quota selection, authentication fallback, and manifest/IAM wiring. Runtime TypeScript, ESLint, Prettier, Terraform validation/formatting, generated documentation, Lambda bundle build, and diff checks pass. Optional package-wide coverage thresholds already fail in existing deregistration/retry code on unchanged main; no thresholds were lowered. TFLint is unavailable locally. No AWS deployment or live registration deletion was performed.

Independent companion PRs: #5463 (incremental scale-down) and #5464 (incremental SSM cleanup).

Compute-provider configuration and IAM are separated from shared cleanup permissions. Deployment uses one janitor per provider and ownership scope. Confirmation checks its time budget before authentication, membership pages, resource lookup, GitHub recheck, and deletion; unfinished records are retried independently.

Each invocation reuses installation clients per App, owner, runner type, and GitHub endpoint while re-evaluating quota/cooldown selection before cache lookup. Authentication failures are not cached, and client caches do not persist between invocations. Discovery emits a summary of scanned pages, queued candidates, retained resources, dry-run candidates, failures, and stop reason. Both handler paths set fresh request context.

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 1 package(s) with unknown licenses.
See the Details below.

License Issues

lambdas/functions/termination-watcher/package.json

PackageVersionLicenseIssue Type
@aws-github-runner/compute-providersNullUnknown License

OpenSSF Scorecard

PackageVersionScoreDetails
npm/@aws-github-runner/compute-providers UnknownUnknown

Scanned Files

  • lambdas/functions/termination-watcher/package.json

@Brend-Smits

Copy link
Copy Markdown
Contributor

@guicaulada how does this compare to the recent changes to the termination watcher? #5055

@edersonbrilhante

Copy link
Copy Markdown
Contributor

@guicaulada I think we should open the scope. Create an interface and make the compute provider the responsible for fetch the instances. So the janitor can work with any provider

@guicaulada guicaulada changed the title feat: add opt-in reconciliation for stale EC2 runner registrations feat: add resumable reconciliation for stale EC2 runner registrations Sep 23, 2026
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 63a47e3 to 8b038db Compare September 23, 2026 14:15
@guicaulada guicaulada changed the title feat: add resumable reconciliation for stale EC2 runner registrations feat: reconcile stale EC2 runner registrations with stateless scans Sep 23, 2026
@guicaulada guicaulada closed this Sep 23, 2026
@guicaulada guicaulada reopened this Sep 23, 2026
@guicaulada

guicaulada commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Accidentally closed, I wasn't aware of the recent changes on the termination watcher, will check if this is still needed and see what are the differences.

I opened a few clean-up PRs today with lambda changes that we had implemented locally after we noticed lots of orphaned resources.

@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 8b038db to 4116b1e Compare September 23, 2026 14:36
@guicaulada guicaulada changed the title feat: reconcile stale EC2 runner registrations with stateless scans feat: add stateless registration cleanup with multi-App support Sep 23, 2026
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 4116b1e to fd284e3 Compare September 23, 2026 14:57
@guicaulada

Copy link
Copy Markdown
Contributor Author

Thanks both for pointing these out!

@Brend-Smits I checked #5055: the termination watcher handles event-driven deregistration and busy-runner retries, while this janitor discovers registrations left behind by missed events, earlier failures, or instances removed before deregistration was enabled. It complements that cleanup.

@edersonbrilhante I’ve also moved runner-name interpretation and backing-resource checks behind a compute-provider interface. EC2 is the first implementation; additional providers can supply their own lookup without changing GitHub discovery or confirmation. The Terraform module currently configures EC2.

Checks happen per candidate, and failed lookups never establish absence. Cleanup remains stateless and incremental, with a delayed second observation before deletion. Tests also cover a non-EC2 provider through discovery and confirmation.

@guicaulada
guicaulada marked this pull request as ready for review September 23, 2026 15:03
@guicaulada
guicaulada requested a review from a team as a code owner September 23, 2026 15:03
@Brend-Smits
Brend-Smits requested a balanced review from Copilot September 23, 2026 16:41

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

A critical Lambda permission defect and unresolved discovery, deadline, client-reuse, documentation, and CI issues must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity · 1 Low severity

Open (3)
What changed in this PR

Adds an opt-in scheduled Lambda and Terraform module for stateless cleanup of orphaned GitHub runner registrations across EC2 regions.

Changes:

  • Adds scheduled discovery, delayed SQS confirmation, dry-run support, IAM, and documentation.
  • Introduces pluggable compute-provider cleanup with EC2 support.
  • Adds multi-App authentication, quota handling, fallback, and tests.
File Description
modules/​registration-janitor/​versions.tf Terraform and AWS provider requirements.
modules/​registration-janitor/​variables.tf Janitor configuration and validation.
modules/​registration-janitor/​tests/​registration-janitor.tftest.hcl Terraform deployment and IAM tests.
modules/​registration-janitor/​README.md Module usage and behavior documentation.
modules/​registration-janitor/​outputs.tf Lambda and dead-letter queue outputs.
modules/​registration-janitor/​main.tf Lambda, queues, scheduling, and IAM resources.
lambdas/​yarn.lock Workspace dependency lock updates.
lambdas/​libs/​compute-providers/​registration-cleanup.ts Cleanup-provider interface and factory.
lambdas/​libs/​compute-providers/​registration-cleanup.test.ts Provider contract tests.
lambdas/​libs/​compute-providers/​providers.config.registration-cleanup.ts Bundled provider registration.
lambdas/​libs/​compute-providers/​package.json Cleanup-provider API export.
lambdas/​libs/​compute-providers/​aws/​ec2/​registration-cleanup.ts EC2 inventory checks.
lambdas/​functions/​termination-watcher/​src/​registration-janitor.ts Discovery, confirmation, and deletion workflow.
lambdas/​functions/​termination-watcher/​src/​registration-janitor.test.ts Janitor behavior and safety tests.
lambdas/​functions/​termination-watcher/​src/​lambda.ts Janitor handler export.
lambdas/​functions/​termination-watcher/​src/​github-app-client.ts Multi-App authentication and quota selection.
lambdas/​functions/​termination-watcher/​src/​github-app-client.test.ts Authentication and fallback tests.
lambdas/​functions/​termination-watcher/​package.json Compute-provider dependency.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread modules/registration-janitor/main.tf Resolved
Comment thread modules/registration-janitor/README.md Resolved
Comment thread modules/registration-janitor/main.tf Resolved
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from fd284e3 to 7246641 Compare September 23, 2026 19:07
guicaulada added a commit that referenced this pull request Sep 24, 2026
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.
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 552d528 to 10ac022 Compare September 24, 2026 13:54

@Brend-Smits Brend-Smits 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.

Deployed to a sandbox with the module wired in and a dedicated prefix. I created offline org registrations with the JIT config API: a made-up instance ID, a stopped instance, a terminated instance and a name with an invalid suffix. Dry-run reported exactly the made-up and terminated ones and queued nothing. In live mode, the confirmation deleted those two after the 15-minute delay. It kept the stopped-instance registration and ignored the invalid name. DLQ stayed empty and there were no errors. Unrelated runners from other prefixes in the same group were untouched. Looks good. A few non-blocking suggestions:

  1. createRunnerInstallationClient runs for every discovery page and every SQS record. Each call mints an App JWT, looks up the installation and mints a token. Reusing one client per invocation, and per App within a batch, would cut this overhead on large groups.
  2. In live mode, discovery logs nothing when it queues candidates. A summary log per run (pages scanned, candidates queued, candidates skipped because the instance exists) would make rollout much easier to observe.
  3. registrationJanitor doesn't call setContext, unlike the other handlers.

@guicaulada

guicaulada commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the sandbox validation and suggestions. Addressed all three in 35687d9:

  1. Added invocation-local installation-client reuse, keyed by App, owner, runner type, and GitHub endpoint. Discovery pages and SQS records share their invocation's cache. App selection still runs before cache lookup, so quota exhaustion or cooldown can switch subsequent work to another App. Failed authentication is not cached, and a new invocation starts with a fresh client cache.
  2. Added an info-level discovery summary with pages scanned, candidates successfully queued, candidates retained because their resource exists, dry-run candidates, page/candidate failures, and stop reason. Failed sends do not inflate queued counts. Deadline and candidate-limit exits also emit the summary.
  3. Added setContext at the handler entry so both discovery and confirmation logs get the current request context.

@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 35687d9 to 5b3d69b Compare October 5, 2026 14:02
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 5b3d69b to 380ffdf Compare October 14, 2026 12:35

Copilot AI 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.

🔵 Needs a closer look

Destructive cross-service reconciliation requires human validation of ownership boundaries and deletion safeguards.

0 open findings

2 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Reuse one SQS client across queued candidate sends

lambdas/​functions/​termination-watcher/​src/​registration-janitor.ts:85

Each queued candidate creates a new SQS client, so sequential sends cannot reuse the same connection pool. A scan can queue up to 1,000 candidates, adding repeated connection setup to its limited execution time. Reuse one SQS client across sends.

Medium severity Reuse EC2 clients across regional resource lookups

lambdas/​libs/​compute-providers/​aws/​ec2/​registration-cleanup.ts:27

Each resource lookup creates a new EC2 client for each region it checks, preventing connection reuse across candidates. Discovery also checks retained runners without consuming the candidate limit, so large scans can repeatedly pay connection setup costs until the deadline. Create one client per region in the provider factory and reuse it across lookups.

🧠 Review effort: Balanced

@guicaulada

guicaulada commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor Author

Addressed both additional suggestions from the latest Copilot review summary in 203fe7a: discovery now reuses one SQS client, and the EC2 provider creates one client per configured region and reuses it across candidate lookups. The 57 janitor tests and 7 registration-provider tests pass, as does ESLint.

Copilot AI 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.

🔵 Needs a closer look

Destructive cross-service cleanup requires human validation of ownership safeguards and resolution of scan-starvation behavior.

1 open finding

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

Destructive cross-service reconciliation requires human review of ownership safeguards and resolution of the retained reliability issues.

2 open findings
1 resolved since last review

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

Cross-service registration deletion requires human validation of ownership boundaries and deployment safety.

0 open findings

2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid double-counting cached first page in randomized traversal

lambdas/​functions/​termination-watcher/​src/​registration-janitor.ts:138

When randomized traversal starts after page 1, this increment counts the metadata response and later counts the same cached page again. A complete three-page scan starting at page 2 therefore reports pagesScanned = 4, overstating discovery work. Skip the increment when reusing firstPage, and assert pagesScanned = 3 in the existing randomized traversal regression.

🧠 Review effort: Balanced

@guicaulada

Copy link
Copy Markdown
Contributor Author

Addressed the additional finding from the Copilot review summary in 9f42a99: discovery no longer counts the cached first page twice when randomized traversal wraps around.

@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch 2 times, most recently from 54eb37e to 73247df Compare October 19, 2026 14:27
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from 73247df to d8b9cbe Compare October 26, 2026 12:46
guicaulada added a commit that referenced this pull request Oct 27, 2026
…#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.

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.
@guicaulada
guicaulada force-pushed the gc/feat/registration-janitor branch from d8b9cbe to c39150a Compare October 27, 2026 15:20
@guicaulada
guicaulada merged commit 5959d43 into main Oct 27, 2026
61 checks passed
@guicaulada
guicaulada deleted the gc/feat/registration-janitor branch October 27, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants