Track runner spec updates with an actionable revision by nikola-jokic · Pull Request #4638 · actions/actions-runner-controller

nikola-jokic · 2026-09-09T10:14:17Z

Layer 4 of a 7-layer stack splitting #4575. Base is nikola-jokic-l3-ars-observed-generation, not master.

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. Today that is detected by hashing Spec.EphemeralRunnerSpec into the actions.github.com/integrity-hash annotation and comparing the stored annotation against a freshly computed hash. This replaces that with a monotonic revision counter split across spec and status: the AutoscalingRunnerSet controller bumps Spec.ActionableRevision when it patches a new runner spec across, and the EphemeralRunnerSet controller advances Status.AppliedActionableRevision once the cleanup has actually finished.

Why `Semantic.DeepEqual` and not `cmp.Equal`

ephemeralRunnerSetActionableSpecChanged compares the two specs with apiequality.Semantic.DeepEqual. That choice is the whole correctness argument of this PR, so it is worth being explicit about.

Most PodSpec collection fields carry omitempty. A user who legitimately writes env: [] (or an empty nodeSelector, tolerations, volumes, ...) in the AutoscalingRunnerSet template produces a desired EphemeralRunnerSet whose field is empty-but-non-nil. When the controller writes that object, the empty slice is dropped by serialization, and the object read back from the API server has nil there instead. cmp.Equal and reflect.DeepEqual both consider nil and empty different, so the drift check would fire on every reconcile: it would bump ActionableRevision each time, and the EphemeralRunnerSet controller would respond by deleting every idle and pending runner, forever, on a scale set that nobody had touched.

Semantic.DeepEqual treats nil and empty as equal, which closes that loop. It also knows how to compare resource.Quantity, where two values that are numerically identical can have different internal representations, and unlike cmp.Equal it cannot panic at runtime on a type with unexported fields — an important property for something reached from a reconcile loop on user-supplied pod templates.

helpers_drift_test.go exists to pin exactly this. It marshals a spec to JSON and back to reproduce the nil/empty split the API server produces, asserts the round trip really did produce it (so the test cannot pass vacuously), and then asserts no drift is reported. A second test walks a table of genuine changes — image, env value, env added, env removed, scale set ID, config URL, container removed — to make sure relaxing nil-vs-empty did not also blind the check to real drift.

Why the applied marker lives in status

ActionableRevision is the writer's intent and AppliedActionableRevision is the reader's acknowledgement, and they are deliberately in different subresources. The EphemeralRunnerSet controller writes the applied revision after cleanUpEphemeralRunners and the proxy secret reconcile have both returned successfully, never before and never in the same write.

That ordering is what makes the mechanism restart-safe. If the controller is killed half way through deleting idle runners, the applied revision is still behind the spec revision when it comes back, so the next reconcile redoes the cleanup rather than concluding the update was already applied. Storing the marker in an annotation next to the spec, or writing it up front, would make a crash silently strand runners that are executing a spec nobody will ever revisit. updateStatus now carries AppliedActionableRevision through so a status-only phase transition does not reset it to zero and re-trigger the cleanup.

Why the status patch takes an optimistic lock

patchAppliedActionableRevisionStatus re-Gets the object inside retry.RetryOnConflict rather than reusing the reconciler's copy, because cleanup can take long enough for that copy to go stale. That re-fetch only means something if a stale write can actually be rejected, so the patch is built with client.MergeFromWithOptimisticLock{}.

A plain merge patch carries no resourceVersion precondition, so the API server can never answer it with a conflict. RetryOnConflict would then be decorative, and worse, the freshness check inside it would be unsound: both the target revision and the re-fetched object come from the cache-backed client, so a stale reconcile can compare a stale target against an equally stale read, pass the check, and write the applied revision backwards. With the server truly at Spec=5/Status=5 and a cache showing Spec=3/Status=2, the cleanup branch fires (3 > 2), the latest.Status.AppliedActionableRevision >= targetAppliedRevision guard passes (2 >= 3 is false), and the patch lands Status=3 over a live 5. That re-satisfies Spec > Status and sends the controller through the idle and pending runner cleanup all over again — the exact deletion storm this mechanism exists to prevent.

The lock closes that off completely rather than making it less likely, because resourceVersion is the version identity: if the version in the patch still matches the live object then the content it was computed from matched live too, so there is no interleaving in which a stale read lands. A successful patch is therefore proof that the monotonicity guard was evaluated against live data, which is what makes the guard sound rather than merely present. The remaining case is a stale target, which the lock says nothing about, and it is harmless: a stale target can only ever under-advance the marker (3 → 4 when live is 5), never regress it, and a later reconcile with fresh data completes the advance.

