Switch the scale set off instead of rebuilding it when runners are outdated by nikola-jokic · Pull Request #4652 · actions/actions-runner-controller

nikola-jokic · 2026-09-10T21:03:23Z

Stacked directly on #4647.

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

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

The change

The ephemeralRunnerSetOutdatedForAppliedRevision && phase == Running case now 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 now also fires when the set is still outdated for its applied revision, which is the recovery path. Reaching that branch means the AutoscalingRunnerSet is not outdated and the case above already claimed every running set that is, so the spec must have been updated since the runners rejected it.

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.

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.

All three fail on the parent commit.

The AutoscalingRunnerSet controller detected changes by hashing the spec and the labels on every reconcile and comparing the result against the actions.github.com/integrity-hash annotation it had written on a previous pass. That is a hand-rolled version of something the API server already does for us: it bumps metadata.generation on every spec write, and nothing else. Recording that value in the status as ObservedGeneration gives the same "has this changed since I last applied it" signal without recomputing a hash, without an extra non-status write to the object, and with a value a user can read and reason about. updateStatus now takes the observed generation and patches it next to the phase, returning early only when both are already what we want. When the generation is ahead of the observed generation we move to Pending but deliberately leave the observed generation where it is; it only catches up at the end of a reconcile that actually applied the change, so a reconcile that fails part way through is retried as pending rather than being mistaken for settled. The old code returned immediately after stamping the hash. That was needed because stamping the annotation was itself a write to the object, so continuing would have worked from a stale copy. Recording the generation only touches the status subresource, which does not bump generation, so the rest of the reconcile can run on the same object and apply the change in the same pass instead of waiting for the next event. One user-visible difference: the old hash covered Labels as well as Spec, and metadata.generation does not move when only labels change. Editing just a label on an AutoscalingRunnerSet no longer forces it through Pending. Labels still propagate to the EphemeralRunnerSet; they simply no longer count as an update that has to be applied. Hash, ListenerSpecHash and RunnerSetSpecHash are removed. The latter two already had no callers, and the first has none now. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Moving update detection to metadata.generation lost a signal. The desired listener is built from the AutoscalingRunnerSet's labels and annotations as well as its spec, and any difference there deletes the listener so it can be re-created. metadata.generation only moves on spec writes, so a label-only edit still tore the listener down while the scale set kept reporting Running. If the rebuild then failed, it reported Running with no listener indefinitely. Under the old label-inclusive hash the phase did move, so this was a regression. Mark the resource pending at the point the listener is deleted rather than trying to infer it from the generation. That is what the phase already means: its own doc comment says pending is when the listener is not yet started. It also covers the case properly, because it keys off the actual decision to rebuild instead of guessing from the trigger. This does not change when the listener is rebuilt. A label-only edit recreates it today and still does; only the reported phase changes. The earlier claim that labels no longer restart anything was wrong: labels are propagated to the listener, so editing one is a restart. What metadata.generation changes is narrower than that, and the test now covers the listener's identity across a label edit rather than implying it is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The specs added with the observed-generation work asserted on end states only. Both the listener rebuild and a stalled reconcile pass through a transient phase, and nothing looked at it: the tests waited for the rebuilt listener and then checked for Running. Removing the pending update that guards the rebuild left them green, which was pointed out in review and is true — I checked by deleting that update and re-running, and they passed. The window was invisible because it closes on its own between two polls. Hold it open instead: the AutoscalingListener controller does not run in this suite, so a finalizer added by the test keeps the deleted listener around until the test removes it. That turns a race into a state the test can sit on and assert against. The label spec now waits for the listener to carry a deletion timestamp while the scale set reports Pending, holds that to show it is not a blip, then releases the finalizer and checks the listener comes back with a new UID and the scale set returns to Running. With the pending update removed it now fails on the phase, which is the point. The same trick gives the failure path its first coverage. A max runners change bumps the generation and forces a listener rebuild, so blocking the rebuild stalls the reconcile part way through. The new spec pins what the contract claims: the observed generation stays behind the live generation and the phase stays Pending for as long as the change cannot be applied, and the marker only catches up once it can. Also drops a stale comment that survived a merge and contradicted the code next to it, still claiming label edits no longer reach the Pending phase. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… finished runners Spec.Replicas is the count the listener asked for when it published Spec.PatchID. When the EphemeralRunnerSet controller cleaned up finished runners in the same reconcile, it then compared that count against a live count the cleanup had just reduced, and created runners to replace jobs that had already completed. Nobody asked for those runners. Delete the finished runners, record the patch ID the cleanup belongs to in Status.FinishedRunnerCleanupPatchID, and return, so the scaling decision is made on the next reconcile against post-cleanup data. Scale up stays suppressed while Spec.PatchID still equals that recorded patch ID: the gap below Spec.Replicas is the one the cleanup opened, not new demand. The listener marks itself dirty on every job completion and publishes a fresh incrementing patch ID, so the suppression lifts as soon as it reports a desired state that accounts for the completions. Runners that are mid-deletion were not counted at all, which let the controller over-create while deletions were still in flight. Count them towards the scale-up total. The cleanup helper is renamed to deleteTerminatedEphemeralRunners because it is no longer specific to finished runners. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…caling up The scale-up suppression read Status.FinishedRunnerCleanupPatchID off the EphemeralRunnerSet that Reconcile fetched through the manager's cached client. That marker is written by the cleanup reconcile, and it is the runner deletions performed by that same reconcile which trigger the next one, so the follow-up reconcile is regularly served from an informer cache that has not yet observed the controller's own status write. The marker read as 0, the guard did not fire, and the controller created a replacement runner for a job that had already finished. That is the exact spurious scale up the guard exists to prevent, so it is a correctness problem in a cluster and not only a flaky test. Make the decision from authoritative state. A cached hit still short circuits, because nothing ever clears the marker, so a hit cannot be a false positive. Only a miss falls through to an uncached Get via a new APIReader, which SetupWithManager fills in from mgr.GetAPIReader() so no construction site can forget it. The extra read is confined to scale-up decisions, where the controller is about to issue creates anyway. The window was transient: updateStatus copies the marker from the in-memory object and patches with MergeFrom, so a stale reconcile produces no diff for that field and cannot clobber the recorded value. The envtest spec that caught this only fails under CI load, so the regression is pinned down directly instead. TestScaleUpServicedByFinished RunnerCleanup drives the decision with a lagging cached client and an API reader that already has the write, and asserts the suppression still holds. Pointing the read back at the cached client fails that case and only that case. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

