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