Switch the scale set off instead of rebuilding it when runners are outdated

[nikola-jokic]

Was stacked on #4647, which has since merged; this is now based directly on master.

Supersedes #4649, which GitHub auto-closed as merged when its base branch was force-pushed during a stack reorder. Same change, now first in the stack instead of second.

The problem

AutoscalingRunnerSetPhaseOutdated was declared and read, but nothing in the codebase ever assigned it.

So when the runners rejected the runner spec (pod exits 7 → EphemeralRunner goes Outdated → EphemeralRunnerSet goes Outdated), the AutoscalingRunnerSet removed the listener and then called cleanupEphemeralRunnerSet, which deletes the set — while leaving its own phase on Running.

Nothing held the scale set switched off. The next reconcile saw a missing EphemeralRunnerSet, created it, created a listener for it, and the fresh runners rejected the same spec again. The scale set churned through create/teardown cycles against the Actions service instead of resting, directly contradicting the comment in that branch saying it "should stay in outdated state until the spec is updated".

The outdated branch at the top of Reconcile already did the right thing — remove the listener, keep the set, pin it to zero replicas — but was dead code, because the phase it keys on was never set.

The intended lifecycle

1. Runner pod exits 7 → EphemeralRunner phase Outdated.
2. EphemeralRunnerSet sees it, goes Outdated, and releases every runner that is not executing a job (cleanUpEphemeralRunners already skips HasJob() runners).
3. AutoscalingRunnerSet goes Outdated and switches the listener off, so no more jobs are acquired. ← this step was missing
4. On an update, the listener comes back and the set scales from 0 upwards on the listener's information.

The change

• When the EphemeralRunnerSet is outdated for its applied revision, the AutoscalingRunnerSet records AutoscalingRunnerSetPhaseOutdated and hands over to the same handler the top-of-reconcile outdated branch uses, which is extracted into reconcileOutdated. The EphemeralRunnerSet is kept and pinned to Replicas = 0, PatchID = 0 rather than deleted.

• ObservedGeneration is carried over unchanged, so a later spec edit still registers as new work and moves the phase back to Pending through the existing generation check.

• The EphemeralRunnerSet spec patch also fires on the recovery path, when the set is still outdated for its applied revision but the spec has since been updated.

  The revision has to advance even when the runner spec itself did not change. The revision is what tells the EphemeralRunnerSet to stop judging itself by the runners that failed (patchAppliedActionableRevisionStatus re-buckets them as stale and clears the outdated phase). Without it, a scale set could only ever be recovered by editing the pod template, and an edit to anything else — maxRunners, the runner group, the config secret — would switch the listener back on against a set permanently parked at zero.

Telling recovery apart from a first rejection

Parking and recovering are the same two states seen from different angles, so the condition that separates them carries the whole correctness argument. Both branches now key on one helper and are exact complements — no gap, no overlap:

case outdated && !ephemeralRunnerSetNeedsOutdatedRecovery(...):  // park
     outdated &&  ephemeralRunnerSetNeedsOutdatedRecovery(...)   // recover

The phase alone cannot make this call. Pending also means a metadata-only listener rebuild, which deliberately leaves ObservedGeneration untouched; recovering there would retry the exact spec the runners just rejected.

Generation > ObservedGeneration alone cannot make it either. That is also true while a newly edited spec is being published, because ObservedGeneration only catches up at the tail of a full reconcile and the intervening passes return early at the EphemeralRunnerSet patch and at listener creation. If the runners reject the new spec inside that window, generation lag alone reads the first rejection as recovery and immediately republishes a revision already known to fail. That window stays open for as long as listener creation keeps failing, so it is not self-limiting.

The missing information is provenance: which AutoscalingRunnerSet generation published the revision the runners rejected. Each desired EphemeralRunnerSet is therefore stamped with the generation that produced it, and recovery requires both an unobserved generation and a stamp older than it. Once a revision has been published for the current generation, its rejection parks the set; only a further edit recovers it. An unstamped set from a previous version is treated as recoverable, so upgrades get exactly one republish.

