Defer scale up until the listener publishes a state that accounts for finished runners by nikola-jokic · Pull Request #4642 · actions/actions-runner-controller

nikola-jokic · 2026-09-09T10:24:31Z

Layer 5 of a 7-layer stack splitting #4575. Layers 1 through 4 have landed (#4634, #4635, #4636, #4638), so this now targets master directly and no longer depends on an unmerged branch.

Those layers merged as squashes, so the commits this branch was built on no longer exist by patch ID and master has been merged in to reconcile the two histories. That merge touches only the five files this layer already changes; the resolution keeps master's content whole and adds this layer on top.

Unlike the rest of the stack, this is not a refactor. Every other layer replaces one way of detecting an update with another and leaves behaviour alone; this one changes when the EphemeralRunnerSet controller creates runners. That is why it is isolated here rather than buried in a detection change.

The problem

Spec.Replicas is not an independent truth. It is the count the listener asked for at the moment it published Spec.PatchID, computed against the runners that were alive then. The controller used to clean up finished runners in a defer and run the scaling comparison in the same reconcile, against a live count that the cleanup was about to reduce. So a patch that said "I want 4" and arrived just after a job finished caused the controller to delete the finished runner and immediately create a replacement for a job that had already completed. Nobody asked for that runner: it registers, idles, and the next listener patch scales it back down.

Separately, runners that were mid-deletion were not counted at all. They still exist and still hold their registration, so ignoring them let the controller create replacements while the deletions were still in flight.

The change

Finished-runner cleanup is now an explicit early return rather than a defer. The controller deletes the finished runners, records the patch ID the cleanup was performed for in the new Status.FinishedRunnerCleanupPatchID, and returns. The scaling decision happens on the next reconcile, against post-cleanup data. The defer also swallowed its error; the explicit path returns it.

While Spec.PatchID still equals the recorded cleanup patch ID, scale up is suppressed. The gap below Spec.Replicas at that point is the one the cleanup just opened, not new demand.

Deleting runners are added to the scale-up total, so the controller does not over-create while deletions are in flight. Scale-down is deliberately left comparing against the live total: counting runners that are already going away as present would block a legitimate scale down.

cleanupFinishedEphemeralRunners becomes deleteTerminatedEphemeralRunners, since layer 7 uses it for stale outdated runners too.

Reading the marker safely

Three follow-ups hardened the guard after review. They are worth understanding together, because each exists because the previous one changed an assumption.

The guard reads Status.FinishedRunnerCleanupPatchID through an uncached APIReader, not the manager's cache. The marker is written by one reconcile and read by the next, and the deletions that cleanup performs are themselves what schedule that next reconcile — which can therefore be served from an informer cache that has not yet observed the controller's own status write. CI caught this as a real failure, not a theoretical one. The extra GET is confined to scale-up decisions, where the controller is about to issue N creates anyway.

The marker is cleared when the applied actionable revision advances. A patch ID is only meaningful within one listener incarnation: applying a new revision deletes the idle and pending runners so they rebuild from the new spec, and it restarts the listener, which renumbers its patches from 0 upwards and so passes through any leftover value rather than colliding with it by chance. Carrying the marker across that boundary would suppress exactly the scale up that rebuilds the pool.

Clearing the marker is also why there is no cached fast path. An earlier version short-circuited on a cached hit, justified by "the marker is only ever set, so a hit cannot be a false positive". Clearing it destroys that invariant: a lagging cache can now show a marker the API server has already cleared. Keeping the fast path alongside the clear would have reintroduced the original bug through the back door.

Both status patches carry an optimistic lock. retry.RetryOnConflict around a plain merge patch is inert — a merge patch has no resourceVersion precondition, so the server cannot reject it, the retry never fires, and the surrounding machinery reads as protection while providing none.

Maintenance warning: the equality check is deliberate

patchFinishedRunnerCleanupPatchIDStatus compares == patchID, while its neighbour patchAppliedActionableRevisionStatus uses >=. Do not make them match. They look like the same monotonic counter and have opposite correctness conditions.