When the AutoscalingRunnerSet's runner spec changes, the EphemeralRunnerSet has to delete its idle and pending runners so they are rebuilt from the new spec. That was detected by hashing Spec.EphemeralRunnerSpec into the actions.github.com/integrity-hash annotation and comparing the annotation against a freshly computed hash. Replace it with a monotonic revision counter split across spec and status. The AutoscalingRunnerSet controller bumps Spec.ActionableRevision when it patches a new runner spec across; the EphemeralRunnerSet controller advances Status.AppliedActionableRevision only after the cleanup has actually succeeded. Because the applied marker lives in status and is written last, a controller that crashes part-way through the cleanup comes back with the applied revision still behind the spec revision and redoes the work, instead of skipping runners that are still running the old spec. The drift check uses apiequality.Semantic.DeepEqual rather than cmp.Equal or reflect.DeepEqual. Most PodSpec collection fields carry omitempty, so a template containing an explicitly empty value (`env: []`) is dropped when the EphemeralRunnerSet is written and reads back as nil. A strict comparison would report drift on every reconcile, bump the revision each time, and delete every idle and pending runner forever. helpers_drift_test.go pins that behaviour with a round-trip-through-JSON test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Status.FinishedRunnerCleanupPatchID was only ever set, never cleared, so a marker recorded under one listener incarnation outlived the patch-ID sequence it described. Applying a new actionable revision deletes the idle and pending runners so they are rebuilt from the new spec, and it restarts the listener. A restarted listener numbers its patches from 0 upwards and counts through every integer, so it does not merely risk reusing the leftover value, it passes through it. If that reuse lands on the reconcile that has to refill the pool, the guard suppresses exactly the scale up the revision change asked for. Clearing the marker where the applied revision advances is enough, because the marker only ever means "the gap below Spec.Replicas was made by cleaning up finished runners for this patch ID", and a spec change invalidates that claim outright. That read now bypasses the cache: the patch is a diff against the object that was read, so a cached copy showing 0 while the server held a marker would emit no entry for the field and leave the stale value behind. Clearing the marker also breaks the invariant the cached fast path in scaleUpServicedByFinishedRunnerCleanup rested on. That path was safe only because a marker was never removed, so a cache hit could not be a false positive. Now it can be: a lagging cache can show a marker the server has already cleared, which suppresses the rebuild. The decision is therefore always made against an uncached read. That costs one GET, and only on reconciles that were about to issue creates anyway. One window remains and is documented rather than papered over. A listener that restarts without a spec change keeps the marker and still renumbers from 0, so a collision is still likely. It costs one suppressed reconcile, not an outage: the listener calls back into scaling on every long-poll timeout rather than only on change, and an idle set at its minimum publishes the collapsed patch ID 0, which is never suppressed. This was found while investigating the update-gha-runner-scale-set e2e failure on the tip of the stack. It is a real defect, but it does not explain that failure, whose cause remains open: the suppression here is self-correcting within a long-poll cycle rather than terminal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Layers 2-4 removed every reader of the actions.github.com/integrity-hash annotation, so what remained were write-only dead paths. Delete the annotation constant, all remaining hash helpers that computed it, the call sites that stamped it onto objects, the integrity-hash fallback in newResourceCacheObjectRef, and the now-unused AutoscalingListenerSpec.Hash and EphemeralRunnerSpec.Hash methods. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

