diff --git a/src/main/claude-usage/store.ts b/src/main/claude-usage/store.ts index c59702c542b..8c182a37d3c 100644 --- a/src/main/claude-usage/store.ts +++ b/src/main/claude-usage/store.ts @@ -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 } diff --git a/src/main/codex-usage/store.ts b/src/main/codex-usage/store.ts index 48bf061d551..7a5c1acce53 100644 --- a/src/main/codex-usage/store.ts +++ b/src/main/codex-usage/store.ts @@ -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 } diff --git a/src/main/daemon/history-manager.test.ts b/src/main/daemon/history-manager.test.ts index 59b58ba84bb..0e1c05613f6 100644 --- a/src/main/daemon/history-manager.test.ts +++ b/src/main/daemon/history-manager.test.ts @@ -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) }) }) }) diff --git a/src/main/durable-file-write.ts b/src/main/durable-file-write.ts index cb17a712ec8..ac57e7d465d 100644 --- a/src/main/durable-file-write.ts +++ b/src/main/durable-file-write.ts @@ -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 + * `..*.tmp` seen during a sweep is a live write, and deleting it fails its rename. */ export async function removeStaleDurableWriteTempFiles(finalPath: string): Promise { 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 { diff --git a/src/main/index.ts b/src/main/index.ts index 587196234e7..054cbe4adc1 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -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 } diff --git a/src/main/ipc/watcher-removal-drain.ts b/src/main/ipc/watcher-removal-drain.ts index 804c9b32834..9a2a5ae7423 100644 --- a/src/main/ipc/watcher-removal-drain.ts +++ b/src/main/ipc/watcher-removal-drain.ts @@ -31,13 +31,17 @@ export async function drainBeforeWatcherRemoval( } let timer: ReturnType | 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 diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 2800ba2500f..c686f499d4d 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -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'), { diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 0d74167915a..6e06d502f1f 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -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 diff --git a/src/main/observability/instrumentation.ts b/src/main/observability/instrumentation.ts index 3d8c9246d6f..3e944b23847 100644 --- a/src/main/observability/instrumentation.ts +++ b/src/main/observability/instrumentation.ts @@ -207,10 +207,19 @@ export async function withWorktreeSpan( ) } +/** 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( - stage: string, + stage: WorktreeRemoveStage, flow: 'folder' | 'remote' | 'local', fn: () => Promise ): Promise { diff --git a/src/main/opencode-usage/store.ts b/src/main/opencode-usage/store.ts index 8bf4c946fac..f0e7709aa90 100644 --- a/src/main/opencode-usage/store.ts +++ b/src/main/opencode-usage/store.ts @@ -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 } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index afd5e364284..f80452ef352 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -7168,11 +7168,21 @@ export class OrcaRuntimeService { connectionId?: string, deadline: WatcherRemovalDeadline = createWatcherRemovalDeadline() ): Promise => { + // 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' diff --git a/src/main/terminal-history-deletion.ts b/src/main/terminal-history-deletion.ts index c3411c8b030..093928a4e51 100644 --- a/src/main/terminal-history-deletion.ts +++ b/src/main/terminal-history-deletion.ts @@ -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 { +/** 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 { + scheduleAllPendingHistoryTreeRemovals() await Promise.all(pendingHistoryTreeRemovals.values()) } diff --git a/src/main/terminal-history.test.ts b/src/main/terminal-history.test.ts index f146fd79836..6285a634bd2 100644 --- a/src/main/terminal-history.test.ts +++ b/src/main/terminal-history.test.ts @@ -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('node:fs/promises')), rm: rmAsyncMock })) diff --git a/tools/benchmarks/worktree-deletion-dev-bench.mjs b/tools/benchmarks/worktree-deletion-dev-bench.mjs index 317c25b1c02..e00e5423ee6 100644 --- a/tools/benchmarks/worktree-deletion-dev-bench.mjs +++ b/tools/benchmarks/worktree-deletion-dev-bench.mjs @@ -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) } }