Applied revisions derive from metadata.generation and only ever climb. Listener patch IDs do not. In cmd/ghalistener/scaler/scaler.go, setDesiredWorkerState publishes 0 whenever the set is idle at MinRunners with nothing dirty, a listener restart resets patchSeq to -1 so the sequence begins again at 0, and there is an explicit math.MaxInt32 wraparound. Spec.PatchID descending is normal operation, and the marker has to follow it.

A marker that refused to descend would strand above every value the listener subsequently publishes, and because the guard suppresses only on an exact match, suppression would never fire again — silently disabling this layer while reading as stricter than before. TestPatchFinishedRunnerCleanupPatchIDStatusRecordsALowerPatchID exists solely to fail if someone makes that change. Nothing in the controller shows you the producer, so the property is not checkable from a diff.

The remaining sharp edge

The suppression assumes the listener publishes a fresh patch ID after each job completes. HandleJobCompleted sets w.dirty = true, and the collapse to patch ID 0 requires !dirty, so a completed job always produces a fresh incrementing patch ID. The PatchID > 0 condition means the idle-at-minimum case — which is exactly the case that publishes 0 — is never suppressed, so a scale set cannot get stuck below its minimum.

A listener restart that does not advance the revision still keeps its marker while renumbering from 0, so a collision remains likely there. That costs one suppressed reconcile rather than an outage: the listener re-enters scaling on every long-poll timeout, not only on a state change (getMessage returns nil, nil on HTTP 202 and the loop calls HandleDesiredRunnerCount unconditionally), so the shortfall is filled on the next cycle. Worth knowing that the recovery is a property of the listener's timer rather than something this guard earns.

Tests

Three behaviours, all in ephemeralrunnerset_controller_test.go. I verified each one fails against the old behaviour rather than just passing against the new:

Finished-runner cleanup defers the scaling decision — a stale patch cleans up the finished runner and stops there, then converges once the listener decrements.

Deleting runners count towards the scale-up total — a runner is deleted with its finalizer held so it lingers in the deleting state, and no replacement appears until the deletion completes. Reverting scaleUpTotal to total fails this with 3 objects instead of 2.

Scale up is suppressed for an already-serviced patch ID and resumes on a new one. Removing the guard fails this with a runner created during the suppression window.

A few existing specs needed updating because they encoded the old behaviour: they patched a new desired count that happened to coincide with a cleanup and expected the replacement in the same step. They now do what the listener does and publish a fresh patch. One Consistently became Eventually for the same reason. An unrelated succeeded != 1 && running != 1 was a bug that made the assertion nearly unfailable; it is now ||.

The later hardening commits add unit-level tests with fake clients, deliberately not envtest: the cache-staleness race they cover is timing-dependent under CI load and passes locally either way. Each was proven non-vacuous by reverting the specific behaviour and confirming that only the intended test fails, and that it fails by assertion rather than by panic.

Validation

go build ./..., go vet ./..., gofmt -l apis/ controllers/ clean. make manifests changes only ephemeralrunnersets.yaml, picking up finishedRunnerCleanupPatchID; both chart CRD copies are byte-identical to config/crd/bases. Full envtest suite for ./controllers/actions.github.com/ and ./apis/... passes.

Copilot

Copilot review overview

🟡 Changes recommended

The new suppression/marker logic has a few correctness/operational sharp edges (notably around cache staleness and status writes) that should be addressed before approval.

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

Review tier: Lite
Findings: 2 · 1

New issues introduced by this change (3)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — When suppression is determined via APIReader (cache is stale), the in-memory EphemeralRunnerSet may…
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — patchFinishedRunnerCleanupPatchIDStatus says it "re-fetches the object" to avoid replaying stale…
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — The error log message still says "cleanup finished" even though the function was renamed and now…

What changed in this PR

This PR changes EphemeralRunnerSet reconciliation so finished-runner cleanup happens in a dedicated reconcile pass (recorded in status) and scale-up is deferred/suppressed until the listener publishes a fresh desired-state patch, preventing the controller from creating “replacement” runners for jobs that already finished. It also counts deleting runners toward scale-up capacity to avoid over-creating while deletions are in flight.