The review suggestion that reworded ActionableRevision's doc comment changed the generated CRD description too, but the manifests were not regenerated. CI compares the chart CRDs against config/crd/bases, so leave them in sync. Description text only; the schema is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Code review raised two upgrade consequences of no longer stamping actions.github.com/integrity-hash. Both are real, and neither is covered by a test, so cover them. The AutoscalingRunnerSet reconciler compares a live listener's annotations against the desired ones exactly, so a listener created by an older controller is replaced on the first reconcile after an upgrade. That rollout is one-time rather than a loop, because the replacement is built by the same path and carries no annotation. It is not additional either: the listener runs the manager's own image, so a controller upgrade already forces the same recreation through the Spec comparison. Reconcilers that update objects in place merge live annotations under the desired ones, so the annotation survives on objects that already carry it. Nothing reads it any more, so it is inert metadata; stripping it would mean patching every managed object on upgrade, which is a separate decision. Both tests were mutation-checked: re-stamping the annotation fails the first, and stripping the key in mergeAnnotations fails the second. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

An EphemeralRunnerSet goes Outdated when a child runner reports that the Actions service rejected its runner spec, and the AutoscalingRunnerSet then tears the listener down so the scale set stops taking jobs. Until now nothing recorded which runner spec a given Outdated report was about, so a report from a runner that predates a spec update kept the set switched off even after the user had already fixed the spec. Runners now carry the EphemeralRunnerSet's actionable revision as an annotation, and Outdated runners are split into two groups: - stale outdated: created before the currently applied revision. Their verdict is about a spec that no longer exists, so they are deleted and replaced by runners built from the current spec. - outdated: created at or after the applied revision. Their verdict is about the current spec, so they hold the set in the Outdated phase. They are deliberately not delete-and-replaced: a fresh runner at the same revision would report Outdated again, forever. patchAppliedActionableRevisionStatus now recomputes the phase in both directions against the revision being applied, because it returns from Reconcile without reaching updateStatus. The AutoscalingRunnerSet teardown guard additionally requires the applied revision to have caught up with the spec revision, so it does not act on an Outdated verdict for a spec that has already been replaced. terminated() now allocates a fresh slice instead of appending into the finished slice, which could alias its backing array. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Port two specs from the source PR that no layer of the stack had picked up. Both assert behaviour that is already implemented, they were simply left without coverage: - the EphemeralRunnerSet, not just the listener, is re-pointed at a newly registered runner scale set; - that re-pointing happens with no spec change on the AutoscalingRunnerSet at all, which is why drift detection cannot be keyed on metadata.generation: re-registration writes the scale set ID as an annotation, and metadata changes do not bump generation. Ported verbatim and passing unmodified. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…tests Six specs construct an EphemeralRunnerSetReconciler literal and call Reconcile directly instead of going through SetupWithManager, so the APIReader backfill never runs and the field stays nil. Nothing is broken today: those specs either publish patch ID 0 or return via the stale-outdated path, so they never reach the scale-up branch where the reader is consulted. But the next direct-Reconcile spec that exercises scale-up with a non-zero patch ID would fail with "APIReader is not configured" instead of the behaviour it meant to test, and the cause would not be obvious from the failure. Set the field the way SetupWithManager does in production, so the specs exercise the same wiring the real controller has. The helper still errors on a nil reader; falling back to the cached read is the race that fix exists to prevent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Layer 5 clears Status.FinishedRunnerCleanupPatchID beneath an early return that this layer removes, so a straight integration left the marker being cleared on every call. Guard the clear on the same advance it belongs to, and keep the phase recompute unconditional, which is what lets a spec update clear the Outdated phase immediately. Also register the resource owner index on the fake client, since the phase recompute lists the child runners the way the real manager does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