At this layer the re-fetch inside the retry still reads through the cache, so a genuine conflict may refetch stale data, exhaust the backoff (retry.DefaultBackoff is four steps, roughly 1.56s) and return the conflict error, requeuing the reconcile. That fails closed — no stale value is written — so correctness is complete here. A later layer makes that read authoritative via APIReader, which buys convergence within a single reconcile instead of detect-and-requeue. Neither layer is load-bearing for the other, which is what made this split clean.

Scope

EphemeralRunnerSet.EphemeralRunnerSpecHash(), ephemeralRunnerSetIntegrityHash and its call site in newEphemeralRunnerSet are removed. EphemeralRunnerSpec.Hash() and the remaining integrity-hash writers for the listener and its supporting resources are untouched; they come out in a later layer. The FinishedRunnerCleanupPatchID status field, the stale-outdated classification, and the finished-runner cleanup rework are all in later layers too.

Validation

go build ./..., go vet ./... and gofmt -l apis/ controllers/ are clean. make manifests changes only ephemeralrunnersets.yaml, adding actionableRevision and appliedActionableRevision, and both chart CRD copies are byte-identical to config/crd/bases. The envtest suite passes, along with the package's unit tests.

The two patch preconditions are covered by unit tests that intercept SubResourcePatch and assert on the bytes the reconciler actually emits, rather than on a patch constructed alongside it, so they cannot pass while the production path builds its patch some other way. Both were mutation checked: reverting to client.MergeFrom fails the precondition test and reports the emitted patch as {"status":{"appliedActionableRevision":7}} with no resourceVersion, and disabling the >= guard fails the backwards-move test. The guard is covered separately from the lock because they are independent mechanisms — the lock stops a stale write landing, the guard stops an older-target reconcile writing at all — and the guard reads like a redundant optimisation, so it is the easier of the two to delete by accident.

For reference on the machine this was written on, BenchmarkEphemeralRunnerSetActionableSpecChanged reports roughly 56us/214 allocs for the no-drift case. cmp.Equal was several times that. The benchmark is there so a future change back to a heavier reflective comparison shows up as an obvious regression rather than as quiet GC pressure in the controller.

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

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

Copilot

Copilot review overview

🟡 Changes recommended

The legacy integrity-hash migration gap could leave runners on an old spec after upgrade.

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
	`apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go` — `ActionableRevision` is the desired, writer-side revision; it is not an applied marker.…

What changed in this PR

This PR replaces hash-based runner-spec drift detection with a restart-safe actionable revision handshake.

Changes:

Adds spec/status revision tracking and cleanup acknowledgement.
Uses semantic comparison to avoid nil-versus-empty drift loops.
Updates controllers, tests, benchmarks, API types, and CRD manifests.

File	Summary	Final review
`controllers/actions.github.com/resourcebuilder.go`	Removes EphemeralRunnerSet integrity-hash generation.	No final comment.
`controllers/actions.github.com/resourcebuilder_test.go`	Updates metadata assertions.	No final comment.
`controllers/actions.github.com/helpers.go`	Adds semantic drift and revision helpers.	No final comment.
`controllers/actions.github.com/helpers_drift_test.go`	Tests semantic equality and genuine drift.	No final comment.
`controllers/actions.github.com/helpers_bench_test.go`	Benchmarks semantic comparison.	No final comment.
`controllers/actions.github.com/ephemeralrunnerset_controller.go`	Performs cleanup and records applied revisions.	Moderate (1 vote): Preserve a one-time legacy-hash migration or fallback.
`controllers/actions.github.com/ephemeralrunnerset_controller_test.go`	Tests revision cleanup behavior.	No final comment.
`controllers/actions.github.com/autoscalingrunnerset_controller.go`	Bumps revisions for spec changes.	No final comment.
`controllers/actions.github.com/autoscalingrunnerset_controller_test.go`	Verifies revision propagation.	No final comment.
`config/crd/bases/actions.github.com_ephemeralrunnersets.yaml`	Updates the base CRD schema.	No final comment.
`charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml`	Updates the controller chart CRD.	No final comment.
`charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml`	Updates the experimental chart CRD.	No final comment.
`apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go`	Defines the revision fields.	Nit (3 votes): Describe `ActionableRevision` as the desired/actionable runner-spec revision.

Suppressed comments (1)

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

