Use lazy copy to patch resources to ensure multiple modifications are applied to the base resource

[nikola-jokic]

Reconcilers read an object far more often than they change it, but every reconcile paid for a full DeepCopy up front just to have a patch base available for the rare write. lazyCopy defers that copy until the moment something is actually about to be mutated, so only the reconciles that patch pay for it.

Behaviour change worth reviewing

EphemeralRunner previously added only one of its two finalizers on the reconcile where both were missing. The old code was:

var addedFinalizers bool
addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName)
addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName)

|| short-circuits. With both finalizers missing, the first AddFinalizer adds one and returns true, so the second call is never evaluated and its finalizer is not added.

It is not lost permanently: the patch triggers another reconcile, where the first AddFinalizer returns false because its finalizer is now present, so the second call runs and adds the remaining one. The effect is a gap of one reconcile — which is the window in which it matters, because a runner deleted while ephemeralRunnerActionsFinalizerName is absent skips the cleanup that unregisters it from the Actions service.

After this PR both are added on the first pass. This is the "ensure multiple modifications are applied" in the title, and no line of the diff announces it — the change reads as a stylistic rewrite — so it is called out here rather than left to be found later.

How it is used

runner := newLazyCopy(&ephemeralRunner)
if !controllerutil.ContainsFinalizer(&ephemeralRunner, name) {
    controllerutil.AddFinalizer(runner.Mutate(), name)
}
if runner.Modified() {
    err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom())
}

Mutate() returns the live object and snapshots it on first call. Reads go through the original as before; every mutation the patch should carry goes through Mutate. MergeFrom forwards controller-runtime's merge options, so a call site needing client.MergeFromWithOptimisticLock{} can ask for it.

The ordering is a caller invariant, not something the type enforces — the caller keeps the pointer it passed to newLazyCopy. A write landing before the first Mutate is already in the snapshot, so the patch comes out empty and the write is silently dropped. That is documented on the type and pinned by a test rather than only described.

Fixes found in review

• Classifier read a superseded revision. patchAppliedActionableRevisionStatus built its desired status as a value copy. The monotonicity guard wrote the advance into the copy while the classifier below still read the applied revision off the fetched object — a line that is textually unchanged and therefore absent from the diff. A runner left over from the superseded revision, missed by the cache-read cleanup, was then counted Outdated against the pre-advance revision and that phase saved next to the advanced marker. Nothing recomputes it: Reconcile returns on the Outdated path before updateStatus, and this function only runs while spec is ahead of applied, so the set stayed switched off until the next spec change. Caught by @salmanmkc.

• The split itself is gone. Keeping the revision in two places is what made the above expressible, so the function now patches through the lazy copy and the revision lives in one place. The value copy was also shallow; all three status fields are scalars today, but a pointer field added later would have been shared between the copy and the object it was meant to be compared against.

Every existing test of that function drove the guard-not-fired branch, where the two revisions agree, so all of them passed with the bug live. The added tests cover the guard firing and the no-patch-when-nothing-changed path, and both were checked against a deliberately broken build to confirm they can fail.

---

Based on #4575

[github-actions]

Hello! Thank you for your contribution.

Please review our contribution guidelines to understand the project's testing and code conventions.

[Copilot]

Pull request overview

This PR modernizes how the controllers detect/propagate desired-state changes and how they compute patches, replacing integrity-hash annotations with explicit revision/generation tracking and adding a shared in-memory resource cache to avoid rebuilding identical desired objects.

Changes:

