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