diff --git a/src/main/git/runner-wsl-gh-fallback.test.ts b/src/main/git/runner-wsl-gh-fallback.test.ts index c77108588..8b802c2f0 100644 --- a/src/main/git/runner-wsl-gh-fallback.test.ts +++ b/src/main/git/runner-wsl-gh-fallback.test.ts @@ -1,3 +1,4 @@ +import { EventEmitter } from 'node:events' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type * as WslModule from '../wsl' @@ -21,11 +22,26 @@ vi.mock('../wsl', async (importOriginal) => ({ import { ghExecFileAsync, glabExecFileAsync } from './runner' +type MockChildProcess = EventEmitter & { + pid: number + kill: ReturnType + unref: ReturnType +} + +function createMockChildProcess(pid: number): MockChildProcess { + const child = new EventEmitter() as MockChildProcess + child.pid = pid + child.kill = vi.fn() + child.unref = vi.fn() + return child +} + describe('ghExecFileAsync WSL fallback', () => { const originalPlatform = process.platform beforeEach(() => { execFileMock.mockReset() + spawnMock.mockReset() getDefaultWslDistroMock.mockReset() getDefaultWslDistroMock.mockReturnValue(null) Object.defineProperty(process, 'platform', { @@ -35,6 +51,7 @@ describe('ghExecFileAsync WSL fallback', () => { }) afterEach(() => { + vi.useRealTimers() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform @@ -352,6 +369,80 @@ describe('ghExecFileAsync WSL fallback', () => { ) }) + it('times out the default-WSL glab fallback and waits for full tree cleanup', async () => { + vi.useFakeTimers() + getDefaultWslDistroMock.mockReturnValue('Ubuntu') + const nativeChild = createMockChildProcess(1200) + const wslChild = createMockChildProcess(2400) + const taskkill = createMockChildProcess(3600) + execFileMock + .mockImplementationOnce((_binary, _args, _options, callback) => { + callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) + return nativeChild + }) + .mockReturnValueOnce(wslChild) + spawnMock.mockReturnValue(taskkill) + + const promise = glabExecFileAsync(['auth', 'status'], { timeout: 1000 }) + const rejection = expect(promise).rejects.toThrow('wsl.exe timed out.') + let rejected = false + void promise.catch(() => { + rejected = true + }) + + await vi.advanceTimersByTimeAsync(999) + expect(spawnMock).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + expect(spawnMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '2400', '/t', '/f'], + expect.objectContaining({ stdio: 'ignore', windowsHide: true }) + ) + await Promise.resolve() + expect(rejected).toBe(false) + + taskkill.emit('close', 0) + await rejection + expect(wslChild.kill).not.toHaveBeenCalled() + }) + + it('aborts the default-WSL glab fallback with full process-tree cleanup', async () => { + getDefaultWslDistroMock.mockReturnValue('Ubuntu') + const nativeChild = createMockChildProcess(1200) + const wslChild = createMockChildProcess(2400) + const taskkill = createMockChildProcess(3600) + execFileMock + .mockImplementationOnce((_binary, _args, _options, callback) => { + callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) + return nativeChild + }) + .mockReturnValueOnce(wslChild) + spawnMock.mockReturnValue(taskkill) + const controller = new AbortController() + + const promise = glabExecFileAsync(['auth', 'status'], { signal: controller.signal }) + const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) + await vi.waitFor(() => expect(execFileMock).toHaveBeenCalledTimes(2)) + controller.abort() + + expect(execFileMock).toHaveBeenNthCalledWith( + 2, + 'wsl.exe', + ['-d', 'Ubuntu', '--', 'bash', '-c', "'glab' 'auth' 'status'"], + expect.not.objectContaining({ signal: controller.signal }), + expect.any(Function) + ) + expect(spawnMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '2400', '/t', '/f'], + expect.objectContaining({ stdio: 'ignore', windowsHide: true }) + ) + taskkill.emit('close', 0) + + await rejection + expect(wslChild.kill).not.toHaveBeenCalled() + }) + it('does not wake the default WSL distro for host-only GitLab diagnostics', async () => { getDefaultWslDistroMock.mockReturnValue('Ubuntu') execFileMock diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 8c70421e3..598ed2a97 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -323,24 +323,57 @@ function createAbortError(): Error { return error } -function killSpawnedCommandTree(child: ChildProcess): void { +const WINDOWS_TREE_KILL_WAIT_MS = 2_000 + +function killSpawnedCommandTree(child: ChildProcess): Promise { const pid = child.pid if (!pid || process.platform !== 'win32') { child.kill() - return + return Promise.resolve() } - try { - // Why: Windows package-manager CLIs are often .cmd shims. Killing only - // cmd.exe leaves the underlying node/npm/pnpm child running. - const killer = spawn('taskkill', ['/pid', String(pid), '/t', '/f'], { - stdio: 'ignore', - windowsHide: true - }) - killer.on('error', () => child.kill()) + return new Promise((resolve) => { + let killer: ChildProcess + try { + // Why: Windows shims and wsl.exe can own descendants; wait for /t tree + // cleanup before settling so a timed-out command cannot outlive its probe. + killer = spawn('taskkill', ['/pid', String(pid), '/t', '/f'], { + stdio: 'ignore', + windowsHide: true + }) + if (!killer || typeof killer.unref !== 'function') { + child.kill() + resolve() + return + } + } catch { + child.kill() + resolve() + return + } + let settled = false + let timer: NodeJS.Timeout | null = null + const finish = (fallbackToChildKill: boolean): void => { + if (settled) { + return + } + settled = true + if (timer) { + clearTimeout(timer) + } + killer.removeAllListeners() + if (fallbackToChildKill) { + child.kill() + } + resolve() + } + killer.once('error', () => finish(true)) + killer.once('close', (code) => finish(code !== 0)) + timer = setTimeout(() => { + killer.kill() + finish(true) + }, WINDOWS_TREE_KILL_WAIT_MS) killer.unref() - } catch { - child.kill() - } + }) } type ExecFileCaptureOptions = Omit & { @@ -376,6 +409,7 @@ function execFileCapture( } let settled = false + let terminating = false let child: ChildProcess | null = null let timer: NodeJS.Timeout | null = null const cleanup = (): void => { @@ -405,14 +439,26 @@ function execFileCapture( resolve({ stdout, stderr }) } const onAbort = (): void => { - if (child) { - killSpawnedCommandTree(child) + if (settled || terminating) { + return } - finish(createAbortError()) + terminating = true + const abortError = createAbortError() + if (!child) { + terminating = false + finish(abortError) + return + } + void killSpawnedCommandTree(child).then(() => { + terminating = false + finish(abortError) + }) } try { const spawnStartedAt = performance.now() + // Why: the abort listener below owns tree-aware cleanup. Node's + // signal handler could kill wsl.exe before taskkill sees its children. child = execFile( command, args, @@ -420,10 +466,12 @@ function execFileCapture( cwd: options.cwd, encoding: options.encoding, maxBuffer: options.maxBuffer ?? DEFAULT_GIT_MAX_BUFFER, - env: options.env, - signal: options.signal + env: options.env }, (error, stdout, stderr) => { + if (terminating) { + return + } if (!error && stderr === undefined && isExecFileResultObject(stdout)) { finish(null, stdout.stdout, stdout.stderr) return @@ -437,21 +485,34 @@ function execFileCapture( return } - child.once('error', (error) => finish(error)) + child.once('error', (error) => { + if (!terminating) { + finish(error) + } + }) if (options.stdin !== undefined) { endSubprocessStdin(child.stdin, options.stdin) } - // Why: Node's native execFile timeout waits for the child to exit after - // signaling it. Some CLIs ignore that signal, so reject the UI operation - // on our own timer and kill the child only as best effort. + // Why: Node's native timeout can wait forever for signal-ignoring CLIs; + // enforce our own deadline and settle after bounded process-tree cleanup. if (options.timeout && options.timeout > 0) { timer = setTimeout(() => { - if (child) { - killSpawnedCommandTree(child) + if (settled || terminating) { + return } - finish(new Error(`${command} timed out.`)) + terminating = true + const timeoutError = new Error(`${command} timed out.`) + if (!child) { + terminating = false + finish(timeoutError) + return + } + void killSpawnedCommandTree(child).then(() => { + terminating = false + finish(timeoutError) + }) }, options.timeout) } options.signal?.addEventListener('abort', onAbort, { once: true }) @@ -484,7 +545,7 @@ async function spawnCommandCapture( recordSubprocessSpawn(spawnCmd, spawnArgs, performance.now() - spawnStartedAt) let timer: NodeJS.Timeout | null = null const onAbort = (): void => { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(createAbortError()) } const cleanupListeners = (): void => { @@ -512,7 +573,7 @@ async function spawnCommandCapture( } timer = options.timeout ? setTimeout(() => { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(new Error(`${command} timed out.`)) }, options.timeout) : null @@ -520,7 +581,7 @@ async function spawnCommandCapture( function onStdoutData(chunk: Buffer): void { stdoutBytes += chunk.byteLength if (options.maxBuffer && stdoutBytes > options.maxBuffer) { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(new Error(`${command} stdout exceeded maxBuffer.`)) return } @@ -529,7 +590,7 @@ async function spawnCommandCapture( function onStderrData(chunk: Buffer): void { stderrBytes += chunk.byteLength if (options.maxBuffer && stderrBytes > options.maxBuffer) { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(new Error(`${command} stderr exceeded maxBuffer.`)) return } @@ -997,7 +1058,7 @@ export async function gitStreamStdout( function onStdoutData(chunk: Buffer): void { stdoutBytes += chunk.byteLength if (stdoutBytes > maxBuffer) { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(new Error('git stdout exceeded maxBuffer.')) return } @@ -1012,7 +1073,7 @@ export async function gitStreamStdout( try { shouldStop = options.onStdout(decoded) } catch (error) { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(error instanceof Error ? error : new Error(String(error))) return } @@ -1020,14 +1081,14 @@ export async function gitStreamStdout( // Why: parser hit its limit. Kill git and resolve cleanly — the // partial output we already parsed is the intended result. stoppedEarly = true - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(null) } } function onStderrData(chunk: Buffer): void { stderrBytes += chunk.byteLength if (stderrBytes > maxBuffer) { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(new Error('git stderr exceeded maxBuffer.')) return } @@ -1044,7 +1105,7 @@ export async function gitStreamStdout( finish(new Error(`git exited with ${code}: ${stderr}`)) } function onAbort(): void { - killSpawnedCommandTree(child) + void killSpawnedCommandTree(child) finish(createAbortError()) } @@ -1468,7 +1529,8 @@ export async function glabExecFileAsync( encoding: (options.encoding ?? 'utf-8') as BufferEncoding, maxBuffer: options.maxBuffer, timeout: options.timeout, - env: options.env + env: options.env, + signal: options.signal }) return { stdout: stdout as string, stderr: stderr as string } } catch (err) { diff --git a/src/main/gitlab/client.ts b/src/main/gitlab/client.ts index 370553db9..eef57b3aa 100644 --- a/src/main/gitlab/client.ts +++ b/src/main/gitlab/client.ts @@ -248,13 +248,9 @@ export async function getProjectSlug( connectionId?: string | null, options: HostedReviewExecutionOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) - return getProjectRef( - repoPath, - knownHosts, - connectionId, - ...hostedReviewLocalGitOptionArgs(options) - ) + const localGitArgs = hostedReviewLocalGitOptionArgs(options) + const knownHosts = await getGlabKnownHosts(connectionId, localGitArgs[0]) + return getProjectRef(repoPath, knownHosts, connectionId, ...localGitArgs) } /** @@ -268,9 +264,9 @@ export async function getMergeRequest( connectionId?: string | null, options: HostedReviewExecutionOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) const localGitArgs = hostedReviewLocalGitOptionArgs(options) const localGitOptions = localGitArgs[0] ?? {} + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const projectRef = await getProjectRef(repoPath, knownHosts, connectionId, ...localGitArgs) await acquire() try { @@ -323,9 +319,9 @@ export async function getMergeRequestForBranch( if (!branchName && linkedMRIid == null) { return null } - const knownHosts = await getGlabKnownHosts(connectionId) const localGitArgs = hostedReviewLocalGitOptionArgs(options) const localGitOptions = localGitArgs[0] ?? {} + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const projectRef = await getProjectRef(repoPath, knownHosts, connectionId, ...localGitArgs) if (!projectRef) { return null @@ -425,7 +421,7 @@ export async function listMergeRequests( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) // Why: MRs sit on `origin` in the fork model (the user's fork is where // they push branches and submit MRs). Mirror github's `getOwnerRepo` // call site by going through the upstream/origin preference resolver @@ -623,7 +619,7 @@ export async function listWorkItems( localGitOptions: LocalGitExecOptions = {} ): Promise> { const issueState = mrStateToIssueState(state) - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -759,7 +755,7 @@ export async function listTodos( ): Promise { const projectRef = await getProjectRef( repoPath, - await getGlabKnownHosts(connectionId), + await getGlabKnownHosts(connectionId, localGitOptions), connectionId, localGitOptions ) @@ -838,7 +834,7 @@ async function withProjectRef( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(connectionId), + await getGlabKnownHosts(connectionId, localGitOptions), connectionId, localGitOptions ) diff --git a/src/main/gitlab/gitlab-known-host-probe.ts b/src/main/gitlab/gitlab-known-host-probe.ts new file mode 100644 index 000000000..1756df4ce --- /dev/null +++ b/src/main/gitlab/gitlab-known-host-probe.ts @@ -0,0 +1,110 @@ +import { glabExecFileAsync } from '../git/runner' +import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' +import { DEFAULT_GITLAB_HOSTS, normalizeGitLabHost } from './project-ref-parser' + +export type LocalGitExecOptions = { + wslDistro?: string +} + +const GLAB_KNOWN_HOSTS_TIMEOUT_MS = 10_000 +const knownHostsCacheByExecutionContext = new Map() +const knownHostsInFlightByExecutionContext = new Map>() + +function knownHostsExecutionKey( + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): string { + if (connectionId) { + // Why: reconnecting can replace the SSH/relay execution host under the same id. + return `connection:${connectionId}:${getSshGitProviderGeneration(connectionId)}` + } + return localGitOptions.wslDistro ? `wsl:${localGitOptions.wslDistro}` : 'native' +} + +/** @internal - exposed for tests only */ +export function _resetKnownHostsCache(): void { + knownHostsCacheByExecutionContext.clear() + knownHostsInFlightByExecutionContext.clear() +} + +export function rememberGlabKnownHost( + host: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): void { + const normalizedHost = normalizeGitLabHost(host) + const key = knownHostsExecutionKey(connectionId, localGitOptions) + const cached = knownHostsCacheByExecutionContext.get(key) + if (!cached || cached.map(normalizeGitLabHost).includes(normalizedHost)) { + return + } + knownHostsCacheByExecutionContext.set(key, [...cached, normalizedHost]) +} + +export async function getGlabKnownHosts( + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + const key = knownHostsExecutionKey(connectionId, localGitOptions) + const cached = knownHostsCacheByExecutionContext.get(key) + if (cached) { + return cached + } + const inFlight = knownHostsInFlightByExecutionContext.get(key) + if (inFlight) { + return inFlight + } + const probe = probeGlabKnownHosts(key, connectionId, localGitOptions) + knownHostsInFlightByExecutionContext.set(key, probe) + try { + return await probe + } finally { + if (knownHostsInFlightByExecutionContext.get(key) === probe) { + knownHostsInFlightByExecutionContext.delete(key) + } + } +} + +async function probeGlabKnownHosts( + key: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + try { + // Why: auth config belongs to the executing host; do not share native, WSL, + // or reconnected SSH/relay results, and bound an otherwise global probe. + const { stdout, stderr } = await glabExecFileAsync(['auth', 'status'], { + timeout: GLAB_KNOWN_HOSTS_TIMEOUT_MS, + ...(!connectionId && localGitOptions.wslDistro + ? { wslDistro: localGitOptions.wslDistro } + : {}) + }) + const hosts = parseGlabAuthStatusHosts(`${stdout}\n${stderr}`) + const merged = Array.from(new Set([...DEFAULT_GITLAB_HOSTS, ...hosts])) + knownHostsCacheByExecutionContext.set(key, merged) + return merged + } catch { + // Keep failures uncached so auth or tunnel recovery is discovered later. + return [...DEFAULT_GITLAB_HOSTS] + } +} + +export function parseGlabAuthStatusHosts(output: string): string[] { + const hosts = new Set() + // Why: self-hosted GitLab can run on a non-default port; preserve it so + // services on the same hostname remain distinct downstream. + for (const match of output.matchAll(/logged in to ([a-zA-Z0-9.-]+(?::\d+)?)/gi)) { + hosts.add(match[1].toLowerCase()) + } + for (const line of output.split('\n')) { + const bareLine = line.trim() + const hostLine = bareLine.endsWith(':') ? bareLine.slice(0, -1) : bareLine + if ( + line === bareLine && + /^[a-zA-Z0-9](?:[a-zA-Z0-9.-]*[a-zA-Z0-9])?(?::\d+)?$/.test(hostLine) + ) { + hosts.add(hostLine.toLowerCase()) + } + } + return Array.from(hosts) +} diff --git a/src/main/gitlab/gitlab-project-ref-resolution.ts b/src/main/gitlab/gitlab-project-ref-resolution.ts index 1dba771d0..218a41869 100644 --- a/src/main/gitlab/gitlab-project-ref-resolution.ts +++ b/src/main/gitlab/gitlab-project-ref-resolution.ts @@ -2,6 +2,11 @@ import { gitExecFileAsync, glabExecFileAsync } from '../git/runner' import type { IssueSourcePreference } from '../../shared/types' import { getSshGitProvider } from '../providers/ssh-git-dispatch' import { clearProjectRefInFlight, runProjectRefProbeOnce } from './project-ref-inflight' +import { + parseGlabAuthStatusHosts, + rememberGlabKnownHost, + type LocalGitExecOptions +} from './gitlab-known-host-probe' import { DEFAULT_GITLAB_HOSTS, normalizeGitLabHost, @@ -12,26 +17,16 @@ import { export { DEFAULT_GITLAB_HOSTS, parseGitLabProjectRef } export type { ProjectRef } - -export type LocalGitExecOptions = { - wslDistro?: string -} +export { + _resetKnownHostsCache, + getGlabKnownHosts, + parseGlabAuthStatusHosts +} from './gitlab-known-host-probe' +export type { LocalGitExecOptions } from './gitlab-known-host-probe' const PROJECT_REF_CACHE_MAX_ENTRIES = 512 const projectRefCache = new Map() -// Why: known hosts are cached PER connection. A repo on an SSH connection -// authenticates against a different glab context than the local one, so a -// process-global cache would leak one connection's hosts into another (and -// poison a connection that probes before its tunnel is ready). The local -// context uses the `'local'` key. -const LOCAL_CONNECTION_KEY = 'local' -const knownHostsCacheByConnection = new Map() - -function connectionCacheKey(connectionId?: string | null): string { - return connectionId ?? LOCAL_CONNECTION_KEY -} - /** @internal - exposed for tests only */ export function _resetProjectRefCache(): void { projectRefCache.clear() @@ -43,11 +38,6 @@ export function _getProjectRefCacheSize(): number { return projectRefCache.size } -/** @internal - exposed for tests only */ -export function _resetKnownHostsCache(): void { - knownHostsCacheByConnection.clear() -} - function rememberProjectRefCacheEntry(cacheKey: string, value: ProjectRef | null): void { projectRefCache.set(cacheKey, value) while (projectRefCache.size > PROJECT_REF_CACHE_MAX_ENTRIES) { @@ -118,7 +108,7 @@ async function resolveProjectRefForRemote( localGitOptions )) ) { - rememberGlabKnownHost(remoteCandidate.host, connectionId) + rememberGlabKnownHost(remoteCandidate.host, connectionId, localGitOptions) rememberProjectRefCacheEntry(cacheKey, remoteCandidate) return remoteCandidate } @@ -230,16 +220,6 @@ export function glabHostnameArgs( return connectionId && projectRef?.host ? ['--hostname', projectRef.host] : [] } -function rememberGlabKnownHost(host: string, connectionId?: string | null): void { - const normalizedHost = normalizeGitLabHost(host) - const key = connectionCacheKey(connectionId) - const cached = knownHostsCacheByConnection.get(key) - if (!cached || cached.map(normalizeGitLabHost).includes(normalizedHost)) { - return - } - knownHostsCacheByConnection.set(key, [...cached, normalizedHost]) -} - async function isGlabConfiguredForRemoteHost( repoPath: string, projectRef: Pick, @@ -262,50 +242,3 @@ async function isGlabConfiguredForRemoteHost( return hosts.includes(normalizeGitLabHost(projectRef.host)) } } - -export async function getGlabKnownHosts(connectionId?: string | null): Promise { - const key = connectionCacheKey(connectionId) - const cached = knownHostsCacheByConnection.get(key) - if (cached) { - return cached - } - try { - // Why: `glab auth status` is host-scoped, not cwd-scoped — glab reads its - // own config to list authenticated hosts. The connectionId is threaded so - // the RESULT is cached per connection (a connected repo can have a - // different set of authenticated self-hosted hosts than the local one), - // mirroring how project-ref resolution caches per connection. - const { stdout, stderr } = await glabExecFileAsync(['auth', 'status']) - const hosts = parseGlabAuthStatusHosts(`${stdout}\n${stderr}`) - const merged = Array.from(new Set([...DEFAULT_GITLAB_HOSTS, ...hosts])) - knownHostsCacheByConnection.set(key, merged) - return merged - } catch { - // Auth check failed (glab not installed, no auth, tunnel not ready, - // etc.) — fall back to the canonical default for THIS call, but do NOT - // cache the fallback. A later probe (e.g. after the SSH tunnel comes - // up) must be able to discover the real self-hosted host. - return [...DEFAULT_GITLAB_HOSTS] - } -} - -export function parseGlabAuthStatusHosts(output: string): string[] { - const hosts = new Set() - // Why: self-hosted GitLab can run on a non-default port (e.g. - // `gitlab.example.com:8443`); capture the optional `:port` so two services - // on the same hostname but different ports stay distinct downstream. - for (const m of output.matchAll(/logged in to ([a-zA-Z0-9.-]+(?::\d+)?)/gi)) { - hosts.add(m[1].toLowerCase()) - } - for (const line of output.split('\n')) { - const bareLine = line.trim() - const hostLine = bareLine.endsWith(':') ? bareLine.slice(0, -1) : bareLine - if ( - line === bareLine && - /^[a-zA-Z0-9](?:[a-zA-Z0-9.-]*[a-zA-Z0-9])?(?::\d+)?$/.test(hostLine) - ) { - hosts.add(hostLine.toLowerCase()) - } - } - return Array.from(hosts) -} diff --git a/src/main/gitlab/gl-utils.test.ts b/src/main/gitlab/gl-utils.test.ts index 7306c5b7f..9ab99ddc4 100644 --- a/src/main/gitlab/gl-utils.test.ts +++ b/src/main/gitlab/gl-utils.test.ts @@ -434,6 +434,7 @@ describe('getGlabKnownHosts', () => { }) await expect(getGlabKnownHosts()).resolves.toEqual(['gitlab.com', 'gitlab.example.com']) + expect(glabExecFileAsyncMock).toHaveBeenCalledWith(['auth', 'status'], { timeout: 10_000 }) }) it('falls back to default when glab auth status fails', async () => { @@ -453,6 +454,55 @@ describe('getGlabKnownHosts', () => { expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(1) }) + it('coalesces many simultaneous callers in one execution context', async () => { + let resolveProbe!: (value: { stdout: string; stderr: string }) => void + glabExecFileAsyncMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveProbe = resolve + }) + ) + + const probes = Array.from({ length: 64 }, () => getGlabKnownHosts()) + + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(1) + resolveProbe({ stdout: 'Logged in to gitlab.concurrent.test as user\n', stderr: '' }) + const results = await Promise.all(probes) + expect(results.every((result) => result === results[0])).toBe(true) + expect(results[0]).toEqual(['gitlab.com', 'gitlab.concurrent.test']) + }) + + it('keeps simultaneous native, WSL distro, and connection probes isolated', async () => { + glabExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'Logged in to ubuntu.test as user\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'Logged in to debian.test as user\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'Logged in to native.test as user\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'Logged in to ssh.test as user\n', stderr: '' }) + + const [ubuntu, ubuntuAgain, debian, native, ssh] = await Promise.all([ + getGlabKnownHosts(undefined, { wslDistro: 'Ubuntu' }), + getGlabKnownHosts(undefined, { wslDistro: 'Ubuntu' }), + getGlabKnownHosts(undefined, { wslDistro: 'Debian' }), + getGlabKnownHosts(), + getGlabKnownHosts('conn-1') + ]) + + expect(ubuntuAgain).toBe(ubuntu) + expect(ubuntu).toEqual(['gitlab.com', 'ubuntu.test']) + expect(debian).toEqual(['gitlab.com', 'debian.test']) + expect(native).toEqual(['gitlab.com', 'native.test']) + expect(ssh).toEqual(['gitlab.com', 'ssh.test']) + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(4) + expect(glabExecFileAsyncMock).toHaveBeenNthCalledWith(1, ['auth', 'status'], { + timeout: 10_000, + wslDistro: 'Ubuntu' + }) + expect(glabExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['auth', 'status'], { + timeout: 10_000, + wslDistro: 'Debian' + }) + }) + it('recognizes a self-hosted host on a non-default port', async () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: '✓ Logged in to gitlab.example.com:8080 as user\n', @@ -500,4 +550,51 @@ describe('getGlabKnownHosts', () => { ]) expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(2) }) + + it('removes a timed-out probe from in-flight state so a later call retries', async () => { + let rejectProbe!: (error: Error) => void + glabExecFileAsyncMock + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectProbe = reject + }) + ) + .mockResolvedValueOnce({ stdout: 'Logged in to recovered.test as user\n', stderr: '' }) + + const first = getGlabKnownHosts(undefined, { wslDistro: 'Ubuntu' }) + const concurrent = getGlabKnownHosts(undefined, { wslDistro: 'Ubuntu' }) + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(1) + rejectProbe(new Error('wsl.exe timed out.')) + + await expect(Promise.all([first, concurrent])).resolves.toEqual([ + ['gitlab.com'], + ['gitlab.com'] + ]) + await expect(getGlabKnownHosts(undefined, { wslDistro: 'Ubuntu' })).resolves.toEqual([ + 'gitlab.com', + 'recovered.test' + ]) + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not reuse a successful result after an SSH provider reconnects', async () => { + const connectionId = 'conn-reconnected' + registerSshGitProvider(connectionId, {} as never) + glabExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'Logged in to old-tunnel.test as user\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'Logged in to new-tunnel.test as user\n', stderr: '' }) + + await expect(getGlabKnownHosts(connectionId)).resolves.toEqual([ + 'gitlab.com', + 'old-tunnel.test' + ]) + registerSshGitProvider(connectionId, {} as never) + await expect(getGlabKnownHosts(connectionId)).resolves.toEqual([ + 'gitlab.com', + 'new-tunnel.test' + ]) + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(2) + unregisterSshGitProvider(connectionId) + }) }) diff --git a/src/main/gitlab/issues.test.ts b/src/main/gitlab/issues.test.ts index edc6d2457..cde9d4f9e 100644 --- a/src/main/gitlab/issues.test.ts +++ b/src/main/gitlab/issues.test.ts @@ -214,7 +214,7 @@ describe('gitlab issue operations', () => { await listIssues('/repo-root', 5, undefined, 'opened', undefined, 'conn-7') - expect(getGlabKnownHostsMock).toHaveBeenCalledWith('conn-7') + expect(getGlabKnownHostsMock).toHaveBeenCalledWith('conn-7', {}) }) it('creates an issue and returns its iid + web_url', async () => { diff --git a/src/main/gitlab/issues.ts b/src/main/gitlab/issues.ts index ec36f68a0..970f9ff1f 100644 --- a/src/main/gitlab/issues.ts +++ b/src/main/gitlab/issues.ts @@ -44,7 +44,7 @@ export async function getIssue( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const projectRef = await getIssueProjectRef(repoPath, knownHosts, connectionId, localGitOptions) // Why: don't fall back to a cwd-inferred `glab issue view` when the project // can't be resolved — on an SSH connection cwd is not the repo dir, so glab @@ -92,7 +92,7 @@ export async function listIssues( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -162,7 +162,7 @@ export async function createIssue( if (!trimmedTitle) { return { ok: false, error: 'Title is required' } } - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -232,7 +232,7 @@ export async function updateIssue( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(connectionId), + await getGlabKnownHosts(connectionId, localGitOptions), connectionId, localGitOptions ) @@ -369,7 +369,7 @@ export async function addIssueComment( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(connectionId), + await getGlabKnownHosts(connectionId, localGitOptions), connectionId, localGitOptions ) @@ -427,7 +427,7 @@ export async function listLabels( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -468,7 +468,7 @@ export async function listAssignableUsers( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const { source: projectRef } = await resolveIssueSource( repoPath, preference, diff --git a/src/main/gitlab/work-item-details.ts b/src/main/gitlab/work-item-details.ts index d9f744c13..0b37206d6 100644 --- a/src/main/gitlab/work-item-details.ts +++ b/src/main/gitlab/work-item-details.ts @@ -361,7 +361,7 @@ export async function getWorkItemDetails( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(connectionId), + await getGlabKnownHosts(connectionId, localGitOptions), connectionId, localGitOptions ) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index c255a6586..17fcec9ba 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -17895,7 +17895,7 @@ export class OrcaRuntimeService { } catch (error) { return { error: error instanceof Error ? error.message : 'Could not resolve git remote.' } } - const knownHosts = await getGlabKnownHosts(repo.connectionId ?? null) + const knownHosts = await getGlabKnownHosts(repo.connectionId ?? null, localWorktreeGitOptions) const projectRef = await getGitLabProjectRefForRemote( repo.path, remote, @@ -18020,7 +18020,7 @@ export class OrcaRuntimeService { connectionId?: string | null, localGitOptions: { wslDistro?: string } = {} ): Promise { - const knownHosts = await getGlabKnownHosts(connectionId) + const knownHosts = await getGlabKnownHosts(connectionId, localGitOptions) const localGitOptionArgs = Object.keys(localGitOptions).length > 0 ? ([localGitOptions] as const) : [] if (preference === 'origin') {