mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(worktrees): request teardown per caller and revalidate uncached
Two defects let the original fix silently strand PTYs: - teardown rode the scan's coalescing promise, so any caller that joined an in-flight scan purged its renderer state without ever asking for a sweep; it now runs per caller against its own known-id snapshot, deduped on the request it actually produces so fan-out still shares one host sweep. - the runtime's authoritative recheck was served from the 30s worktree-scan cache, which can still list a directory git already dropped. The renderer purges either way, so a stale miss leaked those processes permanently. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -31409,6 +31409,41 @@ describe('OrcaRuntimeService', () => {
|
||||
)
|
||||
})
|
||||
|
||||
// Why (#10562): the renderer purges its state regardless of the sweep result, so
|
||||
// revalidating against a cached scan (30s TTL) that still lists an already-deleted
|
||||
// directory would strand those PTYs permanently — nothing asks a second time.
|
||||
it('revalidates against a fresh scan instead of a warm worktree-scan cache', async () => {
|
||||
const deletedId = `${TEST_REPO_ID}::/tmp/deleted`
|
||||
const localProvider = {
|
||||
listProcesses: vi.fn(async () => [
|
||||
{ id: `${deletedId}@@deleted-session`, cwd: '/tmp/deleted', title: 'shell' }
|
||||
]),
|
||||
shutdown: vi.fn(async () => {})
|
||||
}
|
||||
const runtime = new OrcaRuntimeService(store, undefined, {
|
||||
getLocalProvider: () => localProvider as never
|
||||
})
|
||||
|
||||
// Warm the scan cache while the worktree still exists.
|
||||
vi.mocked(listWorktrees).mockResolvedValueOnce([
|
||||
{ path: '/tmp/deleted', head: 'abc', branch: 'd', isBare: false, isMainWorktree: false }
|
||||
])
|
||||
await runtime.teardownMissingManagedWorktreeTerminals(`id:${TEST_REPO_ID}`, [deletedId])
|
||||
expect(localProvider.shutdown).not.toHaveBeenCalled()
|
||||
|
||||
// The worktree is now gone; the cached scan must not mask that.
|
||||
vi.mocked(listWorktrees).mockResolvedValue([])
|
||||
const result = await runtime.teardownMissingManagedWorktreeTerminals(`id:${TEST_REPO_ID}`, [
|
||||
deletedId
|
||||
])
|
||||
|
||||
expect(result).toEqual({ stoppedWorktreeIds: [deletedId] })
|
||||
expect(localProvider.shutdown).toHaveBeenCalledWith(
|
||||
`${deletedId}@@deleted-session`,
|
||||
expect.objectContaining({ immediate: true })
|
||||
)
|
||||
})
|
||||
|
||||
it('does not stop sessions after a non-authoritative revalidation', async () => {
|
||||
const localProvider = {
|
||||
listProcesses: vi.fn(async () => []),
|
||||
|
||||
@@ -18005,6 +18005,10 @@ export class OrcaRuntimeService {
|
||||
connectionId?: string | null
|
||||
): Promise<{ stoppedWorktreeIds: string[] }> {
|
||||
const repo = await this.resolveRepoSelectorForConnection(repoSelector, connectionId)
|
||||
// Why: killing PTYs must be proven against the host right now — a cached scan
|
||||
// (30s TTL) can still list a directory git already dropped, and the renderer
|
||||
// purges its state either way, so a stale miss strands those processes for good.
|
||||
this.invalidateWorktreeScanCacheForRepo(repo.id)
|
||||
const detected = await this.listDetectedManagedWorktrees(`id:${repo.id}`, connectionId)
|
||||
if (!detected.authoritative) {
|
||||
return { stoppedWorktreeIds: [] }
|
||||
|
||||
@@ -656,6 +656,82 @@ describe('fetchWorktrees', () => {
|
||||
expect(mockApi.worktrees.listDetected).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
// Why (#10562): the scan coalesces but each caller carries its own known-id
|
||||
// snapshot, so a caller that joined an in-flight scan must still request
|
||||
// teardown — otherwise it purges renderer state and strands live PTYs.
|
||||
it('stops terminals for a caller that coalesced onto an in-flight scan', async () => {
|
||||
const store = createTestStore()
|
||||
const deleted = makeWorktree({ id: 'repo1::/p/deleted', repoId: 'repo1', path: '/p/deleted' })
|
||||
const surviving = makeWorktree({ id: 'repo1::/p/surv', repoId: 'repo1', path: '/p/surv' })
|
||||
|
||||
// Caller A starts before hydration, so its known-id snapshot is empty.
|
||||
store.setState({
|
||||
repos: [{ id: 'repo1', path: '/p/repo1', displayName: 'R', badgeColor: '#000', addedAt: 0 }],
|
||||
worktreesByRepo: {},
|
||||
detectedWorktreesByRepo: {},
|
||||
tabsByWorktree: {}
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
let releaseScan!: () => void
|
||||
const scanStarted = new Promise<void>((resolve) => {
|
||||
mockApi.worktrees.listDetected.mockImplementationOnce(async ({ repoId }) => {
|
||||
resolve()
|
||||
await new Promise<void>((release) => {
|
||||
releaseScan = release
|
||||
})
|
||||
return makeDetectedResult(repoId, [surviving])
|
||||
})
|
||||
})
|
||||
|
||||
const callerA = store.getState().fetchDetectedWorktrees('repo1')
|
||||
await scanStarted
|
||||
|
||||
// Hydration lands mid-scan: the renderer now owns `deleted` and its tabs.
|
||||
store.setState({
|
||||
worktreesByRepo: { repo1: [deleted, surviving] },
|
||||
detectedWorktreesByRepo: { repo1: makeDetectedResult('repo1', [deleted, surviving]) },
|
||||
tabsByWorktree: { [deleted.id]: [{ id: 'tab-d', worktreeId: deleted.id }] }
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
const callerB = store.getState().fetchWorktrees('repo1')
|
||||
releaseScan()
|
||||
await Promise.all([callerA, callerB])
|
||||
|
||||
// One shared scan, but the coalesced caller still asked the host to sweep.
|
||||
expect(mockApi.worktrees.listDetected).toHaveBeenCalledTimes(1)
|
||||
expect(mockApi.runtime.call).toHaveBeenCalledWith({
|
||||
method: 'worktree.teardownMissingTerminals',
|
||||
params: { repo: 'repo1', worktreeIds: [deleted.id], connectionId: null }
|
||||
})
|
||||
expect(store.getState().tabsByWorktree[deleted.id]).toBeUndefined()
|
||||
})
|
||||
|
||||
// Why: teardown rides outside the scan coalescer, so identical fan-out requests
|
||||
// must share one host sweep instead of re-scanning the host per caller.
|
||||
it('shares one teardown sweep across identical concurrent refreshes', async () => {
|
||||
const store = createTestStore()
|
||||
const deleted = makeWorktree({ id: 'repo1::/p/deleted', repoId: 'repo1', path: '/p/deleted' })
|
||||
const surviving = makeWorktree({ id: 'repo1::/p/surv', repoId: 'repo1', path: '/p/surv' })
|
||||
|
||||
store.setState({
|
||||
repos: [{ id: 'repo1', path: '/p/repo1', displayName: 'R', badgeColor: '#000', addedAt: 0 }],
|
||||
worktreesByRepo: { repo1: [deleted, surviving] },
|
||||
detectedWorktreesByRepo: { repo1: makeDetectedResult('repo1', [deleted, surviving]) },
|
||||
tabsByWorktree: { [deleted.id]: [{ id: 'tab-d', worktreeId: deleted.id }] }
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
mockApi.worktrees.listDetected.mockResolvedValueOnce(makeDetectedResult('repo1', [surviving]))
|
||||
|
||||
await Promise.all([
|
||||
store.getState().fetchWorktrees('repo1'),
|
||||
store.getState().fetchDetectedWorktrees('repo1'),
|
||||
store.getState().fetchWorktrees('repo1')
|
||||
])
|
||||
|
||||
expect(mockApi.runtime.call).toHaveBeenCalledTimes(1)
|
||||
expect(store.getState().tabsByWorktree[deleted.id]).toBeUndefined()
|
||||
})
|
||||
|
||||
it('coalesces fetchDetectedWorktrees with a matching fetchWorktrees refresh', async () => {
|
||||
const store = createTestStore()
|
||||
const refreshed = makeWorktree({
|
||||
|
||||
@@ -633,6 +633,11 @@ function isRuntimeMethodNotFoundError(error: unknown): boolean {
|
||||
return error instanceof RuntimeRpcCallError && error.code === 'method_not_found'
|
||||
}
|
||||
|
||||
// Why: teardown cannot ride the scan's coalescing (each caller has its own known-id
|
||||
// snapshot), so dedupe on the request it actually produces — identical fan-out
|
||||
// requests share one host sweep instead of re-scanning per caller.
|
||||
const missingWorktreeTeardownsInFlight = new Map<string, Promise<void>>()
|
||||
|
||||
async function teardownMissingWorktreeTerminalsBestEffort(
|
||||
settings: AppState['settings'],
|
||||
repoId: string,
|
||||
@@ -648,16 +653,38 @@ async function teardownMissingWorktreeTerminalsBestEffort(
|
||||
if (missingIds.length === 0) {
|
||||
return
|
||||
}
|
||||
const target = getActiveRuntimeTarget(settings)
|
||||
const normalizedConnectionId = connectionId ?? null
|
||||
const key = [
|
||||
target.kind === 'local' ? 'local' : `runtime:${target.environmentId}`,
|
||||
repoId,
|
||||
normalizedConnectionId ?? '',
|
||||
[...missingIds].sort().join('\n')
|
||||
].join('\0')
|
||||
const existing = missingWorktreeTeardownsInFlight.get(key)
|
||||
if (existing) {
|
||||
return existing
|
||||
}
|
||||
const teardown = (async () => {
|
||||
try {
|
||||
await callRuntimeRpc(
|
||||
target,
|
||||
'worktree.teardownMissingTerminals',
|
||||
{ repo: repoId, worktreeIds: missingIds, connectionId: normalizedConnectionId },
|
||||
{ timeoutMs: 30_000 }
|
||||
)
|
||||
} catch (error) {
|
||||
if (!isRuntimeMethodNotFoundError(error)) {
|
||||
console.warn(`Failed to stop terminals for missing worktrees in repo ${repoId}:`, error)
|
||||
}
|
||||
}
|
||||
})()
|
||||
missingWorktreeTeardownsInFlight.set(key, teardown)
|
||||
try {
|
||||
await callRuntimeRpc(
|
||||
getActiveRuntimeTarget(settings),
|
||||
'worktree.teardownMissingTerminals',
|
||||
{ repo: repoId, worktreeIds: missingIds, connectionId: connectionId ?? null },
|
||||
{ timeoutMs: 30_000 }
|
||||
)
|
||||
} catch (error) {
|
||||
if (!isRuntimeMethodNotFoundError(error)) {
|
||||
console.warn(`Failed to stop terminals for missing worktrees in repo ${repoId}:`, error)
|
||||
await teardown
|
||||
} finally {
|
||||
if (missingWorktreeTeardownsInFlight.get(key) === teardown) {
|
||||
missingWorktreeTeardownsInFlight.delete(key)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1075,35 +1102,40 @@ async function listDetectedWorktreesForRepoCoalesced(
|
||||
? getRuntimeEnvironmentConnectionGeneration(target.environmentId)
|
||||
: null
|
||||
const existing = detectedWorktreeRefreshesInFlight.get(key)
|
||||
if (existing) {
|
||||
return existing
|
||||
}
|
||||
// Why: startup/event fan-out can request the same repo/host refresh many times at once; share only the scan promise so state-merge semantics stay local.
|
||||
const refresh = listDetectedWorktreesForRepo(settings, repoId, {
|
||||
reuseRecentCompatibilityFailure: options.reuseRecentCompatibilityFailure
|
||||
})
|
||||
detectedWorktreeRefreshesInFlight.set(key, refresh)
|
||||
try {
|
||||
const result = await refresh
|
||||
if (
|
||||
target.kind === 'environment' &&
|
||||
(getEnvironmentSshStateGeneration(target.environmentId) !== connectionGeneration ||
|
||||
getRuntimeEnvironmentConnectionGeneration(target.environmentId) !==
|
||||
runtimeConnectionGeneration)
|
||||
) {
|
||||
throw new Error('runtime_environment_generation_changed')
|
||||
}
|
||||
await teardownMissingWorktreeTerminalsBestEffort(
|
||||
settings,
|
||||
repoId,
|
||||
options.connectionId,
|
||||
options.knownWorktreeIds,
|
||||
result
|
||||
)
|
||||
return result
|
||||
} finally {
|
||||
if (detectedWorktreeRefreshesInFlight.get(key) === refresh) {
|
||||
detectedWorktreeRefreshesInFlight.delete(key)
|
||||
// Why: the scan coalesces, but teardown must not — each caller carries its own
|
||||
// known-id snapshot and purges its own state, so a caller that joined an
|
||||
// in-flight scan would otherwise purge without ever stopping those terminals.
|
||||
const result = existing ? await existing : await runCoalescedDetectedWorktreeScan()
|
||||
await teardownMissingWorktreeTerminalsBestEffort(
|
||||
settings,
|
||||
repoId,
|
||||
options.connectionId,
|
||||
options.knownWorktreeIds,
|
||||
result
|
||||
)
|
||||
return result
|
||||
|
||||
async function runCoalescedDetectedWorktreeScan(): Promise<DetectedWorktreeListResult> {
|
||||
// Why: startup/event fan-out can request the same repo/host refresh many times at once; share only the scan promise so state-merge semantics stay local.
|
||||
const refresh = listDetectedWorktreesForRepo(settings, repoId, {
|
||||
reuseRecentCompatibilityFailure: options.reuseRecentCompatibilityFailure
|
||||
})
|
||||
detectedWorktreeRefreshesInFlight.set(key, refresh)
|
||||
try {
|
||||
const scanned = await refresh
|
||||
if (
|
||||
target.kind === 'environment' &&
|
||||
(getEnvironmentSshStateGeneration(target.environmentId) !== connectionGeneration ||
|
||||
getRuntimeEnvironmentConnectionGeneration(target.environmentId) !==
|
||||
runtimeConnectionGeneration)
|
||||
) {
|
||||
throw new Error('runtime_environment_generation_changed')
|
||||
}
|
||||
return scanned
|
||||
} finally {
|
||||
if (detectedWorktreeRefreshesInFlight.get(key) === refresh) {
|
||||
detectedWorktreeRefreshesInFlight.delete(key)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user