patchAppliedActionableRevisionStatus wrapped retry.RetryOnConflict around a plain client.MergeFrom status patch. A merge patch carries no resourceVersion precondition, so the API server can never reject the write as conflicting and the retry wrapper could never fire. Worse, the monotonicity check inside the retry was unsound. Both the target revision and the re-fetched object come from the cache-backed client, so a stale reconcile could compare a stale target against an equally stale read, pass the guard, and patch AppliedActionableRevision backwards. That re-satisfies Spec.ActionableRevision > Status.AppliedActionableRevision and sends the controller through the idle and pending runner cleanup again. MergeFromWithOptimisticLock stamps the re-fetched resourceVersion into the patch, so the server accepts it only if that object is still live. A successful patch therefore proves the guard was evaluated against live data, which is what makes the applied marker monotonic. A stale target can now only ever under-advance the marker, never regress it, and a later reconcile with fresh data completes the advance. At this layer the re-fetch inside the retry still uses the cached client, so a genuine conflict may refetch stale data, exhaust the backoff and return the conflict error. That requeues rather than writing a bad value, so it fails closed; it is only noisier than necessary. A later change makes the refetch authoritative so the retry can resolve the conflict in place. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The optimistic lock and the monotonicity guard in patchAppliedActionableRevisionStatus both prevent the applied revision marker from moving backwards, and neither was covered. Reproducing the lost update needs two reconciles racing on one object with a stale cached read, which is awkward to provoke deterministically. The observable property is cheaper: the emitted status patch must carry a resourceVersion precondition, since that is what lets the API server reject a stale write. The test intercepts SubResourcePatch and asserts on the bytes the reconciler actually emits, so it cannot pass while the patch is built some other way. Both assertions were mutation checked. Reverting the patch option to client.MergeFrom fails the precondition test, reporting the emitted patch as {"status":{"appliedActionableRevision":7}} with no resourceVersion. Disabling the >= guard fails the backwards test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…sion' into nikola-jokic-ers-scale-up-after-cleanup # Conflicts: # controllers/actions.github.com/ephemeralrunnerset_controller.go

patchFinishedRunnerCleanupPatchIDStatus wrapped a plain merge patch in retry.RetryOnConflict. A plain merge patch carries no resourceVersion precondition, so the API server has nothing to reject: the write always succeeds, the retry can never fire, and the surrounding machinery reads as protection while providing none. The exposure is worse here than for the applied revision, which at least refuses to move backwards. This helper compares for equality and then writes whatever patch ID the reconcile is carrying, so it is willing to lower the marker. A reconcile serving an older patch ID can therefore overwrite a marker recorded for a newer one, and the scale-up guard then stops suppressing for the patch it actually serviced -- creating the replacement runners this layer exists to prevent. Re-fetching through the API reader narrows that window to the gap between the read and the patch, but it cannot close it, because the decision is only as fresh as the moment it was taken. The optimistic lock is what makes the write conditional on that decision still holding; a stale attempt now conflicts and is retried rather than silently recording the wrong patch ID. The test asserts on the patch bytes the reconciler actually emits rather than on an independently constructed patch, so it cannot pass while the production code builds its patch some other way. A second test covers the early return, since an unconditional write would churn the status on every reconcile for the same patch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…cleanup' into nikola-jokic-remove-integrity-hash-annotation

…sh-annotation' into nikola-jokic-revision-aware-outdated

