fix(gitlab): expire project-ref negatives instead of caching them forever (#12390)
GitLab's project-ref cache stored `null` forever and returned any cached value straight from the map, so a repo probed before `origin` was configured — or before `glab auth login` ran for its self-hosted host — kept hosted-review provider detection stale until app restart. The negative-TTL work that shipped for Azure DevOps / Bitbucket / Gitea skipped it. Mirror `createRemoteRefProbeCache`'s semantics: negatives expire on the shared interval, positives stay, the SSH provider generation joins the cache signature so a reconnect re-asks, and a probe abandoned as stale can no longer publish over its successor. Transient git/SSH failures stay uncached. Expiring negatives would otherwise turn `glab auth status --hostname` into one spawn per repo per interval on the hosted-review poll, since a non-GitLab remote reaches it too, so remember the unauthenticated answer per host — not per repo — on the same clock.
This commit is contained in:
parent
a69ca6d5b7
commit
c052ca10a3
|
|
@ -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<string, readonly string[]>()
|
||||
const knownHostsInFlightByExecutionContext: CoalescedProbes<readonly string[]> = new Map()
|
||||
const unauthenticatedHostExpiries = new Map<string, number>()
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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<string, ProjectRef | null>()
|
||||
|
||||
type CachedProjectRef = { value: ProjectRef | null; expiresAt: number }
|
||||
|
||||
const projectRefCache = new Map<string, CachedProjectRef>()
|
||||
|
||||
/** @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<ProjectRef | null> {
|
||||
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<ProjectRef | null> {
|
||||
// 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<boolean> {
|
||||
// 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
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@ export function clearProjectRefInFlight(): void {
|
|||
|
||||
export async function runProjectRefProbeOnce(
|
||||
cacheKey: string,
|
||||
createProbe: () => Promise<ProjectRef | null>
|
||||
createProbe: (ownsKey: () => boolean) => Promise<ProjectRef | null>
|
||||
): Promise<ProjectRef | null> {
|
||||
// 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).
|
||||
|
|
|
|||
Loading…
Reference in New Issue