Changes:

Replace deferred finished-runner cleanup with an explicit early-return cleanup path that records status.finishedRunnerCleanupPatchID.
Suppress scale-up when the shortfall is attributable to finished-runner cleanup for the current listener patch ID, and count deleting runners in scale-up totals.
Add/adjust tests and extend the CRD + Go types with the new status field.

File	Description
controllers/actions.github.com/ephemeralrunnerset_controller.go	Implements early-return cleanup, scale-up suppression via APIReader, and deleting-runner accounting.
controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go	New focused unit test for the cache-vs-APIReader suppression decision.
controllers/actions.github.com/ephemeralrunnerset_controller_test.go	Updates/extends envtest specs to assert the new cleanup/deferral/suppression behaviors.
apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go	Adds `FinishedRunnerCleanupPatchID` to EphemeralRunnerSetStatus.
config/crd/bases/actions.github.com_ephemeralrunnersets.yaml	CRD schema update for `finishedRunnerCleanupPatchID`.
charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml	Vendored CRD updated to include `finishedRunnerCleanupPatchID`.
charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml	Vendored CRD updated to include `finishedRunnerCleanupPatchID`.

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

humh25 · 2026-09-09T17:45:15Z

536975948763 humbertohlgl El sept 9, 2026, 04:24 -0600, Nikola Jokic ***@***.***>, escribió:

Copilot

Copilot review overview

🟡 Changes recommended

Address the two moderate controller issues and strengthen the cleanup regression test.

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

Review tier: Lite
Findings: 1 · 1

New issues introduced by this change (2)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — The deletion is irreversible, but the durable marker is written only afterward. If the status patch…
	`controllers/actions.github.com/ephemeralrunnerset_controller_test.go` — This test is meant to prove that finished runners remain present until the patch ID changes, but…

Issues resolved since last review (3)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — The error log message still says "cleanup finished" even though the function was renamed and now… View resolved comment
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — patchFinishedRunnerCleanupPatchIDStatus says it "re-fetches the object" to avoid replaying stale… View resolved comment
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — When suppression is determined via APIReader (cache is stale), the in-memory EphemeralRunnerSet may… View resolved comment

Suppressed comments (1)

controllers/actions.github.com/ephemeralrunnerset_controller.go:364

FinishedRunnerCleanupPatchID is only keyed by the raw patch ID, but the listener's patchSeq is initialized from -1 and starts over after a listener restart (cmd/ghalistener/scaler/scaler.go:47-60). A restarted listener can therefore publish a nonzero ID equal to this stale marker; when the set is below Spec.Replicas, this guard suppresses a legitimate scale-up until another patch arrives, and repeated restarts can keep capacity below demand. Include a listener incarnation in the marker/comparison or clear the marker when that incarnation changes.

// One window remains. A listener that restarts without a spec change keeps the
// marker but starts its patch sequence again from 0 and counts up through every
// integer, so it passes through the recorded value with near-certainty rather
// than by coincidence. If that collision lands on a reconcile that needs to
// scale up, that reconcile is suppressed.

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>

Copilot

Copilot review overview

🔵 Needs a closer look

Address the controller marker/cache issues and restore the weakened regression assertion.

Review tier: Lite
Findings: None

Issues resolved since last review (2)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller_test.go` — This test is meant to prove that finished runners remain present until the patch ID changes, but… View resolved comment
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — The deletion is irreversible, but the durable marker is written only afterward. If the status patch… View resolved comment

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>

Copilot

Copilot review overview

🟡 Changes recommended

The controller may restore a stale cleanup marker through a later status update, potentially suppressing valid scale-up.

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/ephemeralrunnerset_controller.go` — Avoid restoring a stale cleanup marker

…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>

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>

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>

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>

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>

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>