Existing EphemeralRunnerSets created before these fields existed deserialize with both revision fields as 0. If an upgrade occurs while the legacy integrity-hash check has a pending cleanup, this condition is false and the new controller ignores that mismatch; the AutoscalingRunnerSet controller also sees equal specs and will not bump a revision, so idle or pending runners can remain on the old spec until a later spec edit. Preserve a one-time legacy-hash migration or fallback before removing the old signal.

	if ephemeralRunnerSet.Spec.ActionableRevision > ephemeralRunnerSet.Status.AppliedActionableRevision {

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

Copilot

Copilot review overview

🔵 Needs a closer look

The cross-controller revision handshake and generated CRD updates warrant final human review, including the noted field-description corrections.

Review tier: Lite
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml` — The generated CRD describes `spec.actionableRevision` as an "applied marker", but this field is the…
	`charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml` — The generated CRD describes `spec.actionableRevision` as an "applied marker", but this field is the…
	`config/crd/bases/actions.github.com_ephemeralrunnersets.yaml` — The generated CRD describes `spec.actionableRevision` as an "applied marker", but this field is the…

Issues resolved since last review (1)

Severity	Finding
	`apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go` — `ActionableRevision` is the desired, writer-side revision; it is not an applied marker.… View resolved comment

Copilot

Copilot review overview

🟡 Changes recommended

One critical and two moderate unresolved issues affect cleanup, status updates, and legacy-hash migration.

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` — This revision path sends every pending EphemeralRunner through…
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — This condition drops the old integrity-hash recovery signal without migrating existing objects.…

Issues resolved since last review (3)

Severity	Finding
	`config/crd/bases/actions.github.com_ephemeralrunnersets.yaml` — The generated CRD describes `spec.actionableRevision` as an "applied marker", but this field is the… View resolved comment
	`charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml` — The generated CRD describes `spec.actionableRevision` as an "applied marker", but this field is the… View resolved comment
	`charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml` — The generated CRD describes `spec.actionableRevision` as an "applied marker", but this field is the… View resolved comment

Suppressed comments (1)

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

retry.RetryOnConflict cannot protect this write because client.MergeFrom(original) does not add an optimistic resourceVersion precondition. Since r.Get uses the manager's cache-backed client, a stale read can overwrite a newer applied marker with this older target revision instead of producing a conflict, violating the marker's monotonicity and retriggering cleanup. Use the optimistic-lock merge patch for this status write.

		return r.Status().Patch(ctx, &latest, client.MergeFrom(original))

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>

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>

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

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

nikola-jokic requested a lite review from Copilot September 9, 2026 21:45

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

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go Outdated

rentziass previously approved these changes Sep 10, 2026

View reviewed changes

nikola-jokic dismissed rentziass’s stale review via 7c5e578 September 10, 2026 09:12

nikola-jokic force-pushed the nikola-jokic-ers-actionable-revision branch from bd07455 to 7c5e578 Compare September 10, 2026 09:13

nikola-jokic force-pushed the nikola-jokic-ers-actionable-revision branch from 7c5e578 to 32d9f52 Compare September 10, 2026 09:20

nikola-jokic force-pushed the nikola-jokic-ers-actionable-revision branch from 32d9f52 to be4aec5 Compare September 10, 2026 09:52

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

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

Copilot AI reviewed Sep 10, 2026

View reviewed changes

nikola-jokic requested a lite review from Copilot September 10, 2026 11:09

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

Copilot AI reviewed Sep 10, 2026

View reviewed changes

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

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

nikola-jokic force-pushed the nikola-jokic-ers-actionable-revision branch from 95cb76b to 26e62ec Compare September 10, 2026 20:56

nikola-jokic mentioned this pull request Sep 11, 2026

Cleanup stalls forever when an EphemeralRunner can never register (RemoveRunner called with RunnerID 0) #4653

Closed

nikola-jokic force-pushed the nikola-jokic-ers-actionable-revision branch from 3d5cf49 to 7f4573b Compare September 11, 2026 10:03

rentziass approved these changes Sep 11, 2026

View reviewed changes

stack merge was automatically disabled September 11, 2026 12:35
Pull Request is not mergeable

Base automatically changed from nikola-jokic-l3-ars-observed-generation to master September 11, 2026 12:38

nikola-jokic and others added 5 commits September 11, 2026 14:38

Apply batched suggestions from code review

c16f131

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

nikola-jokic force-pushed the nikola-jokic-ers-actionable-revision branch from 7f4573b to e42e1b5 Compare September 11, 2026 12:38

nikola-jokic merged commit 6f89d05 into master Sep 11, 2026
31 checks passed

nikola-jokic deleted the nikola-jokic-ers-actionable-revision branch September 11, 2026 12:44

nikola-jokic mentioned this pull request Sep 11, 2026

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

Merged

Navigation Menu

Uh oh!

Track runner spec updates with an actionable revision - #4638

Track runner spec updates with an actionable revision#4638
nikola-jokic merged 5 commits into
masterfrom
nikola-jokic-ers-actionable-revision

nikola-jokic commented Sep 9, 2026 •

edited

Loading

Uh oh!

humh25 commented Sep 9, 2026 via email

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

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!

Why Semantic.DeepEqual and not cmp.Equal

Why the applied marker lives in status

Why the status patch takes an optimistic lock

Scope

Validation

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nikola-jokic commented Sep 9, 2026 •

edited

Loading

Why `Semantic.DeepEqual` and not `cmp.Equal`