Keep the outdated phase until the runner spec itself changes - #4660
Open
nikola-jokic wants to merge 2 commits into
Open
nikola-jokic wants to merge 2 commits into
nikola-jokic wants to merge 2 commits into
Conversation
The outdated phase means the runners rejected the runner spec they were given, so the listener is switched off and the EphemeralRunnerSet is pinned at zero replicas. Recovery was keyed on metadata.generation instead of on that spec: the generation check moved the phase back to pending before the outdated short-circuit ran, so editing maxRunners, the runner group or the scale set name switched the listener back on to acquire jobs for runners that reject the spec exactly as before. Gate recovery on ephemeralRunnerSetActionableSpecChanged, the predicate that compares the runner spec the set is running with the one derived from the AutoscalingRunnerSet. The outdated check now runs before the generation comparison, and leaves the phase only when that spec differs. Recovery still goes through the existing patch path, so the actionable revision advances with the new spec and the EphemeralRunnerSet stops judging itself by the runners that failed. ephemeralRunnerSetNeedsOutdatedRecovery has no callers left and is removed. The generation annotation on the EphemeralRunnerSet is kept as a record of the spec's provenance, but nothing keys off it any more. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nikola-jokic
requested review from
a team,
Steve-Glass,
mumoshu,
rentziass and
toast-gear
as code owners
September 15, 2026 14:49
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Add a test covering missing EphemeralRunnerSet recovery and recreation while outdated.
Review tier: Lite
Findings: None
What changed in this PR
Updates AutoscalingRunnerSet recovery so the Outdated phase persists until the runner specification changes.
Changes:
- Replaces generation-based recovery with runner-spec comparison.
- Keeps outdated scale sets disabled for unrelated edits.
- Adds lifecycle and fake-client coverage.
A moderate issue remains: add coverage for recovery and recreation when the EphemeralRunnerSet is missing while outdated.
| File | Description |
|---|---|
controllers/actions.github.com/helpers.go |
Removes obsolete generation-based recovery logic. |
controllers/actions.github.com/constants.go |
Clarifies generation annotation semantics. |
controllers/actions.github.com/autoscalingrunnerset_outdated_recovery_test.go |
Adds focused recovery tests. |
controllers/actions.github.com/autoscalingrunnerset_controller.go |
Implements spec-aware outdated handling. |
controllers/actions.github.com/autoscalingrunnerset_controller_test.go |
Tests sticky outdated lifecycle behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The EphemeralRunnerSet spec carries the metadata stamped onto the runners it creates alongside the runner spec itself. Both are published by the AutoscalingRunnerSet and both change what the next runner is handed, so both recover a scale set from the outdated phase; Replicas, PatchID and ActionableRevision stay out of the comparison, since they are scaling bookkeeping pinned to zero while the set is parked. Recovery on a metadata-only change has to advance the actionable revision as well. Otherwise the metadata patch lands without it, the EphemeralRunnerSet keeps judging itself by the runners that failed, and the scale set is pushed straight back to outdated. The fake-client fixture published the live generation on the EphemeralRunnerSet, which is the one value that made the old generation-based signal park for its own reasons, so the sticky-phase test passed without the fix. The published generation is now explicit per test, which also makes room for the opposite failure: with the generation current and the runner spec genuinely different, the old signal refused a legitimate recovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+166
to
+167
| if !corrected { | ||
| return r.reconcileOutdated(ctx, &autoscalingRunnerSet, log) |
Comment on lines
+13
to
+15
| // It is also the sole signal that recovers a scale set from the outdated phase: | ||
| // the runners rejected this spec, so nothing short of changing it is reason to | ||
| // retry. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up to #4652, which switches the scale set off instead of rebuilding it when the runners reject the runner spec. The teardown works; the recovery condition was wrong in both directions.
The bugs
Outdatedwas left on ametadata.generationsignal rather than on the spec the runners actually rejected, and that signal is wrong twice over:Reconcileran before theOutdatedshort-circuit, so any spec edit —maxRunners,minRunners,runnerGroup,runnerScaleSetName— moved the phase back toPending, switched the listener on, and resumed acquiring jobs for runners that reject the spec exactly as before. That is the churn loop Switch the scale set off instead of rebuilding it when runners are outdated #4652 set out to end.EphemeralRunnerSetis already current,ephemeralRunnerSetNeedsOutdatedRecoveryrefuses recovery even though the runner spec genuinely changed. Reachable whenever the derived runner spec moves withoutmetadata.generationmoving with it. Covered byTestAutoscalingRunnerSetRecoversWithoutAnUnobservedGeneration, which fails onmaster.The fix
Decide recovery by comparing what the runners would be handed next, via
ephemeralRunnerSetDesiredSpecChanged: theEphemeralRunnerSpec(through the existingephemeralRunnerSetActionableSpecChanged,Semantic.DeepEqualfor the reasons in its doc comment) plusEphemeralRunnerMetadata.Replicas,PatchIDandActionableRevisionare excluded — scaling bookkeeping, pinned to zero while parked.Outdatedhandling moves above the generation comparison and becomes spec-aware. It sits after the deletion, version and finalizer handling, so deletion still wins over the phase.ActionableRevisionthroughnextActionableRevision.patchAppliedActionableRevisionStatustherefore still fires, re-buckets the failed runners as stale, clears theEphemeralRunnerSet's ownOutdatedphase, and lets it scale from 0. A metadata-only recovery bumps the revision for the same reason — without it the metadata patch would land alone and the set would be pushed straight back toOutdated.ephemeralRunnerSetOutdatedForAppliedRevisionstill requires the applied revision to have caught up, so the window between patching the fix and the set processing it does not re-trigger teardown. The new guard is "spec changed", not "phase != Outdated", so thePending-pushed-back-to-Outdateddeadlock shape does not reappear.ephemeralRunnerSetNeedsOutdatedRecoveryis removed. The generation annotation on theEphemeralRunnerSetstays as a record of provenance; nothing keys off it now.Behaviour reviewers should weigh
minRunners/maxRunnerscan never un-park an outdated scale set. They live on theAutoscalingRunnerSetand not inEphemeralRunnerSpec, so they are no longer a recovery route. This is intended, and it reverses what Switch the scale set off instead of rebuilding it when runners are outdated #4652 allowed.EphemeralRunnerSpec.EphemeralRunnerSetwhile outdated counts as corrected. There is nothing left to compare against, so the set is rebuilt from the current spec — and if that spec is still bad it re-entersOutdatedimmediately. It is not an escape hatch, only a way out of a phase that could otherwise never be left.Tests
envtest (
Test AutoscalingRunnerSet outdated lifecycle):maxRunners, runner group and scale set name edited while outdated must keep the phaseOutdated, the listener absent and the revision unchanged — asserted withConsistently, since the bug is a spurious transition. Runner-spec and runner-metadata corrections must recover fully.It("recovers when a field outside the runner spec is updated")asserted the old contract and is replaced by the sticky cases.Every new
Itwas confirmed to fail before the fix and pass after, run individually since the container isOrdered. At unit level, four of the five fake-client tests fail on the pre-fix controller — parking the first rejection, staying parked without a spec change, recovering without an unobserved generation, and recovering on a metadata change — each for a different reason. The published generation is now an explicit fixture parameter, because defaulting it to the live generation made the sticky-phase test pass for the wrong reason.go build ./...,go vet ./...andgo test ./controllers/... ./cmd/... ./apis/...are green.