The stamp is an annotation on the owned EphemeralRunnerSet rather than a new AutoscalingRunnerSet status field, which keeps the CRDs and shipped chart copies unchanged. It is not a return to annotation fingerprinting: what #4575 removed was a hash over the whole object recomputed every reconcile; this is a single int64 generation formatted once at build time, and it is already covered by the existing owned-resource predicates.

Tests

The outdated path had no coverage at all in autoscalingrunnerset_controller_test.go, which is why deleting the runner set went unnoticed. Added Test AutoscalingRunnerSet outdated lifecycle covering:

• the scale set is switched off: phase becomes Outdated, the listener is deleted, and the EphemeralRunnerSet survives at zero replicas (asserted with Consistently, since the bug was that it got deleted);
• recovery when the runner spec is corrected;
• recovery when a field outside the runner spec is updated;
• a metadata-only listener rebuild does not retry the rejected spec, and does not transiently recreate the listener — the assertion watches listener UIDs, because the replacement was created and then removed again on the following reconcile.

The set is seeded with nonzero Replicas/PatchID before being reported outdated. reconcileOutdated short-circuits when both are already zero, and the builder creates them at zero, so without the seed the zeroing patch under test never executed.

TestAutoscalingRunnerSetParksFirstRejectionOfPublishedGeneration pins the race directly: an unobserved generation whose revision has already been published, then rejected, must park rather than republish.

All of these fail on the parent commit, and each was mutation-checked by reverting its guard individually.

[Copilot]

Copilot review overview

🟡 Changes recommended

Gate outdated recovery on an actual spec-generation change to prevent retrying the same rejected runner specification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	controllers/​actions.github.com/​autoscalingrunnerset_controller.go — Do not recover on listener-only Pending state

What changed in this PR

This PR prevents outdated runner specifications from causing scale-set teardown and recreation loops by disabling the scale set and preserving its runner set.

Changes:

• Persists the Outdated phase, removes the listener, and pins replicas to zero.
• Adds recovery revision handling and lifecycle tests.
• A recovery condition must avoid advancing revisions when only listener changes occur without a spec update.

File	Description
controllers/​actions.github.com/​autoscalingrunnerset_controller.go	Implements outdated-state handling and recovery.
controllers/​actions.github.com/​autoscalingrunnerset_controller_test.go	Adds shutdown and recovery lifecycle coverage.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

A metadata rebuild can re-enable the listener while the EphemeralRunnerSet remains outdated.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1

Open (1)

• Keep outdated sets off during listener rebuilds · New

Resolved since last review (1)

• Do not recover on listener-only Pending state

[Copilot]

Copilot review overview

🟡 Changes recommended

Address the recovery race and strengthen the outdated-state test coverage.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1 · 1

Open (2)

• Generation guard can retry a newly rejected spec · New
• Test does not verify zeroing a nonzero runner set · New

Resolved since last review (1)

• Keep outdated sets off during listener rebuilds

[Copilot]

Copilot review overview

🟢 Approval recommended

No blocking issues remain; the outstanding documentation nit does not block approval.

Review tier: Lite
Findings: None

Resolved since last review (2)

• Generation guard can retry a newly rejected spec
• Test does not verify zeroing a nonzero runner set

[satishweb]

Downstream ARC recovery integration is ready, but it cannot complete against this head because the native Outdated lifecycle is not yet observable as the accepted bounded recovery contract. Please expose the following on this PR before release:

1. A typed AutoscalingRunnerSet Degraded condition or Warning Event for the Outdated transition, including the affected resource, controller, reason, observed generation, and first transition time.
2. A Prometheus recovery-attempt counter.
3. A durable incident epoch value exposed as a metric value, not an unbounded label.
4. A circuit-open or recurrence signal.

The recovery budget must permit at most one automatic attempt for an AutoscalingRunnerSet incident epoch. A repeated rejection in that epoch must open the circuit and must not retry. Metric labels must remain bounded. Existing listener assigned, running, and started-job metrics already provide downstream objective-work and semantic-progress inputs. Please publish the exact condition or Event reasons, metric names, label set, and release version so the deployed scrape and recovery proof can use the producer interface without guessing.