fix(worktrees): prevent deletion from blocking Orca

Add timeout-based draining of watcher closes so SSH round-trip delays don't
indefinitely block the worktree removal path. Also: order durable temp-file
sweeps ahead of writes to reclaim orphans before accumulation, skip own-process
temps to avoid deleting live writes, swallow persistence errors so disk failures
don't cascade to query callers, and measure history-deletion progress by loop
turns rather than timer gaps to detect blocking on CI runners.
This commit is contained in:
Jinjing
2026-07-28 21:18:12 -07:00
parent 8177643907
commit 6dc510b3af
14 changed files with 180 additions and 57 deletions
+6 -3
View File
@@ -346,7 +346,8 @@ export class ClaudeUsageStore {
this.store = store
this.state = this.load()
// Why: a crash between write and rename orphans a 20 MB temp file; reclaim once per launch.
void removeStaleDurableWriteTempFiles(getClaudeUsageFile())
// Seeding the write queue with it also orders the sweep ahead of the first write of this launch.
this.pendingWrite = removeStaleDurableWriteTempFiles(getClaudeUsageFile())
}
private load(): ClaudeUsagePersistedState {
@@ -488,10 +489,12 @@ export class ClaudeUsageStore {
this.state.worktreeFingerprint = worktreeFingerprint
this.state.scanState.lastScanCompletedAt = Date.now()
this.state.scanState.lastScanError = null
await this.writeToDisk()
// Why swallow: persistence is a cache concern. A disk failure must not turn a good scan into
// a scan error and reject refresh() for every query caller; writeToDisk already logs it.
await this.writeToDisk().catch(() => {})
} catch (error) {
this.state.scanState.lastScanError = error instanceof Error ? error.message : String(error)
await this.writeToDisk()
await this.writeToDisk().catch(() => {})
} finally {
this.scanPromise = null
}
+6 -3
View File
@@ -381,7 +381,8 @@ export class CodexUsageStore {
this.store = store
this.state = this.load()
// Why: a crash between write and rename orphans a 60 MB temp file; reclaim once per launch.
void removeStaleDurableWriteTempFiles(getCodexUsageFile())
// Seeding the write queue with it also orders the sweep ahead of the first write of this launch.
this.pendingWrite = removeStaleDurableWriteTempFiles(getCodexUsageFile())
}
private load(): CodexUsagePersistedState {
@@ -502,10 +503,12 @@ export class CodexUsageStore {
this.state.worktreeFingerprint = worktreeFingerprint
this.state.scanState.lastScanCompletedAt = Date.now()
this.state.scanState.lastScanError = null
await this.writeToDisk()
// Why swallow: persistence is a cache concern. A disk failure must not turn a good scan into
// a scan error and reject refresh() for every query caller; writeToDisk already logs it.
await this.writeToDisk().catch(() => {})
} catch (error) {
this.state.scanState.lastScanError = error instanceof Error ? error.message : String(error)
await this.writeToDisk()
await this.writeToDisk().catch(() => {})
} finally {
this.scanPromise = null
}
+16 -12
View File
@@ -492,28 +492,32 @@ describe('HistoryManager', () => {
const sessionId = 'bulky'
await mgr.openSession(sessionId, { cwd: '/tmp', cols: 80, rows: 24 })
const sessionDir = join(dir, getHistorySessionDirName(sessionId))
// Enough entries that a synchronous recursive rm is unmistakable in the timer gaps.
// Enough entries that an async rm has to cycle the loop thousands of times to finish.
for (let i = 0; i < 3_000; i++) {
writeFileSync(join(sessionDir, `chunk-${i}.log`), `payload-${i}`)
}
// Why: removeSession runs in the Electron main process, so the probe is main-thread lag, not duration.
let maxGapMs = 0
let previousTickAt = performance.now()
const ticker = setInterval(() => {
const now = performance.now()
maxGapMs = Math.max(maxGapMs, now - previousTickAt)
previousTickAt = now
}, 2)
// Why loop turns, not timer gaps: removeSession runs on the Electron main thread, and the
// regression is a blocked loop. Wall-clock gap bounds measure runner scheduling and flake;
// a turn count doesn't — a sync recursive rm yields exactly zero turns while it walks the
// tree, an async one yields one per libuv completion regardless of how fast the box is.
let loopTurns = 0
let counting = true
const countTurn = (): void => {
if (counting) {
loopTurns++
setImmediate(countTurn)
}
}
setImmediate(countTurn)
try {
previousTickAt = performance.now()
await mgr.removeSession(sessionId)
} finally {
clearInterval(ticker)
counting = false
}
expect(existsSync(sessionDir)).toBe(false)
expect(maxGapMs).toBeLessThan(30)
expect(loopTurns).toBeGreaterThan(100)
})
})
})
+6 -2
View File
@@ -104,16 +104,20 @@ export function durableWriteTempPath(finalPath: string): string {
/**
* Sweep temp files orphaned by a death between write and rename — for multi-MB payloads they would
* otherwise accumulate forever. Racing another instance's in-flight save at worst loses that save,
* the trade already accepted for rename-based atomicity.
* the trade already accepted for rename-based atomicity. This process's own temps are skipped: a
* `<file>.<our pid>.*.tmp` seen during a sweep is a live write, and deleting it fails its rename.
*/
export async function removeStaleDurableWriteTempFiles(finalPath: string): Promise<void> {
const directory = dirname(finalPath)
const prefix = `${basename(finalPath)}.`
const ownPrefix = `${prefix}${process.pid}.`
try {
const names = await readdir(directory)
await Promise.all(
names
.filter((name) => name.startsWith(prefix) && name.endsWith('.tmp'))
.filter(
(name) => name.startsWith(prefix) && name.endsWith('.tmp') && !name.startsWith(ownPrefix)
)
.map((name) => rm(join(directory, name), { force: true }).catch(() => {}))
)
} catch {
+2 -3
View File
@@ -65,8 +65,7 @@ import { DesktopRelayService } from './runtime/relay/desktop-relay-service'
import type { RelayBrokerStatus } from './runtime/relay/relay-session-broker'
import { awaitRuntimeFileWatcherUnsubscribes } from './runtime/orca-runtime-files'
import { clearRuntimeMetadataIfOwned } from './runtime/runtime-metadata'
import { schedulePendingHistoryTreeRemovals } from './terminal-history-deletion'
import { getHistoryRoot } from './terminal-history-paths'
import { scheduleAllPendingHistoryTreeRemovals } from './terminal-history-deletion'
import { ensureMainI18n, setMainPluginLanguagePacks, setMainUiLanguage } from './i18n/main-i18n'
import {
getNextDefaultOnAppearanceSettingValue,
@@ -2688,7 +2687,7 @@ void app.whenReady().then(async () => {
automations.start()
// Why: serve deletes worktrees too, and the history GC that normally drains delete tombstones is
// armed from the main window — without this, a quit mid-removal leaks the tree until a desktop launch.
schedulePendingHistoryTreeRemovals(getHistoryRoot())
scheduleAllPendingHistoryTreeRemovals()
await printServeReady(serveOptions)
return
}
+5 -1
View File
@@ -31,13 +31,17 @@ export async function drainBeforeWatcherRemoval(
}
let timer: ReturnType<typeof setTimeout> | undefined
try {
const settled = promise.then(() => 'settled' as const)
const outcome = await Promise.race([
promise.then(() => 'settled' as const),
settled,
new Promise<'timeout'>((resolve) => {
timer = setTimeout(() => resolve('timeout'), deadline.remainingMs())
})
])
if (outcome === 'timeout') {
// Why: nobody awaits the abandoned promise anymore, so a late rejection (an aborted native
// subscribe finally reporting) would be an unhandled rejection — fatal in the main process.
void settled.catch(() => {})
console.warn(`[watcher-removal] Timed out waiting for ${label}; continuing removal`)
}
return outcome
+26
View File
@@ -7901,6 +7901,32 @@ describe('registerWorktreeHandlers', () => {
}
})
it('traces a local archive hook as flow local, not remote', async () => {
const records: RedactableSpan[] = []
setActiveSink({
push: (record) => records.push(record as RedactableSpan),
flush: () => {},
close: () => {}
})
try {
mockKnownFeatureWorktree()
// The archive hook block is shared by both flows, so a local repo must not land under 'remote'.
getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'pnpm worktree:archive' } })
runHookMock.mockResolvedValue({ success: true, output: '' })
removeWorktreeMock.mockResolvedValue({})
await handlers['worktrees:remove'](null, { worktreeId: 'repo-1::/workspace/feature-wt' })
const archiveStage = records.find((record) => record.name === 'worktree.remove.archive_hook')
expect(archiveStage?.attributes).toMatchObject({
kind: 'worktree',
'worktree.flow': 'local'
})
} finally {
_resetTracerForTests()
}
})
it('prunes git worktree tracking when removing an orphaned worktree', async () => {
mockKnownFeatureWorktree()
const orphanError = Object.assign(new Error('git worktree remove failed'), {
+21 -15
View File
@@ -2472,23 +2472,29 @@ export function registerWorktreeHandlers(
const hooks = await getArchiveHooksForRemoval(repo)
const archiveScript = hooks?.scripts.archive
if (archiveScript && !args.skipArchive) {
await withWorktreeRemoveStageSpan('archive_hook', 'remote', async () => {
const result = repo.connectionId
? await runRemoteArchiveHook(repo, canonicalWorktreePath, archiveScript)
: await runHook(
'archive',
canonicalWorktreePath,
repo,
undefined,
localWorktreeGitOptions
// Why the branch on connectionId: this block is shared by both flows, so a hardcoded
// 'remote' would file every local archive hook under the SSH breakdown.
await withWorktreeRemoveStageSpan(
'archive_hook',
repo.connectionId ? 'remote' : 'local',
async () => {
const result = repo.connectionId
? await runRemoteArchiveHook(repo, canonicalWorktreePath, archiveScript)
: await runHook(
'archive',
canonicalWorktreePath,
repo,
undefined,
localWorktreeGitOptions
)
if (!result.success) {
console.error(
`[hooks] archive hook failed for ${canonicalWorktreePath}:`,
result.output
)
if (!result.success) {
console.error(
`[hooks] archive hook failed for ${canonicalWorktreePath}:`,
result.output
)
}
}
})
)
}
const remoteConnectionId = repo.connectionId ?? undefined
+10 -1
View File
@@ -207,10 +207,19 @@ export async function withWorktreeSpan<T>(
)
}
/** Closed set so a typo can't silently mint an orphan span name. */
export type WorktreeRemoveStage =
| 'archive_hook'
| 'cache_invalidation'
| 'git_remove'
| 'metadata_purge'
| 'pty_sweep'
| 'watcher_gate'
/** Wrap one stage of a worktree removal. Children share the parent's `kind` so `kind`-filtered
* views keep the whole tree, and `worktree.flow` separates the folder/remote/local removal paths. */
export async function withWorktreeRemoveStageSpan<T>(
stage: string,
stage: WorktreeRemoveStage,
flow: 'folder' | 'remote' | 'local',
fn: () => Promise<T>
): Promise<T> {
+6 -3
View File
@@ -167,7 +167,8 @@ export class OpenCodeUsageStore {
this.store = store
this.state = this.load()
// Why: a crash between write and rename orphans a multi-MB temp file; reclaim once per launch.
void removeStaleDurableWriteTempFiles(getOpenCodeUsageFile())
// Seeding the write queue with it also orders the sweep ahead of the first write of this launch.
this.pendingWrite = removeStaleDurableWriteTempFiles(getOpenCodeUsageFile())
}
private load(): OpenCodeUsagePersistedState {
@@ -290,10 +291,12 @@ export class OpenCodeUsageStore {
this.state.worktreeFingerprint = worktreeFingerprint
this.state.scanState.lastScanCompletedAt = Date.now()
this.state.scanState.lastScanError = null
await this.writeToDisk()
// Why swallow: persistence is a cache concern. A disk failure must not turn a good scan into
// a scan error and reject refresh() for every query caller; writeToDisk already logs it.
await this.writeToDisk().catch(() => {})
} catch (error) {
this.state.scanState.lastScanError = error instanceof Error ? error.message : String(error)
await this.writeToDisk()
await this.writeToDisk().catch(() => {})
} finally {
this.scanPromise = null
}
+12 -2
View File
@@ -7168,11 +7168,21 @@ export class OrcaRuntimeService {
connectionId?: string,
deadline: WatcherRemovalDeadline = createWatcherRemovalDeadline()
): Promise<void> => {
// Why drain the remote/explorer closes: they await SSH round trips and lease suspends that a dead
// link never answers. The local close bounds its own awaits against the same deadline internally.
const results = await Promise.allSettled([
connectionId
? closeRemoteWatcherForWorktreePath(connectionId, worktreePath)
? drainBeforeWatcherRemoval(
closeRemoteWatcherForWorktreePath(connectionId, worktreePath),
deadline,
`remote watcher close for ${worktreePath}`
)
: closeLocalWatcherForWorktreePath(worktreePath, deadline),
this.fileCommands.closeFileExplorerWatchersForPath(worktreePath, connectionId)
drainBeforeWatcherRemoval(
this.fileCommands.closeFileExplorerWatchersForPath(worktreePath, connectionId),
deadline,
`file explorer watcher close for ${worktreePath}`
)
])
const failure = results.find((result): result is PromiseRejectedResult => {
return result.status === 'rejected'
+8 -3
View File
@@ -73,13 +73,18 @@ export function schedulePendingHistoryTreeRemovals(historyRoot: string): void {
}
}
/** Drain every history root's tombstones and await the in-flight removals. Tests only: production
* schedules the same drain from startup GC and headless serve without ever blocking on it. */
export async function flushPendingWorktreeHistoryDeletions(): Promise<void> {
/** Schedule tombstoned trees under every history root, native and WSL. */
export function scheduleAllPendingHistoryTreeRemovals(): void {
schedulePendingHistoryTreeRemovals(getHistoryRoot())
for (const distroRoot of listWslHistoryRoots()) {
schedulePendingHistoryTreeRemovals(distroRoot)
}
}
/** Drain every history root's tombstones and await the in-flight removals. Tests only: production
* schedules the same drain from startup GC and headless serve without ever blocking on it. */
export async function flushPendingWorktreeHistoryDeletions(): Promise<void> {
scheduleAllPendingHistoryTreeRemovals()
await Promise.all(pendingHistoryTreeRemovals.values())
}
+5 -1
View File
@@ -1,3 +1,4 @@
import type * as FsPromises from 'node:fs/promises'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
const {
@@ -35,7 +36,10 @@ vi.mock('fs', () => ({
statSync: statSyncMock
}))
vi.mock('node:fs/promises', () => ({
// Spread the real module: this factory replaces node:fs/promises for the whole import graph, so a
// transitive readFile/mkdir would otherwise resolve to undefined.
vi.mock('node:fs/promises', async () => ({
...(await vi.importActual<typeof FsPromises>('node:fs/promises')),
rm: rmAsyncMock
}))
@@ -13,7 +13,9 @@ import { rm } from 'node:fs/promises'
import net from 'node:net'
import os from 'node:os'
import path from 'node:path'
import { chromium } from 'playwright'
// Why @playwright/test, not playwright: only the former is a declared devDependency; it re-exports
// the same browser types, so the benchmark resolves without relying on a hoisted transitive install.
import { chromium } from '@playwright/test'
const DEFAULT_ITERATIONS = 3
const DEFAULT_HISTORY_FILES = 10_000
@@ -132,6 +134,8 @@ function launchDevInstance({ label, repoRoot }, fixture, port) {
cwd: repoRoot,
env,
stdio: ['ignore', 'pipe', 'pipe'],
// Why: own process group so teardown can signal the Electron/Vite grandchildren, not just the launcher.
detached: process.platform !== 'win32',
windowsHide: true
})
const log = { stdout: '', stderr: '' }
@@ -159,8 +163,15 @@ async function connectToOrca(instance) {
}
try {
const browser = await chromium.connectOverCDP(instance.endpoint)
const page = await findOrcaPage(browser)
return { browser, page }
try {
// CDP answers long before the renderer exposes window.__store; without this, every
// findOrcaPage timeout drops a live browser handle and leaks a connection per retry.
const page = await findOrcaPage(browser)
return { browser, page }
} catch (error) {
await browser.close().catch(() => undefined)
throw error
}
} catch (error) {
lastError = error
await delay(500)
@@ -296,6 +307,7 @@ async function measureDeletion(page, worktreeId, rootWorktreeId) {
}
store.getState().setActiveWorktree(fallbackId)
let settled = false
let failure = null
const startedAt = performance.now()
const deletion = store
.getState()
@@ -304,6 +316,13 @@ async function measureDeletion(page, worktreeId, rootWorktreeId) {
settled = true
return result
})
.catch((error) => {
// Why: without this the poll loop below spins forever on a rejected delete — the exact
// failure this benchmark exists to surface would read as a hang.
settled = true
failure = error
return null
})
const ipcSamplesMs = []
let maxRendererTimerDriftMs = 0
let expectedTimerAt = performance.now() + 10
@@ -311,7 +330,6 @@ async function measureDeletion(page, worktreeId, rootWorktreeId) {
await new Promise((resolve) => window.setTimeout(resolve, 10))
const now = performance.now()
maxRendererTimerDriftMs = Math.max(maxRendererTimerDriftMs, now - expectedTimerAt)
expectedTimerAt = now + 10
const ipcStartedAt = performance.now()
await Promise.race([
window.api.repos.list(),
@@ -320,8 +338,14 @@ async function measureDeletion(page, worktreeId, rootWorktreeId) {
)
])
ipcSamplesMs.push(performance.now() - ipcStartedAt)
// Why re-read the clock here: anchoring on `now` would fold the IPC round trip that
// ipcSamplesMs already records into the next iteration's drift.
expectedTimerAt = performance.now() + 10
}
const result = await deletion
if (failure) {
throw new Error(`removeWorktree rejected: ${String(failure)}`)
}
const postDeleteIpcStartedAt = performance.now()
await window.api.repos.list()
const finalState = store.getState()
@@ -464,7 +488,14 @@ async function benchmarkInstance(instanceConfig, index, options) {
await browser.close()
browser = null
await stopDevInstance(instance)
await verifyRestart(instanceConfig, fixture, port, repoState.repoId)
// Why a fresh port: the debug listener may still hold the old one, so reusing it can attach to
// the dying endpoint or burn the whole start timeout.
await verifyRestart(
instanceConfig,
fixture,
await findAvailablePort(port + 1),
repoState.repoId
)
return {
label: instanceConfig.label,
repoRoot: instanceConfig.repoRoot,
@@ -508,20 +539,32 @@ function isPortAvailable(port) {
}
async function stopDevInstance(instance) {
if (instance.child.exitCode !== null) {
// Why signalCode too: a signal-terminated child leaves exitCode null, so an exitCode-only guard
// re-enters the kill path for a dead process and burns the full 8s race on every teardown.
if (instance.child.exitCode !== null || instance.child.signalCode !== null) {
return
}
if (process.platform === 'win32') {
spawnSync('taskkill.exe', ['/PID', String(instance.child.pid), '/T', '/F'], { stdio: 'ignore' })
return
}
instance.child.kill('SIGINT')
killDevInstanceTree(instance.child, 'SIGINT')
const exited = await Promise.race([
new Promise((resolve) => instance.child.once('exit', () => resolve(true))),
delay(8_000).then(() => false)
])
if (!exited) {
instance.child.kill('SIGKILL')
killDevInstanceTree(instance.child, 'SIGKILL')
}
}
/** POSIX: signal the whole group, or the launcher's Electron/Vite grandchildren survive and keep the
* CDP port and fixture directory busy. */
function killDevInstanceTree(child, signal) {
try {
process.kill(-child.pid, signal)
} catch {
child.kill(signal)
}
}