You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Finalizing an EphemeralRunner called RemoveRunner inline, and only deleted the runner pod and the jitconfig secret once that call returned. Local cleanup was gated on an external API, so a burst of completing jobs left completed pods standing around waiting on calls they have no real dependency on.
Change
A runner that exits with code 0 removed its own registration on the way out, so phase Succeeded now costs no API call at all. That is the path every completed job takes.
Everything else goes to RunnerUnregistrationQueue, drained by max(4, ephemeral runner concurrency) workers started next to the controllers. Push appends to a slice and never blocks. The finalizer is removed before the push, so a failed patch cannot queue the same removal twice. A removal refused with JobStillRunning goes back on the queue for 30 seconds later.
A registration is claimed while it is queued and stays claimed until the request is done with, across retries, so the service is asked to remove it exactly once. The key is the runner and its ID rather than the ID alone, since the service assigns runner IDs per GitHub scope and one controller can serve several. This mostly matters on the retry path, where two copies of a request for a runner still executing a job would retry in lockstep for as long as the job runs.
The other deregistration paths in the runner controller were moved over too. markAsFailed and markAsOutdated queue instead of blocking the phase transition, since those runners wait on the set to delete them. The HasJob path in deleteEphemeralRunnerOrPod deleted the runner and then removed the registration, which the finalizer now does on its own.
On scale down the set already removes the registration before deleting, so it drops the registration finalizer to say so and the finalizer does not queue a second removal for every runner.
A runner is registered before its status can record the ID, so a runner deleted in that window is unregistered under the ID from its jitconfig secret. That read only happens for a runner with no recorded ID.
The queue is in memory only. Requests still queued when the controller restarts or loses leader election are dropped, and the Actions service reaps those registrations on its own once the runner stops reporting in. Making it durable would mean writing to the API server per deletion, which is the cost being removed here.
One behaviour change: deleting an EphemeralRunner by hand while its job is running now deletes the pod immediately instead of holding it alive. The set paths are unaffected, since they refuse to delete a runner with a job assigned and deregister before deleting on scale down.
Benchmarks
go test ./controllers/actions.github.com/ -run '^$' \
-bench 'BenchmarkEphemeralRunnerFinalize|BenchmarkRunnerUnregistrationQueue' \
-benchtime=300x -count=6 | tee bench.txt
benchstat bench.txt
BenchmarkEphemeralRunnerFinalize runs one finalizer pass against a range of Actions service latencies, comparing the queued path with the synchronous one it replaces.
service latency
synchronous
queued
0
1.60 ms
2.16 ms
1 ms
2.85 ms
1.87 ms
25 ms
27.22 ms
1.65 ms
Skipping entirely, which is what phase Succeeded does, is 1.87 ms.
The signal is the synchronous column: it tracks service latency about 1:1, and at 25 ms the gap is the full round trip, which is the time a completed pod used to wait before it could be collected. Queued is flat, because the reconcile no longer waits for the service at all. The queued and skipped rows differ only by noise; a push is 660 ns against a floor of roughly 1.8 ms, so the benchmark cannot see it.
The queue itself:
Push: 660 ns, 1 alloc, 1.6 KiB, almost all of it the deep copy of the runner. Claiming and releasing is about 130 ns of that. This is the whole cost the service now imposes on a reconcile, and it is what the Succeeded skip avoids.
Drain: 33 / 24 / 22 ns per request at bursts of 100 / 1k / 10k, zero allocations. Flat, so a burst does not get more expensive per runner while holding the lock Push needs.
Reading the numbers: the fake API server sets a floor of 1.6 to 2.2 ms per reconcile depending on how quiet the machine is, and every row except the synchronous ones with latency is measuring that floor. Treat them as shapes rather than cluster timings. synchronous/latency=25ms and the queue benchmarks reproduce across runs to within a percent or two; the rest move around by 10%.
Tests
./controllers/actions.github.com/... passes, including unit coverage for the exit-0 skip, the ID resolution, the queue ordering, burst promotion, the retry and the de-duplication, plus envtest specs for each phase, for the ID recovered from the secret, for a runner the set already deregistered, and one that blocks RemoveRunner on a channel and asserts the pod and secret are gone while the call is still in flight.
Deleting an EphemeralRunner called RemoveRunner inline in the finalizer,
and only deleted the runner pod and the jitconfig secret once that call
came back. Local cleanup was therefore gated on an external API, and
under burst scale-down completed pods piled up waiting on it.
A runner that exits with code 0 has already removed its own registration
on the way out, so phase Succeeded, the path every completed job takes,
now costs no API call at all. The same goes for a runner that never got
a RunnerID.
Everything else is handed to RunnerUnregistrationQueue, drained by
max(4, ephemeral runner concurrency) workers started next to the
controllers. Push appends to a slice and never blocks, the finalizer is
removed first so a failed patch cannot queue the same removal twice, and
a removal refused because the job is still running goes back on the
queue for 30 seconds later. Requests held in memory are lost on restart
or leader change; the service reaps those registrations on its own.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Covers the finalizer pass against a range of Actions service latencies,
and the queue itself: what a deletion pays to hand a removal over, and
what the workers pay to take a burst back off.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Moderate findings: avoid redundant deregistration during set-driven scale-down (2 votes); route failed-pod and outdated paths through the queue (3 votes).
markAsFailed and markAsOutdated removed the registration inline, holding
a phase transition behind the service for runners the set does not
delete promptly. They queue it instead. The HasJob path in
deleteEphemeralRunnerOrPod deleted the runner and then removed the
registration, which the finalizer now does on its own.
The set removes the registration before deleting a runner it is scaling
down, so it drops the registration finalizer to say so, and the
finalizer no longer queues a second removal for every runner a scale
down takes. A removal that comes back not found there is the outcome the
call wanted, not a failure to retry forever.
A runner is registered before its status can record the ID, so a runner
deleted in that window is now unregistered under the ID from its
jitconfig secret rather than left to the service. That read only happens
for a runner without a recorded ID.
Promoting delayed requests removed them one at a time, shifting the rest
of the list on every promotion while holding the lock Push needs. A
burst refused together comes due together, so it is partitioned in one
pass.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The skip was real but unfindable. It lived inside registeredRunnerID,
which returns an int, so the finalizer only ever read `runnerID != 0`
and nothing at the decision point named the case it was skipping.
Lift it out into a named branch, and leave registeredRunnerID to do
only what it says: name a registration. Callers decide whether a
removal is needed at all.
markAsFailed and markAsOutdated now drop the registration finalizer
when they queue, so the deletion that follows does not queue the same
runner a second time.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Push appended unconditionally, so a registration could be queued twice:
a reconcile that fails after the push re-reads a stale cache, finds the
finalizer still there and queues it again. Harmless on its own, since
the second call 404s, but a duplicate that lands on the job-still-running
retry path stays in lockstep with the original and doubles the calls for
as long as the job runs.
Claim a registration when it is queued and hold the claim until the
request is done with, including across a retry. The key is the runner
and its ID rather than the ID alone, because the service assigns runner
IDs per GitHub scope and one controller can serve several.
The queue benchmarks pushed the same runner every iteration, which now
measures a duplicate being turned away. They push distinct registrations
instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The queues were built with the global logger, which is a promise nobody
fulfills in a benchmark binary. Thirty seconds in it resolves itself to
a fallback that writes a stack trace to stderr, so a long enough run got
its output corrupted mid-line, cost benchstat a sample, and paid for the
writes in the timings it was reporting.
Discard instead, as the reconciler under benchmark already does.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Three findings from review, all real.
A failed read of the jitconfig secret was answered as if the runner had
never been registered, which dropped the finalizer and lost the last
record of a registration that may well exist. Only the absence of the
secret means that. Anything else is now an error and gets retried, which
costs a read against the local API server, not the call this change
exists to get off the deletion path.
markAsFailed and markAsOutdated release the registration as they record
the phase, but that patch can fail once the phase is already recorded,
and the retry does not land back in them: the runner is terminal by
then, so the reconcile takes the IsDone path. That path now picks the
release back up, so the error costs a reconcile rather than holding the
registration until the set deletes the runner. queueUnregistration
guards on the finalizer and owns the exit-0 skip, which makes it safe to
call on every pass.
The finalize benchmark built every iteration's runner with the same name
and ID against a queue shared across iterations, so from the second
iteration on the claim rejected every push and the queued variant
measured de-duplication rather than the push it was meant to. Each
iteration gets its own registration.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
It introduces a new concurrent background worker subsystem and changes finalizer/deregistration semantics across multiple lifecycle paths, which warrants final human validation.
Push() always DeepCopies the EphemeralRunner before checking whether the unregistration key is already claimed. When a duplicate push happens (e.g., overlapping reconciles), this pays the largest part of Push’s cost (allocation-heavy DeepCopy) even though the request will be dropped. Consider claiming first, then DeepCopy/enqueue only when the claim succeeds.
A worker with nothing ready sleeps until the earliest retry comes due,
which reads like it commits the whole pool to that deadline. It does
not: the sleep also wakes on a push, so the deadline is an upper bound
and a request needing no wait is served straight away.
That is load bearing and invisible, so it gets a test. Parking every
worker on a 30 second retry and then pushing a thousand ready removals
drains them in 20 milliseconds; dropping the wake case from the select
makes the same test fail on the 20 second deadline.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The ready-queue implementation can grow its backing slice without bound under steady churn when the queue rarely fully drains, risking unbounded memory growth in long-running controllers.
Get a fresh assessment by requesting another Copilot review.
// Drop the reference so that the consumed entry does not keep the copy of
// the runner alive until the slice is reused.
q.ready[q.readyHead] = runnerUnregistration{}
q.readyHead++
if q.readyHead == len(q.ready) {
q.ready = q.ready[:0]
q.readyHead = 0
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4632
Problem
Finalizing an
EphemeralRunnercalledRemoveRunnerinline, and only deleted the runner pod and the jitconfig secret once that call returned. Local cleanup was gated on an external API, so a burst of completing jobs left completed pods standing around waiting on calls they have no real dependency on.Change
A runner that exits with code 0 removed its own registration on the way out, so phase
Succeedednow costs no API call at all. That is the path every completed job takes.Everything else goes to
RunnerUnregistrationQueue, drained bymax(4, ephemeral runner concurrency)workers started next to the controllers.Pushappends to a slice and never blocks. The finalizer is removed before the push, so a failed patch cannot queue the same removal twice. A removal refused withJobStillRunninggoes back on the queue for 30 seconds later.A registration is claimed while it is queued and stays claimed until the request is done with, across retries, so the service is asked to remove it exactly once. The key is the runner and its ID rather than the ID alone, since the service assigns runner IDs per GitHub scope and one controller can serve several. This mostly matters on the retry path, where two copies of a request for a runner still executing a job would retry in lockstep for as long as the job runs.
The other deregistration paths in the runner controller were moved over too.
markAsFailedandmarkAsOutdatedqueue instead of blocking the phase transition, since those runners wait on the set to delete them. TheHasJobpath indeleteEphemeralRunnerOrPoddeleted the runner and then removed the registration, which the finalizer now does on its own.On scale down the set already removes the registration before deleting, so it drops the registration finalizer to say so and the finalizer does not queue a second removal for every runner.
A runner is registered before its status can record the ID, so a runner deleted in that window is unregistered under the ID from its jitconfig secret. That read only happens for a runner with no recorded ID.
The queue is in memory only. Requests still queued when the controller restarts or loses leader election are dropped, and the Actions service reaps those registrations on its own once the runner stops reporting in. Making it durable would mean writing to the API server per deletion, which is the cost being removed here.
One behaviour change: deleting an
EphemeralRunnerby hand while its job is running now deletes the pod immediately instead of holding it alive. The set paths are unaffected, since they refuse to delete a runner with a job assigned and deregister before deleting on scale down.Benchmarks
BenchmarkEphemeralRunnerFinalizeruns one finalizer pass against a range of Actions service latencies, comparing the queued path with the synchronous one it replaces.Skipping entirely, which is what phase
Succeededdoes, is 1.87 ms.The signal is the synchronous column: it tracks service latency about 1:1, and at 25 ms the gap is the full round trip, which is the time a completed pod used to wait before it could be collected. Queued is flat, because the reconcile no longer waits for the service at all. The queued and skipped rows differ only by noise; a push is 660 ns against a floor of roughly 1.8 ms, so the benchmark cannot see it.
The queue itself:
Push: 660 ns, 1 alloc, 1.6 KiB, almost all of it the deep copy of the runner. Claiming and releasing is about 130 ns of that. This is the whole cost the service now imposes on a reconcile, and it is what theSucceededskip avoids.Pushneeds.Reading the numbers: the fake API server sets a floor of 1.6 to 2.2 ms per reconcile depending on how quiet the machine is, and every row except the synchronous ones with latency is measuring that floor. Treat them as shapes rather than cluster timings.
synchronous/latency=25msand the queue benchmarks reproduce across runs to within a percent or two; the rest move around by 10%.Tests
./controllers/actions.github.com/...passes, including unit coverage for the exit-0 skip, the ID resolution, the queue ordering, burst promotion, the retry and the de-duplication, plus envtest specs for each phase, for the ID recovered from the secret, for a runner the set already deregistered, and one that blocksRemoveRunneron a channel and asserts the pod and secret are gone while the call is still in flight.