fix(P1-D): coalesce remote-ref probes, TTL negatives, and bound unsettled keys (#12065)
* fix(P1-D): coalesce remote-ref probes, TTL negatives, and bound unsettled keys Keep forge resolution from stampeding git under worktree fan-out, let remotes added mid-session be discovered without a restart, and refuse pathological new-branch waves once the unsettled map is full. * fix(P1-D): stop abandoned probes publishing, and split capacity refusals A coalesced probe abandoned as stale kept running and still wrote its answer to the cache, so a late permanent miss could land over the successor's fresher one. Probes now publish only while they still own the in-flight key. The hosted-review capacity refusal told brand-new branches that an earlier attempt of their own never answered when the refusal was really the unsettled map or the process-wide detached cap; each cap now says what it is. Also caches stable "no such remote" SSH misses under the negative TTL instead of re-spawning the probe on every poll. Co-authored-by: Orca <help@stably.ai> * Bound SSH remote URL probe with deadline to prevent hangs The SSH branch of remote URL probes was unbounded — the relay's bounds are per-phase and reset on every frame, so a relay dribbling output would outlive them. Pass AbortSignal.timeout to the SSH provider's exec call to enforce the same 30s deadline as local probes. Treat AbortError as a transient probe error: it signals unavailable infrastructure (deadline or cancellation), not a negative answer about the remote. --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
parent
711491b40a
commit
1562f12f78
|
|
@ -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', () => ({
|
||||
|
|
|
|||
|
|
@ -8,16 +8,21 @@
|
|||
* progress is what lets a host recover in-session.
|
||||
*/
|
||||
|
||||
export type CoalescedProbe<T> = { startedAt: number; promise: Promise<T> }
|
||||
export type CoalescedProbe<T> = { startedAt: number; token: object; promise: Promise<T> }
|
||||
export type CoalescedProbes<T> = Map<string, CoalescedProbe<T>>
|
||||
|
||||
/** 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<T>(
|
||||
probes: CoalescedProbes<T>,
|
||||
key: string,
|
||||
createProbe: () => Promise<T>,
|
||||
createProbe: (ownsKey: () => boolean) => Promise<T>,
|
||||
staleAfterMs: number = PROBE_COALESCE_STALE_MS
|
||||
): Promise<T> {
|
||||
const now = Date.now()
|
||||
|
|
@ -29,7 +34,12 @@ export async function runCoalescedProbe<T>(
|
|||
// The abandoned probe keeps running; nothing here will await it again.
|
||||
void existing.promise.catch(() => {})
|
||||
}
|
||||
const entry: CoalescedProbe<T> = { startedAt: now, promise: createProbe() }
|
||||
const token = {}
|
||||
const entry: CoalescedProbe<T> = {
|
||||
startedAt: now,
|
||||
token,
|
||||
promise: createProbe(() => probes.get(key)?.token === token)
|
||||
}
|
||||
probes.set(key, entry)
|
||||
try {
|
||||
return await entry.promise
|
||||
|
|
|
|||
|
|
@ -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<T>(): { promise: Promise<T>; resolve: (value: T) => void } {
|
||||
let resolve: (value: T) => void = () => {}
|
||||
const promise = new Promise<T>((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)
|
||||
})
|
||||
})
|
||||
|
|
@ -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<Ref> = { value: Ref | null; expiresAt: number }
|
||||
|
||||
export type RemoteRefLocalGitOptions = {
|
||||
wslDistro?: string
|
||||
}
|
||||
|
|
@ -26,10 +41,14 @@ export type RemoteRefProbeCache<Ref> = {
|
|||
export function createRemoteRefProbeCache<Ref>(
|
||||
parseRemoteUrl: (remoteUrl: string) => Ref | null
|
||||
): RemoteRefProbeCache<Ref> {
|
||||
const repoRefCache = new Map<string, Ref | null>()
|
||||
const repoRefCache = new Map<string, CachedRepoRef<Ref>>()
|
||||
const inFlight: CoalescedProbes<Ref | null> = 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<Ref>(
|
|||
}
|
||||
}
|
||||
|
||||
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<Ref | null> {
|
||||
// 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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
)
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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<unknown>): 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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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 */
|
||||
|
|
|
|||
Loading…
Reference in New Issue