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