From 3ff67abc5ad3701cf3ff90320c2ebac45641baeb Mon Sep 17 00:00:00 2001 From: Anatoly Rabkin Date: Wed, 18 Mar 2026 19:04:59 +0200 Subject: [PATCH] Fix timeout implementation and address review feedback - Kill git process on timeout: use child_process.spawn directly for timeout-eligible operations so we have a ChildProcess handle to send SIGTERM (then SIGKILL after 5s). On Windows, SIGTERM is a forced kill so the SIGKILL fallback is effectively a no-op there. - Fix timeout:0 not working: replace falsy || coalescion with explicit empty-string check so that '0' is not replaced by the default '300'. - Refactor execGit to use an options object instead of 5 positional parameters, eliminating error-prone filler args (false, false, {}). - Pass allowAllExitCodes through to execGitWithTimeout so both code paths have consistent behavior for non-zero exit codes. - Add settled guard to prevent double-reject when both close and error events fire on the spawned process. - Handle null exit code (process killed by signal) as an error rather than silently treating it as success. - Capture stderr in error messages for the timeout path, matching the information level of the non-timeout exec path. - Log SIGKILL failures at debug level instead of empty catch block. - Warn on customListeners being ignored in the timeout path. - Emit core.warning() when invalid input values are silently replaced with defaults, so users know their configuration was rejected. - Add input validation in setTimeout (reject negative values). - Clarify retry-max-attempts semantics: total attempts including the initial attempt (3 = 1 initial + 2 retries). - Remove Kubernetes probe references from descriptions. - Use non-exhaustive list (e.g.) for network operations in docs to avoid staleness if new operations are added. - Add tests for timeout/retry input parsing (defaults, timeout:0, custom values, invalid input with warnings, backoff clamping) and command manager configuration (setTimeout, setRetryConfig, fetch). Co-Authored-By: Claude Opus 4.6 (1M context) --- README.md | 14 +- __test__/git-command-manager.test.ts | 81 +++++++++ __test__/input-helper.test.ts | 54 ++++++ action.yml | 11 +- dist/index.js | 220 ++++++++++++++++++----- src/git-command-manager.ts | 255 ++++++++++++++++++++++----- src/git-source-settings.ts | 9 +- src/input-helper.ts | 33 +++- 8 files changed, 556 insertions(+), 121 deletions(-) diff --git a/README.md b/README.md index 8948c3b..5a0d300 100644 --- a/README.md +++ b/README.md @@ -155,20 +155,20 @@ Please refer to the [release page](https://github.com/actions/checkout/releases/ # Default: true set-safe-directory: '' - # Timeout in seconds for each git network operation attempt (fetch, lfs-fetch, - # ls-remote). If a single attempt exceeds this, it is killed and retried. Set to 0 - # to disable. Default is 300 (5 minutes). Similar to Kubernetes probe - # timeoutSeconds. + # Timeout in seconds for each git network operation attempt (e.g. fetch, + # lfs-fetch, ls-remote). If a single attempt exceeds this, the process is + # terminated. If retries are configured (see retry-max-attempts), the operation + # will be retried. Set to 0 to disable. Default is 300 (5 minutes). # Default: 300 timeout: '' - # Maximum number of retry attempts for failed git network operations. Similar to - # Kubernetes probe failureThreshold. + # Total number of attempts for each git network operation (including the initial + # attempt). For example, 3 means one initial attempt plus up to 2 retries. # Default: 3 retry-max-attempts: '' # Minimum backoff time in seconds between retry attempts. The actual backoff is - # randomly chosen between min and max. Similar to Kubernetes probe periodSeconds. + # randomly chosen between min and max. # Default: 10 retry-min-backoff: '' diff --git a/__test__/git-command-manager.test.ts b/__test__/git-command-manager.test.ts index 8a97d82..f79ba46 100644 --- a/__test__/git-command-manager.test.ts +++ b/__test__/git-command-manager.test.ts @@ -5,6 +5,17 @@ import * as commandManager from '../lib/git-command-manager' let git: commandManager.IGitCommandManager let mockExec = jest.fn() +function createMockGit(): Promise { + mockExec.mockImplementation((path, args, options) => { + if (args.includes('version')) { + options.listeners.stdout(Buffer.from('2.18')) + } + return 0 + }) + jest.spyOn(exec, 'exec').mockImplementation(mockExec) + return commandManager.createCommandManager('test', false, false) +} + describe('git-auth-helper tests', () => { beforeAll(async () => {}) @@ -494,3 +505,73 @@ describe('git user-agent with orchestration ID', () => { ) }) }) + +describe('timeout and retry configuration', () => { + beforeEach(async () => { + jest.spyOn(fshelper, 'fileExistsSync').mockImplementation(jest.fn()) + jest.spyOn(fshelper, 'directoryExistsSync').mockImplementation(jest.fn()) + }) + + afterEach(() => { + jest.restoreAllMocks() + }) + + it('setTimeout accepts valid values', async () => { + git = await createMockGit() + git.setTimeout(30) + git.setTimeout(0) + }) + + it('setTimeout rejects negative values', async () => { + git = await createMockGit() + expect(() => git.setTimeout(-1)).toThrow(/non-negative/) + }) + + it('setRetryConfig accepts valid parameters', async () => { + git = await createMockGit() + git.setRetryConfig(5, 2, 15) + }) + + it('setRetryConfig rejects min > max backoff', async () => { + git = await createMockGit() + expect(() => git.setRetryConfig(3, 20, 5)).toThrow( + /min seconds should be less than or equal to max seconds/ + ) + }) + + it('fetch without timeout uses exec', async () => { + git = await createMockGit() + // timeout defaults to 0 (disabled) + + mockExec.mockClear() + await git.fetch(['refs/heads/main'], {}) + + // exec.exec is used (via retryHelper) when no timeout + const fetchCalls = mockExec.mock.calls.filter( + (call: any[]) => (call[1] as string[]).includes('fetch') + ) + expect(fetchCalls).toHaveLength(1) + }) + + it('fetch with timeout does not use exec', async () => { + git = await createMockGit() + // Short timeout and single attempt so the test completes quickly + git.setTimeout(1) + git.setRetryConfig(1, 0, 0) + + mockExec.mockClear() + + // fetch will use spawn path (which will fail/timeout since there's + // no real git repo), but we verify exec.exec was NOT called for fetch + try { + await git.fetch(['refs/heads/main'], {}) + } catch { + // Expected: spawn will fail/timeout in test environment + } + + const fetchCalls = mockExec.mock.calls.filter( + (call: any[]) => (call[1] as string[]).includes('fetch') + ) + expect(fetchCalls).toHaveLength(0) + }, 10000) +}) diff --git a/__test__/input-helper.test.ts b/__test__/input-helper.test.ts index 9514cb4..97c8851 100644 --- a/__test__/input-helper.test.ts +++ b/__test__/input-helper.test.ts @@ -144,4 +144,58 @@ describe('input-helper tests', () => { const settings: IGitSourceSettings = await inputHelper.getInputs() expect(settings.workflowOrganizationId).toBe(123456) }) + + it('sets timeout and retry defaults', async () => { + const settings: IGitSourceSettings = await inputHelper.getInputs() + expect(settings.timeout).toBe(300) + expect(settings.retryMaxAttempts).toBe(3) + expect(settings.retryMinBackoff).toBe(10) + expect(settings.retryMaxBackoff).toBe(20) + }) + + it('allows timeout 0 to disable', async () => { + inputs.timeout = '0' + const settings: IGitSourceSettings = await inputHelper.getInputs() + expect(settings.timeout).toBe(0) + }) + + it('parses custom timeout and retry values', async () => { + inputs.timeout = '30' + inputs['retry-max-attempts'] = '5' + inputs['retry-min-backoff'] = '2' + inputs['retry-max-backoff'] = '15' + const settings: IGitSourceSettings = await inputHelper.getInputs() + expect(settings.timeout).toBe(30) + expect(settings.retryMaxAttempts).toBe(5) + expect(settings.retryMinBackoff).toBe(2) + expect(settings.retryMaxBackoff).toBe(15) + }) + + it('clamps retry-max-backoff to min when less than min and warns', async () => { + inputs['retry-min-backoff'] = '20' + inputs['retry-max-backoff'] = '5' + const settings: IGitSourceSettings = await inputHelper.getInputs() + expect(settings.retryMaxBackoff).toBe(20) + expect(core.warning).toHaveBeenCalledWith( + expect.stringContaining("'retry-max-backoff' (5) is less than 'retry-min-backoff' (20)") + ) + }) + + it('defaults invalid timeout to 300 and warns', async () => { + inputs.timeout = 'garbage' + const settings: IGitSourceSettings = await inputHelper.getInputs() + expect(settings.timeout).toBe(300) + expect(core.warning).toHaveBeenCalledWith( + expect.stringContaining("Invalid value 'garbage' for 'timeout'") + ) + }) + + it('defaults negative retry-max-attempts to 3 and warns', async () => { + inputs['retry-max-attempts'] = '-1' + const settings: IGitSourceSettings = await inputHelper.getInputs() + expect(settings.retryMaxAttempts).toBe(3) + expect(core.warning).toHaveBeenCalledWith( + expect.stringContaining("Invalid value '-1' for 'retry-max-attempts'") + ) + }) }) diff --git a/action.yml b/action.yml index ca7c406..03789b6 100644 --- a/action.yml +++ b/action.yml @@ -97,21 +97,20 @@ inputs: default: true timeout: description: > - Timeout in seconds for each git network operation attempt (fetch, lfs-fetch, ls-remote). - If a single attempt exceeds this, it is killed and retried. + Timeout in seconds for each git network operation attempt (e.g. fetch, lfs-fetch, ls-remote). + If a single attempt exceeds this, the process is terminated. + If retries are configured (see retry-max-attempts), the operation will be retried. Set to 0 to disable. Default is 300 (5 minutes). - Similar to Kubernetes probe timeoutSeconds. default: 300 retry-max-attempts: description: > - Maximum number of retry attempts for failed git network operations. - Similar to Kubernetes probe failureThreshold. + Total number of attempts for each git network operation (including the initial attempt). + For example, 3 means one initial attempt plus up to 2 retries. default: 3 retry-min-backoff: description: > Minimum backoff time in seconds between retry attempts. The actual backoff is randomly chosen between min and max. - Similar to Kubernetes probe periodSeconds. default: 10 retry-max-backoff: description: > diff --git a/dist/index.js b/dist/index.js index c968f6f..00a67e1 100644 --- a/dist/index.js +++ b/dist/index.js @@ -655,6 +655,7 @@ const io = __importStar(__nccwpck_require__(7436)); const path = __importStar(__nccwpck_require__(1017)); const regexpHelper = __importStar(__nccwpck_require__(3120)); const retryHelper = __importStar(__nccwpck_require__(2155)); +const child_process_1 = __nccwpck_require__(2081); const git_version_1 = __nccwpck_require__(3142); // Auth header not supported before 2.9 // Wire protocol v2 not supported before 2.18 @@ -737,7 +738,7 @@ class GitCommandManager { } }; // Suppress the output in order to avoid flooding annotations with innocuous errors. - yield this.execGit(args, false, true, listeners); + yield this.execGit(args, { silent: true, customListeners: listeners }); core.debug(`stderr callback is: ${stderr}`); core.debug(`errline callback is: ${errline}`); core.debug(`stdout callback is: ${stdout}`); @@ -825,7 +826,7 @@ class GitCommandManager { '--name-only', '--get-regexp', pattern - ], true); + ], { allowAllExitCodes: true }); return output.exitCode === 0; }); } @@ -854,7 +855,7 @@ class GitCommandManager { } const that = this; yield this.networkRetryHelper.execute(() => __awaiter(this, void 0, void 0, function* () { - yield that.execGit(args, false, false, {}, that.timeoutMs); + yield that.execGit(args, { timeoutMs: that.timeoutMs }); })); }); } @@ -869,7 +870,7 @@ class GitCommandManager { '--symref', repositoryUrl, 'HEAD' - ], false, false, {}, this.timeoutMs); + ], { timeoutMs: this.timeoutMs }); })); if (output) { // Satisfy compiler, will always be set @@ -906,7 +907,7 @@ class GitCommandManager { isDetached() { return __awaiter(this, void 0, void 0, function* () { // Note, "branch --show-current" would be simpler but isn't available until Git 2.22 - const output = yield this.execGit(['rev-parse', '--symbolic-full-name', '--verify', '--quiet', 'HEAD'], true); + const output = yield this.execGit(['rev-parse', '--symbolic-full-name', '--verify', '--quiet', 'HEAD'], { allowAllExitCodes: true }); return !output.stdout.trim().startsWith('refs/heads/'); }); } @@ -915,7 +916,7 @@ class GitCommandManager { const args = ['lfs', 'fetch', 'origin', ref]; const that = this; yield this.networkRetryHelper.execute(() => __awaiter(this, void 0, void 0, function* () { - yield that.execGit(args, false, false, {}, that.timeoutMs); + yield that.execGit(args, { timeoutMs: that.timeoutMs }); })); }); } @@ -927,8 +928,8 @@ class GitCommandManager { log1(format) { return __awaiter(this, void 0, void 0, function* () { const args = format ? ['log', '-1', format] : ['log', '-1']; - const silent = format ? false : true; - const output = yield this.execGit(args, false, silent); + const silent = !format; + const output = yield this.execGit(args, { silent }); return output.stdout; }); } @@ -958,7 +959,7 @@ class GitCommandManager { shaExists(sha) { return __awaiter(this, void 0, void 0, function* () { const args = ['rev-parse', '--verify', '--quiet', `${sha}^{object}`]; - const output = yield this.execGit(args, true); + const output = yield this.execGit(args, { allowAllExitCodes: true }); return output.exitCode === 0; }); } @@ -979,7 +980,10 @@ class GitCommandManager { if (recursive) { args.push('--recursive'); } - yield this.execGit(args); + const that = this; + yield this.networkRetryHelper.execute(() => __awaiter(this, void 0, void 0, function* () { + yield that.execGit(args, { timeoutMs: that.timeoutMs }); + })); }); } submoduleUpdate(fetchDepth, recursive) { @@ -992,12 +996,15 @@ class GitCommandManager { if (recursive) { args.push('--recursive'); } - yield this.execGit(args); + const that = this; + yield this.networkRetryHelper.execute(() => __awaiter(this, void 0, void 0, function* () { + yield that.execGit(args, { timeoutMs: that.timeoutMs }); + })); }); } submoduleStatus() { return __awaiter(this, void 0, void 0, function* () { - const output = yield this.execGit(['submodule', 'status'], true); + const output = yield this.execGit(['submodule', 'status'], { allowAllExitCodes: true }); core.debug(output.stdout); return output.exitCode === 0; }); @@ -1010,7 +1017,7 @@ class GitCommandManager { } tryClean() { return __awaiter(this, void 0, void 0, function* () { - const output = yield this.execGit(['clean', '-ffdx'], true); + const output = yield this.execGit(['clean', '-ffdx'], { allowAllExitCodes: true }); return output.exitCode === 0; }); } @@ -1021,7 +1028,7 @@ class GitCommandManager { globalConfig ? '--global' : '--local', '--unset-all', configKey - ], true); + ], { allowAllExitCodes: true }); return output.exitCode === 0; }); } @@ -1035,19 +1042,19 @@ class GitCommandManager { args.push(globalConfig ? '--global' : '--local'); } args.push('--unset', configKey, configValue); - const output = yield this.execGit(args, true); + const output = yield this.execGit(args, { allowAllExitCodes: true }); return output.exitCode === 0; }); } tryDisableAutomaticGarbageCollection() { return __awaiter(this, void 0, void 0, function* () { - const output = yield this.execGit(['config', '--local', 'gc.auto', '0'], true); + const output = yield this.execGit(['config', '--local', 'gc.auto', '0'], { allowAllExitCodes: true }); return output.exitCode === 0; }); } tryGetFetchUrl() { return __awaiter(this, void 0, void 0, function* () { - const output = yield this.execGit(['config', '--local', '--get', 'remote.origin.url'], true); + const output = yield this.execGit(['config', '--local', '--get', 'remote.origin.url'], { allowAllExitCodes: true }); if (output.exitCode !== 0) { return ''; } @@ -1068,7 +1075,7 @@ class GitCommandManager { args.push(globalConfig ? '--global' : '--local'); } args.push('--get-all', configKey); - const output = yield this.execGit(args, true); + const output = yield this.execGit(args, { allowAllExitCodes: true }); if (output.exitCode !== 0) { return []; } @@ -1088,7 +1095,7 @@ class GitCommandManager { args.push(globalConfig ? '--global' : '--local'); } args.push('--name-only', '--get-regexp', pattern); - const output = yield this.execGit(args, true); + const output = yield this.execGit(args, { allowAllExitCodes: true }); if (output.exitCode !== 0) { return []; } @@ -1100,7 +1107,7 @@ class GitCommandManager { } tryReset() { return __awaiter(this, void 0, void 0, function* () { - const output = yield this.execGit(['reset', '--hard', 'HEAD'], true); + const output = yield this.execGit(['reset', '--hard', 'HEAD'], { allowAllExitCodes: true }); return output.exitCode === 0; }); } @@ -1109,9 +1116,22 @@ class GitCommandManager { return this.gitVersion; }); } + /** + * Sets the timeout for network git operations. + * @param timeoutSeconds Timeout in seconds. 0 disables the timeout. + */ setTimeout(timeoutSeconds) { + if (timeoutSeconds < 0) { + throw new Error(`Timeout must be non-negative, got ${timeoutSeconds}`); + } this.timeoutMs = timeoutSeconds * 1000; } + /** + * Configures retry behavior for network git operations. + * @param maxAttempts Total attempts including the initial one. Must be >= 1. + * @param minBackoffSeconds Minimum backoff between retries. Must be <= maxBackoffSeconds. + * @param maxBackoffSeconds Maximum backoff between retries. + */ setRetryConfig(maxAttempts, minBackoffSeconds, maxBackoffSeconds) { this.networkRetryHelper = new retryHelper.RetryHelper(maxAttempts, minBackoffSeconds, maxBackoffSeconds); } @@ -1123,8 +1143,19 @@ class GitCommandManager { }); } execGit(args_1) { - return __awaiter(this, arguments, void 0, function* (args, allowAllExitCodes = false, silent = false, customListeners = {}, timeoutMs = 0) { + return __awaiter(this, arguments, void 0, function* (args, options = {}) { + const { allowAllExitCodes = false, silent = false, customListeners = {}, timeoutMs = 0 } = options; fshelper.directoryExistsSync(this.workingDirectory, true); + // Use child_process.spawn directly when timeout is set, + // so we can kill the process on timeout and avoid orphaned git processes. + // Note: customListeners are not supported in the timeout path. + if (timeoutMs > 0) { + if (customListeners && + Object.keys(customListeners).length > 0) { + core.debug('customListeners are not supported with timeoutMs and will be ignored'); + } + return yield this.execGitWithTimeout(args, timeoutMs, silent, allowAllExitCodes); + } const result = new GitOutput(); const env = {}; for (const key of Object.keys(process.env)) { @@ -1140,37 +1171,123 @@ class GitCommandManager { }; const mergedListeners = Object.assign(Object.assign({}, defaultListener), customListeners); const stdout = []; - const options = { + const execOptions = { cwd: this.workingDirectory, env, silent, ignoreReturnCode: allowAllExitCodes, listeners: mergedListeners }; - const execPromise = exec.exec(`"${this.gitPath}"`, args, options); - if (timeoutMs > 0) { - let timer; - const timeoutPromise = new Promise((_, reject) => { - timer = global.setTimeout(() => { - reject(new Error(`Git operation timed out after ${timeoutMs / 1000} seconds: git ${args.slice(0, 3).join(' ')}...`)); - }, timeoutMs); - }); - try { - result.exitCode = yield Promise.race([execPromise, timeoutPromise]); - } - finally { - clearTimeout(timer); - } - } - else { - result.exitCode = yield execPromise; - } + result.exitCode = yield exec.exec(`"${this.gitPath}"`, args, execOptions); result.stdout = stdout.join(''); core.debug(result.exitCode.toString()); core.debug(result.stdout); return result; }); } + /** + * Executes a git command with a timeout. Uses child_process.spawn directly + * (instead of @actions/exec) so we can kill the process on timeout and + * terminate it cleanly. Does not support customListeners. + */ + execGitWithTimeout(args, timeoutMs, silent, allowAllExitCodes) { + return __awaiter(this, void 0, void 0, function* () { + const result = new GitOutput(); + const env = {}; + for (const key of Object.keys(process.env)) { + env[key] = process.env[key]; + } + for (const key of Object.keys(this.gitEnv)) { + env[key] = this.gitEnv[key]; + } + const stdout = []; + const stderr = []; + return new Promise((resolve, reject) => { + var _a; + const child = (0, child_process_1.spawn)(this.gitPath, args, { + cwd: this.workingDirectory, + env, + stdio: ['ignore', 'pipe', 'pipe'] + }); + (_a = child.stdout) === null || _a === void 0 ? void 0 : _a.on('data', (data) => { + stdout.push(data.toString()); + }); + if (child.stderr) { + child.stderr.on('data', (data) => { + stderr.push(data.toString()); + if (!silent) { + process.stderr.write(data); + } + }); + } + let settled = false; + let timedOut = false; + let forceKillTimer; + const cleanup = () => { + clearTimeout(timer); + if (forceKillTimer) { + clearTimeout(forceKillTimer); + } + }; + const timer = global.setTimeout(() => { + timedOut = true; + // SIGTERM first, then force SIGKILL after 5 seconds. + // On Windows, SIGTERM is equivalent to a forced kill, so + // the SIGKILL fallback is effectively a no-op there. + child.kill('SIGTERM'); + forceKillTimer = global.setTimeout(() => { + try { + child.kill('SIGKILL'); + } + catch (killErr) { + core.debug(`Failed to SIGKILL git process: ${killErr}`); + } + }, 5000); + if (forceKillTimer.unref) { + forceKillTimer.unref(); + } + }, timeoutMs); + if (timer.unref) { + timer.unref(); + } + child.on('close', (code) => { + if (settled) + return; + settled = true; + cleanup(); + if (timedOut) { + reject(new Error(`Git operation timed out after ${timeoutMs / 1000} seconds: git ${args.slice(0, 5).join(' ')}...`)); + return; + } + // null code means killed by signal (e.g. OOM killer, external SIGTERM) + if (code === null) { + const stderrText = stderr.join('').trim(); + reject(new Error(`The process 'git' was killed by a signal` + + (stderrText ? `\n${stderrText}` : ''))); + return; + } + if (code !== 0 && !allowAllExitCodes) { + const stderrText = stderr.join('').trim(); + reject(new Error(`The process 'git' failed with exit code ${code}` + + (stderrText ? `\n${stderrText}` : ''))); + return; + } + result.exitCode = code; + result.stdout = stdout.join(''); + core.debug(result.exitCode.toString()); + core.debug(result.stdout); + resolve(result); + }); + child.on('error', (err) => { + if (settled) + return; + settled = true; + cleanup(); + reject(err); + }); + }); + }); + } initializeCommandManager(workingDirectory, lfs, doSparseCheckout) { return __awaiter(this, void 0, void 0, function* () { this.workingDirectory = workingDirectory; @@ -2124,29 +2241,38 @@ function getInputs() { // Determine the GitHub URL that the repository is being hosted from result.githubServerUrl = core.getInput('github-server-url'); core.debug(`GitHub Host URL = ${result.githubServerUrl}`); - // Timeout (per-attempt, like k8s timeoutSeconds) - result.timeout = Math.floor(Number(core.getInput('timeout') || '300')); + // Timeout per network operation attempt + const timeoutInput = core.getInput('timeout'); + result.timeout = Math.floor(Number(timeoutInput !== '' ? timeoutInput : '300')); if (isNaN(result.timeout) || result.timeout < 0) { + core.warning(`Invalid value '${timeoutInput}' for 'timeout' input. Using default: 300 seconds.`); result.timeout = 300; } core.debug(`timeout = ${result.timeout}`); - // Retry max attempts (like k8s failureThreshold) - result.retryMaxAttempts = Math.floor(Number(core.getInput('retry-max-attempts') || '3')); + // Retry max attempts (total attempts including initial) + const retryMaxAttemptsInput = core.getInput('retry-max-attempts'); + result.retryMaxAttempts = Math.floor(Number(retryMaxAttemptsInput !== '' ? retryMaxAttemptsInput : '3')); if (isNaN(result.retryMaxAttempts) || result.retryMaxAttempts < 1) { + core.warning(`Invalid value '${retryMaxAttemptsInput}' for 'retry-max-attempts' input. Using default: 3.`); result.retryMaxAttempts = 3; } core.debug(`retry max attempts = ${result.retryMaxAttempts}`); - // Retry backoff (like k8s periodSeconds, but as a min/max range) - result.retryMinBackoff = Math.floor(Number(core.getInput('retry-min-backoff') || '10')); + // Retry backoff range + const retryMinBackoffInput = core.getInput('retry-min-backoff'); + result.retryMinBackoff = Math.floor(Number(retryMinBackoffInput !== '' ? retryMinBackoffInput : '10')); if (isNaN(result.retryMinBackoff) || result.retryMinBackoff < 0) { + core.warning(`Invalid value '${retryMinBackoffInput}' for 'retry-min-backoff' input. Using default: 10 seconds.`); result.retryMinBackoff = 10; } core.debug(`retry min backoff = ${result.retryMinBackoff}`); - result.retryMaxBackoff = Math.floor(Number(core.getInput('retry-max-backoff') || '20')); + const retryMaxBackoffInput = core.getInput('retry-max-backoff'); + result.retryMaxBackoff = Math.floor(Number(retryMaxBackoffInput !== '' ? retryMaxBackoffInput : '20')); if (isNaN(result.retryMaxBackoff) || result.retryMaxBackoff < 0) { + core.warning(`Invalid value '${retryMaxBackoffInput}' for 'retry-max-backoff' input. Using default: 20 seconds.`); result.retryMaxBackoff = 20; } if (result.retryMaxBackoff < result.retryMinBackoff) { + core.warning(`'retry-max-backoff' (${result.retryMaxBackoff}) is less than 'retry-min-backoff' (${result.retryMinBackoff}). Using retry-min-backoff value for both.`); result.retryMaxBackoff = result.retryMinBackoff; } core.debug(`retry max backoff = ${result.retryMaxBackoff}`); diff --git a/src/git-command-manager.ts b/src/git-command-manager.ts index 580bffc..40bb4e7 100644 --- a/src/git-command-manager.ts +++ b/src/git-command-manager.ts @@ -7,6 +7,7 @@ import * as path from 'path' import * as refHelper from './ref-helper' import * as regexpHelper from './regexp-helper' import * as retryHelper from './retry-helper' +import {spawn} from 'child_process' import {GitVersion} from './git-version' // Auth header not supported before 2.9 @@ -176,7 +177,7 @@ class GitCommandManager { } // Suppress the output in order to avoid flooding annotations with innocuous errors. - await this.execGit(args, false, true, listeners) + await this.execGit(args, {silent: true, customListeners: listeners}) core.debug(`stderr callback is: ${stderr}`) core.debug(`errline callback is: ${errline}`) @@ -277,7 +278,7 @@ class GitCommandManager { '--get-regexp', pattern ], - true + {allowAllExitCodes: true} ) return output.exitCode === 0 } @@ -321,7 +322,7 @@ class GitCommandManager { const that = this await this.networkRetryHelper.execute(async () => { - await that.execGit(args, false, false, {}, that.timeoutMs) + await that.execGit(args, {timeoutMs: that.timeoutMs}) }) } @@ -337,10 +338,7 @@ class GitCommandManager { repositoryUrl, 'HEAD' ], - false, - false, - {}, - this.timeoutMs + {timeoutMs: this.timeoutMs} ) }) @@ -386,7 +384,7 @@ class GitCommandManager { // Note, "branch --show-current" would be simpler but isn't available until Git 2.22 const output = await this.execGit( ['rev-parse', '--symbolic-full-name', '--verify', '--quiet', 'HEAD'], - true + {allowAllExitCodes: true} ) return !output.stdout.trim().startsWith('refs/heads/') } @@ -396,7 +394,7 @@ class GitCommandManager { const that = this await this.networkRetryHelper.execute(async () => { - await that.execGit(args, false, false, {}, that.timeoutMs) + await that.execGit(args, {timeoutMs: that.timeoutMs}) }) } @@ -406,8 +404,8 @@ class GitCommandManager { async log1(format?: string): Promise { const args = format ? ['log', '-1', format] : ['log', '-1'] - const silent = format ? false : true - const output = await this.execGit(args, false, silent) + const silent = !format + const output = await this.execGit(args, {silent}) return output.stdout } @@ -436,7 +434,7 @@ class GitCommandManager { async shaExists(sha: string): Promise { const args = ['rev-parse', '--verify', '--quiet', `${sha}^{object}`] - const output = await this.execGit(args, true) + const output = await this.execGit(args, {allowAllExitCodes: true}) return output.exitCode === 0 } @@ -457,7 +455,10 @@ class GitCommandManager { args.push('--recursive') } - await this.execGit(args) + const that = this + await this.networkRetryHelper.execute(async () => { + await that.execGit(args, {timeoutMs: that.timeoutMs}) + }) } async submoduleUpdate(fetchDepth: number, recursive: boolean): Promise { @@ -471,11 +472,14 @@ class GitCommandManager { args.push('--recursive') } - await this.execGit(args) + const that = this + await this.networkRetryHelper.execute(async () => { + await that.execGit(args, {timeoutMs: that.timeoutMs}) + }) } async submoduleStatus(): Promise { - const output = await this.execGit(['submodule', 'status'], true) + const output = await this.execGit(['submodule', 'status'], {allowAllExitCodes: true}) core.debug(output.stdout) return output.exitCode === 0 } @@ -486,7 +490,7 @@ class GitCommandManager { } async tryClean(): Promise { - const output = await this.execGit(['clean', '-ffdx'], true) + const output = await this.execGit(['clean', '-ffdx'], {allowAllExitCodes: true}) return output.exitCode === 0 } @@ -501,7 +505,7 @@ class GitCommandManager { '--unset-all', configKey ], - true + {allowAllExitCodes: true} ) return output.exitCode === 0 } @@ -520,14 +524,14 @@ class GitCommandManager { } args.push('--unset', configKey, configValue) - const output = await this.execGit(args, true) + const output = await this.execGit(args, {allowAllExitCodes: true}) return output.exitCode === 0 } async tryDisableAutomaticGarbageCollection(): Promise { const output = await this.execGit( ['config', '--local', 'gc.auto', '0'], - true + {allowAllExitCodes: true} ) return output.exitCode === 0 } @@ -535,7 +539,7 @@ class GitCommandManager { async tryGetFetchUrl(): Promise { const output = await this.execGit( ['config', '--local', '--get', 'remote.origin.url'], - true + {allowAllExitCodes: true} ) if (output.exitCode !== 0) { @@ -563,7 +567,7 @@ class GitCommandManager { } args.push('--get-all', configKey) - const output = await this.execGit(args, true) + const output = await this.execGit(args, {allowAllExitCodes: true}) if (output.exitCode !== 0) { return [] @@ -588,7 +592,7 @@ class GitCommandManager { } args.push('--name-only', '--get-regexp', pattern) - const output = await this.execGit(args, true) + const output = await this.execGit(args, {allowAllExitCodes: true}) if (output.exitCode !== 0) { return [] @@ -601,7 +605,7 @@ class GitCommandManager { } async tryReset(): Promise { - const output = await this.execGit(['reset', '--hard', 'HEAD'], true) + const output = await this.execGit(['reset', '--hard', 'HEAD'], {allowAllExitCodes: true}) return output.exitCode === 0 } @@ -609,10 +613,23 @@ class GitCommandManager { return this.gitVersion } + /** + * Sets the timeout for network git operations. + * @param timeoutSeconds Timeout in seconds. 0 disables the timeout. + */ setTimeout(timeoutSeconds: number): void { + if (timeoutSeconds < 0) { + throw new Error(`Timeout must be non-negative, got ${timeoutSeconds}`) + } this.timeoutMs = timeoutSeconds * 1000 } + /** + * Configures retry behavior for network git operations. + * @param maxAttempts Total attempts including the initial one. Must be >= 1. + * @param minBackoffSeconds Minimum backoff between retries. Must be <= maxBackoffSeconds. + * @param maxBackoffSeconds Maximum backoff between retries. + */ setRetryConfig( maxAttempts: number, minBackoffSeconds: number, @@ -641,13 +658,42 @@ class GitCommandManager { private async execGit( args: string[], - allowAllExitCodes = false, - silent = false, - customListeners = {}, - timeoutMs = 0 + options: { + allowAllExitCodes?: boolean + silent?: boolean + customListeners?: {} + timeoutMs?: number + } = {} ): Promise { + const { + allowAllExitCodes = false, + silent = false, + customListeners = {}, + timeoutMs = 0 + } = options + fshelper.directoryExistsSync(this.workingDirectory, true) + // Use child_process.spawn directly when timeout is set, + // so we can kill the process on timeout and avoid orphaned git processes. + // Note: customListeners are not supported in the timeout path. + if (timeoutMs > 0) { + if ( + customListeners && + Object.keys(customListeners).length > 0 + ) { + core.debug( + 'customListeners are not supported with timeoutMs and will be ignored' + ) + } + return await this.execGitWithTimeout( + args, + timeoutMs, + silent, + allowAllExitCodes + ) + } + const result = new GitOutput() const env = {} @@ -667,7 +713,7 @@ class GitCommandManager { const mergedListeners = {...defaultListener, ...customListeners} const stdout: string[] = [] - const options = { + const execOptions = { cwd: this.workingDirectory, env, silent, @@ -675,27 +721,7 @@ class GitCommandManager { listeners: mergedListeners } - const execPromise = exec.exec(`"${this.gitPath}"`, args, options) - - if (timeoutMs > 0) { - let timer: ReturnType - const timeoutPromise = new Promise((_, reject) => { - timer = global.setTimeout(() => { - reject( - new Error( - `Git operation timed out after ${timeoutMs / 1000} seconds: git ${args.slice(0, 3).join(' ')}...` - ) - ) - }, timeoutMs) - }) - try { - result.exitCode = await Promise.race([execPromise, timeoutPromise]) - } finally { - clearTimeout(timer!) - } - } else { - result.exitCode = await execPromise - } + result.exitCode = await exec.exec(`"${this.gitPath}"`, args, execOptions) result.stdout = stdout.join('') @@ -705,6 +731,137 @@ class GitCommandManager { return result } + /** + * Executes a git command with a timeout. Uses child_process.spawn directly + * (instead of @actions/exec) so we can kill the process on timeout and + * terminate it cleanly. Does not support customListeners. + */ + private async execGitWithTimeout( + args: string[], + timeoutMs: number, + silent: boolean, + allowAllExitCodes: boolean + ): Promise { + const result = new GitOutput() + + const env: {[key: string]: string} = {} + for (const key of Object.keys(process.env)) { + env[key] = process.env[key] as string + } + for (const key of Object.keys(this.gitEnv)) { + env[key] = this.gitEnv[key] + } + + const stdout: string[] = [] + const stderr: string[] = [] + + return new Promise((resolve, reject) => { + const child = spawn(this.gitPath, args, { + cwd: this.workingDirectory, + env, + stdio: ['ignore', 'pipe', 'pipe'] + }) + + child.stdout?.on('data', (data: Buffer) => { + stdout.push(data.toString()) + }) + + if (child.stderr) { + child.stderr.on('data', (data: Buffer) => { + stderr.push(data.toString()) + if (!silent) { + process.stderr.write(data) + } + }) + } + + let settled = false + let timedOut = false + let forceKillTimer: ReturnType | undefined + + const cleanup = (): void => { + clearTimeout(timer) + if (forceKillTimer) { + clearTimeout(forceKillTimer) + } + } + + const timer = global.setTimeout(() => { + timedOut = true + // SIGTERM first, then force SIGKILL after 5 seconds. + // On Windows, SIGTERM is equivalent to a forced kill, so + // the SIGKILL fallback is effectively a no-op there. + child.kill('SIGTERM') + forceKillTimer = global.setTimeout(() => { + try { + child.kill('SIGKILL') + } catch (killErr) { + core.debug( + `Failed to SIGKILL git process: ${killErr}` + ) + } + }, 5000) + if (forceKillTimer.unref) { + forceKillTimer.unref() + } + }, timeoutMs) + if (timer.unref) { + timer.unref() + } + + child.on('close', (code: number | null) => { + if (settled) return + settled = true + cleanup() + + if (timedOut) { + reject( + new Error( + `Git operation timed out after ${timeoutMs / 1000} seconds: git ${args.slice(0, 5).join(' ')}...` + ) + ) + return + } + + // null code means killed by signal (e.g. OOM killer, external SIGTERM) + if (code === null) { + const stderrText = stderr.join('').trim() + reject( + new Error( + `The process 'git' was killed by a signal` + + (stderrText ? `\n${stderrText}` : '') + ) + ) + return + } + + if (code !== 0 && !allowAllExitCodes) { + const stderrText = stderr.join('').trim() + reject( + new Error( + `The process 'git' failed with exit code ${code}` + + (stderrText ? `\n${stderrText}` : '') + ) + ) + return + } + + result.exitCode = code + result.stdout = stdout.join('') + core.debug(result.exitCode.toString()) + core.debug(result.stdout) + resolve(result) + }) + + child.on('error', (err: Error) => { + if (settled) return + settled = true + cleanup() + reject(err) + }) + }) + } + private async initializeCommandManager( workingDirectory: string, lfs: boolean, diff --git a/src/git-source-settings.ts b/src/git-source-settings.ts index fd44634..22ce33e 100644 --- a/src/git-source-settings.ts +++ b/src/git-source-settings.ts @@ -120,20 +120,19 @@ export interface IGitSourceSettings { githubServerUrl: string | undefined /** - * Timeout in seconds for each network git operation attempt (fetch, lfs-fetch, ls-remote). - * 0 means no timeout. Similar to Kubernetes probe timeoutSeconds. + * Timeout in seconds for each network git operation attempt (e.g. fetch, lfs-fetch, ls-remote). + * 0 means no timeout. */ timeout: number /** - * Maximum number of retry attempts for failed network git operations. - * Similar to Kubernetes probe failureThreshold. + * Total number of attempts for each network git operation (including the initial attempt). + * For example, 3 means one initial attempt plus up to 2 retries. */ retryMaxAttempts: number /** * Minimum backoff time in seconds between retry attempts. - * Similar to Kubernetes probe periodSeconds. */ retryMinBackoff: number diff --git a/src/input-helper.ts b/src/input-helper.ts index a40a169..4fc2368 100644 --- a/src/input-helper.ts +++ b/src/input-helper.ts @@ -161,38 +161,57 @@ export async function getInputs(): Promise { result.githubServerUrl = core.getInput('github-server-url') core.debug(`GitHub Host URL = ${result.githubServerUrl}`) - // Timeout (per-attempt, like k8s timeoutSeconds) - result.timeout = Math.floor(Number(core.getInput('timeout') || '300')) + // Timeout per network operation attempt + const timeoutInput = core.getInput('timeout') + result.timeout = Math.floor(Number(timeoutInput !== '' ? timeoutInput : '300')) if (isNaN(result.timeout) || result.timeout < 0) { + core.warning( + `Invalid value '${timeoutInput}' for 'timeout' input. Using default: 300 seconds.` + ) result.timeout = 300 } core.debug(`timeout = ${result.timeout}`) - // Retry max attempts (like k8s failureThreshold) + // Retry max attempts (total attempts including initial) + const retryMaxAttemptsInput = core.getInput('retry-max-attempts') result.retryMaxAttempts = Math.floor( - Number(core.getInput('retry-max-attempts') || '3') + Number(retryMaxAttemptsInput !== '' ? retryMaxAttemptsInput : '3') ) if (isNaN(result.retryMaxAttempts) || result.retryMaxAttempts < 1) { + core.warning( + `Invalid value '${retryMaxAttemptsInput}' for 'retry-max-attempts' input. Using default: 3.` + ) result.retryMaxAttempts = 3 } core.debug(`retry max attempts = ${result.retryMaxAttempts}`) - // Retry backoff (like k8s periodSeconds, but as a min/max range) + // Retry backoff range + const retryMinBackoffInput = core.getInput('retry-min-backoff') result.retryMinBackoff = Math.floor( - Number(core.getInput('retry-min-backoff') || '10') + Number(retryMinBackoffInput !== '' ? retryMinBackoffInput : '10') ) if (isNaN(result.retryMinBackoff) || result.retryMinBackoff < 0) { + core.warning( + `Invalid value '${retryMinBackoffInput}' for 'retry-min-backoff' input. Using default: 10 seconds.` + ) result.retryMinBackoff = 10 } core.debug(`retry min backoff = ${result.retryMinBackoff}`) + const retryMaxBackoffInput = core.getInput('retry-max-backoff') result.retryMaxBackoff = Math.floor( - Number(core.getInput('retry-max-backoff') || '20') + Number(retryMaxBackoffInput !== '' ? retryMaxBackoffInput : '20') ) if (isNaN(result.retryMaxBackoff) || result.retryMaxBackoff < 0) { + core.warning( + `Invalid value '${retryMaxBackoffInput}' for 'retry-max-backoff' input. Using default: 20 seconds.` + ) result.retryMaxBackoff = 20 } if (result.retryMaxBackoff < result.retryMinBackoff) { + core.warning( + `'retry-max-backoff' (${result.retryMaxBackoff}) is less than 'retry-min-backoff' (${result.retryMinBackoff}). Using retry-min-backoff value for both.` + ) result.retryMaxBackoff = result.retryMinBackoff } core.debug(`retry max backoff = ${result.retryMaxBackoff}`)