The neighbouring applied-revision helper refuses to move its marker backwards, so the obvious tidy-up is to make this one match. That would break it. Applied revisions derive from metadata.generation and only ever climb. Listener patch IDs do not. setDesiredWorkerState publishes 0 whenever the set is idle at MinRunners with nothing dirty, restarts its sequence from 0 when the listener restarts, and wraps explicitly at MaxInt32, so Spec.PatchID legitimately moves down. The marker only means "the gap below Spec.Replicas was created by cleaning up for exactly this patch ID", so it has to follow the patch ID wherever it goes. A marker that refused to descend would sit above every value the listener went on to publish, and since the guard suppresses only on an exact match, suppression would never fire again -- silently disabling the behaviour this layer adds. This also settles what the optimistic lock is and is not for. It cannot make an older patch ID unwritable, because the retry re-reads and re-applies the same argument, and recording the patch ID whose cleanup actually happened is a true statement regardless of ordering. What it prevents is a write decided against state that has since changed. The test pins the descending case, so a future reader who reaches for >= gets a failure rather than a silently inert guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…cleanup' into nikola-jokic-remove-integrity-hash-annotation

patchAppliedActionableRevisionStatus reads the EphemeralRunnerSet through the uncached reader precisely because a cached read cannot be trusted there, then derived Status.Phase from a cached list of the child runners. The optimistic lock on the patch covers the EphemeralRunnerSet object alone, so it could not vouch for that list, while a successful write made the result look consistent. The list also sits inside RetryOnConflict, where a cached read can return the same stale data on every attempt. The ownership filter moves into the process, because resourceOwnerKey is a client-side index on the manager's cache and the API server rejects .metadata.controller as an unsupported field label. The predicate lives next to the indexer so the two do not drift apart. Add the index to the revision patch test fakes, which need it now that the function lists children, and a test that separates the two reads: it fails when the list goes through the cache and passes when it does not. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…sh-annotation' into nikola-jokic-revision-aware-outdated

Reconcilers deep copied the object they had just fetched on every single reconcile, purely so a merge patch could be computed on the rare pass that actually changes something. The copy is a full recursive walk and allocation of the object, and the overwhelming majority of reconciles throw it away untouched. Introduce lazyCopy, which takes the snapshot on the first call to Mutate and hands back the live object. Because Mutate is the only way to reach the object, the snapshot cannot be taken after the mutation it is supposed to be diffed against, which is the way this optimization is usually gotten wrong. Apply it to the four Reconcile entry points, and move the two EphemeralRunnerSet status copies inside the branch that patches, so they are only paid for when the status really changed. While here, drop the short circuit in the EphemeralRunner finalizer block: `addedFinalizers || AddFinalizer(...)` skipped adding the actions finalizer whenever the first finalizer was added. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

lazycopy.go introduces deepCopyObject, a generic type constraint rather than a collaborator, and the package's "all: true" mockery config picks it up and emits a large mock that nothing can use. Switch the package to an explicit include/exclude regex pair so every other interface is still discovered automatically while deepCopyObject is skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The controller derived Status.Phase directly from the pod phase, so a runner became Running as soon as its pod started, whether or not it had picked up a job. That made Running mean "the pod is up" instead of "the runner is busy", and it left the EphemeralRunnerSet scale-down path unable to tell an idle runner from one that is executing a job. The listener already knows when a job is assigned to a specific runner, so move the transition there. HandleJobStarted now reads the runner first and only promotes it to Running when it is not terminal (Failed, Succeeded or Outdated) and not being deleted, then patches the phase alongside the job fields it already writes. The listener role gains "get" on ephemeralrunners for that read. On the controller side updateRunStatusFromPod keeps publishing the initial Pending phase while the pod is starting, and no longer promotes to Running. Runners waiting for work now stay Pending, so scale-down picks them before runners that are actually executing a job. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Every controller in this package woke up on every update of the resources it owns, including the ones that only touch fields it never reads. An EphemeralRunner status carries readiness, failure bookkeeping and the job details written by the listener, a pod reports IPs, its node, a start time and several conditions, and an EphemeralRunnerSet rewrites Status.FinishedRunnerCleanupPatchID for every listener patch id. None of that is input to the owner, yet all of it enqueued a reconcile. Add an update predicate to each owned watch. A predicate is only allowed to be an optimisation, so each one is written as the projection of the fields its reconciler actually reads and drops an update only when all of them are equal: - AutoscalingRunnerSet -> EphemeralRunnerSet: object metadata, the whole spec, and the Status.Phase and Status.AppliedActionableRevision pair that ephemeralRunnerSetOutdatedForAppliedRevision consults. - EphemeralRunnerSet -> EphemeralRunner: object metadata, spec, Status.Phase and Status.RunnerID. - EphemeralRunner -> Pod: UID, deletion timestamp, pod phase, reason and message, the container and init container statuses, and the Ready condition. An event of an unexpected type is always delivered, and create, delete and generic events are untouched. The primary watches keep seeing every update, so the status patches a reconciler makes to hand work to its own next pass, such as marking a runner Failed or Outdated before cleaning up its resources, still re-enqueue. The Ready condition lookup is extracted into podReady so that the predicate and updateRunStatusFromPod cannot disagree about what readiness means. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…tdated When the runners reject the runner spec they were given, the listener has to stop acquiring jobs the scale set cannot run. The controller removed the listener but then deleted the EphemeralRunnerSet, and left the AutoscalingRunnerSet phase on Running. AutoscalingRunnerSetPhaseOutdated was declared and read, but never assigned by anything. Nothing held the scale set switched off as a result. The next reconcile saw a missing EphemeralRunnerSet, created it, created a listener for it, and the fresh runners rejected the same spec again, so the scale set churned through create and teardown cycles against the Actions service instead of resting. Record the outdated phase and keep the EphemeralRunnerSet, pinned to zero replicas and patch id. It releases every runner that is not executing a job while the phase keeps the listener from being rebuilt, and the revision bookkeeping that decides when the scale set may run again is preserved. Recovery is driven by the spec update that the phase is waiting for: it moves the phase back to pending, and the runner spec is then republished to the set with an advanced revision 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, so without advancing it a scale set could only be recovered by editing the pod template, and an edit to anything else would switch the listener back on against a set parked at zero. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

