From 505d6719c075eb0344dac86867a4e4030e970a1d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:23:02 -0700 Subject: [PATCH] Refuse a workspace parent that would close a loop, even when the worktree cache is cold (#29340) * fix(worktree): refuse a parent loop even when the worktree cache is cold The parent cycle walk read each workspace's current instance from the resolved-worktree cache, which the reparent itself invalidates, so it only knew the two endpoints and accepted A->B->C->A. Read saved worktree metadata instead. * fix(worktree): fall back to locator metadata and tolerate loops saved earlier * fix(worktree): skip another host's locator metadata in the parent loop walk * fix(worktree): read loop-check identities from a read-only, ownership-scoped view The per-host metadata accessor migrates a hostless legacy row into the queried host, so the loop check could adopt another host's worktree and refuse a valid parent. Read one read-only host snapshot instead, counting a row only when its project is registered on that host (the resolved list's ownership rule). * fix: import host-qualified worktree meta helpers once * test: match main's host-targeted lineage write contract --- .../construction/runtime-service-state.ts | 1 - .../lineage-and-scan-cache-part-02.spec.ts | 70 ++++++++++++++++++ .../runtime-worktree-lineage-controller.ts | 74 ++++++++++++++----- src/main/wsl-move-scan-pruning.test.ts | 1 - 4 files changed, 127 insertions(+), 19 deletions(-) diff --git a/src/main/runtime/construction/runtime-service-state.ts b/src/main/runtime/construction/runtime-service-state.ts index 80d686e76ef..7066c212b71 100644 --- a/src/main/runtime/construction/runtime-service-state.ts +++ b/src/main/runtime/construction/runtime-service-state.ts @@ -102,7 +102,6 @@ export abstract class OrcaRuntimeWithStopRequestedPtyIds extends OrcaRuntimeWith protected readonly worktreeLineage = new RuntimeWorktreeLineageController({ getStore: () => this.store, - getCachedWorktrees: () => this.resolvedWorktrees.peek()?.worktrees ?? null, getDb: () => this.getOrchestrationDbIfAvailable(), resolveWorktree: (selector) => this.resolveWorktreeSelector(selector), listResolvedWorktrees: () => this.listResolvedWorktrees(), diff --git a/src/main/runtime/orca-runtime-tests/lineage-and-scan-cache-part-02.spec.ts b/src/main/runtime/orca-runtime-tests/lineage-and-scan-cache-part-02.spec.ts index 81abead9edd..643080b98da 100644 --- a/src/main/runtime/orca-runtime-tests/lineage-and-scan-cache-part-02.spec.ts +++ b/src/main/runtime/orca-runtime-tests/lineage-and-scan-cache-part-02.spec.ts @@ -112,6 +112,76 @@ describe('OrcaRuntimeService', () => { ) }) + it('refuses a reparent that closes a three-workspace loop through saved lineage', async () => { + const names = ['a', 'b', 'c', 'd', 'e', 'f', 'h'] + const [aId, bId, cId, dId, eId, fId, hId] = names.map( + (name) => `${TEST_REPO_ID}::/tmp/worktree-${name}` + ) + // A remote project's row saved without a host by an older build. + const gId = 'repo-remote::/remote/worktree-g' + const metaById: Record = Object.fromEntries( + [aId, bId, cId, dId, eId, fId, gId, hId].map((id) => [ + id, + makeWorktreeMeta({ instanceId: `${id}-instance` }) + ]) + ) + const edge = (child: string, parent: string): WorktreeLineage => ({ + worktreeId: child, + worktreeInstanceId: `${child}-instance`, + parentWorktreeId: parent, + parentWorktreeInstanceId: `${parent}-instance`, + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' }, + createdAt: 1 + }) + const lineageById: Record = { + [aId]: edge(aId, bId), + [bId]: edge(bId, cId), + // d <-> e is a loop saved by an older build. + [dId]: edge(dId, eId), + [eId]: edge(eId, dId), + // Cross-host links an older create could save; readers never follow them. + [fId]: edge(fId, gId), + [gId]: edge(gId, hId) + } + const setWorktreeLineage = vi.fn((_target: HostWorktreeTarget, lineage) => lineage) + const runtimeStore = { + ...store, + getRepos: () => [ + ...store.getRepos(), + { id: 'repo-remote', path: '/remote', displayName: 'remote', connectionId: 'box' } + ], + getAllWorktreeMeta: () => metaById, + getWorktreeMeta: (worktreeId: string) => metaById[worktreeId], + getWorktreeLineage: (worktreeId: string) => lineageById[worktreeId], + setWorktreeLineage + } + vi.mocked(listWorktrees).mockResolvedValue( + names.map((name) => makeWorktreeInfo(`/tmp/worktree-${name}`)) + ) + const runtime = new OrcaRuntimeService(runtimeStore as never) + + // Why: the reparent itself invalidates the resolved-worktree cache, so the walk must not need it. + await expect( + runtime.updateManagedWorktreeMeta(`id:${cId}`, { lineage: { parentWorktree: `id:${aId}` } }) + ).rejects.toThrow('lineage cycle') + expect(setWorktreeLineage).not.toHaveBeenCalled() + + for (const [childId, parentId] of [ + [cId, dId], + [hId, fId] + ]) { + await runtime.updateManagedWorktreeMeta(`id:${childId}`, { + lineage: { parentWorktree: `id:${parentId}` } + }) + expect(setWorktreeLineage).toHaveBeenLastCalledWith( + expect.objectContaining({ id: childId }), + expect.objectContaining({ parentWorktreeId: parentId }) + ) + } + expect(metaById[gId].hostId).toBeUndefined() + }) + it('rejects lineage updates when upgraded metadata is missing a parent instance id', async () => { const parentPath = '/tmp/worktree-parent' const childPath = '/tmp/worktree-child' diff --git a/src/main/runtime/runtime-worktree-lineage-controller.ts b/src/main/runtime/runtime-worktree-lineage-controller.ts index 89702882fb6..ea94fa1c513 100644 --- a/src/main/runtime/runtime-worktree-lineage-controller.ts +++ b/src/main/runtime/runtime-worktree-lineage-controller.ts @@ -1,5 +1,6 @@ import type { WorktreeLineage, WorkspaceLineage } from '../../shared/worktree/lineage-types' -import type { ExecutionHostId } from '../../shared/execution-host' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import { getRepoIdFromWorktreeId } from '../../shared/worktree/id' import type { WorkspaceKey } from '../../shared/folder-workspace-types' import { folderWorkspaceKey, @@ -9,8 +10,12 @@ import { import { sharesResolvedWorktreeLineageBoundary } from '../../shared/resolved-worktree-lineage' import type { OrchestrationDb } from './orchestration/db' import type { RuntimeStore } from './runtime-store-contract' -import { worktreeRowTarget } from '../persistence/host-qualified-worktree-meta' +import { + readAllWorktreeMetaForHost, + worktreeRowTarget +} from '../persistence/host-qualified-worktree-meta' import type { ResolvedWorktree } from './runtime-worktree-path-identity' +import { getRepoOwnedWorktreeMeta } from '../worktree-metadata-ownership' import { isWslMoveWorktreeBlocked } from '../persistence/moving-wsl-projects/wsl-move-background-backoff' import { RuntimeLineageError, @@ -24,13 +29,40 @@ import { type RuntimeWorktreeLineageDependencies = { getStore(): RuntimeStore | null - getCachedWorktrees(): readonly ResolvedWorktree[] | null getDb(): OrchestrationDb | null resolveWorktree(selector: string): Promise listResolvedWorktrees(): Promise showTerminal(handle: string): Promise<{ worktreeId: string }> } +/** + * Read-only and ownership-scoped like the resolved list: the migrating per-host accessor would adopt + * another host's legacy row into this host just to answer a validation. + */ +function savedInstanceIdReader( + store: RuntimeStore, + hostId: ExecutionHostId | undefined +): (id: string) => string | undefined { + if (!hostId) { + return (id) => store.getWorktreeMeta(id)?.instanceId + } + const repos = store.getRepos() + const ownerCounts = new Map() + for (const repo of repos) { + ownerCounts.set(repo.id, (ownerCounts.get(repo.id) ?? 0) + 1) + } + const reposOnHost = new Map( + repos.filter((repo) => getRepoExecutionHostId(repo) === hostId).map((repo) => [repo.id, repo]) + ) + const metaById = readAllWorktreeMetaForHost(store, hostId) + return (id) => { + const repo = reposOnHost.get(getRepoIdFromWorktreeId(id)) + return repo + ? getRepoOwnedWorktreeMeta(repo, id, metaById, ownerCounts.get(repo.id) ?? 0)?.instanceId + : undefined + } +} + export class RuntimeWorktreeLineageController { constructor(private readonly deps: RuntimeWorktreeLineageDependencies) {} @@ -101,32 +133,40 @@ export class RuntimeWorktreeLineageController { 'Parent worktree must be on the same execution host.' ) } - const hostId = child.hostId ?? parent.hostId - const instanceById = new Map( - this.deps - .getCachedWorktrees() - ?.filter((worktree) => !hostId || !worktree.hostId || worktree.hostId === hostId) - .map((worktree) => [worktree.id, worktree.instanceId]) ?? [] - ) - instanceById.set(child.id, child.instanceId) - instanceById.set(parent.id, parent.instanceId) + const store = this.deps.getStore() + let readSavedInstanceId: ((id: string) => string | undefined) | undefined + // Why: saved metadata, not the resolved-worktree cache, which the reparent itself invalidates; + // an unknown node would end the walk and let a longer loop through. + const instanceIdOf = (id: string): string | undefined => { + if (id === child.id || id === parent.id) { + return id === child.id ? child.instanceId : parent.instanceId + } + readSavedInstanceId ??= store + ? savedInstanceIdReader(store, child.hostId ?? parent.hostId) + : () => undefined + return readSavedInstanceId(id) + } let cursor: string | undefined = parent.id - const visited = new Set([child.id]) + const visited = new Set() while (cursor) { - if (visited.has(cursor)) { + if (cursor === child.id) { throw new RuntimeLineageError( 'LINEAGE_PARENT_CYCLE', 'Parent selector would create a lineage cycle.' ) } + // Why: a loop saved before this walk read saved data must not block unrelated parents. + if (visited.has(cursor)) { + break + } visited.add(cursor) - const lineage = this.deps.getStore()?.getWorktreeLineage?.(cursor) + const lineage = store?.getWorktreeLineage?.(cursor) if (!lineage) { break } if ( - instanceById.get(cursor) !== lineage.worktreeInstanceId || - instanceById.get(lineage.parentWorktreeId) !== lineage.parentWorktreeInstanceId + instanceIdOf(cursor) !== lineage.worktreeInstanceId || + instanceIdOf(lineage.parentWorktreeId) !== lineage.parentWorktreeInstanceId ) { break } diff --git a/src/main/wsl-move-scan-pruning.test.ts b/src/main/wsl-move-scan-pruning.test.ts index b87c2e8b1c4..a2e46e43b3a 100644 --- a/src/main/wsl-move-scan-pruning.test.ts +++ b/src/main/wsl-move-scan-pruning.test.ts @@ -202,7 +202,6 @@ describe('inferred lineage hydration under a WSL move', () => { ]) const controller = new RuntimeWorktreeLineageController({ getStore: () => store, - getCachedWorktrees: () => null, getDb: () => null, resolveWorktree: async () => { throw new Error('unused')