Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TingluoHuang
Copy link
Member

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

@Copilot Copilot AI review requested due to automatic review settings May 2, 2025 03:54
@TingluoHuang TingluoHuang requested a review from a team as a code owner May 2, 2025 03:54
Copy link
Contributor

@Copilot Copilot AI left a 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>();
Copy link
Member Author

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;
Copy link
Member Author

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.

@TingluoHuang TingluoHuang force-pushed the users/tihuang/termgrace branch from bfce94e to bd127e6 Compare May 2, 2025 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant