mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
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.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
const generationByRepoId = new Map<string, number>()
|
||||
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()
|
||||
}
|
||||
|
||||
@@ -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<ResolvedWorktree[]> {
|
||||
@@ -80,7 +80,7 @@ export class OrcaRuntimeWithListKnownResolvedWorktreesForExplicitTarget extends
|
||||
return this.resolvedWorktrees.getSnapshot(
|
||||
() => this.computeResolvedWorktrees(),
|
||||
RESOLVED_WORKTREE_CACHE_TTL_MS,
|
||||
getWorktreeScanGenerationSequence()
|
||||
getWorktreeScanMutationRevision()
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -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<ResolvedWorktreeSnapshot> => {
|
||||
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)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user