mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
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
This commit is contained in:
@@ -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(),
|
||||
|
||||
@@ -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<string, WorktreeMeta> = 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<string, WorktreeLineage> = {
|
||||
[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'
|
||||
|
||||
@@ -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<ResolvedWorktree>
|
||||
listResolvedWorktrees(): Promise<ResolvedWorktree[]>
|
||||
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<string, number>()
|
||||
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<string>([child.id])
|
||||
const visited = new Set<string>()
|
||||
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
|
||||
}
|
||||
|
||||
@@ -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')
|
||||
|
||||
Reference in New Issue
Block a user