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>
This commit is contained in:
Anatoly Rabkin 2026-03-18 19:04:59 +02:00
parent 5df58a66d1
commit 3ff67abc5a
8 changed files with 556 additions and 121 deletions

View file

@ -5,6 +5,17 @@ import * as commandManager from '../lib/git-command-manager'
let git: commandManager.IGitCommandManager
let mockExec = jest.fn()
function createMockGit(): Promise<commandManager.IGitCommandManager> {
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)
})

View file

@ -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'")
)
})
})