From a0e14d02cde0d3e432bb2c97099a46ca2e7d9241 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 18:06:35 -0700 Subject: [PATCH] fix(worktrees): key the snapshot on repo mutations only, not on generation reads Two things the headless-reattach lane surfaced. The revision I keyed the snapshot on was `generationSequence`, which `getLocalWorktreeScanGeneration` also advances when it mints a key for a repo id nothing has scanned yet. That is a read, not a mutation, so a read path could discard a snapshot that was still perfectly valid -- the mirror image of the staleness this fixes, and a way to make a lookup fail that would otherwise have succeeded. The counter now advances only where the scan generation is actually bumped: repo add, removal, update, and scan-cache invalidation. Separately, `pty-restore-record-seeding.test.ts` primed the cache by writing its private `resolved` field with a literal spelling out `worktrees`, `platformByRepoId` and `expiresAt`. That literal is a second copy of the cache's freshness contract, so adding a field to the real entry left the fake one failing the check: the primed snapshot was rejected, resolution fell through to a real scan, and the headless fixture -- which has no git -- got `selector_not_found`. It now primes through `getSnapshot` so the cache stamps its own entry and the two cannot drift again. The revision never moved during that test (0 before and after), so nothing was being invalidated; the fake entry simply never satisfied the contract. --- .../ipc/pty-restore-record-seeding.test.ts | 24 +++++++---- src/main/local-worktree-scan-generation.ts | 20 ++++++--- ...-resolved-worktrees-for-explicit-target.ts | 6 +-- .../runtime-resolved-worktree-cache.test.ts | 42 +++++++++++++++++++ 4 files changed, 76 insertions(+), 16 deletions(-) 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 6beb8da7c48..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,16 +14,22 @@ export function getLocalWorktreeScanGeneration(repoId: string): number { export function bumpLocalWorktreeScanGeneration(repoId: string): void { generationByRepoId.set(repoId, ++generationSequence) + mutationRevision += 1 } /** - * The shared counter behind every per-repo generation above. It advances on each repo add, removal - * and update, and on the first key handed out for a repo id nothing has scanned yet — so a cache - * that must not answer for a repo it never saw can compare it in O(1) instead of walking the repo - * list. Ordering-only: the value itself means nothing outside a same-process comparison. + * 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 getWorktreeScanGenerationSequence(): number { - return generationSequence +export function getWorktreeScanMutationRevision(): number { + return mutationRevision } export function isLocalWorktreeScanGenerationCurrent(repoId: string, generation: number): boolean { @@ -31,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 b06a1eb800c..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,7 +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 { getWorktreeScanGenerationSequence } from '../local-worktree-scan-generation' +import { getWorktreeScanMutationRevision } from '../local-worktree-scan-generation' import { resolveLocalProjectRuntimeForRepo, resolveLocalProjectRuntimesForRepos @@ -66,7 +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 { - return this.resolvedWorktrees.isFresh(getWorktreeScanGenerationSequence()) + return this.resolvedWorktrees.isFresh(getWorktreeScanMutationRevision()) } protected async listResolvedWorktrees(): Promise { @@ -80,7 +80,7 @@ export class OrcaRuntimeWithListKnownResolvedWorktreesForExplicitTarget extends return this.resolvedWorktrees.getSnapshot( () => this.computeResolvedWorktrees(), RESOLVED_WORKTREE_CACHE_TTL_MS, - getWorktreeScanGenerationSequence() + getWorktreeScanMutationRevision() ) } diff --git a/src/main/runtime/runtime-resolved-worktree-cache.test.ts b/src/main/runtime/runtime-resolved-worktree-cache.test.ts index b004fe93396..31cc4fa2b60 100644 --- a/src/main/runtime/runtime-resolved-worktree-cache.test.ts +++ b/src/main/runtime/runtime-resolved-worktree-cache.test.ts @@ -1,6 +1,11 @@ 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 { @@ -67,4 +72,41 @@ describe('RuntimeResolvedWorktreeCache', () => { 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) + }) })