diff --git a/src/main/github/github-repository-identity.signed-cache.test.ts b/src/main/github/github-repository-identity.signed-cache.test.ts new file mode 100644 index 00000000000..c07e4886afc --- /dev/null +++ b/src/main/github/github-repository-identity.signed-cache.test.ts @@ -0,0 +1,114 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GitRunner from '../git/runner' + +// A resolved repo identity used to expire on a flat 30s clock, so a client +// polling PR state re-ran `git remote get-url` for every repo every half +// minute. On Windows that spawn costs 250-800ms in the field. The identity is +// a read of `.git/config`, and the config signature already exists to say when +// that file changed — so a signed answer is now held for the same five minutes +// a signed negative already was, and revalidated against the signature. + +const { gitExecFileAsyncMock, readLocalGitConfigSignatureMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + readLocalGitConfigSignatureMock: vi.fn<() => Promise>(async () => 'sig-1') +})) + +vi.mock('../git/runner', async (importOriginal) => ({ + ...(await importOriginal()), + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: () => null, + getSshGitProviderGeneration: () => 0, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE: 'ssh git provider unavailable' +})) + +vi.mock('./local-git-config-signature', () => ({ + readLocalGitConfigSignature: readLocalGitConfigSignatureMock +})) + +import { getOwnerRepoForRemote, _resetOwnerRepoCache } from './github-repository-identity' + +const REPO = '/tmp/signed-cache-repo' +const THIRTY_SECONDS = 30_000 +const FOUR_MINUTES = 4 * 60_000 + +let remoteUrl = 'https://github.com/stablyai/orca.git' + +const remoteGetUrlCalls = (): number => + gitExecFileAsyncMock.mock.calls.filter(([args]) => (args as string[])[1] === 'get-url').length + +beforeEach(() => { + _resetOwnerRepoCache() + vi.useRealTimers() + gitExecFileAsyncMock.mockReset() + remoteUrl = 'https://github.com/stablyai/orca.git' + readLocalGitConfigSignatureMock.mockReset() + readLocalGitConfigSignatureMock.mockImplementation(async () => 'sig-1') + gitExecFileAsyncMock.mockImplementation(async () => ({ stdout: remoteUrl })) +}) + +describe('owner/repo identity cache', () => { + it('holds a signed identity past the unsigned TTL instead of re-spawning git', async () => { + vi.useFakeTimers() + await expect(getOwnerRepoForRemote(REPO, 'origin')).resolves.toEqual({ + owner: 'stablyai', + repo: 'orca' + }) + expect(remoteGetUrlCalls()).toBe(1) + + // Why 4 minutes: past the 30s unsigned TTL that forced the re-probe, and + // still inside the signed window this change introduces. + vi.setSystemTime(Date.now() + FOUR_MINUTES) + await expect(getOwnerRepoForRemote(REPO, 'origin')).resolves.toEqual({ + owner: 'stablyai', + repo: 'orca' + }) + expect(remoteGetUrlCalls()).toBe(1) + vi.useRealTimers() + }) + + it('re-probes as soon as the git config signature changes', async () => { + vi.useFakeTimers() + await getOwnerRepoForRemote(REPO, 'origin') + expect(remoteGetUrlCalls()).toBe(1) + + remoteUrl = 'https://github.com/other-org/orca.git' + readLocalGitConfigSignatureMock.mockImplementation(async () => 'sig-2') + vi.setSystemTime(Date.now() + THIRTY_SECONDS) + + // Why not "after the TTL": a `git remote set-url` must be visible on the + // very next lookup, which is what the longer hold is allowed to rely on. + await expect(getOwnerRepoForRemote(REPO, 'origin')).resolves.toEqual({ + owner: 'other-org', + repo: 'orca' + }) + expect(remoteGetUrlCalls()).toBe(2) + vi.useRealTimers() + }) + + it('keeps the short TTL when no signature can be read', async () => { + vi.useFakeTimers() + readLocalGitConfigSignatureMock.mockImplementation(async () => undefined) + await getOwnerRepoForRemote(REPO, 'origin') + expect(remoteGetUrlCalls()).toBe(1) + + // Why: with no signature nothing invalidates on change, so the entry must + // still expire on the clock rather than silently pinning a stale identity. + vi.setSystemTime(Date.now() + THIRTY_SECONDS + 1) + await getOwnerRepoForRemote(REPO, 'origin') + expect(remoteGetUrlCalls()).toBe(2) + vi.useRealTimers() + }) + + it('still coalesces concurrent lookups onto one probe', async () => { + const [first, second] = await Promise.all([ + getOwnerRepoForRemote(REPO, 'origin'), + getOwnerRepoForRemote(REPO, 'origin') + ]) + expect(first).toEqual({ owner: 'stablyai', repo: 'orca' }) + expect(second).toEqual(first) + expect(remoteGetUrlCalls()).toBe(1) + }) +}) diff --git a/src/main/github/github-repository-identity.ts b/src/main/github/github-repository-identity.ts index c912f424417..6a570f86635 100644 --- a/src/main/github/github-repository-identity.ts +++ b/src/main/github/github-repository-identity.ts @@ -61,6 +61,14 @@ export function ghRepoExecOptions(context: GitHubRepoContext): { const OWNER_REPO_POSITIVE_CACHE_TTL_MS = 30_000 const OWNER_REPO_NEGATIVE_CACHE_TTL_MS = 5 * 60_000 +/** + * A signature-backed answer is held as long as a negative one. `git remote + * get-url` reads `.git/config`, and the signature covers that file plus every + * path it includes — so while the signature holds, a re-probe can only return + * what is already cached. The short TTL above is the no-signature fallback + * (remote runtimes, an unreadable gitdir), where nothing invalidates on change. + */ +const OWNER_REPO_SIGNED_CACHE_TTL_MS = 5 * 60_000 const OWNER_REPO_CACHE_MAX_ENTRIES = 512 type OwnerRepoCacheEntry = { @@ -106,10 +114,10 @@ export async function getRemoteUrlForRepo( } function getOwnerRepoCacheTtl(value: OwnerRepo | null, configSignature?: string): number { - if (value) { - return OWNER_REPO_POSITIVE_CACHE_TTL_MS + if (configSignature) { + return value ? OWNER_REPO_SIGNED_CACHE_TTL_MS : OWNER_REPO_NEGATIVE_CACHE_TTL_MS } - return configSignature ? OWNER_REPO_NEGATIVE_CACHE_TTL_MS : OWNER_REPO_POSITIVE_CACHE_TTL_MS + return OWNER_REPO_POSITIVE_CACHE_TTL_MS } export async function getOwnerRepoForRemote( @@ -135,7 +143,11 @@ export async function getOwnerRepoForRemote( pruneOwnerRepoCache(now) const cached = ownerRepoCache.get(cacheKey) if (cached && cached.expiresAt > now) { - if (cached.value === null && cached.configSignature !== undefined) { + // Why every signed entry, not only the negatives: a positive identity is + // held for the same five minutes now, so the same revalidation is what + // keeps a `git remote set-url` visible within one lookup rather than five + // minutes. The signature read is fs stat/readFile, not a Git subprocess. + if (cached.configSignature !== undefined) { const currentSignature = await readLocalGitConfigSignature(context) if (currentSignature !== cached.configSignature) { ownerRepoCache.delete(cacheKey) @@ -203,9 +215,14 @@ async function resolveOwnerRepoForRemote( // Why: PR mutations need the effective host behind an SSH alias. const classification = await classifyGitHubOwnerRepoFromRemoteUrl(remoteUrl, context) if (classification.kind === 'github') { + // Why store the signature: without it this entry can only expire on the + // clock, which put a `git remote get-url` (plus its ssh-alias probe) on + // every PR refresh cycle — the second most frequent Git subprocess in a + // Windows field trace, on a host spawning Git at ~250-800ms a call. ownerRepoCache.set(cacheKey, { value: classification.ownerRepo, - expiresAt: now + getOwnerRepoCacheTtl(classification.ownerRepo, configSignature) + expiresAt: now + getOwnerRepoCacheTtl(classification.ownerRepo, configSignature), + ...(configSignature ? { configSignature } : {}) }) pruneOwnerRepoCache(now) return classification.ownerRepo