…rs-scale-up-after-cleanup The stack was rebased server-side, so every branch below this one was rewritten and this branch's earlier merge of the actionable-revision base now points at commits that no longer exist on it. Git therefore fell back to the nearest shared ancestor, which is master at the point layer 2 landed, and re-reported the whole of layers 3 and 4 as divergent. That is why the conflicts look large: they are a rebase artifact, not a real disagreement about the code. Checking the base commits against this branch with patch IDs shows every one of them is already present here, so for the five conflicted files this branch is the base plus its own work and its side is the correct resolution. The conflicts in ephemeralrunnerset_types.go, the three generated CRDs and ephemeralrunnerset_controller.go are all the same shape: the base's content with FinishedRunnerCleanupPatchID threaded through it. The merge is still needed rather than being discarded outright, because the base has moved on to a newer master than this branch has and carries the dependabot action bumps in .github/workflows. Those merged cleanly and are kept. Resolved by taking this branch's side for the five conflicted files, then regenerating the CRDs, which produced no drift. Build, vet and gofmt are clean and the controller envtest suite passes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Review has now read the marker assignment in Reconcile the wrong way round twice, in opposite directions: once as updateStatus writing a stale zero over an authoritative value, and once as it restoring a superseded value over a newer one. Both readings assume updateStatus can emit that field at all. It cannot. updateStatus copies the marker and the applied revision verbatim into desiredStatus and diffs against a copy taken at entry, so both sides of the diff hold the same value and a JSON merge patch emits nothing for it. The only key the function can produce is the phase. That is what makes it safe for Reconcile to carry a cached object into the final status patch, and it is not visible from reading the assignment. Assert it on the emitted patch bytes rather than on the resulting object, because the two concerns raised differ only in which value the patch would have carried. Capturing the bytes shows the field is absent, which answers both at once. Restoring the marker from a cached read would become real if the field were ever computed inside updateStatus instead of copied, so the test is written to fail on that change: with original's marker forced to zero it fails both cases with {"status":{"finishedRunnerCleanupPatchID":4,"phase":"Running"}}. No production change. The assignment stays: it is inert with respect to the API server either way, and it keeps the in-memory object honest if updateStatus ever stops copying. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The doc comment claimed the test fails if updateStatus ever computes the cleanup marker instead of copying it. It did not. The fixture set Spec.PatchID and the carried status marker to the same number, on the reasoning that the cleanup path assigns one from the other, so computing the field from the spec produced the same value as copying it from the status and no assertion could see the difference. Substituting Spec.PatchID into desiredStatus passed. Give the spec its own value and both readings the test was written to refute become visible: the emitted patch carries {"status":{"finishedRunnerCleanupPatchID":7,...}} over a server the reconcile had no authority to change. The other axis, taking original before the assignment rather than after, still fails as it did. Distinct values are also the more representative fixture. Outside the cleanup path the marker records some earlier patch while Spec.PatchID is whatever the listener has since published, so equality was the unusual case, not the normal one. run now refuses equal values rather than documenting the requirement, because a comment asking future readers to keep two numbers different is the kind of thing that gets tidied away. Two independent values given the same number cannot distinguish the implementations that read them, and the resulting test passes while appearing to cover the case. Test-only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Layers 3 and 4 landed on master as squash merges (#4636, #4638), so the commits this branch built on no longer exist by patch ID. Git fell back to the dependabot bump both sides share and re-reported all of both layers as divergent, conflicting in the five files this layer also touches. Took this side for all five. That is only sound if this branch already contains everything the squashes added, so I checked that from the inputs rather than assuming it. For the types file and the three CRDs the resolution is a pure addition over master -- seven added lines, zero deleted -- which is a stronger statement than patch-ID equivalence because it also rules out a base change having been partly reverted here later. The controller was the one file needing real justification, since it drops fifteen lines of master. Each is a line this layer deliberately replaces: the deferred cleanup, cleanupFinishedEphemeralRunners, the scale-up comparing against total rather than scaleUpTotal, and the cached read in patchAppliedActionableRevisionStatus. Of the fifty lines master added, three are absent, and all three are accounted for: the cached Get, and the Phase and AppliedActionableRevision fields, which are present but realigned by gofmt because the struct literal gained a longer field name. Checked by function set rather than by diff alone, since alignment can swallow a whole function without leaving a mark. The only one of master's functions missing is cleanupFinishedEphemeralRunners, which this layer renames to deleteTerminatedEphemeralRunners. Layer 4's optimistic lock and its >= guard survive; the lock count goes from one to two because this layer adds its own. No Ginkgo spec was lost from either side: every It() name on master and on this branch is present in the result. make manifests reports no drift and the chart CRDs stay byte-identical to config/crd/bases. rerere staged all five resolutions automatically. That is agreement with a cached resolution, not evidence, so none of the above rests on it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🔵 Needs a closer look

The controller’s reconciliation and scaling behavior changes are operationally significant and warrant final human review; a documentation nit remains.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — Cleanup marker ordering comment contradicts the implementation

Issues resolved since last review (1)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — Avoid restoring a stale cleanup marker View resolved comment

+// Like the applied revision above, this is written after the deletions succeed
+// and re-fetches the object inside the retry, so a conflicting write is never
+// resolved by replaying a status that predates the cleanup.


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

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

nikola-jokic mentioned this pull request Sep 9, 2026

Use ActionableRevision and observed generation to track update propagation #4575

Draft

Copilot AI lite review requested due to automatic review settings September 9, 2026 12:51

Copilot started reviewing on behalf of nikola-jokic September 9, 2026 12:51 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

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

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

Comment thread controllers/actions.github.com/ephemeralrunnerset_controller.go Outdated

rentziass previously approved these changes Sep 10, 2026

View reviewed changes

nikola-jokic dismissed rentziass’s stale review via e3127cd September 10, 2026 09:00

nikola-jokic force-pushed the nikola-jokic-ers-scale-up-after-cleanup branch from 5af8379 to b359cb4 Compare September 10, 2026 09:13

nikola-jokic force-pushed the nikola-jokic-ers-scale-up-after-cleanup branch from b359cb4 to 97a2d1a Compare September 10, 2026 09:20

nikola-jokic mentioned this pull request Sep 10, 2026

Make the Outdated phase revision-aware #4644

Open

nikola-jokic force-pushed the nikola-jokic-ers-scale-up-after-cleanup branch from 97a2d1a to c317bf1 Compare September 10, 2026 09:52

nikola-jokic requested a lite review from Copilot September 10, 2026 14:40

Copilot started reviewing on behalf of nikola-jokic September 10, 2026 14:41 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread controllers/actions.github.com/ephemeralrunnerset_controller.go Outdated

Comment thread controllers/actions.github.com/ephemeralrunnerset_controller_test.go Outdated

nikola-jokic and others added 7 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>

Copilot AI reviewed Sep 11, 2026

View reviewed changes

nikola-jokic requested a lite review from Copilot September 11, 2026 08:36

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

Copilot AI reviewed Sep 11, 2026

View reviewed changes

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

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

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

Apply batched suggestions from code review

223250b

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

nikola-jokic and others added 3 commits September 11, 2026 12:12

Base automatically changed from nikola-jokic-ers-actionable-revision to master September 11, 2026 12:44

nikola-jokic requested a lite review from Copilot September 11, 2026 12:55

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

Copilot AI reviewed Sep 11, 2026

View reviewed changes

rentziass approved these changes Sep 11, 2026

View reviewed changes

nikola-jokic merged commit 484564e into master Sep 11, 2026
29 checks passed

nikola-jokic deleted the nikola-jokic-ers-scale-up-after-cleanup branch September 11, 2026 13:29

Navigation Menu

Uh oh!

Defer scale up until the listener publishes a state that accounts for finished runners - #4642

Defer scale up until the listener publishes a state that accounts for finished runners#4642
nikola-jokic merged 30 commits into
masterfrom
nikola-jokic-ers-scale-up-after-cleanup

nikola-jokic commented Sep 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

humh25 commented Sep 9, 2026 via email

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

nikola-jokic commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

The change

Reading the marker safely

Maintenance warning: the equality check is deliberate

The remaining sharp edge

Tests

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

humh25 commented Sep 9, 2026 via email

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nikola-jokic commented Sep 9, 2026 •

edited

Loading