diff --git a/src/main/ipc/pty-restore-record-seeding.test.ts b/src/main/ipc/pty-restore-record-seeding.test.ts index 0434be17e67..0f1646e7812 100644 --- a/src/main/ipc/pty-restore-record-seeding.test.ts +++ b/src/main/ipc/pty-restore-record-seeding.test.ts @@ -12,6 +12,9 @@ import { setupPtyIpcSuite } from './pty-ipc-test-harness' import { getDefaultWorkspaceSession } from '../../shared/constants' import { makePaneKey } from '../../shared/stable-pane-id' import { OrcaRuntimeService } from '../runtime/orca-runtime' +import type { RuntimeResolvedWorktreeCache } from '../runtime/runtime-resolved-worktree-cache' +import type { ResolvedWorktree } from '../runtime/runtime-worktree-path-identity' +import { getWorktreeScanMutationRevision } from '../local-worktree-scan-generation' import { registerPtyHandlers, clearProviderPtyState, @@ -359,15 +362,22 @@ describe('registerPtyHandlers', () => { } as never) // Why: selector resolution shells out to git for real repos; prime the // resolved-worktree cache so this headless fixture resolves offline. + // + // Why through getSnapshot and not a hand-written `resolved` entry: the cache decides freshness + // from fields it stamps itself, so a literal that mirrors them is a second copy of that + // contract and goes stale the moment a field is added. Let the cache stamp its own entry. const worktreeResolutionInternals = runtime as unknown as { - buildResolvedWorktreeFromId(id: string): unknown - resolvedWorktrees: object + buildResolvedWorktreeFromId(id: string): ResolvedWorktree + resolvedWorktrees: RuntimeResolvedWorktreeCache } - Reflect.set(worktreeResolutionInternals.resolvedWorktrees, 'resolved', { - worktrees: [worktreeResolutionInternals.buildResolvedWorktreeFromId(worktreeId)], - platformByRepoId: new Map([[repo.id, process.platform]]), - expiresAt: Date.now() + 60_000 - }) + await worktreeResolutionInternals.resolvedWorktrees.getSnapshot( + async () => ({ + worktrees: [worktreeResolutionInternals.buildResolvedWorktreeFromId(worktreeId)], + platformByRepoId: new Map([[repo.id, process.platform]]) + }), + 60_000, + getWorktreeScanMutationRevision() + ) setLocalPtyProvider({ spawn: vi.fn(async () => ({ id: ptyId, diff --git a/src/main/local-worktree-scan-generation.ts b/src/main/local-worktree-scan-generation.ts index c8cc3bd0ff9..a2c86afcc33 100644 --- a/src/main/local-worktree-scan-generation.ts +++ b/src/main/local-worktree-scan-generation.ts @@ -1,5 +1,6 @@ const generationByRepoId = new Map() let generationSequence = 0 +let mutationRevision = 0 export function getLocalWorktreeScanGeneration(repoId: string): number { const existing = generationByRepoId.get(repoId) @@ -13,6 +14,22 @@ export function getLocalWorktreeScanGeneration(repoId: string): number { export function bumpLocalWorktreeScanGeneration(repoId: string): void { generationByRepoId.set(repoId, ++generationSequence) + mutationRevision += 1 +} + +/** + * Advances on every event above that can change what a worktree scan would find — repo add, + * removal, update, and scan-cache invalidation — and on nothing else. A cache that must not answer + * for repos it never saw compares this in O(1) instead of walking the repo list. + * + * Why not `generationSequence`: that also advances when `getLocalWorktreeScanGeneration` mints a key + * for a repo id nothing has scanned yet, which is a read. Keying a snapshot on it would let a read + * path discard a snapshot that is still perfectly valid. + * + * Ordering-only: the value means nothing outside a same-process comparison. + */ +export function getWorktreeScanMutationRevision(): number { + return mutationRevision } export function isLocalWorktreeScanGenerationCurrent(repoId: string, generation: number): boolean { @@ -21,5 +38,6 @@ export function isLocalWorktreeScanGenerationCurrent(repoId: string, generation: export function resetLocalWorktreeScanGenerationsForTests(): void { generationSequence += 1 + mutationRevision += 1 generationByRepoId.clear() } diff --git a/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts b/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts index 1f0dbe591a3..152ef547889 100644 --- a/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts +++ b/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts @@ -5,6 +5,7 @@ import { splitWorktreeIdForFilesystem } from '../../shared/worktree/id' import { isPathInsideOrEqual } from '../../shared/cross-platform-path' import type { ResolvedWorktreeSnapshot } from './runtime-resolved-worktree-cache' import { RESOLVED_WORKTREE_CACHE_TTL_MS } from './orca-runtime-postlude' +import { getWorktreeScanMutationRevision } from '../local-worktree-scan-generation' import { resolveLocalProjectRuntimeForRepo, resolveLocalProjectRuntimesForRepos @@ -65,8 +66,7 @@ export class OrcaRuntimeWithListKnownResolvedWorktreesForExplicitTarget extends /** A warm fleet snapshot already answers any selector for free, so scoped scanning must yield to it. */ protected hasFreshResolvedWorktreeCache(): boolean { - const cached = this.resolvedWorktrees.peek() - return Boolean(cached && cached.expiresAt > Date.now()) + return this.resolvedWorktrees.isFresh(getWorktreeScanMutationRevision()) } protected async listResolvedWorktrees(): Promise { @@ -79,7 +79,8 @@ export class OrcaRuntimeWithListKnownResolvedWorktreesForExplicitTarget extends } return this.resolvedWorktrees.getSnapshot( () => this.computeResolvedWorktrees(), - RESOLVED_WORKTREE_CACHE_TTL_MS + RESOLVED_WORKTREE_CACHE_TTL_MS, + getWorktreeScanMutationRevision() ) } diff --git a/src/main/runtime/orca-runtime-tests/worktree-scan-cache-ttl.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-scan-cache-ttl.spec.ts index 8dc8d7db3a2..f8ec37f7c2c 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-scan-cache-ttl.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-scan-cache-ttl.spec.ts @@ -5,6 +5,7 @@ import { resolveWorktreeScanCacheTtlMs } from '../orca-runtime-test-mocks.spec' import { store } from '../orca-runtime-test-fixtures.spec' +import { bumpLocalWorktreeScanGeneration } from '../../local-worktree-scan-generation' describe('resolveWorktreeScanCacheTtlMs', () => { const BASE_TTL_MS = 30_000 @@ -84,4 +85,34 @@ describe('resolveWorktreeScanCacheTtlMs', () => { vi.useRealTimers() } }) + + it('scans a repo registered after the last snapshot instead of answering from it', async () => { + // Why: the fleet snapshot only covers the repos that existed when it ran, so for a full TTL it + // reported a just-connected SSH host as having no worktrees at all — and callers that resolve a + // workspace through it turned that gap into `skill-install-workspace-not-found`. + vi.mocked(listWorktrees).mockClear() + const addedPath = '/tmp/repo-registered-later' + const repos = [ + { id: 'repo-1', path: '/tmp/repo', displayName: 'repo', badgeColor: 'blue', addedAt: 1 } + ] + const runtime = new OrcaRuntimeService({ ...store, getRepos: () => repos } as never) + const internals = runtime as unknown as { listResolvedWorktrees: () => Promise } + const scanCallsFor = (path: string): number => + vi.mocked(listWorktrees).mock.calls.filter((call) => call[0] === path).length + + await internals.listResolvedWorktrees() + expect(scanCallsFor(addedPath)).toBe(0) + + repos.push({ + id: 'repo-added', + path: addedPath, + displayName: 'added', + badgeColor: 'blue', + addedAt: 2 + }) + bumpLocalWorktreeScanGeneration('repo-added') + + await internals.listResolvedWorktrees() + expect(scanCallsFor(addedPath)).toBe(1) + }) }) diff --git a/src/main/runtime/runtime-resolved-worktree-cache.test.ts b/src/main/runtime/runtime-resolved-worktree-cache.test.ts new file mode 100644 index 00000000000..31cc4fa2b60 --- /dev/null +++ b/src/main/runtime/runtime-resolved-worktree-cache.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from 'vitest' +import { RuntimeResolvedWorktreeCache } from './runtime-resolved-worktree-cache' +import type { ResolvedWorktreeSnapshot } from './runtime-resolved-worktree-cache' +import { + bumpLocalWorktreeScanGeneration, + getLocalWorktreeScanGeneration, + getWorktreeScanMutationRevision +} from '../local-worktree-scan-generation' + +function snapshotOf(ids: string[]): ResolvedWorktreeSnapshot { + return { + worktrees: ids.map((id) => ({ id }) as ResolvedWorktreeSnapshot['worktrees'][number]), + platformByRepoId: new Map() + } +} + +describe('RuntimeResolvedWorktreeCache', () => { + it('reuses a snapshot inside the TTL while the repo inventory is unchanged', async () => { + const cache = new RuntimeResolvedWorktreeCache() + let computes = 0 + const compute = async (): Promise => { + computes += 1 + return snapshotOf(['repo-1::/a']) + } + + await cache.getSnapshot(compute, 60_000, 7) + const second = await cache.getSnapshot(compute, 60_000, 7) + + expect(computes).toBe(1) + expect(second.worktrees.map((worktree) => worktree.id)).toEqual(['repo-1::/a']) + }) + + it('recomputes when the repo inventory moved, even well inside the TTL', async () => { + // Why: this is the whole point. A snapshot taken before a repo was registered cannot testify + // that the repo's worktrees are absent — callers read the gap as "workspace not found". + const cache = new RuntimeResolvedWorktreeCache() + const results = [snapshotOf(['repo-1::/a']), snapshotOf(['repo-1::/a', 'repo-2::/b'])] + let computes = 0 + const compute = async (): Promise => results[computes++] + + await cache.getSnapshot(compute, 60_000, 7) + const afterRegistration = await cache.getSnapshot(compute, 60_000, 8) + + expect(computes).toBe(2) + expect(afterRegistration.worktrees.map((worktree) => worktree.id)).toEqual([ + 'repo-1::/a', + 'repo-2::/b' + ]) + }) + + it('does not join an in-flight compute that started under a stale inventory', async () => { + const cache = new RuntimeResolvedWorktreeCache() + const computed: number[] = [] + const compute = async (): Promise => { + computed.push(computed.length) + return snapshotOf([]) + } + + const first = cache.getSnapshot(compute, 60_000, 7) + const second = cache.getSnapshot(compute, 60_000, 8) + await Promise.all([first, second]) + + expect(computed).toHaveLength(2) + }) + + it('reports freshness against the inventory the snapshot was computed under', async () => { + const cache = new RuntimeResolvedWorktreeCache() + await cache.getSnapshot(async () => snapshotOf([]), 60_000, 7) + + expect(cache.isFresh(7)).toBe(true) + expect(cache.isFresh(8)).toBe(false) + cache.invalidateResolved() + expect(cache.isFresh(7)).toBe(false) + }) + + it('keeps a primed snapshot servable when nothing mutated', async () => { + // Why: the headless-reattach fixtures prime this cache once and then resolve a selector off it + // without any git available. Losing freshness for a reason other than a mutation strands them + // on a real scan, which is the failure this pairs with — a lookup that finds nothing because + // the snapshot was dropped, not because the worktree is gone. + const cache = new RuntimeResolvedWorktreeCache() + let computes = 0 + const prime = async (): Promise => { + computes += 1 + return snapshotOf(['repo-restore::/tmp/restore-records']) + } + await cache.getSnapshot(prime, 60_000, getWorktreeScanMutationRevision()) + + // A read that mints a scan generation for a repo nothing has scanned yet is not a mutation. + getLocalWorktreeScanGeneration(`repo-never-scanned-${Math.random()}`) + + expect(cache.isFresh(getWorktreeScanMutationRevision())).toBe(true) + const served = await cache.getSnapshot(prime, 60_000, getWorktreeScanMutationRevision()) + expect(computes).toBe(1) + expect(served.worktrees.map((worktree) => worktree.id)).toEqual([ + 'repo-restore::/tmp/restore-records' + ]) + }) +}) + +describe('getWorktreeScanMutationRevision', () => { + it('advances on a repo mutation and not on a first-seen generation read', () => { + const repoId = `repo-${Math.random()}` + const before = getWorktreeScanMutationRevision() + + getLocalWorktreeScanGeneration(repoId) + expect(getWorktreeScanMutationRevision()).toBe(before) + + bumpLocalWorktreeScanGeneration(repoId) + expect(getWorktreeScanMutationRevision()).toBe(before + 1) + }) +}) diff --git a/src/main/runtime/runtime-resolved-worktree-cache.ts b/src/main/runtime/runtime-resolved-worktree-cache.ts index b7ce735eefe..ef7532a6e91 100644 --- a/src/main/runtime/runtime-resolved-worktree-cache.ts +++ b/src/main/runtime/runtime-resolved-worktree-cache.ts @@ -5,9 +5,10 @@ export type ResolvedWorktreeSnapshot = { platformByRepoId: ReadonlyMap } -type ResolvedCache = ResolvedWorktreeSnapshot & { expiresAt: number } +type ResolvedCache = ResolvedWorktreeSnapshot & { expiresAt: number; inventoryRevision: number } type ResolvedInFlight = { generation: number + inventoryRevision: number promise: Promise } export class RuntimeResolvedWorktreeCache { @@ -19,25 +20,43 @@ export class RuntimeResolvedWorktreeCache { return this.resolved } + /** + * Why the revision and not the TTL alone: a snapshot only answers for the repos that were + * registered when it ran. A repo added afterwards — a remote host the user just connected — + * is missing from it for reasons that have nothing to do with what exists on that host, and + * callers read the gap as a verdict that the worktree does not exist. + */ + isFresh(inventoryRevision: number, now = Date.now()): boolean { + return Boolean( + this.resolved && + this.resolved.inventoryRevision === inventoryRevision && + this.resolved.expiresAt > now + ) + } + async getSnapshot( compute: () => Promise, - ttlMs: number + ttlMs: number, + inventoryRevision: number ): Promise { - if (this.resolved && this.resolved.expiresAt > Date.now()) { + if (this.resolved && this.isFresh(inventoryRevision)) { return this.resolved } const generation = this.resolvedGeneration - if (this.resolvedInFlight?.generation === generation) { + if ( + this.resolvedInFlight?.generation === generation && + this.resolvedInFlight.inventoryRevision === inventoryRevision + ) { return this.resolvedInFlight.promise } const promise = compute() - this.resolvedInFlight = { generation, promise } + this.resolvedInFlight = { generation, inventoryRevision, promise } try { const result = await promise if (generation === this.resolvedGeneration) { // Why stamped on completion, not entry: a compute that spent longer than the TTL would // otherwise publish an already-expired entry, so the next poll recomputes the same slow path. - this.resolved = { ...result, expiresAt: Date.now() + ttlMs } + this.resolved = { ...result, inventoryRevision, expiresAt: Date.now() + ttlMs } } return result } finally {