Compare commits

..

2 Commits

Author SHA1 Message Date
Anatoly Rabkin 047af58489
Merge 3ff67abc5a into 0c366fd6a8 2026-03-18 23:52:21 +02:00
Anatoly Rabkin 3ff67abc5a 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) <noreply@anthropic.com>
2026-03-18 23:52:14 +02:00
5 changed files with 68 additions and 11 deletions

View File

@ -516,19 +516,29 @@ describe('timeout and retry configuration', () => {
jest.restoreAllMocks()
})
it('setTimeout converts seconds to milliseconds', async () => {
it('setTimeout accepts valid values', async () => {
git = await createMockGit()
// Should not throw
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()
// Should not throw
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)
@ -540,6 +550,28 @@ describe('timeout and retry configuration', () => {
const fetchCalls = mockExec.mock.calls.filter(
(call: any[]) => (call[1] as string[]).includes('fetch')
)
expect(fetchCalls.length).toBeGreaterThan(0)
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)
})

View File

@ -171,11 +171,14 @@ describe('input-helper tests', () => {
expect(settings.retryMaxBackoff).toBe(15)
})
it('clamps retry-max-backoff to min when less than min', async () => {
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 () => {

16
dist/index.js vendored
View File

@ -980,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) {
@ -993,7 +996,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 });
}));
});
}
submoduleStatus() {
@ -1241,13 +1247,16 @@ class GitCommandManager {
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, 3).join(' ')}...`));
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)
@ -2263,6 +2272,7 @@ function getInputs() {
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}`);

View File

@ -455,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<void> {
@ -469,7 +472,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 submoduleStatus(): Promise<boolean> {
@ -799,6 +805,9 @@ class GitCommandManager {
forceKillTimer.unref()
}
}, timeoutMs)
if (timer.unref) {
timer.unref()
}
child.on('close', (code: number | null) => {
if (settled) return
@ -808,7 +817,7 @@ class GitCommandManager {
if (timedOut) {
reject(
new Error(
`Git operation timed out after ${timeoutMs / 1000} seconds: git ${args.slice(0, 3).join(' ')}...`
`Git operation timed out after ${timeoutMs / 1000} seconds: git ${args.slice(0, 5).join(' ')}...`
)
)
return

View File

@ -209,6 +209,9 @@ export async function getInputs(): Promise<IGitSourceSettings> {
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}`)