+		if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) ||
+			ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) {


nikola-jokic and others added 17 commits September 10, 2026 22:56

Apply batched suggestions from code review

92f87b1

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Apply batched suggestions from code review

5326101

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Update log message for terminated ephemeral runners

145a4fc

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

nikola-jokic requested review from a team, Steve-Glass, mumoshu, rentziass and toast-gear as code owners September 10, 2026 21:03

This was referenced Sep 10, 2026

Guard the listener's Running phase write with resourceVersion #4648

Open

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

Merged

nikola-jokic added this pull request to stack #4645 September 10, 2026 21:11

nikola-jokic and others added 5 commits September 11, 2026 09:03

Move deletion of terminated runners after patch update

e69d9e3

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix formatting in ephemeralrunnerset_controller_test.go

f862d6f

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Merge remote-tracking branch 'origin/nikola-jokic-ers-actionable-revi…

5464e15

…sion' into nikola-jokic-ers-scale-up-after-cleanup # Conflicts: # controllers/actions.github.com/ephemeralrunnerset_controller.go

nikola-jokic and others added 11 commits September 11, 2026 10:57

Merge remote-tracking branch 'origin/nikola-jokic-ers-scale-up-after-…

c749648

…cleanup' into nikola-jokic-remove-integrity-hash-annotation

Merge remote-tracking branch 'origin/nikola-jokic-remove-integrity-ha…

bf2b89a

…sh-annotation' into nikola-jokic-revision-aware-outdated

Merge remote-tracking branch 'origin/nikola-jokic-ers-scale-up-after-…

6b6011c

…cleanup' into nikola-jokic-remove-integrity-hash-annotation

Merge remote-tracking branch 'origin/nikola-jokic-remove-integrity-ha…

f7dd0d2

…sh-annotation' into nikola-jokic-revision-aware-outdated

nikola-jokic force-pushed the nikola-jokic/ars-outdated-phase branch from 24beb32 to e2768dd Compare September 11, 2026 09:45

Copilot AI lite review requested due to automatic review settings September 11, 2026 09:45

Copilot started reviewing on behalf of nikola-jokic September 11, 2026 09:46 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread controllers/actions.github.com/autoscalingrunnerset_controller.go

Comment on lines +232 to +233

if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) ||

ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) {

Navigation Menu

Uh oh!

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

Switch the scale set off instead of rebuilding it when runners are outdated#4652
nikola-jokic wants to merge 34 commits into
nikola-jokic/workqueue-predicatesfrom
nikola-jokic/ars-outdated-phase

nikola-jokic commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) \|\|
		ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) {

Uh oh!

Conversation

nikola-jokic commented Sep 10, 2026

The problem

The intended lifecycle

The change

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants