fix(garm): remove a scaleset's runners before deleting it - #325
Conversation
GARM rejects deleting a scaleset that still owns runners, so an orphaned scaleset with active runners was never removed and its runners were left behind. Remove each runner first, ignoring provider errors so an instance GARM can no longer reach does not block the cleanup. A runner executing a workflow job is left in place and removed on a later reconcile once the job finishes, so an in-flight job is never failed. On a 401 (expired forge credentials) the removal is retried with GARM's GitHub Unauthorized bypass; any other failure is left for the next reconcile rather than escalated into a runner orphaned in GitHub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to the runner-removal change, addressing three issues found in review against GARM's own source at the pinned commit. Force-remove is no longer unconditional. GARM's provider worker retries a plain delete's teardown with a backoff indefinitely, but a forced one logs the provider error and marks the instance deleted anyway, leaving the VM running in the cloud with nothing pointing at it. It is now applied only once the instance already sits in a pending-delete state, so a transient provider error gets a retry cycle while a permanently unreachable instance still cannot block the scaleset delete forever. A failed disable no longer aborts the removal. Disabling a scaleset goes through the forge, so expired credentials fail it -- exactly the case the unauthorized-bypass escalation exists for -- and treating it as a hard precondition stranded the scaleset instead. Leaving the scaleset enabled only risks GARM replacing a removed runner for one pass, which it cannot do when the forge is what is broken. The busy-runner guard now matches the job statuses that actually hold a runner. GARM's job status is a closed set, so treating anything other than "completed" as running meant a stale or unhydrated job record pinned the runner as busy on every reconcile and the scaleset was never deleted. Also deduplicates the deferred-delete warning and records the residual list-then-delete race, which GARM's API offers no way to close. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A forced delete leaves the instance in "pending_force_delete" until the provider worker picks it up, which the pending-delete set did not cover: the next pass read it as a fresh instance and issued a plain delete, downgrading an escalation already in flight so a stuck instance could never clear. A failed disable is now split by cause. Continuing unconditionally was only justified for a forge rejection -- GARM cannot launch replacements when it cannot reach the forge either -- but a GARM-side failure leaves the scaleset enabled and still sized up, so removing its runners just had GARM re-provision them, churning instances on every pass. Runner removal now goes ahead only on an unauthorized disable; any other failure leaves the runners alone and still attempts the scaleset delete, so an already empty scaleset is not stranded either. update_scaleset classifies a 401 into GarmUnauthorizedError to make that distinction available. A job record now only protects its runner while it could still describe a live job. GARM reconciles stale queued jobs against the forge but not in-progress ones, so a dropped completion webhook pinned the runner as busy forever and stranded the scaleset -- the same symptom this branch exists to fix. Records older than GitHub's six-hour job ceiling are read as stale, and a naive timestamp is read as UTC rather than raising. Also corrects a test docstring that described the opposite of the force-remove policy its assertion checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The force escalation keyed off the instance's delete-pending status alone, but that is also what a healthy teardown looks like while it runs, and a cloud instance takes minutes to disappear. A reconcile landing in that window escalated a normal in-flight delete, turning a retryable failure into a leaked instance -- the outcome the plain-delete-first design exists to avoid. GARM records the provider's error against the runner when a teardown fails, so the escalation now requires both a delete-pending status and that fault. The stale-job ceiling was the GitHub-hosted job limit, but GARM provisions self-hosted runners, which GitHub allows to run for five days. A legitimate job past six hours whose runner status had not caught up was classified stale and its runner deleted mid-job. The bound is now the real ceiling: overshooting leaves a scaleset around longer, undershooting fails someone's running job. An unreachable forge no longer stalls silently. GARM learns what a runner is doing from the forge, so expired credentials freeze a mid-job runner as busy on every pass and the scaleset can never drain. The runner's own job reports to GitHub with its registration token rather than GARM's credential, so it may still be working and deleting it on state GARM cannot confirm would fail a live job -- the cleanup now names the stuck scaleset and points at the credentials instead of retrying in silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The heading was dated 2026-08-21 but every commit on this branch is dated 2026-08-24, which the file's own "versioned by the date of the revision" convention asks for. Fixes a typo in the force-escalation clause, and documents two user-visible behaviours the entry had not caught up with: what happens when disabling the scaleset itself fails (an unauthorized rejection still drains, anything else defers), and the case the charm cannot resolve on its own -- an unreachable forge while a runner is mid-job, where restoring credentials is the remedy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rs-before-scaleset-delete # Conflicts: # charms/garm/src/garm_api.py # charms/garm/tests/unit/test_garm_api.py # docs/changelog.md
…rs-before-scaleset-delete
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
Initial review with some questions. Thank you!
There was a problem hiding this comment.
nit: why are we using the term forge across the file , instead of github? seems like an unnecessary abstraction (scaleset is a github concept)
| * ``force_remove`` makes GARM drop the runner from its database even when the | ||
| provider teardown fails, leaving the instance running in the cloud with no | ||
| record of it. A plain delete instead retries the teardown with a backoff | ||
| indefinitely, so it is only forced once the instance is already sitting in | ||
| a pending-delete state — GARM accepted an earlier delete and has not managed | ||
| to carry it out. | ||
| * ``bypass_gh_unauthorized`` drops the runner from the provider and GARM's | ||
| database without deregistering it in GitHub, orphaning it there. It is | ||
| reserved for a 401 — the only status GARM returns for an unauthorized forge | ||
| error. Any other failure (a connection error, a 5xx, or a 400 for a runner | ||
| that is not yet in a deletable state) is transient, so it is left for the | ||
| next reconcile rather than escalated into a GitHub orphan. |
There was a problem hiding this comment.
this seems to be rather suited for a technical comment instead of an interface description? The caller has no option to influence force_remove or bypass
| logger.warning( | ||
| "Could not remove runner %s: GitHub rejected the request as unauthorized;" | ||
| " retrying with the bypass (this may leave the runner registered in" | ||
| " GitHub, where it must be removed manually): %s", | ||
| instance_name, | ||
| exc, | ||
| ) |
There was a problem hiding this comment.
in which cases would you expect a 401 here?
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
Follow-up review. I checked the GARM-side claims against the commit the rock pins (47811d0, garm-rockcraft.yaml:21), since most of the reasoning in this branch rests on them.
The core change is right: Runner.DeleteScaleSetByID does reject a delete while the scaleset owns runners, so drain-before-delete is the correct shape, and the mid-job guard is well founded. _is_delete_stuck is the strongest part — and for a reason the PR doesn't state: Runner.DeleteRunner writes UpdateInstanceParams{Status: ...} with no fault, and updateInstance assigns ProviderFault unconditionally, so every delete clears it. A fault on a pending_delete really did come from a failed teardown. Unit coverage is thorough.
Two things I'd like to resolve. CI is red on the changelog (markdown-lint). And the forge_reachable branch rests on a premise I couldn't confirm against the pinned GARM — details inline. If that holds, a good chunk of this can come out: the tri-state return, the parameter threaded into _remove_runners, one test, and part of the changelog entry. Smaller is better here, given how much of the logic is inference about GARM internals.
Two more that have no line in the diff to hang off:
Integration coverage. charms/tests/integration/test_garm.py:702 — _delete_scalesets / _delete_scaleset_drained currently hand-roll disable-drain-delete as test teardown. That's the behaviour the charm now owns, so it looks like the natural place to assert the charm drains on its own. Every premise in this branch is about live GARM behaviour and none of it is exercised against one; the checklist ticks "unit and integration" but only unit tests changed.
PR title is currently the raw branch name, while the commits on the branch are conventional — something like fix(garm): remove a scaleset's runners before deleting it would match.
🤖 AI-generated message.
|
|
||
| ## 2026-08-24 | ||
|
|
||
| - `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is sitting on a delete it has accepted and the provider has recorded a fault carrying it out — not merely one still in flight, which is what a healthy teardown looks like too — so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the five days GitHub allows a job on a self-hosted runner to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. Disabling the scaleset before the drain can itself fail: when the forge rejects it as unauthorized the drain still goes ahead, since GARM cannot launch replacement runners while the forge is unreachable either, but any other failure leaves the runners in place for the next reconcile rather than having GARM immediately replace them. One case the charm cannot resolve on its own: if the forge is unreachable while a runner is mid-job, GARM cannot refresh what that runner is doing, so it reads as busy on every pass and the scaleset is not removed. The charm logs a warning naming the scaleset rather than retrying silently — restoring valid credentials is what clears it. |
There was a problem hiding this comment.
markdown-lint fails here (MD032/MD022): the entry needs a blank line before ## 2026-08-19.
Separately, this bullet is ~350 words where the neighbouring entries are two to four sentences. Most of it is design rationale that belongs in the code; the changelog wants what changed for the user.
🤖 AI-generated message.
| scaleset_id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) | ||
| ) | ||
| return True | ||
| except GarmUnauthorizedError as exc: |
There was a problem hiding this comment.
the forge rejected the request as unauthorized
I don't think update_scaleset can produce this at 47811d0. Runner.UpdateScaleSetByID only calls GitHub inside the update callback, and only when Name, GitHubRunnerGroup or DisableUpdate change — enabled and min_idle_runners don't trigger it. The github.Client() / scalesets.NewClient() constructions above it make no network call (ghinstallation's transport is lazy, and the RateLimit probe's error is discarded). That leaves GARM's own JWT middleware and !auth.IsAdmin(ctx) as the only 401 sources, and _reconcile_runners does a fresh from_login as admin every pass, so neither fires here.
If that's right, forge_reachable=False is unreachable, and with it: the bool | None tri-state, the forge_reachable parameter on _remove_runners, the warn-and-stall branch, test_busy_runner_survives_an_unreachable_forge, and the last two changelog sentences. _disable collapses to "did it work".
Have I missed a path where the disable does reach GitHub?
🤖 AI-generated message.
| if scaleset.id is None: | ||
| return | ||
| forge_reachable = self._disable(scaleset.id, name) | ||
| if forge_reachable is not None: |
There was a problem hiding this comment.
DeleteScaleSetByID checks two preconditions, not one:
if len(scaleSet.Instances) > 0 { return NewBadRequestError("scale set has runners") }
if scaleSet.Enabled { return NewBadRequestError("scale set is enabled; disable it first") }So when _disable returns None the scaleset is still enabled and this delete 400s whether or not it holds runners — _disable's "so an already-empty scaleset is not stranded either" only holds if an earlier pass already disabled it. Not harmful (warn + retry), but worth correcting the rationale, and the comment here documents only the "has runners" half.
test_garm_side_disable_failure_leaves_runners_in_place asserts client.deleted == [42], which the fake accepts and real GARM would reject.
🤖 AI-generated message.
| # the provider teardown is failing and being retried with a backoff. The forced | ||
| # variant belongs here too — dropping it would downgrade an escalation already in | ||
| # flight back to a plain delete, so a stuck instance could never clear. | ||
| PENDING_DELETE_STATUSES = frozenset({"pending_delete", "pending_force_delete", "deleting"}) |
There was a problem hiding this comment.
"deleting" doesn't seem to earn its place. In workers/provider/instance_manager.go the worker sets deleting with providerFault=nil immediately before the teardown, and on failure sets pending_delete with the fault — so deleting + a fault never coexist, and the deleting-with-fault-forced parametrize case tests a state GARM can't produce.
Related: Runner.DeleteRunner only accepts running | error | pending_delete | pending_force_delete, so plain-deleting an instance sitting in deleting (or creating / pending_create) 400s on every pass. Worth only issuing a delete for a status GARM will accept, and leaving alone the ones it's already tearing down.
🤖 AI-generated message.
| class GarmUnauthorizedError(GarmApiError): | ||
| """Raised when GARM answers a request with 401 Unauthorized. | ||
|
|
||
| GARM's error handler returns 401 only for an unauthorized error, so this |
There was a problem hiding this comment.
GARM's error handler returns 401 only for an unauthorized error
True, but ambiguous about whose authorization — the JWT middleware and the !auth.IsAdmin check produce the same status as an expired GitHub credential. Worth naming that here.
The bigger one: ScaleSetClient.Do maps 401 and 403 to ErrUnauthorized, and GitHub answers 403 for things that aren't credential problems — secondary rate limits, SSO enforcement. Those will reach _delete_runner's escalation and orphan the runner in GitHub. The bypass may still be the right call, but the tradeoff is worth writing down as it is, rather than as "the only status GARM returns for an unauthorized forge error".
🤖 AI-generated message.
| scaleset_id: Id of the scaleset being deleted. | ||
| name: Name of the scaleset being deleted, for logging. | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
Same point as my comment on _delete_runner: this is rationale for a decision the caller can't influence.
The Returns: opens as an interface description, then argues why a non-401 failure isn't survivable — that justifies the branch _delete_orphaned takes, so it belongs at the call site. Keeping the interface line and pushing the argument down is what AGENTS.md asks for.
🤖 AI-generated message.
| Args: | ||
| instance: The runner instance to inspect. | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
Same here: "Both halves are needed: ..." argues against an alternative implementation no caller can select, so it reads better inside the body, next to the condition it defends.
🤖 AI-generated message.
| ) | ||
| return None | ||
|
|
||
| def _remove_runners(self, scaleset_id: int, name: str, forge_reachable: bool) -> None: |
There was a problem hiding this comment.
Why "forge" rather than "GitHub" across the new code? The rest of the charm says GitHub throughout (GithubReconciler, bypass_gh_unauthorized, DEFAULT_GITHUB_ENDPOINT). "Forge" is GARM's internal vocabulary for also supporting Gitea, and GARM's own CreateEntityScaleSet rejects non-GitHub entities for scale sets, so it can't become true here either.
🤖 AI-generated message.
_is_delete_stuck required a recorded provider fault before escalating to a forced removal, so a runner GARM had already parked in pending_force_delete but with no readable fault fell back to a plain delete. That downgrades an escalation already in flight — whether from an earlier reconcile or applied by an operator — and the runner can never clear. Treat pending_force_delete as stuck on its own: the escalation has already been applied, so the fault that justified it need not still be readable, and re-forcing leaks nothing that is not already forfeit. The other two pending-delete statuses still require a fault, since forcing those would turn a retryable teardown into a leaked instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review against GARM at the pinned commit (47811d0) settled three claims the branch had inferred rather than checked, and each one removes code. The disable never reaches GitHub. UpdateScaleSetByID only calls the forge from its update callback, and only when the name, runner group or update setting changes; enabled and min_idle_runners do not trigger it, and neither the github.Client() construction nor its RateLimit probe propagates an error. So the unauthorized branch was unreachable, and with it the bool | None tri-state, the forge_reachable parameter, and the warn-and-stall path for a runner frozen mid-job. A failed disable now defers the whole cleanup: DeleteScaleSetByID rejects an enabled scaleset as well as one that still owns runners, so the delete that used to be attempted anyway could only 400. A delete is only issued for a status GARM accepts. DeleteRunner takes running, error, pending_delete and pending_force_delete and 400s on everything else, so a runner still being created, or already being torn down, was retried into the same error on every pass. "deleting" also leaves the pending-delete set: the provider worker sets it with a nil fault immediately before the teardown and records the fault only on dropping back to pending_delete, so the two can never coexist and the case the parametrize covered was unproducible. Names GitHub rather than "forge" in the new code, and writes down what a 401 actually pins down: GARM's own JWT and admin checks answer it too, and its scaleset client folds 403 into the same error, so a secondary rate limit or SSO enforcement reaches the unauthorized bypass and orphans the runner in GitHub. That trade is now stated as it is rather than as "the only status GARM returns for an unauthorized forge error". Adds the integration coverage the branch was missing — renaming the desired scaleset makes the charm drain and delete the orphan against a live GARM, the sequence the test teardown otherwise hand-rolls — moves the docstring rationale that argued against alternatives no caller can select into the bodies it defends, and cuts the changelog entry to the size of its neighbours. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yhaliaw
changed the title
DELETABLE_STATUSES said "statuses" where the file names both scaleset and runner state, and where the sibling guard for the same GARM constraint — resource_cleanup._DELETABLE_RUNNER_STATES — already says "runner". It is now DELETABLE_RUNNER_STATUSES. (The two sets still disagree on content, which is a real gap in resource_cleanup rather than a naming one: it omits pending_delete and pending_force_delete, both of which DeleteRunner accepts.) FORCED_DELETE_STATUS dropped the "pending" its own value carries, sitting directly beneath PENDING_DELETE_STATUSES, which keeps it. Renamed to PENDING_FORCE_DELETE_STATUS so the pair reads as the pair it is. The integration helper introduced with the drain test was a third copy of a wait its two neighbours had already inlined, under a name — _wait_for_reconcile — that described neither what it waits on nor the _wait_for_<thing> shape of every other helper in the file. It is now _wait_for_config_applied, used at all three call sites, and sits below the first of them. Also adds the article the surrounding test names carry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A 404 from delete_instance means the runner GARM was asked to remove is no longer there — the outcome the delete was after, not a failure. It fell into the generic handler and logged "will retry on next reconcile" against a name no later pass can act on. Both delete attempts now treat GarmNotFoundError as done, the way resource_cleanup already does. Two test corrections found alongside it. The listing-failure test asserted neither half of its own name: the scaleset delete is still attempted after an unreadable runner list, deliberately, since the list says nothing about whether the scaleset owns runners and an empty one should not be stranded on a failure to read it. It now asserts that and says why. The integration test claimed to cover the drain, which it cannot: the configurator's min-idle-runner defaults to 0 and the OpenStack provider is a stand-in, so the orphaned scaleset never owns a runner. It covers disable and delete, and is now named and documented as that; the drain stays unit-tested. Also gives _log_deferred_runner_delete the docstring the rest of the file carries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
What this PR does
On removal of all configurator charm, the charm should cleanup the scale sets.
AI summary
This pull request introduces robust support for removing orphaned runners and scalesets in the GARM charm, ensuring that runners are only deleted when it is safe to do so and handling error cases more gracefully. The changes add new API client methods, improve error handling (especially around authorization), and update the scaleset reconciliation logic to avoid disrupting active jobs. Comprehensive unit tests are also included for the new logic.
API client enhancements:
list_scaleset_instancesanddelete_instancemethods toGarmAuthenticatedClient, allowing the charm to list and delete runner instances directly, including support for force-removal and bypassing GitHub unauthorized errors. Introduced a newGarmUnauthorizedErrorfor distinguishing 401 errors. [1] [2] [3] [4]Scaleset reconciliation improvements:
ScalesetReconcilerto first disable the scaleset, then safely remove all associated runners, only deleting the scaleset once all runners are gone. Runners currently executing jobs are left in place until idle, and force-removal/bypass logic is applied only when necessary._is_running_job,RUNNER_STATUS_ACTIVE,JOB_STATUSES_HOLDING_RUNNER,PENDING_DELETE_STATUSES). [1] [2]Testing improvements:
Dependency updates:
These changes collectively make the charm more reliable and robust in managing the lifecycle of runners and scalesets, particularly in error and edge cases.
Why we need it
Checklist
CONTRIBUTING.mdhas been updated upon changes to the contribution/development process (e.g. changes to the way tests are run)docs/changelog.mdwith user-relevant changes(e.g., in
.github/workflows/integration_tests.yaml, ensure themoduleslist is correct)terraform fmtpasses andtflintreports no errorsAGENTS.md.copilot-collections.yamlor.github/instructions/: I re-checked whether theAGENTS.md"12-factor divergences" guidance still matches the upstream copilot-collections guidance