diff --git a/src/main/azure-devops/pull-request-creation.test.ts b/src/main/azure-devops/pull-request-creation.test.ts index 2b3be3d9eea..f4a8c2a5c81 100644 --- a/src/main/azure-devops/pull-request-creation.test.ts +++ b/src/main/azure-devops/pull-request-creation.test.ts @@ -16,7 +16,8 @@ vi.mock('../git/runner', () => ({ })) vi.mock('../providers/ssh-git-dispatch', () => ({ - getSshGitProvider: getSshGitProviderMock + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: () => 0 })) vi.mock('../source-control/pull-request-template', () => ({ diff --git a/src/main/git/coalesced-probe.ts b/src/main/git/coalesced-probe.ts index 57bc705135e..ce09ba4eaeb 100644 --- a/src/main/git/coalesced-probe.ts +++ b/src/main/git/coalesced-probe.ts @@ -8,16 +8,21 @@ * progress is what lets a host recover in-session. */ -export type CoalescedProbe = { startedAt: number; promise: Promise } +export type CoalescedProbe = { startedAt: number; token: object; promise: Promise } export type CoalescedProbes = Map> /** Every step under a probe is bounded well inside this, so an older one is wedged, not slow. */ export const PROBE_COALESCE_STALE_MS = 60_000 +/** + * `createProbe` receives `ownsKey`: false once this probe has been abandoned for + * a successor, which is what tells a late one not to publish its answer over the + * fresher one someone else is already being served. + */ export async function runCoalescedProbe( probes: CoalescedProbes, key: string, - createProbe: () => Promise, + createProbe: (ownsKey: () => boolean) => Promise, staleAfterMs: number = PROBE_COALESCE_STALE_MS ): Promise { const now = Date.now() @@ -29,7 +34,12 @@ export async function runCoalescedProbe( // The abandoned probe keeps running; nothing here will await it again. void existing.promise.catch(() => {}) } - const entry: CoalescedProbe = { startedAt: now, promise: createProbe() } + const token = {} + const entry: CoalescedProbe = { + startedAt: now, + token, + promise: createProbe(() => probes.get(key)?.token === token) + } probes.set(key, entry) try { return await entry.promise diff --git a/src/main/git/remote-ref-probe-cache.test.ts b/src/main/git/remote-ref-probe-cache.test.ts new file mode 100644 index 00000000000..c1f6b0f0981 --- /dev/null +++ b/src/main/git/remote-ref-probe-cache.test.ts @@ -0,0 +1,198 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { getSshGitProviderMock, getSshGitProviderGenerationMock, gitExecFileAsyncMock } = vi.hoisted( + () => ({ + getSshGitProviderMock: vi.fn(), + getSshGitProviderGenerationMock: vi.fn(() => 0), + gitExecFileAsyncMock: vi.fn() + }) +) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: getSshGitProviderGenerationMock, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE: 'SSH Git provider unavailable' +})) + +vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) + +import { PROBE_COALESCE_STALE_MS } from './coalesced-probe' +import { createRemoteRefProbeCache, NEGATIVE_ENTRY_TTL_MS } from './remote-ref-probe-cache' + +/** Stands in for a forge's parser: claims one host, rejects everything else. */ +function parseExampleRef(remoteUrl: string): { repo: string } | null { + const match = remoteUrl.trim().match(/^git@example\.com:(.+?)(?:\.git)?$/) + return match ? { repo: match[1] } : null +} + +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve: (value: T) => void = () => {} + const promise = new Promise((settle) => { + resolve = settle + }) + return { promise, resolve } +} + +describe('remote ref probe cache (P1-D)', () => { + beforeEach(() => { + getSshGitProviderMock.mockReset() + getSshGitProviderGenerationMock.mockReset() + getSshGitProviderGenerationMock.mockReturnValue(0) + gitExecFileAsyncMock.mockReset() + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('answers concurrent lookups for one repo with a single probe', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@example.com:team/repo.git\n' }) + + const answers = await Promise.all([ + cache.get('/repo', 'origin'), + cache.get('/repo', 'origin'), + cache.get('/repo', 'origin') + ]) + + expect(answers).toEqual([{ repo: 'team/repo' }, { repo: 'team/repo' }, { repo: 'team/repo' }]) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('re-probes a repo whose remotes could have changed since the miss', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + gitExecFileAsyncMock.mockRejectedValueOnce(new Error("fatal: No such remote 'origin'")) + + await expect(cache.get('/repo', 'origin')).resolves.toBeNull() + await expect(cache.get('/repo', 'origin')).resolves.toBeNull() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + + // A remote added after the miss is only visible once the negative expires; + // nothing here watches .git/config, and SSH/WSL repos have no file to watch. + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@example.com:team/repo.git\n' }) + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS + 1) + + await expect(cache.get('/repo', 'origin')).resolves.toEqual({ repo: 'team/repo' }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('keeps a resolved ref without re-probing', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@example.com:team/repo.git\n' }) + + await expect(cache.get('/repo', 'origin')).resolves.toEqual({ repo: 'team/repo' }) + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS * 10) + await expect(cache.get('/repo', 'origin')).resolves.toEqual({ repo: 'team/repo' }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('does not let a lookup on a reconnected provider join the old connection probe', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + let releaseStalled = (): void => {} + const execMock = vi + .fn() + .mockImplementationOnce( + async () => await new Promise((resolve) => (releaseStalled = () => resolve({ stdout: '' }))) + ) + .mockResolvedValue({ stdout: 'git@example.com:team/repo.git\n' }) + getSshGitProviderMock.mockReturnValue({ exec: execMock }) + + const stalled = cache.get('/repo', 'origin', 'conn-1') + getSshGitProviderGenerationMock.mockReturnValue(1) + + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toEqual({ repo: 'team/repo' }) + expect(execMock).toHaveBeenCalledTimes(2) + + releaseStalled() + await expect(stalled).resolves.toBeNull() + }) + + it('does not let a probe abandoned as stale overwrite its successor answer', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + const stalled = deferred<{ stdout: string }>() + gitExecFileAsyncMock + .mockImplementationOnce(async () => await stalled.promise) + .mockResolvedValue({ stdout: 'git@example.com:team/repo.git\n' }) + + const abandoned = cache.get('/repo', 'origin') + vi.setSystemTime(1_000_000 + PROBE_COALESCE_STALE_MS + 1) + await expect(cache.get('/repo', 'origin')).resolves.toEqual({ repo: 'team/repo' }) + + // The abandoned probe still answers the caller that started it, but the repo + // state it read is older than the one already cached — it must not publish. + stalled.resolve({ stdout: 'git@elsewhere.example:team/repo.git\n' }) + await expect(abandoned).resolves.toBeNull() + + await expect(cache.get('/repo', 'origin')).resolves.toEqual({ repo: 'team/repo' }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('holds a parsed-as-not-mine remote for the negative interval, then re-asks', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'git@elsewhere.example:team/repo.git\n' }) + + await expect(cache.get('/repo', 'origin')).resolves.toBeNull() + await expect(cache.get('/repo', 'origin')).resolves.toBeNull() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS + 1) + await expect(cache.get('/repo', 'origin')).resolves.toBeNull() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('never caches a miss from a runtime that could not be asked at all', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + getSshGitProviderMock.mockReturnValueOnce(undefined) + + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toBeNull() + + // The provider was gone, not the remote: reconnecting must not need the TTL. + getSshGitProviderMock.mockReturnValue({ + exec: vi.fn(async () => ({ stdout: 'git@example.com:team/repo.git\n' })) + }) + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toEqual({ repo: 'team/repo' }) + }) + + it('holds an SSH repo that has no such remote instead of re-asking every poll', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + const exec = vi.fn(async () => { + throw new Error("fatal: No such remote 'origin'") + }) + getSshGitProviderMock.mockReturnValue({ exec }) + + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toBeNull() + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toBeNull() + expect(exec).toHaveBeenCalledTimes(1) + + vi.setSystemTime(1_000_000 + NEGATIVE_ENTRY_TTL_MS + 1) + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toBeNull() + expect(exec).toHaveBeenCalledTimes(2) + }) + + it('keeps re-asking an SSH repo whose probe died with its transport', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + const exec = vi + .fn() + .mockRejectedValueOnce(new Error('relay request failed: connection closed')) + .mockResolvedValue({ stdout: 'git@example.com:team/repo.git\n' }) + getSshGitProviderMock.mockReturnValue({ exec }) + + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toBeNull() + await expect(cache.get('/repo', 'origin', 'conn-1')).resolves.toEqual({ repo: 'team/repo' }) + expect(exec).toHaveBeenCalledTimes(2) + }) + + it('does not cache a probe killed on its deadline as a definitive miss', async () => { + const cache = createRemoteRefProbeCache(parseExampleRef) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('git timed out.')) + .mockResolvedValueOnce({ stdout: 'git@example.com:team/repo.git\n' }) + + await expect(cache.get('/repo', 'origin')).resolves.toBeNull() + await expect(cache.get('/repo', 'origin')).resolves.toEqual({ repo: 'team/repo' }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/git/remote-ref-probe-cache.ts b/src/main/git/remote-ref-probe-cache.ts index 166e511ede0..3bc2f676fbb 100644 --- a/src/main/git/remote-ref-probe-cache.ts +++ b/src/main/git/remote-ref-probe-cache.ts @@ -1,4 +1,7 @@ +import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' +import { runCoalescedProbe, type CoalescedProbes } from './coalesced-probe' import { isTransientGitProbeError, readRemoteUrl } from './remote-url-probe' +import { isStableMissingGitRemoteError } from './stable-missing-git-remote-error' /** * The "is this repo mine?" probe every forge integration runs: read the remote's @@ -8,6 +11,18 @@ import { isTransientGitProbeError, readRemoteUrl } from './remote-url-probe' const REPO_REF_CACHE_MAX_ENTRIES = 512 +/** + * Why: "not this provider" only holds until someone edits the repo's remotes — + * and a repo first probed before it had any remote answers that way too. + * Nothing here watches `.git/config`, and a watcher cannot cover the SSH and WSL + * runtimes this cache also serves, so negatives expire instead: one probe per + * repo per interval is what lets a remote added mid-session be picked up without + * a restart. Positives stay, as they did before. + */ +export const NEGATIVE_ENTRY_TTL_MS = 5 * 60_000 + +type CachedRepoRef = { value: Ref | null; expiresAt: number } + export type RemoteRefLocalGitOptions = { wslDistro?: string } @@ -26,10 +41,14 @@ export type RemoteRefProbeCache = { export function createRemoteRefProbeCache( parseRemoteUrl: (remoteUrl: string) => Ref | null ): RemoteRefProbeCache { - const repoRefCache = new Map() + const repoRefCache = new Map>() + const inFlight: CoalescedProbes = new Map() function remember(cacheKey: string, value: Ref | null): void { - repoRefCache.set(cacheKey, value) + repoRefCache.set(cacheKey, { + value, + expiresAt: value === null ? Date.now() + NEGATIVE_ENTRY_TTL_MS : Number.POSITIVE_INFINITY + }) while (repoRefCache.size > REPO_REF_CACHE_MAX_ENTRIES) { const oldestKey = repoRefCache.keys().next().value if (oldestKey === undefined) { @@ -39,41 +58,83 @@ export function createRemoteRefProbeCache( } } - return { - async get(repoPath, remoteName, connectionId, localGitOptions = {}) { - const runtimeKey = connectionId ?? `local:${localGitOptions.wslDistro ?? 'host'}` - const cacheKey = `${runtimeKey}\0${repoPath}\0${remoteName}` - if (repoRefCache.has(cacheKey)) { - return repoRefCache.get(cacheKey)! + async function probe( + cacheKey: string, + ownsKey: () => boolean, + repoPath: string, + remoteName: string, + connectionId: string | null | undefined, + localGitOptions: RemoteRefLocalGitOptions + ): Promise { + // Why: a probe abandoned as stale still runs, and its answer describes a repo + // state older than whatever the successor is about to store — or already has. + // It may still answer its own callers; it may not publish. + const publish = (value: Ref | null): void => { + if (ownsKey()) { + remember(cacheKey, value) } - try { - const stdout = await readRemoteUrl( - { - repoPath, - connectionId, - ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) - }, - remoteName - ) - if (stdout === null) { - return null - } - const result = parseRemoteUrl(stdout) - remember(cacheKey, result) - return result - } catch (error) { - if (connectionId || isTransientGitProbeError(error)) { - // Why: SSH provider failures are often transient reconnect/tunnel states, - // and a probe killed on its deadline says nothing about the remote either; - // caching them as "not this provider" would poison the repo for the session. - return null - } - remember(cacheKey, null) + } + try { + const stdout = await readRemoteUrl( + { + repoPath, + connectionId, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) + }, + remoteName + ) + // Why: null is the SSH runtime being disconnected, not an answer about the + // remote — and it costs no `git`, so there is nothing here to spare. It is + // deliberately the one negative with no TTL floor: flooring it would make a + // reconnected host wait the interval out for a probe it could serve now. + if (stdout === null) { return null } + const result = parseRemoteUrl(stdout) + publish(result) + return result + } catch (error) { + // Why: a probe killed on its deadline says nothing about the remote, and an + // SSH failure is usually a reconnect or tunnel state rather than an answer. + // Only "no such remote" is the repo itself saying it is not this provider's + // — anything else cached would poison it, on SSH for the generation's life. + if (isTransientGitProbeError(error)) { + return null + } + if (connectionId && !isStableMissingGitRemoteError(error)) { + return null + } + publish(null) + return null + } + } + + return { + async get(repoPath, remoteName, connectionId, localGitOptions = {}) { + // Why: a reconnect retires the connection an answer came from, and with it + // the probe still running on it — stamping the generation stops a caller on + // the new connection from adopting either. + const runtimeKey = connectionId + ? `${connectionId}:${getSshGitProviderGeneration(connectionId)}` + : `local:${localGitOptions.wslDistro ?? 'host'}` + const cacheKey = `${runtimeKey}\0${repoPath}\0${remoteName}` + const cached = repoRefCache.get(cacheKey) + if (cached) { + if (cached.expiresAt > Date.now()) { + return cached.value + } + repoRefCache.delete(cacheKey) + } + // Why: every branch of a repo resolves its forge through this probe, so a + // poll of the worktree list arrives as a burst of identical lookups. One + // young probe answers all of them instead of spawning a `git` per branch. + return runCoalescedProbe(inFlight, cacheKey, (ownsKey) => + probe(cacheKey, ownsKey, repoPath, remoteName, connectionId, localGitOptions) + ) }, clear() { repoRefCache.clear() + inFlight.clear() }, size() { return repoRefCache.size diff --git a/src/main/git/remote-url-probe.test.ts b/src/main/git/remote-url-probe.test.ts index a2bd0c8f52d..8a7672cb229 100644 --- a/src/main/git/remote-url-probe.test.ts +++ b/src/main/git/remote-url-probe.test.ts @@ -47,6 +47,42 @@ describe('remote URL probe', () => { expect(REMOTE_URL_PROBE_TIMEOUT_MS).toBe(30_000) }) + it('bounds the SSH remote read with the same deadline as the local one', async () => { + const exec = vi.fn( + async (_args: string[], _cwd: string, _options?: { signal?: AbortSignal }) => ({ + stdout: 'git@github.com:acme/orca.git\n' + }) + ) + getSshGitProviderMock.mockReturnValue({ exec }) + + await expect( + readRemoteUrl({ repoPath: '/repo', connectionId: 'ssh-1' }, 'origin') + ).resolves.toContain('github.com') + + const [args, cwd, options] = exec.mock.calls[0] + expect(args).toEqual(['remote', 'get-url', 'origin']) + expect(cwd).toBe('/repo') + // The relay bounds each phase separately and resets on every frame, so only a + // signal spans the whole round trip. + expect(options?.signal).toBeInstanceOf(AbortSignal) + expect(options?.signal?.aborted).toBe(false) + }) + + it('reports a probe cut off by its deadline as unavailable, not as an answer', async () => { + const aborted = new Error('Request was cancelled') + aborted.name = 'AbortError' + getSshGitProviderMock.mockReturnValue({ + exec: vi.fn(async () => { + throw aborted + }) + }) + + expect(isTransientGitProbeError(aborted)).toBe(true) + await expect( + assertRemoteUrlReadable({ repoPath: '/repo', connectionId: 'ssh-1' }) + ).rejects.toBe(aborted) + }) + it('rethrows a timed-out local probe so callers can report unavailable', async () => { const timeout = new Error('git timed out.') gitExecFileAsyncMock.mockRejectedValue(timeout) diff --git a/src/main/git/remote-url-probe.ts b/src/main/git/remote-url-probe.ts index 4615e74e4b0..7b18c1deac9 100644 --- a/src/main/git/remote-url-probe.ts +++ b/src/main/git/remote-url-probe.ts @@ -12,8 +12,9 @@ import { gitExecFileAsync } from './runner' * wedged host — a dead network mount or stalled WSL interop. Unbounded, the call * never returns and every caller above it hangs with it. Passing a timeout is * also what arms the runner's kill path; Node's own waits forever on a child - * that ignores signals. The SSH branch is bounded by the relay mux's own 30s - * request timeout. + * that ignores signals. The SSH branch spends the same budget as one deadline + * over the whole round trip: the relay's own bounds are per-phase and restart on + * every frame, so a relay dribbling output outlives them. */ export const REMOTE_URL_PROBE_TIMEOUT_MS = 30_000 @@ -33,7 +34,9 @@ export async function readRemoteUrl( if (!provider) { return null } - const { stdout } = await provider.exec(['remote', 'get-url', remoteName], context.repoPath) + const { stdout } = await provider.exec(['remote', 'get-url', remoteName], context.repoPath, { + signal: AbortSignal.timeout(REMOTE_URL_PROBE_TIMEOUT_MS) + }) return stdout } const { stdout } = await gitExecFileAsync(['remote', 'get-url', remoteName], { @@ -58,6 +61,15 @@ const TRANSIENT_PROBE_PATTERNS = [ * report it as "no review": it is an unavailable result, not a negative one. */ export function isTransientGitProbeError(error: unknown): boolean { + // Why: an abort — this probe's deadline, or a caller cancelling — carries no + // message a pattern could match, but it is the emptiest answer of all. + if ( + typeof error === 'object' && + error !== null && + (error as { name?: unknown }).name === 'AbortError' + ) { + return true + } const parts: string[] = [] if (error instanceof Error) { parts.push(error.message) diff --git a/src/main/github/stable-missing-git-remote-error.ts b/src/main/git/stable-missing-git-remote-error.ts similarity index 100% rename from src/main/github/stable-missing-git-remote-error.ts rename to src/main/git/stable-missing-git-remote-error.ts diff --git a/src/main/gitea/pull-request-creation.test.ts b/src/main/gitea/pull-request-creation.test.ts index 9680726c302..a8dad194408 100644 --- a/src/main/gitea/pull-request-creation.test.ts +++ b/src/main/gitea/pull-request-creation.test.ts @@ -12,7 +12,8 @@ vi.mock('../git/runner', () => ({ })) vi.mock('../providers/ssh-git-dispatch', () => ({ - getSshGitProvider: getSshGitProviderMock + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: () => 0 })) vi.mock('../source-control/pull-request-template', () => ({ @@ -131,7 +132,9 @@ describe('Gitea pull request creation', () => { ok: true, number: 14 }) - expect(remoteGit.exec).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], '/remote/repo') + expect(remoteGit.exec).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], '/remote/repo', { + signal: expect.any(AbortSignal) + }) expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) diff --git a/src/main/gitea/repository-ref.test.ts b/src/main/gitea/repository-ref.test.ts index 7169bd69b7c..678dd5c6912 100644 --- a/src/main/gitea/repository-ref.test.ts +++ b/src/main/gitea/repository-ref.test.ts @@ -189,7 +189,9 @@ describe('Gitea repository ref parsing', () => { repo: 'project' }) - expect(sshExecMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], '/repo') + expect(sshExecMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], '/repo', { + signal: expect.any(AbortSignal) + }) expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) diff --git a/src/main/github/gh-utils.test.ts b/src/main/github/gh-utils.test.ts index 9c4461e5e18..0d7329e21e3 100644 --- a/src/main/github/gh-utils.test.ts +++ b/src/main/github/gh-utils.test.ts @@ -209,7 +209,10 @@ describe('github owner/repo resolution', () => { expect(getSshGitProviderMock).toHaveBeenCalledWith('openclaw-2') expect(sshProvider.exec).toHaveBeenCalledWith( ['remote', 'get-url', 'origin'], - '/home/user/orca' + '/home/user/orca', + { + signal: expect.any(AbortSignal) + } ) }) diff --git a/src/main/github/github-repository-identity.ts b/src/main/github/github-repository-identity.ts index 0490b19c5aa..e348fbc6459 100644 --- a/src/main/github/github-repository-identity.ts +++ b/src/main/github/github-repository-identity.ts @@ -9,7 +9,7 @@ import { type GitHubRemoteIdentity } from './github-remote-identity-parsing' import { classifyGitHubOwnerRepoFromRemoteUrl } from './github-ssh-host-alias-resolution' -import { isStableMissingGitRemoteError } from './stable-missing-git-remote-error' +import { isStableMissingGitRemoteError } from '../git/stable-missing-git-remote-error' export type OwnerRepo = GitHubOwnerRepo diff --git a/src/main/gitlab/gl-utils.test.ts b/src/main/gitlab/gl-utils.test.ts index 1e1e3d0cca4..23e159671c6 100644 --- a/src/main/gitlab/gl-utils.test.ts +++ b/src/main/gitlab/gl-utils.test.ts @@ -165,7 +165,9 @@ describe('gitlab project ref resolution', () => { path: 'remote/orca' }) - expect(sshExecMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], '/repo') + expect(sshExecMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], '/repo', { + signal: expect.any(AbortSignal) + }) expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) diff --git a/src/main/source-control/hosted-review-branch-cache.test.ts b/src/main/source-control/hosted-review-branch-cache.test.ts index 17c69038d58..a9b509873c3 100644 --- a/src/main/source-control/hosted-review-branch-cache.test.ts +++ b/src/main/source-control/hosted-review-branch-cache.test.ts @@ -9,7 +9,9 @@ import { HOSTED_REVIEW_LOOKUP_DEADLINE_MS, LOOKUP_BACKOFF_MAX_MS, MAX_BRANCH_MAP_ENTRIES, + MAX_DETACHED_LOOKUPS, MAX_INFLIGHT_LOOKUPS, + MAX_UNSETTLED_LOOKUP_KEYS, MAX_UNSETTLED_LOOKUPS_PER_KEY } from './hosted-review-refresh-pacing' @@ -788,12 +790,16 @@ describe('hosted review branch cache (#11532)', () => { } const filler = stuckLookup() const wedged = stuckLookup() - /** Drops the branch's in-flight record without expiring it, so it runs on untracked. */ - const evictInflightRecords = (round: number): void => { + /** + * Drops the branch's in-flight record without expiring it, so it runs on + * untracked. Reuses one set of filler branches per round: fresh keys every + * round would spend the unsettled-map bound instead of the in-flight cap. + */ + const evictInflightRecords = (): void => { for (let index = 0; index < MAX_INFLIGHT_LOOKUPS; index += 1) { swallow( withHostedReviewBranchCache( - { ...identity, branch: `filler/${round}/${index}` }, + { ...identity, branch: `filler/${index}` }, { headOid: null }, filler.lookup ) @@ -803,7 +809,7 @@ describe('hosted review branch cache (#11532)', () => { for (let attempt = 0; attempt < MAX_UNSETTLED_LOOKUPS_PER_KEY; attempt += 1) { swallow(withHostedReviewBranchCache(identity, { headOid: null }, wedged.lookup)) - evictInflightRecords(attempt) + evictInflightRecords() } expect(wedged.lookup).toHaveBeenCalledTimes(MAX_UNSETTLED_LOOKUPS_PER_KEY) @@ -816,6 +822,71 @@ describe('hosted review branch cache (#11532)', () => { expect(wedged.lookup).toHaveBeenCalledTimes(MAX_UNSETTLED_LOOKUPS_PER_KEY) }) + it('stops admitting new branches once the unsettled map is full', async () => { + const swallow = (promise: Promise): void => { + void promise.catch(() => {}) + } + const filler = stuckLookup() + for (let index = 0; index < MAX_UNSETTLED_LOOKUP_KEYS; index += 1) { + swallow( + withHostedReviewBranchCache( + { ...identity, branch: `filler/${index}` }, + { headOid: null }, + filler.lookup + ) + ) + } + expect(filler.lookup).toHaveBeenCalledTimes(MAX_UNSETTLED_LOOKUP_KEYS) + + // Nothing has reached its deadline, so only the map bound can hold this + // back — without it the wave keeps widening until the detached cap does. + // This branch never started a lookup, so it must not be told one of its own + // is still out there. + const fresh = vi.fn(async () => openReview) + const refusal = await withHostedReviewBranchCache( + { ...identity, branch: 'fresh' }, + { headOid: null }, + fresh + ).catch((error: unknown) => (error as Error).message) + expect(refusal).toMatch(/Too many hosted review lookups are already in progress/) + expect(refusal).not.toMatch(/never answered/) + expect(fresh).not.toHaveBeenCalled() + + // A branch already counted keeps its second attempt: the bound is on new + // keys, not on the retry that proves a host recovered. + const retry = stuckLookup() + swallow( + withHostedReviewBranchCache( + { ...identity, branch: 'filler/0' }, + { headOid: null }, + retry.lookup + ) + ) + expect(retry.lookup).toHaveBeenCalledTimes(1) + }) + + it('names the process-wide cap when abandoned lookups have filled it', async () => { + const wedged = stuckLookup() + for (let index = 0; index < MAX_DETACHED_LOOKUPS; index += 1) { + void withHostedReviewBranchCache( + { ...identity, branch: `wedged/${index}` }, + { headOid: null }, + wedged.lookup + ).catch(() => {}) + } + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + + // The host wedged every branch on it, not this one in particular. + const fresh = vi.fn(async () => openReview) + const refusal = await withHostedReviewBranchCache( + { ...identity, branch: 'fresh' }, + { headOid: null }, + fresh + ).catch((error: unknown) => (error as Error).message) + expect(refusal).toMatch(/abandoned without answering/) + expect(fresh).not.toHaveBeenCalled() + }) + it('does not adopt a straggler whose invalidated scope was evicted', async () => { const stale = stuckLookup() const inflight = withHostedReviewBranchCache(identity, { headOid: null }, stale.lookup) diff --git a/src/main/source-control/hosted-review-branch-cache.ts b/src/main/source-control/hosted-review-branch-cache.ts index b99943bda0e..6d0338a1a5c 100644 --- a/src/main/source-control/hosted-review-branch-cache.ts +++ b/src/main/source-control/hosted-review-branch-cache.ts @@ -6,7 +6,7 @@ import { } from './hosted-review-active-branch-claims' import { __resetUnsettledHostedReviewLookupsForTests, - hasLookupCapacity, + lookupCapacityRefusal, noteDetachedLookup, noteLookupStarted, settleDetachedLookup, @@ -251,10 +251,7 @@ function lookupUnavailableReason(key: string): string | null { until ).toLocaleTimeString()}.` } - if (!hasLookupCapacity(key)) { - return 'Hosted review lookup is still running from an earlier attempt that never answered. It will be retried once that attempt settles.' - } - return null + return lookupCapacityRefusal(key) } /** diff --git a/src/main/source-control/hosted-review-refresh-pacing.ts b/src/main/source-control/hosted-review-refresh-pacing.ts index ed36463afe5..991478f29ab 100644 --- a/src/main/source-control/hosted-review-refresh-pacing.ts +++ b/src/main/source-control/hosted-review-refresh-pacing.ts @@ -60,6 +60,14 @@ export const MAX_INFLIGHT_LOOKUPS = MAX_BRANCH_MAP_ENTRIES */ export const MAX_UNSETTLED_LOOKUPS_PER_KEY = 2 +/** + * How many branches may hold unsettled lookups at once. Deliberately above the + * in-flight cap: that one evicts, which costs memory but never an answer, so it + * has to be what fires at realistic fan-out. Refusing a branch outright is for a + * wave no plausible worktree list explains, and only bounds the map holding it. + */ +export const MAX_UNSETTLED_LOOKUP_KEYS = 2 * MAX_BRANCH_MAP_ENTRIES + /** Process-wide backstop for the same leak when many branches wedge at once. */ export const MAX_DETACHED_LOOKUPS = 64 diff --git a/src/main/source-control/hosted-review-unsettled-lookups.ts b/src/main/source-control/hosted-review-unsettled-lookups.ts index 96ed0eddfe0..fd8f1aabdda 100644 --- a/src/main/source-control/hosted-review-unsettled-lookups.ts +++ b/src/main/source-control/hosted-review-unsettled-lookups.ts @@ -1,4 +1,8 @@ -import { MAX_DETACHED_LOOKUPS, MAX_UNSETTLED_LOOKUPS_PER_KEY } from './hosted-review-refresh-pacing' +import { + MAX_DETACHED_LOOKUPS, + MAX_UNSETTLED_LOOKUP_KEYS, + MAX_UNSETTLED_LOOKUPS_PER_KEY +} from './hosted-review-refresh-pacing' /** * Accounting for lookups that have not settled (P1-D). @@ -48,12 +52,28 @@ export function settleDetachedLookup(): void { detachedTotal = Math.max(0, detachedTotal - 1) } -/** False once this branch — or the process — is holding too many unsettled lookups. */ -export function hasLookupCapacity(key: string): boolean { - return ( - (unsettledByKey.get(key) ?? 0) < MAX_UNSETTLED_LOOKUPS_PER_KEY && - detachedTotal < MAX_DETACHED_LOOKUPS - ) +/** + * Why this branch cannot start a lookup, or null when it can. Only the per-branch + * cap may blame an attempt of its own: the other two hold back branches that have + * never asked for anything, and telling those users to wait on a lookup they + * never started sends them looking for a stall that is not theirs. + */ +export function lookupCapacityRefusal(key: string): string | null { + if ((unsettledByKey.get(key) ?? 0) >= MAX_UNSETTLED_LOOKUPS_PER_KEY) { + return 'Hosted review lookup is still running from an earlier attempt that never answered. It will be retried once that attempt settles.' + } + // Why: this map is bounded by refusing new branches, not by evicting old ones — + // its entries are lookups still out there, and dropping a key would re-admit + // one for a branch already wedged. The bound sits above the in-flight cap so a + // wave wide enough to reach it is pathological, not the ordinary fan-out of a + // client polling a long worktree list. + if (!unsettledByKey.has(key) && unsettledByKey.size >= MAX_UNSETTLED_LOOKUP_KEYS) { + return 'Too many hosted review lookups are already in progress to start another. This branch will be retried once some of them settle.' + } + if (detachedTotal >= MAX_DETACHED_LOOKUPS) { + return 'Too many hosted review lookups have been abandoned without answering. This branch will be retried once the host catches up.' + } + return null } /** @internal - exposed for tests only */