diff --git a/src/main/gitlab/gitlab-known-host-probe.ts b/src/main/gitlab/gitlab-known-host-probe.ts index cc4ecf5fa..577baf9ad 100644 --- a/src/main/gitlab/gitlab-known-host-probe.ts +++ b/src/main/gitlab/gitlab-known-host-probe.ts @@ -1,4 +1,5 @@ import { runCoalescedProbe, type CoalescedProbes } from '../git/coalesced-probe' +import { NEGATIVE_ENTRY_TTL_MS } from '../git/remote-ref-probe-cache' import { glabExecFileAsync } from '../git/runner' import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' import { DEFAULT_GITLAB_HOSTS, normalizeGitLabHost } from './project-ref-parser' @@ -8,8 +9,10 @@ export type LocalGitExecOptions = { } const GLAB_KNOWN_HOSTS_TIMEOUT_MS = 10_000 +const UNAUTHENTICATED_HOSTS_MAX_ENTRIES = 128 const knownHostsCacheByExecutionContext = new Map() const knownHostsInFlightByExecutionContext: CoalescedProbes = new Map() +const unauthenticatedHostExpiries = new Map() function knownHostsExecutionKey( connectionId?: string | null, @@ -26,6 +29,63 @@ function knownHostsExecutionKey( export function _resetKnownHostsCache(): void { knownHostsCacheByExecutionContext.clear() knownHostsInFlightByExecutionContext.clear() + unauthenticatedHostExpiries.clear() +} + +/** @internal - exposed for tests only */ +export function _resetGlabUnauthenticatedHosts(): void { + unauthenticatedHostExpiries.clear() +} + +function unauthenticatedHostKey( + host: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): string { + return `${knownHostsExecutionKey(connectionId, localGitOptions)}\0${normalizeGitLabHost(host)}` +} + +/** + * Why: `glab auth status --hostname` is how a self-hosted instance that plain + * `glab auth status` did not list gets discovered, so a remote that is not + * GitLab at all runs it too. Project-ref negatives expire now, and without this + * that becomes one `glab` spawn per repo per interval on the hosted-review poll. + * The answer is per host, not per repo, and expires on the same clock so a + * login still lands within an interval. + */ +export function isGlabHostKnownUnauthenticated( + host: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): boolean { + const key = unauthenticatedHostKey(host, connectionId, localGitOptions) + const expiresAt = unauthenticatedHostExpiries.get(key) + if (expiresAt === undefined) { + return false + } + if (expiresAt > Date.now()) { + return true + } + unauthenticatedHostExpiries.delete(key) + return false +} + +export function rememberGlabHostUnauthenticated( + host: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): void { + unauthenticatedHostExpiries.set( + unauthenticatedHostKey(host, connectionId, localGitOptions), + Date.now() + NEGATIVE_ENTRY_TTL_MS + ) + while (unauthenticatedHostExpiries.size > UNAUTHENTICATED_HOSTS_MAX_ENTRIES) { + const oldestKey = unauthenticatedHostExpiries.keys().next().value + if (oldestKey === undefined) { + return + } + unauthenticatedHostExpiries.delete(oldestKey) + } } export function rememberGlabKnownHost( @@ -52,6 +112,9 @@ export function rememberGlabKnownHosts( } seen.add(normalizedHost) additions.push(normalizedHost) + unauthenticatedHostExpiries.delete( + unauthenticatedHostKey(normalizedHost, connectionId, localGitOptions) + ) } if (additions.length === 0) { return diff --git a/src/main/gitlab/gitlab-project-ref-resolution.ts b/src/main/gitlab/gitlab-project-ref-resolution.ts index 4b33a4c43..ee7203ce6 100644 --- a/src/main/gitlab/gitlab-project-ref-resolution.ts +++ b/src/main/gitlab/gitlab-project-ref-resolution.ts @@ -1,9 +1,14 @@ import { glabExecFileAsync } from '../git/runner' import { isTransientGitProbeError, readRemoteUrl } from '../git/remote-url-probe' +import { NEGATIVE_ENTRY_TTL_MS } from '../git/remote-ref-probe-cache' +import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' import type { IssueSourcePreference } from '../../shared/types' import { clearProjectRefInFlight, runProjectRefProbeOnce } from './project-ref-inflight' import { + _resetGlabUnauthenticatedHosts, + isGlabHostKnownUnauthenticated, parseGlabAuthStatusHosts, + rememberGlabHostUnauthenticated, rememberGlabKnownHost, type LocalGitExecOptions } from './gitlab-known-host-probe' @@ -25,12 +30,16 @@ export { export type { LocalGitExecOptions } from './gitlab-known-host-probe' const PROJECT_REF_CACHE_MAX_ENTRIES = 512 -const projectRefCache = new Map() + +type CachedProjectRef = { value: ProjectRef | null; expiresAt: number } + +const projectRefCache = new Map() /** @internal - exposed for tests only */ export function _resetProjectRefCache(): void { projectRefCache.clear() clearProjectRefInFlight() + _resetGlabUnauthenticatedHosts() } /** @internal - exposed for tests only */ @@ -39,7 +48,14 @@ export function _getProjectRefCacheSize(): number { } function rememberProjectRefCacheEntry(cacheKey: string, value: ProjectRef | null): void { - projectRefCache.set(cacheKey, value) + // Why: "not GitLab" only holds until someone configures `origin` or logs into + // `glab` — a repo probed before either kept hosted-review detection stale for + // the life of the process. Negatives expire the way every other forge's do; + // positives still stay (see `createRemoteRefProbeCache`). + projectRefCache.set(cacheKey, { + value, + expiresAt: value === null ? Date.now() + NEGATIVE_ENTRY_TTL_MS : Number.POSITIVE_INFINITY + }) while (projectRefCache.size > PROJECT_REF_CACHE_MAX_ENTRIES) { const oldestKey = projectRefCache.keys().next().value if (oldestKey === undefined) { @@ -56,19 +72,30 @@ export async function getProjectRefForRemote( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const runtimeKey = connectionId ?? `local:${localGitOptions.wslDistro ?? 'host'}` + // Why: a reconnect replaces the host an answer came from under the same id, so + // the generation is part of the signature; `knownHosts` carries the glab auth + // state, so logging into a self-hosted instance re-asks rather than reusing a + // ref resolved while that host was unknown. + const runtimeKey = connectionId + ? `${connectionId}:${getSshGitProviderGeneration(connectionId)}` + : `local:${localGitOptions.wslDistro ?? 'host'}` const cacheKey = `${runtimeKey}\0${repoPath}\0${remoteName}\0${knownHosts.join(',')}` - if (projectRefCache.has(cacheKey)) { - return projectRefCache.get(cacheKey)! + const cached = projectRefCache.get(cacheKey) + if (cached) { + if (cached.expiresAt > Date.now()) { + return cached.value + } + projectRefCache.delete(cacheKey) } - return runProjectRefProbeOnce(cacheKey, () => + return runProjectRefProbeOnce(cacheKey, (ownsKey) => resolveProjectRefForRemote( repoPath, remoteName, knownHosts, connectionId, cacheKey, + ownsKey, localGitOptions ) ) @@ -80,8 +107,17 @@ async function resolveProjectRefForRemote( knownHosts: readonly string[], connectionId: string | null | undefined, cacheKey: string, + ownsKey: () => boolean, localGitOptions: LocalGitExecOptions ): Promise { + // Why: a probe abandoned as stale still runs, and the repo state it read is + // older than whatever its successor already published. It may answer its own + // callers; it may not overwrite the cache. + const publish = (value: ProjectRef | null): void => { + if (ownsKey()) { + rememberProjectRefCacheEntry(cacheKey, value) + } + } try { const stdout = await readRemoteUrl( { @@ -96,7 +132,7 @@ async function resolveProjectRefForRemote( } const result = parseGitLabProjectRef(stdout, knownHosts) if (result) { - rememberProjectRefCacheEntry(cacheKey, result) + publish(result) return result } const remoteCandidate = parseRemoteProjectRefCandidate(stdout) @@ -110,17 +146,20 @@ async function resolveProjectRefForRemote( )) ) { rememberGlabKnownHost(remoteCandidate.host, connectionId, localGitOptions) - rememberProjectRefCacheEntry(cacheKey, remoteCandidate) + publish(remoteCandidate) return remoteCandidate } } catch (error) { // Why: a wedged or killed probe is not evidence the remote is not GitLab — - // caching it would misdetect the forge for the life of the process (P1-D). + // caching it would misdetect the forge until the negative expires (P1-D). + // SSH failures stay uncached outright rather than adopting the generic + // cache's stable-missing-remote exception: keeping a connected host's + // detection fresh is worth the extra probe. if (connectionId || isTransientGitProbeError(error)) { return null } } - rememberProjectRefCacheEntry(cacheKey, null) + publish(null) return null } @@ -229,12 +268,22 @@ async function isGlabConfiguredForRemoteHost( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + // Why: this probe is per host, but the project-ref miss that reaches it is per + // repo — without the memo, every non-GitLab repo re-spawns `glab` each time + // its negative expires. + if (isGlabHostKnownUnauthenticated(projectRef.host, connectionId, localGitOptions)) { + return false + } try { const result = await glabExecFileAsync( ['auth', 'status', '--hostname', projectRef.host], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) - return result !== undefined + if (result === undefined) { + rememberGlabHostUnauthenticated(projectRef.host, connectionId, localGitOptions) + return false + } + return true } catch (error) { const execLike = error as { stdout?: unknown; stderr?: unknown; message?: unknown } const output = @@ -242,6 +291,10 @@ async function isGlabConfiguredForRemoteHost( .filter((value): value is string => typeof value === 'string' && value.trim().length > 0) .join('\n') || String(error) const hosts = parseGlabAuthStatusHosts(output).map(normalizeGitLabHost) - return hosts.includes(normalizeGitLabHost(projectRef.host)) + if (hosts.includes(normalizeGitLabHost(projectRef.host))) { + return true + } + rememberGlabHostUnauthenticated(projectRef.host, connectionId, localGitOptions) + return false } } diff --git a/src/main/gitlab/gl-utils.test.ts b/src/main/gitlab/gl-utils.test.ts index 23e159671..e1937f3f2 100644 --- a/src/main/gitlab/gl-utils.test.ts +++ b/src/main/gitlab/gl-utils.test.ts @@ -28,16 +28,19 @@ import { import { rememberGlabKnownHost, rememberGlabKnownHosts } from './gitlab-known-host-probe' import { registerSshGitProvider, unregisterSshGitProvider } from '../providers/ssh-git-dispatch' import { REMOTE_URL_PROBE_TIMEOUT_MS } from '../git/remote-url-probe' +import { NEGATIVE_ENTRY_TTL_MS } from '../git/remote-ref-probe-cache' describe('gitlab project ref resolution', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() + glabExecFileAsyncMock.mockReset() sshExecMock.mockReset() unregisterSshGitProvider('conn-1') _resetProjectRefCache() }) afterEach(() => { + vi.useRealTimers() unregisterSshGitProvider('conn-1') }) @@ -211,6 +214,110 @@ describe('gitlab project ref resolution', () => { path: 'remote/orca' }) }) + + it('does not cache a local probe killed on its deadline as a definitive miss', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('git timed out.')) + .mockResolvedValueOnce({ stdout: 'git@gitlab.com:fork/orca.git\n' }) + + await expect(getProjectRef('/repo')).resolves.toBeNull() + await expect(getProjectRef('/repo')).resolves.toEqual({ + host: 'gitlab.com', + path: 'fork/orca' + }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('re-probes a repo whose GitLab remote could have been added since the miss', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + gitExecFileAsyncMock.mockRejectedValueOnce(new Error("error: No such remote 'origin'")) + + await expect(getProjectRef('/repo')).resolves.toBeNull() + await expect(getProjectRef('/repo')).resolves.toBeNull() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + + // Nothing watches `.git/config`, and SSH/WSL repos have no file to watch, so + // a remote configured after the miss is only visible once the negative ages out. + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@gitlab.com:fork/orca.git\n' }) + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS + 1) + + await expect(getProjectRef('/repo')).resolves.toEqual({ + host: 'gitlab.com', + path: 'fork/orca' + }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('keeps a resolved project ref past the negative interval', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@gitlab.com:fork/orca.git\n' }) + + await expect(getProjectRef('/repo')).resolves.toEqual({ + host: 'gitlab.com', + path: 'fork/orca' + }) + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS * 10) + await expect(getProjectRef('/repo')).resolves.toEqual({ + host: 'gitlab.com', + path: 'fork/orca' + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('re-resolves a self-hosted remote once glab auth knows its host', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@gitlab.internal:team/orca.git\n' }) + glabExecFileAsyncMock.mockRejectedValue(new Error('not authenticated')) + + await expect(getProjectRefForRemote('/repo', 'origin', ['gitlab.com'])).resolves.toBeNull() + await expect( + getProjectRefForRemote('/repo', 'origin', ['gitlab.com', 'gitlab.internal']) + ).resolves.toEqual({ host: 'gitlab.internal', path: 'team/orca' }) + }) + + it('asks glab about an unauthenticated host once per interval, not once per repo', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@github.com:team/orca.git\n' }) + glabExecFileAsyncMock.mockRejectedValue(new Error('not authenticated')) + + // Expiring project-ref negatives must not turn the hosted-review poll into a + // `glab auth status` spawn per repo per interval — the answer is per host. + for (const repoPath of ['/repo-a', '/repo-b', '/repo-c']) { + await expect(getProjectRef(repoPath)).resolves.toBeNull() + } + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(1) + + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS + 1) + for (const repoPath of ['/repo-a', '/repo-b', '/repo-c']) { + await expect(getProjectRef(repoPath)).resolves.toBeNull() + } + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not serve a project ref resolved on a retired SSH connection', async () => { + sshExecMock + .mockResolvedValueOnce({ stdout: 'git@gitlab.com:before/orca.git\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'git@gitlab.com:after/orca.git\n', stderr: '' }) + registerSshGitProvider('conn-1', { exec: sshExecMock } as never) + + await expect(getProjectRefForRemote('/repo', 'origin', undefined, 'conn-1')).resolves.toEqual({ + host: 'gitlab.com', + path: 'before/orca' + }) + + // A reconnect can swap the execution host under the same connection id. + unregisterSshGitProvider('conn-1') + registerSshGitProvider('conn-1', { exec: sshExecMock } as never) + + await expect(getProjectRefForRemote('/repo', 'origin', undefined, 'conn-1')).resolves.toEqual({ + host: 'gitlab.com', + path: 'after/orca' + }) + expect(sshExecMock).toHaveBeenCalledTimes(2) + }) }) describe('resolveIssueSource', () => { diff --git a/src/main/gitlab/project-ref-inflight.ts b/src/main/gitlab/project-ref-inflight.ts index a9de9d757..cea4173c4 100644 --- a/src/main/gitlab/project-ref-inflight.ts +++ b/src/main/gitlab/project-ref-inflight.ts @@ -9,7 +9,7 @@ export function clearProjectRefInFlight(): void { export async function runProjectRefProbeOnce( cacheKey: string, - createProbe: () => Promise + createProbe: (ownsKey: () => boolean) => Promise ): Promise { // Why: joining only a probe that is still young keeps a wedged host's dead // promise from pinning every later retry for the process lifetime (P1-D).