-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implement termination grace period support for the runner #3830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for a configurable termination grace period for the runner, allowing it to finish its current job before shutting down.
- Detect and flag a non‑empty
ACTIONS_RUNNER_TERMINATION_GRACE_PERIOD_SECONDS
environment variable - Introduce
GetShutdownDelay()
to compute and apply a shutdown delay when the runner is busy - Extend
HostContext.ShutdownRunner
to accept an optional delay and wire it into unload/Ctrl+C logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Runner.Listener/Runner.cs | Read and store the grace‑period env var; wrap shutdown calls with GetShutdownDelay() ; add job‑status handler |
src/Runner.Common/HostContext.cs | Change ShutdownRunner signature to accept a delay; apply CancelAfter(delay) when set |
src/Runner.Common/Constants.cs | Add ACTIONS_RUNNER_TERMINATION_GRACE_PERIOD_SECONDS constant |
Comments suppressed due to low confidence (2)
src/Runner.Listener/Runner.cs:843
- Consider adding unit tests for
GetShutdownDelay()
to cover scenarios where the runner is busy versus not busy, valid and invalid env var values, and boundary conditions for the delay range.
private TimeSpan GetShutdownDelay()
src/Runner.Listener/Runner.cs:317
- [nitpick] Enhance this log message by including the actual grace period value (e.g. seconds) to make debugging easier when the variable is set.
Trace.Verbose($"Runner has termination grace period set");
@@ -430,9 +452,13 @@ private async Task<int> RunAsync(RunnerSettings settings, bool runOnce = false) | |||
bool autoUpdateInProgress = false; | |||
Task<bool> selfUpdateTask = null; | |||
bool runOnceJobReceived = false; | |||
jobDispatcher = HostContext.CreateService<IJobDispatcher>(); | |||
jobDispatcher = HostContext.GetService<IJobDispatcher>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to GetService
, so i can fetch the same instance in the event handler.
We never need more than 1 jobDispatcher in the listener, so switching to Get should be safe.
@@ -37,6 +37,8 @@ public sealed class Runner : RunnerService, IRunner | |||
private readonly object _authMigrationTelemetryLock = new(); | |||
private IRunnerServer _runnerServer; | |||
private CancellationTokenSource _authMigrationTelemetryTokenSource = new(); | |||
private bool _runnerExiting = false; | |||
private bool _hasTerminationGracePeriod = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i use _hasTerminationGracePeriod
as the feature flag, so unless the new ENV is set, no new code will get executed.
bfce94e
to
bd127e6
Compare
export ACTIONS_RUNNER_TERMINATION_GRACE_PERIOD_SECONDS=100
The runner will not cancel its current runner step during the grace period.
However, depends on how child process launched in the step, those process might still exit by itself if they dont'
trap '' INT TERM
.Try to improve: #3308