• Introduces a ResourceCache used by ResourceBuilder to reuse desired objects based on a main-object key plus dependency refs.
• Replaces integrity-hash–based update detection with ActionableRevision/AppliedActionableRevision for EphemeralRunnerSet and ObservedGeneration for AutoscalingRunnerSet (and updates CRDs accordingly).
• Refactors patch flows to use a lazy DeepCopy helper (once) so multiple in-place mutations can be safely applied before client.MergeFrom(...).

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
main.go	Instantiates and wires a shared ResourceCache into ResourceBuilder.
controllers/actions.github.com/utils.go	Adds once lazy-copy helper used to build correct MergeFrom patches.
controllers/actions.github.com/utils_test.go	Moves test-only random string helper into tests.
controllers/actions.github.com/resourcecache.go	Adds typed resource cache with dependency-key tracking and eviction by owner UID.
controllers/actions.github.com/resourcecache_test.go	Adds unit tests for cache semantics (dependency ordering, deletion, invalid inputs).
controllers/actions.github.com/resourcebuilder.go	Uses cache for desired-object reuse; removes integrity-hash annotations; refactors label/annotation merging.
controllers/actions.github.com/resourcebuilder_test.go	Updates tests for removed integrity-hash annotation; adds merge-map tests and cache expectations.
controllers/actions.github.com/helpers.go	Adds helper predicates for actionable revision and pod recreation decisions.
controllers/actions.github.com/ephemeralrunnerset_controller.go	Switches to actionable revision + status patching with conflict retries; refactors patching with lazy copies.
controllers/actions.github.com/ephemeralrunnerset_controller_test.go	Updates/extends integration tests for cache cleanup and actionable revision behavior.
controllers/actions.github.com/ephemeralrunner_controller.go	Refactors finalizer patching to use lazy-copy pattern; deletes cached entries on deletion.
controllers/actions.github.com/ephemeralrunner_controller_test.go	Extends tests to assert cache cleanup on runner deletion.
controllers/actions.github.com/autoscalingrunnerset_controller.go	Uses observed generation for Pending detection; switches spec-change handling to actionable revision; refactors patch logic.
controllers/actions.github.com/autoscalingrunnerset_controller_test.go	Updates tests to assert observed generation and actionable revision behavior; adds cache assertions.
controllers/actions.github.com/autoscalinglistener_controller.go	Refactors patching of dependent resources using lazy copies; updates pod recreation decision logic.
controllers/actions.github.com/autoscalinglistener_controller_test.go	Extends tests to assert resources are cached and evicted appropriately.
config/crd/bases/actions.github.com_ephemeralrunnersets.yaml	Adds actionableRevision, appliedActionableRevision, finishedRunnerCleanupPatchID schema fields.
config/crd/bases/actions.github.com_autoscalingrunnersets.yaml	Adds observedGeneration to status schema.
charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml	Mirrors ERS CRD schema additions into chart CRDs.
charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml	Mirrors ARS CRD schema additions into chart CRDs.
charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml	Mirrors ERS CRD schema additions into experimental chart CRDs.
charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml	Mirrors ARS CRD schema additions into experimental chart CRDs.
apis/actions.github.com/v1alpha1/version.go	Minor condition reordering in version allowance check.
apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go	Adds ActionableRevision spec field and applied/cleanup status fields.
apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go	Adds ObservedGeneration to status.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Copilot review overview

🟡 Changes recommended

Immutable RoleBinding and Secret fields are patched in place, which can leave reconciliation permanently failing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (2)

Severity	Finding
	controllers/​actions.github.com/​autoscalinglistener_controller.go — A Secret's immutable flag cannot be unset after it becomes true. For an immutable live proxy…
	controllers/​actions.github.com/​autoscalinglistener_controller.go — RoleBinding.roleRef is immutable after creation. If this comparison ever detects drift, the…

Issues resolved since last review (3)

Severity	Finding
	controllers/​actions.github.com/​utils.go — The panic message in (*once).Get() is very generic. Using a more specific message will make… View resolved comment
	controllers/​actions.github.com/​autoscalinglistener_controller.go — rulesModified is misleading here because this block is checking/modifying RoleRef, not RBAC… View resolved comment
	controllers/​actions.github.com/​autoscalingrunnerset_controller.go — Typo in the variable name listnerLabelsModified makes the code harder to read/search. Rename it… View resolved comment

Suppressed comments (1)

controllers/actions.github.com/autoscalinglistener_controller.go:459

• A Secret's immutable flag cannot be unset after it becomes true. Setting it to nil guarantees this patch is rejected for an immutable config Secret, leaving reconciliation stuck; preserve it when data is unchanged, or delete and recreate the Secret when the config changes.

		if listenerConfigSecret.Immutable != nil {
			original.Do()
			listenerConfigSecret.Immutable = nil
		}

The merge-base changed after approval.

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation is coherent and tested; the identified documentation correction is non-blocking.

Review tier: Balanced
Findings: 1 · 1 · 1

New issues introduced by this change (1)

Severity	Finding
	controllers/​actions.github.com/​lazycopy.go — Do not claim mutation ordering is enforced

Pre-existing issues (2)

Severity	Finding
	controllers/​actions.github.com/​autoscalinglistener_controller.go — A Secret's immutable flag cannot be unset after it becomes true. For an immutable live proxy… View comment
	controllers/​actions.github.com/​autoscalinglistener_controller.go — RoleBinding.roleRef is immutable after creation. If this comparison ever detects drift, the… View comment

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation is consistent and tested; only a non-blocking documentation correction remains.

Review tier: Balanced
Findings: None

Issues resolved since last review (3)

Severity	Finding
	controllers/​actions.github.com/​lazycopy.go — Do not claim mutation ordering is enforced View resolved comment
	controllers/​actions.github.com/​autoscalinglistener_controller.go — RoleBinding.roleRef is immutable after creation. If this comparison ever detects drift, the… View resolved comment
	controllers/​actions.github.com/​autoscalinglistener_controller.go — A Secret's immutable flag cannot be unset after it becomes true. For an immutable live proxy… View resolved comment

The merge-base changed after approval.

[Copilot]

Copilot review overview

🟡 Changes recommended

The one-reconcile finalizer fix needs a regression test that fails when the finalizers are added across separate reconciles.

Get a fresh assessment by requesting another Copilot review.

Review tier: Balanced
Findings: 1

Open findings (1)

• Cover atomic addition of both runner finalizers · New

Resolved findings (1)

• Do not claim mutation ordering is enforced