Make the Outdated phase revision-aware - #4644
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
patchAppliedActionableRevisionStatus can regress Status.AppliedActionableRevision under a stale reconcile, which can reintroduce incorrect Outdated/teardown behavior.
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 — patchAppliedActionableRevisionStatus now unconditionally sets… |
What changed in this PR
This PR makes the EphemeralRunnerSet “Outdated” phase revision-aware so that Outdated reports from runners built from a superseded runner spec don’t keep the set switched off after the user has already updated the spec. It does this by stamping runners with the set’s actionable revision and splitting “Outdated” runners into “current” vs “stale” based on the applied revision being judged, and it tightens the AutoscalingRunnerSet teardown condition to avoid acting on transient/stale Outdated status.
Changes:
- Stamp each
EphemeralRunnerwithactions.github.com/actionable-revisionand classify Outdated runners asoutdatedvsstaleOutdatedbased on the revision being applied/judged. - Recompute
EphemeralRunnerSetphase duringpatchAppliedActionableRevisionStatusagainst the revision being applied, so a spec update can clear Outdated immediately and stale phase values don’t persist. - Update controller logic and tests to delete/replace stale-outdated runners and to gate listener teardown on “outdated for the applied revision”.
| File | Description |
|---|---|
| controllers/actions.github.com/resourcebuilder.go | Stamp new runners with the set’s actionable revision annotation. |
| controllers/actions.github.com/helpers.go | Add ephemeralRunnerSetOutdatedForAppliedRevision guard for safe teardown decisions. |
| controllers/actions.github.com/helpers_outdated_test.go | Add focused unit tests for revision-scoped outdated classification and teardown guard. |
| controllers/actions.github.com/ephemeralrunnerset_controller.go | Revision-aware runner classification, stale-outdated deletion path, phase recomputation in status patching, and safer terminated() concatenation. |
| controllers/actions.github.com/ephemeralrunnerset_controller_test.go | Add envtest coverage for deleting stale-outdated runners and ensuring set doesn’t go Outdated because of them; update existing spec to annotate revision. |
| controllers/actions.github.com/constants.go | Introduce AnnotationKeyActionableRevision. |
| controllers/actions.github.com/autoscalingrunnerset_controller.go | Use the new “outdated for applied revision” guard instead of a bare phase check. |
| controllers/actions.github.com/autoscalingrunnerset_controller_test.go | Add/extend coverage around re-registration propagation behavior (incl. EphemeralRunnerSet scale set ID + revision bump). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
536975948763
humbertohlgl
El sept 9, 2026, 04:50 -0600, Copilot ***@***.***>, escribió:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 3
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
controllers/actions.github.com/helpers_outdated_test.go — This test does not exercise the aliasing bug it claims to guard: with one finished and one… |
|
controllers/actions.github.com/resourcebuilder.go — The new revision-scoping behavior depends on this annotation, but the existing ResourceBuilder test… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
controllers/actions.github.com/ephemeralrunnerset_controller.go — patchAppliedActionableRevisionStatus now unconditionally sets… View comment |
Suppressed comments (1)
controllers/actions.github.com/ephemeralrunnerset_controller.go:327
- This assignment can regress the applied marker under a stale/concurrent reconcile. The function re-fetches inside a retry, and
WithMaxConcurrentReconcilesallows multiple reconciles, so another invocation may already have advancedlatest.Status.AppliedActionableRevisionbeyondtargetAppliedRevisionbefore this block runs. Writing the older target back—and classifying the runners below against that older revision—can make the controller redo cleanup or treat a superseded Outdated report as current. Keep the applied revision monotonic and use the effective latest revision for the classification.
latest.Status.AppliedActionableRevision = targetAppliedRevision
|
536975948763
humbertohlgl
El sept 9, 2026, 04:40 -0600, Nikola Jokic ***@***.***>, escribió:
|
The merge-base changed after approval.
eb57c9f to
a68ee9e
Compare
|
Note for reviewers — cross-layer coupling with #4642 that is not visible in either diff on its own. #4642 clears This PR deliberately removes that early return, because the phase recompute has to run unconditionally for a spec update to clear The failure mode is worth noting in itself: git auto-merges the two cleanly, there is no compile error, and neither layer has a test that fails in its own tree. It surfaces only when the combined tree is executed — and even then the package prints Resolution in Two things a reviewer should check rather than take on trust:
Residual, documented in the code rather than glossed: a listener restart without a spec change keeps the marker and still renumbers from 0, so a collision remains possible. It costs one suppressed reconcile, not an outage, because the listener re-enters scaling on every long-poll timeout and an idle set at |
a68ee9e to
201c5ea
Compare
aaf895f to
2140ac5
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
An unresolved moderate finding requests regression coverage for the actionable-revision annotation, with additional phase and replacement coverage gaps.
Review tier: Lite
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
controllers/actions.github.com/resourcebuilder.go — The new revision-scoping behavior depends on this annotation, but the existing ResourceBuilder test… View comment |
|
controllers/actions.github.com/helpers_outdated_test.go — This test does not exercise the aliasing bug it claims to guard: with one finished and one… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
controllers/actions.github.com/ephemeralrunnerset_controller.go — patchAppliedActionableRevisionStatus now unconditionally sets… View resolved comment |
Suppressed comments (3)
controllers/actions.github.com/ephemeralrunnerset_controller.go:377
- This branch is responsible for clearing a previously
Outdatedset when a newer revision is applied, but the added envtest starts withRunningand the revision already applied, so it never exercises that transition. Please add a regression case that starts with an Outdated status, advances the spec revision while the old runner remains, and verifies the status returns to Running; otherwise a later change could leave the scale set switched off after the fix is applied.
if len(state.outdated) > 0 {
latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated
} else {
latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning
controllers/actions.github.com/ephemeralrunnerset_controller_test.go:2856
- The stale-outdated branch returns immediately after deleting the runner, so the following reconcile's scale-up path is what must create its replacement. This fixture leaves
Spec.Replicasat its zero default and never asserts a new runner, so the test would still pass if this return path permanently lost capacity; use a positive desired replica count and verify that a replacement is created from the current revision.
// The stale runner is removed rather than being treated as a verdict on the
// current spec.
Eventually(func(g Gomega) {
controllers/actions.github.com/resourcebuilder.go:833
- This new annotation is the only stamp used by
ephemeralRunnerActionableRevisionto decide whether an Outdated report belongs to the current spec, but the existingnewEphemeralRunnertest never asserts it. A regression here would make every newly created runner look like revision 0 and be discarded as stale after the first update; please add a resource-builder assertion that the annotation equalsSpec.ActionableRevision(and remains controller-owned when runner metadata supplies annotations).
annotations[AnnotationKeyActionableRevision] = strconv.FormatInt(ephemeralRunnerSet.Spec.ActionableRevision, 10)
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Two moderate issues remain: stale retry revision handling and missing builder-level annotation coverage.
Review tier: Lite
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
controllers/actions.github.com/resourcebuilder.go — The new revision-scoping behavior depends on this annotation, but the existing ResourceBuilder test… View comment |
|
controllers/actions.github.com/helpers_outdated_test.go — This test does not exercise the aliasing bug it claims to guard: with one finished and one… View comment |
2140ac5 to
d099740
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate findings remain in the controller.
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 (2)
| Severity | Finding |
|---|---|
 |
controllers/actions.github.com/ephemeralrunnerset_controller.go — Use the live applied revision when classifying runners |
|
controllers/actions.github.com/ephemeralrunnerset_controller.go — Avoid namespace-wide API reads during revision cleanup |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
controllers/actions.github.com/resourcebuilder.go — The new revision-scoping behavior depends on this annotation, but the existing ResourceBuilder test… View resolved comment |
|
controllers/actions.github.com/helpers_outdated_test.go — This test does not exercise the aliasing bug it claims to guard: with one finished and one… View resolved comment |
The merge-base changed after approval.
729ec84 to
854c051
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fix the Outdated cleanup path so deleting stale legacy runners also clears the phase when no current Outdated runners remain.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 1
Open findings (1)
Resolved findings (1)
| if len(ephemeralRunnersByState.staleOutdated) > 0 { | ||
| log.Info( | ||
| "Deleting outdated ephemeral runners created before the last spec update so they can be replaced", | ||
| "count", len(ephemeralRunnersByState.staleOutdated), | ||
| "appliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision, | ||
| ) | ||
| if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.staleOutdated, log); err != nil { | ||
| log.Error(err, "failed to delete stale outdated ephemeral runners") | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) |
There was a problem hiding this comment.
Your control-flow observation is correct, but the harm it's built on isn't reachable — and the suggested remedy would break the phase outright. I've added a regression test pinning why, in 6e540195.
What's right: the stale-runner recovery block is unreachable while the set is already PhaseOutdated. The Outdated branch returns through cleanUpEphemeralRunners without recomputing the phase. That's accurate.
Why it's harmless: stale runners are still collected on that path — terminated() includes staleOutdated, and cleanUpEphemeralRunners deletes all of terminated(). So nothing leaks; the recovery block is an optimisation for the non-Outdated path, not the only deleter.
Why the upgrade scenario can't happen. It requires a legacy runner to put an AppliedActionableRevision > 0 set into Outdated. It can't:
applied=0 -> outdated=1 staleOutdated=0
applied=1 -> outdated=0 staleOutdated=1
applied=5 -> outdated=0 staleOutdated=1
An annotation-less runner parses to revision 0, so it's classified outdated only when applied == 0; above that it's staleOutdated. And updateStatus switches to Outdated on len(state.outdated) alone. So on an upgraded set the legacy runner is stale and cannot set the phase — the state you describe isn't reachable, and "classified stale, deleted, and still leave the set Outdated" would require something else to have set Outdated, namely a current-revision runner, in which case Outdated is correct.
Worth adding: the escape from Outdated isn't the recovery block at all — it's the Spec.ActionableRevision > Status.AppliedActionableRevision branch, which is checked before the Outdated early return and calls patchAppliedActionableRevisionStatus, which recomputes the phase in both directions.
Why I'm not taking the suggested fix. "Patch the phase back to Running after that cleanup when no current-revision Outdated runners remain" looks conditional but is unconditional: cleanUpEphemeralRunners deletes terminated(), which includes outdated, so immediately after it runs "no outdated runners remain" is trivially true every time. The set would return to Running against an unchanged spec, scale a replacement from that same spec, be told Outdated again, and loop — deleting and recreating runners forever. The stickiness is load-bearing, which is why a current-revision Outdated runner is deliberately not delete-and-replaced.
Added TestUpdateStatusStaleOutdatedRunnersNeverSetTheOutdatedPhase, asserting both directions: a legacy runner on an upgraded set leaves the phase Running, and on a set at revision 0 it still reports Outdated (preserving today's upgrade behaviour). Mutation-checked rather than merely observed passing — making updateStatus count staleOutdated fails it with expected: "Running", actual: "Outdated", i.e. it fails precisely if your scenario ever becomes real, while the revision-0 subtest keeps passing so it isn't a blanket assertion. Full envtest green.
cef33b4 to
6e54019
Compare
An EphemeralRunner that exits as Outdated marks its EphemeralRunnerSet
Outdated, and the AutoscalingRunnerSet then stops scaling it. Without a way
to tell which runner spec an Outdated report refers to, a report from a
runner built before the spec was updated keeps the set switched off after
the update that was supposed to fix it.
Stamp each runner with the actionable revision it was built from, and judge
Outdated reports against the revision the set has applied:
- A runner whose revision is older than the applied one is reporting on a
spec that has already been replaced. It is deleted and rebuilt from the
current spec, and it does not hold the set Outdated.
- A runner whose revision is current is reporting on the live spec, so the
set stays Outdated. It is deliberately not delete-and-replaced: a fresh
runner at the same revision would report Outdated again, forever.
Runners without the annotation parse to revision 0, which matches the zero
value of Status.AppliedActionableRevision, so existing runners keep their
current behaviour across an upgrade. That is also what makes the Outdated
early return in Reconcile safe: it returns without recomputing the phase, so
the stale-runner recovery below it is unreachable while the set is already
Outdated, and a stale runner must therefore be unable to set that phase at
all. updateStatus switches on len(outdated) alone, so it cannot.
The phase is derived inside patchAppliedActionableRevisionStatus, from a
list read through the same authoritative reader as the set itself, because
the optimistic lock on that patch covers the EphemeralRunnerSet object only
and cannot vouch for a separately-read list. The runners are classified
against the live applied revision rather than the revision this call was
asked to apply: the caller reads the spec from the cache while this function
re-reads the status from the API server, so a lagging reconcile can arrive
with a target behind the live marker, and judging against it would flip a
set that has already moved on back to Outdated.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
6e54019 to
2edfcc3
Compare
Layer 7/7 — the final layer of the stacked split of #4575. Base:
nikola-jokic-remove-integrity-hash-annotation.The problem
An
EphemeralRunnerSetgoesOutdatedwhen a child runner reports that the Actions service rejected its runner spec, and theAutoscalingRunnerSetreacts by tearing the listener down so the scale set stops taking jobs. That is the intended behaviour — a broken runner spec should switch the scale set off rather than churn runners.But nothing recorded which runner spec a given
Outdatedreport was about. A runner that was busy with a job while the spec was updated survives the revision cleanup and only exits — with the outdated exit code — afterwards. Its verdict is about a spec that no longer exists, yet it kept the whole set inOutdated, discarding the fix the user had just applied and leaving the scale set switched off until something else nudged it.The fix
newEphemeralRunnernow stamps each runner with the set's actionable revision (actions.github.com/actionable-revision), andnewEphemeralRunnersByStatestakes the revision the runners are being judged against, splittingOutdatedrunners into two groups:staleOutdated— actionable revision older than the applied revision. Their report says nothing about the current spec, so they must not hold the set inOutdated. They are deleted and replaced by runners built from the current spec.outdated— actionable revision at or after the applied revision. Their report is about the current spec, so they drive the set intoOutdated. These are deliberately not delete-and-replaced: a fresh runner at the same revision would be rejected the same way and reportOutdatedagain, looping forever. Leaving them in place is what makes the phase stick until the spec actually changes.Runners created before the annotation existed parse to
0, which matches the zero value ofStatus.AppliedActionableRevision, so an in-place upgrade behaves exactly as today until the spec is updated for the first time.patchAppliedActionableRevisionStatusnow lists the child runners and recomputes the phase in both directions against the revision being applied. It has to: that path returns fromReconcilewithout reachingupdateStatus, so a staleRunningwould hide genuinely outdated runners from the cleanup path, and a staleOutdatedwould keep the set switched off after the spec that caused it was replaced. Judging against the revision being applied (rather than the one in status) is what lets a spec update clearOutdatedimmediately instead of waiting for the pre-update runners to be collected.Finally, the
AutoscalingRunnerSetteardown guard moves from a bare phase check toephemeralRunnerSetOutdatedForAppliedRevision, which additionally requires the applied revision to have caught up with the spec revision. That closes the window between theAutoscalingRunnerSetpatching a new runner spec onto the set and theEphemeralRunnerSetcontroller processing it, during which the set still reportsOutdatedfor a spec that is already gone.Also here
terminated()allocated its result by appending intos.finished, which could write through that slice's backing array. It now allocates a fresh, correctly-sized slice, and includesstaleOutdated.Tests
helpers_outdated_test.gocoveringephemeralRunnerSetOutdatedForAppliedRevision.Outdatedagainst a set running revision 2 is deleted, and the set never reportsOutdatedbecause of it.go build ./...,go vet ./...,gofmtclean;make manifestsproduces no diff and the chart CRD copies are byte-identical toconfig/crd/bases; the fullcontrollers/actions.github.com+apissuite passes.