From 638c3ca5d958752265b0ff6744b8cdf89e794238 Mon Sep 17 00:00:00 2001 From: waryan <60338207+forwaryan@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:26:40 +0800 Subject: [PATCH 1/7] fix(agent-status): prevent ghost sidebar row on completed split-pane detach (#10698) * fix(agent-status): prevent ghost sidebar row on completed split-pane detach Detaching a done-state split pane into its own tab migrated the agent paneKey from oldTab:leaf to newTab:leaf. useRetainedAgentsSync only saw the old key vanish and, finding no suppressor, resurrected it as an unclickable duplicate sidebar row (and inflated the worktree count). Plant a one-shot retention suppressor on the source key during transferAgentPaneAuthority, but only when the source actually held a live agent, so a suppressor is never leaked for a pane that had none. Fixes #10675 Co-Authored-By: Claude * fix(agent-status): annotate suppressor record type and condense retention comments Type the migrated retentionSuppressedPaneKeys as Record so a computed-key `true` isn't widened to boolean, which broke the web typecheck. Also condense the retention rationale comments per review. Co-Authored-By: Claude --------- Co-authored-by: Claude --- src/renderer/src/store/slices/agent-status.ts | 105 +++++++------ ...erminal-pane-detach-agent-identity.test.ts | 4 +- ...rminal-pane-detach-agent-retention.test.ts | 140 ++++++++++++++++++ 3 files changed, 205 insertions(+), 44 deletions(-) create mode 100644 src/renderer/src/store/slices/terminal-pane-detach-agent-retention.test.ts diff --git a/src/renderer/src/store/slices/agent-status.ts b/src/renderer/src/store/slices/agent-status.ts index f49aec0428f..90603abb19b 100644 --- a/src/renderer/src/store/slices/agent-status.ts +++ b/src/renderer/src/store/slices/agent-status.ts @@ -1303,50 +1303,69 @@ export const createAgentStatusSlice: StateCreator ({ - agentStatusByPaneKey: movePaneKeyedRecord(s.agentStatusByPaneKey, from, to, (entry) => ({ - ...entry, - paneKey: to, - tabId: targetTabId - })), - runtimeAgentOrchestrationByPaneKey: movePaneKeyedRecord( - s.runtimeAgentOrchestrationByPaneKey, - from, - to - ), - retainedAgentsByPaneKey: movePaneKeyedRecord( - s.retainedAgentsByPaneKey, - from, - to, - (retained) => ({ - ...retained, - entry: { ...retained.entry, paneKey: to, tabId: targetTabId }, - tab: targetTabId ? { ...retained.tab, id: targetTabId } : retained.tab - }) - ), - sleepingAgentSessionsByPaneKey: movePaneKeyedRecord( - s.sleepingAgentSessionsByPaneKey, - from, - to, - (record) => ({ ...record, paneKey: to, tabId: targetTabId }) - ), - agentLaunchConfigByPaneKey: movePaneKeyedRecord( - s.agentLaunchConfigByPaneKey, - from, - to, - (entry) => ({ + set((s) => { + // Plant a one-shot suppressor on the source key so useRetainedAgentsSync — + // which only sees `from` vanish, not the migration — can't resurrect a ghost row. + const movedSuppressors = movePaneKeyedRecord(s.retentionSuppressedPaneKeys, from, to) + // Guard on fromWasLive: a suppressor is consumed only on a live→gone transition, else it leaks. + const fromWasLive = from in s.agentStatusByPaneKey + const retentionSuppressedPaneKeys: Record = + fromWasLive && !(from in movedSuppressors) + ? { ...movedSuppressors, [from]: true } + : movedSuppressors + return { + agentStatusByPaneKey: movePaneKeyedRecord(s.agentStatusByPaneKey, from, to, (entry) => ({ ...entry, - identity: { ...entry.identity, tabId: targetTabId, leafId: targetLeafId } - }) - ), - acknowledgedAgentsByPaneKey: movePaneKeyedRecord(s.acknowledgedAgentsByPaneKey, from, to), - paneForegroundAgentByPaneKey: movePaneKeyedRecord(s.paneForegroundAgentByPaneKey, from, to), - unreadTerminalPanes: movePaneKeyedRecord(s.unreadTerminalPanes, from, to), - unreadAgentCompletionPanes: movePaneKeyedRecord(s.unreadAgentCompletionPanes, from, to), - lastTerminalInputAtByPaneKey: movePaneKeyedRecord(s.lastTerminalInputAtByPaneKey, from, to), - cacheTimerByKey: movePaneKeyedRecord(s.cacheTimerByKey, from, to), - retentionSuppressedPaneKeys: movePaneKeyedRecord(s.retentionSuppressedPaneKeys, from, to) - })) + paneKey: to, + tabId: targetTabId + })), + runtimeAgentOrchestrationByPaneKey: movePaneKeyedRecord( + s.runtimeAgentOrchestrationByPaneKey, + from, + to + ), + retainedAgentsByPaneKey: movePaneKeyedRecord( + s.retainedAgentsByPaneKey, + from, + to, + (retained) => ({ + ...retained, + entry: { ...retained.entry, paneKey: to, tabId: targetTabId }, + tab: targetTabId ? { ...retained.tab, id: targetTabId } : retained.tab + }) + ), + sleepingAgentSessionsByPaneKey: movePaneKeyedRecord( + s.sleepingAgentSessionsByPaneKey, + from, + to, + (record) => ({ ...record, paneKey: to, tabId: targetTabId }) + ), + agentLaunchConfigByPaneKey: movePaneKeyedRecord( + s.agentLaunchConfigByPaneKey, + from, + to, + (entry) => ({ + ...entry, + identity: { ...entry.identity, tabId: targetTabId, leafId: targetLeafId } + }) + ), + acknowledgedAgentsByPaneKey: movePaneKeyedRecord(s.acknowledgedAgentsByPaneKey, from, to), + paneForegroundAgentByPaneKey: movePaneKeyedRecord( + s.paneForegroundAgentByPaneKey, + from, + to + ), + unreadTerminalPanes: movePaneKeyedRecord(s.unreadTerminalPanes, from, to), + unreadAgentCompletionPanes: movePaneKeyedRecord(s.unreadAgentCompletionPanes, from, to), + lastTerminalInputAtByPaneKey: movePaneKeyedRecord( + s.lastTerminalInputAtByPaneKey, + from, + to + ), + cacheTimerByKey: movePaneKeyedRecord(s.cacheTimerByKey, from, to), + retentionSuppressedPaneKeys + } + }) if (typeof window !== 'undefined') { window.api?.agentStatus?.transferPaneAuthority?.({ fromPaneKey: from, diff --git a/src/renderer/src/store/slices/terminal-pane-detach-agent-identity.test.ts b/src/renderer/src/store/slices/terminal-pane-detach-agent-identity.test.ts index 6d12b987b30..6e9c0541e17 100644 --- a/src/renderer/src/store/slices/terminal-pane-detach-agent-identity.test.ts +++ b/src/renderer/src/store/slices/terminal-pane-detach-agent-identity.test.ts @@ -103,7 +103,9 @@ describe('syncPaneDetachPtyOwnership agent identity', () => { tabId: targetTabId, providerSession: { key: 'session_id', id: 'session-1' } }) - expect(state.retentionSuppressedPaneKeys[sourcePaneKey]).toBeUndefined() + // Why: detach plants a one-shot suppressor on the source key so the sidebar + // retention hook can't resurrect the migrated pane as an unclickable ghost row. + expect(state.retentionSuppressedPaneKeys[sourcePaneKey]).toBe(true) expect(state.paneForegroundAgentByPaneKey[siblingPaneKey]).toEqual({ agent: 'antigravity', shellForeground: false diff --git a/src/renderer/src/store/slices/terminal-pane-detach-agent-retention.test.ts b/src/renderer/src/store/slices/terminal-pane-detach-agent-retention.test.ts new file mode 100644 index 00000000000..3c0cabdf052 --- /dev/null +++ b/src/renderer/src/store/slices/terminal-pane-detach-agent-retention.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it } from 'vitest' +import { makePaneKey } from '../../../../shared/stable-pane-id' +import { collectRetainedAgentsOnDisappear } from '@/components/dashboard/useRetainedAgents' +import { createTestStore, makeTab, makeWorktree, seedStore } from './store-test-helpers' +import type { AgentStatusEntry } from '../../../../shared/agent-status-types' +import type { DashboardAgentRow } from '@/components/dashboard/useDashboardData' + +// Detaching a completed split pane migrates the paneKey `sourceTab:leaf → targetTab:leaf`; +// these tests pin the store→hook contract that stops the sidebar resurrecting a ghost row. + +const WORKTREE_ID = 'repo::/repo/worktree' +const SOURCE_TAB = 'tab-source' +const TARGET_TAB = 'tab-target' +const LEAF = '11111111-1111-4111-8111-111111111111' +const SIBLING_LEAF = '22222222-2222-4222-8222-222222222222' +const SOURCE_PANE_KEY = makePaneKey(SOURCE_TAB, LEAF) +const TARGET_PANE_KEY = makePaneKey(TARGET_TAB, LEAF) + +function makeDoneEntry(paneKey: string, tabId: string): AgentStatusEntry { + return { + state: 'done', + prompt: 'Fix it', + updatedAt: 100, + stateStartedAt: 100, + paneKey, + tabId, + terminalTitle: 'Claude', + stateHistory: [], + agentType: 'claude', + interrupted: false + } +} + +function makeRow(paneKey: string, tabId: string): DashboardAgentRow { + return { + paneKey, + entry: makeDoneEntry(paneKey, tabId), + tab: makeTab({ id: tabId, worktreeId: WORKTREE_ID }), + agentType: 'claude', + state: 'done', + startedAt: 100 + } +} + +function detachDoneAgentAndReadStore() { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { + repo: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo', path: '/repo/worktree' })] + }, + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ id: SOURCE_TAB, worktreeId: WORKTREE_ID, ptyId: 'pty-a' }), + makeTab({ id: TARGET_TAB, worktreeId: WORKTREE_ID, ptyId: null }) + ] + }, + ptyIdsByTabId: { [SOURCE_TAB]: ['pty-a', 'pty-sibling'], [TARGET_TAB]: [] } + }) + store.getState().setAgentStatus(SOURCE_PANE_KEY, { + state: 'done', + prompt: 'Fix it', + agentType: 'claude' + }) + store.getState().syncPaneDetachPtyOwnership({ + detachedLeafId: LEAF, + detachedPtyId: 'pty-a', + sourceLayout: { + root: { type: 'leaf', leafId: SIBLING_LEAF }, + activeLeafId: SIBLING_LEAF, + expandedLeafId: null, + ptyIdsByLeafId: { [SIBLING_LEAF]: 'pty-sibling' } + }, + sourceTabId: SOURCE_TAB, + targetTabId: TARGET_TAB + }) + return store.getState() +} + +describe('detach completed split pane → sidebar retention', () => { + it('plants a source-key suppressor so the retention hook drops the migrated pane', () => { + const state = detachDoneAgentAndReadStore() + + // Status migrated to the new tab's pane key. + expect(state.agentStatusByPaneKey[SOURCE_PANE_KEY]).toBeUndefined() + expect(state.agentStatusByPaneKey[TARGET_PANE_KEY]?.state).toBe('done') + + // The retention hook sees the old key vanish (prevRef held source, current + // holds target). Feed it the REAL post-detach suppressor state. + const result = collectRetainedAgentsOnDisappear({ + previousAgents: new Map([ + [SOURCE_PANE_KEY, { row: makeRow(SOURCE_PANE_KEY, SOURCE_TAB), worktreeId: WORKTREE_ID }] + ]), + currentAgents: new Map([ + [TARGET_PANE_KEY, { row: makeRow(TARGET_PANE_KEY, TARGET_TAB), worktreeId: WORKTREE_ID }] + ]), + retainedAgentsByPaneKey: {}, + retentionSuppressedPaneKeys: state.retentionSuppressedPaneKeys, + recentlyClosedAgentStatusTabIds: {} + }) + + // No ghost row; the one-shot suppressor is consumed instead of retained. + expect(result.toRetain).toEqual([]) + expect(result.consumedSuppressedPaneKeys).toEqual([SOURCE_PANE_KEY]) + }) + + it('does not plant a suppressor when the detached pane never had a live agent', () => { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { + repo: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo', path: '/repo/worktree' })] + }, + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ id: SOURCE_TAB, worktreeId: WORKTREE_ID, ptyId: 'pty-a' }), + makeTab({ id: TARGET_TAB, worktreeId: WORKTREE_ID, ptyId: null }) + ] + }, + ptyIdsByTabId: { [SOURCE_TAB]: ['pty-a', 'pty-sibling'], [TARGET_TAB]: [] } + }) + + // No setAgentStatus here: a plain terminal pane with no agent. + store.getState().syncPaneDetachPtyOwnership({ + detachedLeafId: LEAF, + detachedPtyId: 'pty-a', + sourceLayout: { + root: { type: 'leaf', leafId: SIBLING_LEAF }, + activeLeafId: SIBLING_LEAF, + expandedLeafId: null, + ptyIdsByLeafId: { [SIBLING_LEAF]: 'pty-sibling' } + }, + sourceTabId: SOURCE_TAB, + targetTabId: TARGET_TAB + }) + + // Why: a suppressor is only consumed on a live→gone transition. The source + // key was never live, so planting one here would leak it forever. + expect(store.getState().retentionSuppressedPaneKeys[SOURCE_PANE_KEY]).toBeUndefined() + expect(store.getState().retentionSuppressedPaneKeys[TARGET_PANE_KEY]).toBeUndefined() + }) +}) From a6423d565b698e6ac43d8eb2e4e42d49170e69e4 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:37:13 -0700 Subject: [PATCH 2/7] fix(macos): prevent stale UI surfaces after wake (#11226) Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- src/main/window/createMainWindow.test.ts | 57 +--- src/main/window/createMainWindow.ts | 19 +- .../window/renderer-viewport-reflow.test.ts | 276 ------------------ src/main/window/renderer-viewport-reflow.ts | 107 ------- src/renderer/index.html | 2 +- src/renderer/popout.html | 2 +- src/renderer/src/App.tsx | 2 +- src/renderer/src/assets/main.css | 13 +- .../src/web/web-viewport-shell.test.ts | 21 +- 9 files changed, 47 insertions(+), 452 deletions(-) delete mode 100644 src/main/window/renderer-viewport-reflow.test.ts delete mode 100644 src/main/window/renderer-viewport-reflow.ts diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 013711e9e38..fc38ca95e77 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -364,7 +364,7 @@ describe('createMainWindow', () => { } }) - it('keeps main-window background throttling enabled while repainting macOS visibility transitions', () => { + it('keeps macOS background throttling enabled while repainting visibility transitions', () => { vi.useFakeTimers() const windowHandlers = new Map void)[]>() let windowSize: [number, number] = [1200, 800] @@ -405,11 +405,8 @@ describe('createMainWindow', () => { withPlatform('darwin', () => createMainWindow(null)) - // Why: throttling-off pins visibilityState 'visible' and renders occluded - // windows at full rate; this guards against reintroducing it. - expect(webContents.setBackgroundThrottling).not.toHaveBeenCalledWith(false) - expect(webContents.setBackgroundThrottling).toHaveBeenCalledWith(true) + expect(webContents.setBackgroundThrottling).not.toHaveBeenCalledWith(false) expect(windowHandlers.get('restore')).toHaveLength(1) expect(windowHandlers.get('show')).toHaveLength(1) expect(windowHandlers.get('focus')).toHaveLength(1) @@ -493,8 +490,7 @@ describe('createMainWindow', () => { expect(browserWindowInstance.setSize).not.toHaveBeenCalled() expect(webContents.invalidate).not.toHaveBeenCalled() - // The genuine reveal (matching sender) runs the full repaint: invalidate + the size jiggle - // that recomputes the stale dvh layout — the recovery bare focus/invalidate misses. + // The genuine reveal runs the pre-Tahoe compositor jiggle that bare focus avoids. revealHandler?.({ sender: webContents } as never) expect(webContents.invalidate).toHaveBeenCalledTimes(1) // Why: the nudge is deferred off the event dispatch turn. @@ -535,9 +531,7 @@ describe('createMainWindow', () => { send: vi.fn(), isDevToolsOpened: vi.fn(), openDevTools: vi.fn(), - closeDevTools: vi.fn(), - enableDeviceEmulation: vi.fn(), - disableDeviceEmulation: vi.fn() + closeDevTools: vi.fn() } const browserWindowInstance = { webContents, @@ -550,8 +544,6 @@ describe('createMainWindow', () => { isMaximized: vi.fn(() => false), isFullScreen: vi.fn(() => false), getSize: vi.fn(() => [1200, 800]), - getContentSize: vi.fn(() => [1200, 800]), - getBounds: vi.fn(() => ({ x: 0, y: 0, width: 1200, height: 840 })), setSize: vi.fn(), maximize: vi.fn(), show: vi.fn(), @@ -571,21 +563,9 @@ describe('createMainWindow', () => { vi.advanceTimersByTime(300) expect(webContents.invalidate).toHaveBeenCalledTimes(2) expect(browserWindowInstance.setSize).not.toHaveBeenCalled() - - // Why (STA-2383): invalidate repaints but never reflows, so Tahoe still has to recompute the - // dvh root — via the emulated viewport, which leaves the deadlock-prone frame untouched. - expect(webContents.enableDeviceEmulation).toHaveBeenCalledWith({ - screenPosition: 'desktop', - screenSize: { width: 0, height: 0 }, - deviceScaleFactor: 2.25, - viewSize: { width: 1200, height: 800 }, - scale: 1 - }) - expect(webContents.disableDeviceEmulation).toHaveBeenCalled() - expect(browserWindowInstance.setSize).not.toHaveBeenCalled() }) - it('still reflows a maximized macOS 26 window, which the size nudge had to skip', () => { + it('invalidates a maximized macOS 26 window without changing its frame', () => { vi.useFakeTimers() macosTahoeMock.value = true const windowHandlers = new Map void)[]>() @@ -599,9 +579,7 @@ describe('createMainWindow', () => { send: vi.fn(), isDevToolsOpened: vi.fn(), openDevTools: vi.fn(), - closeDevTools: vi.fn(), - enableDeviceEmulation: vi.fn(), - disableDeviceEmulation: vi.fn() + closeDevTools: vi.fn() } const browserWindowInstance = { webContents, @@ -611,13 +589,9 @@ describe('createMainWindow', () => { windowHandlers.set(event, handlers) }), isDestroyed: vi.fn(() => false), - // Why: the maximized/fullscreen bail-out only ever protected setSize from un-maximizing - // the window; emulation leaves the frame alone, so the reflow must still happen here. isMaximized: vi.fn(() => true), isFullScreen: vi.fn(() => true), getSize: vi.fn(() => [1200, 800]), - getContentSize: vi.fn(() => [1200, 800]), - getBounds: vi.fn(() => ({ x: 0, y: 0, width: 1200, height: 840 })), setSize: vi.fn(), maximize: vi.fn(), show: vi.fn(), @@ -633,12 +607,11 @@ describe('createMainWindow', () => { windowHandlers.get('show')?.[0]?.() vi.advanceTimersByTime(300) - expect(webContents.enableDeviceEmulation).toHaveBeenCalled() - expect(webContents.disableDeviceEmulation).toHaveBeenCalled() + expect(webContents.invalidate).toHaveBeenCalledTimes(2) expect(browserWindowInstance.setSize).not.toHaveBeenCalled() }) - it('reflows without the size nudge when macOS 26 wakes from sleep', () => { + it('invalidates without frame or device emulation when macOS 26 wakes from sleep', () => { vi.useFakeTimers() macosTahoeMock.value = true const windowHandlers = new Map void)[]>() @@ -667,8 +640,6 @@ describe('createMainWindow', () => { isMaximized: vi.fn(() => false), isFullScreen: vi.fn(() => false), getSize: vi.fn(() => [1200, 800]), - getContentSize: vi.fn(() => [1200, 800]), - getBounds: vi.fn(() => ({ x: 0, y: 0, width: 1200, height: 840 })), setSize: vi.fn(), maximize: vi.fn(), show: vi.fn(), @@ -681,8 +652,6 @@ describe('createMainWindow', () => { withPlatform('darwin', () => createMainWindow(null)) - // Why: 'resume' is the other AppKit dispatch context implicated in the 109-minute freeze, - // so it must take the frame-free path too — not just show/restore. const resumeHandler = powerMonitorOnMock.mock.calls.find( ([event]) => event === 'resume' )?.[1] as (() => void) | undefined @@ -692,14 +661,8 @@ describe('createMainWindow', () => { expect(webContents.invalidate).toHaveBeenCalled() vi.advanceTimersByTime(300) expect(browserWindowInstance.setSize).not.toHaveBeenCalled() - expect(webContents.enableDeviceEmulation).toHaveBeenCalledWith({ - screenPosition: 'desktop', - screenSize: { width: 0, height: 0 }, - deviceScaleFactor: 2.25, - viewSize: { width: 1200, height: 800 }, - scale: 1 - }) - expect(webContents.disableDeviceEmulation).toHaveBeenCalled() + expect(webContents.enableDeviceEmulation).not.toHaveBeenCalled() + expect(webContents.disableDeviceEmulation).not.toHaveBeenCalled() }) it('supports all minus key variants for terminal zoom out', () => { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index ffed4d013c2..519e1817051 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -49,7 +49,6 @@ import { rectHasVisibleAreaOnAnyDisplay } from './window-bounds-validation' import { closeDashboardPopout } from './dashboard-popout-window' import { installPrivilegedWindowNavigationPolicy } from './privileged-window-navigation' import { isMacosTahoeOrNewer } from './macos-tahoe-release' -import { reflowRendererViewport } from './renderer-viewport-reflow' import { registerPluginPanelNavigationGuard } from '../plugins/plugin-panel-navigation-guard' // Why: show/restore/resume can overlap before the size nudge resets; never capture the temporary width as the next baseline. @@ -62,12 +61,9 @@ function forceRepaint(window: BrowserWindow): void { return } window.webContents.invalidate() - // Why: macOS 26 scene-backed windows deadlock the main thread on frame mutation, but invalidate - // alone never reflows the dvh root (STA-2383); emulation reflows without touching the frame. - // Runs before the maximized/fullscreen bail-out below, which only exists to protect setSize — - // emulation leaves those states intact, and a maximized window goes stale just the same. + // Why: macOS 26 scene-backed windows deadlock on frame mutation, and device emulation can + // strand the compositor after wake. The native shell no longer relies on dvh reflow. if (isMacosTahoeOrNewer()) { - reflowRendererViewport(window) return } if (window.isMaximized() || window.isFullScreen() || activeRepaintJiggles.has(window)) { @@ -125,10 +121,8 @@ function installMacosVisibilityRepaint(window: BrowserWindow): void { } } - // Why (STA-2383): occlusion-uncover fires no restore/show, so the renderer relays its genuine - // hidden→visible reveal instead. Unlike a bare focus (every Cmd+Tab, window never hidden), this - // only fires when the window was actually occluded/throttled — exactly when the stale dvh layout - // stranded the bottom status bar off-screen — so the full repaint's size jiggle is warranted. + // Why: occlusion reveal can fire no restore/show, so preserve the renderer relay without + // trusting events from another window. const onRendererRevealed = (event: Electron.IpcMainEvent): void => { if (window.isDestroyed() || window.webContents.isDestroyed()) { return @@ -142,7 +136,7 @@ function installMacosVisibilityRepaint(window: BrowserWindow): void { window.on('restore', repaintAfterVisibilityTransition) window.on('show', repaintAfterVisibilityTransition) - // Why: occlusion-uncover fires no restore/show, only focus; invalidate only — the setSize jiggle would SIGWINCH every terminal on Cmd+Tab. The renderer-reveal relay above covers the stale-layout recovery that invalidate alone misses. + // Why: occlusion-uncover can fire only focus; invalidate without resizing terminals on Cmd+Tab. window.on('focus', () => { if (!window.isDestroyed() && !window.webContents.isDestroyed()) { window.webContents.invalidate() @@ -303,7 +297,8 @@ export function createMainWindow( setTrustedUIRendererWebContentsId(rendererWebContentsId) if (process.platform === 'darwin') { - // Why: throttle the main window while hidden (guests self-unthrottle); toggle only while visible or Chromium blanks the surface (electron#42378). + // Why: preserve hidden-window power savings; stable native sizing and frame-only invalidation + // make wake recovery independent of the throttled viewport. mainWindow.webContents.setBackgroundThrottling(true) installMacosVisibilityRepaint(mainWindow) } diff --git a/src/main/window/renderer-viewport-reflow.test.ts b/src/main/window/renderer-viewport-reflow.test.ts deleted file mode 100644 index a9c83595e51..00000000000 --- a/src/main/window/renderer-viewport-reflow.test.ts +++ /dev/null @@ -1,276 +0,0 @@ -import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' -import type { BrowserWindow } from 'electron' - -const scaleFactorMock = { value: 1 } -vi.mock('electron', () => ({ - screen: { getDisplayMatching: vi.fn(() => ({ scaleFactor: scaleFactorMock.value })) } -})) - -import { - clearCrashBreadcrumbsForTest, - getCrashBreadcrumbSnapshot -} from '../crash-reporting/crash-breadcrumb-store' -import { - reflowRendererViewport, - VIEWPORT_REFLOW_RESTORE_ATTEMPTS, - VIEWPORT_REFLOW_SETTLE_MS -} from './renderer-viewport-reflow' - -function createWindow(overrides: Record = {}): BrowserWindow { - const webContents = { - isDestroyed: vi.fn(() => false), - enableDeviceEmulation: vi.fn(), - disableDeviceEmulation: vi.fn() - } - return { - webContents, - isDestroyed: vi.fn(() => false), - getContentSize: vi.fn(() => [1200, 800]), - getBounds: vi.fn(() => ({ x: 0, y: 0, width: 1200, height: 840 })), - setSize: vi.fn(), - setBounds: vi.fn(), - ...overrides - } as unknown as BrowserWindow -} - -describe('reflowRendererViewport', () => { - beforeEach(() => { - vi.useFakeTimers() - scaleFactorMock.value = 1 - clearCrashBreadcrumbsForTest() - }) - afterEach(() => { - vi.useRealTimers() - }) - - it('never mutates the native window frame', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.setSize).not.toHaveBeenCalled() - expect(window.setBounds).not.toHaveBeenCalled() - }) - - it('defers the emulation off the caller stack, then restores the real viewport', () => { - const window = createWindow() - reflowRendererViewport(window) - // Why: nothing may run while AppKit's callout frame is still on the stack. - expect(window.webContents.enableDeviceEmulation).not.toHaveBeenCalled() - - vi.advanceTimersByTime(0) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledWith({ - screenPosition: 'desktop', - screenSize: { width: 0, height: 0 }, - deviceScaleFactor: 1.25, - viewSize: { width: 1200, height: 800 }, - scale: 1 - }) - expect(window.webContents.disableDeviceEmulation).not.toHaveBeenCalled() - - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(1) - }) - - // Why: a CSS-pixel delta re-grids terminals sitting on an xterm row boundary, so the emulated - // viewport must match the real one exactly — the scale factor is the only thing allowed to move. - it('emulates the real viewport size so no terminal re-grids mid-reflow', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(0) - const [params] = (window.webContents.enableDeviceEmulation as ReturnType).mock - .calls[0] as [Electron.Parameters] - expect(params.viewSize).toEqual({ width: 1200, height: 800 }) - expect(params.scale).toBe(1) - }) - - // Why: Blink applies screenSize/viewPosition ahead of the desktop branch, so overriding them - // moves screen.width and window.screenX for the whole hold — a context menu opened mid-reflow - // would land at the wrong coordinates. Only the scale factor may move. - it('leaves screen geometry and window position untouched while emulating', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(0) - const [params] = (window.webContents.enableDeviceEmulation as ReturnType).mock - .calls[0] as [Electron.Parameters] - expect(params.screenSize).toEqual({ width: 0, height: 0 }) - expect(params).not.toHaveProperty('viewPosition') - }) - - // Why: a constant scale factor would equal the real one on a 1.25x display and reflow nothing. - it('offsets from the display the window is actually on', () => { - scaleFactorMock.value = 1.25 - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(0) - const [params] = (window.webContents.enableDeviceEmulation as ReturnType).mock - .calls[0] as [Electron.Parameters] - expect(params.deviceScaleFactor).toBe(1.5) - }) - - // Why: a display reporting 0 would emulate 0.25 and shrink everything. - it('falls back to 1x when the display reports no scale factor', () => { - scaleFactorMock.value = 0 - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(0) - const [params] = (window.webContents.enableDeviceEmulation as ReturnType).mock - .calls[0] as [Electron.Parameters] - expect(params.deviceScaleFactor).toBe(1.25) - }) - - it('collapses a burst of reveals into one emulation cycle', () => { - const window = createWindow() - for (let i = 0; i < 5; i += 1) { - reflowRendererViewport(window) - } - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(1) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(1) - }) - - it('re-arms once the cycle finishes', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(2) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(2) - }) - - it('skips a window destroyed before the deferred turn runs', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.mocked(window.isDestroyed).mockReturnValue(true) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).not.toHaveBeenCalled() - }) - - it('skips a webContents destroyed before the deferred turn runs', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.mocked(window.webContents.isDestroyed).mockReturnValue(true) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).not.toHaveBeenCalled() - }) - - it('does not leave emulation on when the window dies mid-cycle', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(0) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(1) - - vi.mocked(window.isDestroyed).mockReturnValue(true) - expect(() => vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS)).not.toThrow() - expect(window.webContents.disableDeviceEmulation).not.toHaveBeenCalled() - - // Why: a stuck latch would silently disable every later reflow for this window. - vi.mocked(window.isDestroyed).mockReturnValue(false) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(2) - }) - - it('recovers the latch when enabling emulation throws', () => { - const window = createWindow() - vi.mocked(window.webContents.enableDeviceEmulation).mockImplementationOnce(() => { - throw new Error('Object has been destroyed') - }) - reflowRendererViewport(window) - expect(() => vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1)).not.toThrow() - // Why: nothing was applied, so no restore should be attempted. - expect(window.webContents.disableDeviceEmulation).not.toHaveBeenCalled() - - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(2) - }) - - it('retries the restore rather than stranding a live renderer at the overshot viewport', () => { - const window = createWindow() - vi.mocked(window.webContents.disableDeviceEmulation).mockImplementationOnce(() => { - throw new Error('transient failure') - }) - reflowRendererViewport(window) - expect(() => vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1)).not.toThrow() - - // Why: the webContents is still alive, so the emulated viewport is still applied — giving up - // here would leave the renderer stuck at the overshot height forever. - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(1) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(2) - - // Why: the retry succeeded, so the latch is free and later reveals still reflow. - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(2) - }) - - it('holds the latch while a restore is still being retried', () => { - const window = createWindow() - vi.mocked(window.webContents.disableDeviceEmulation).mockImplementation(() => { - throw new Error('still failing') - }) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - - // Why: stacking a second emulation over an unrestored viewport would compound the overshoot. - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(1) - }) - - it('stops retrying the restore and re-arms after the attempt budget', () => { - const window = createWindow() - vi.mocked(window.webContents.disableDeviceEmulation).mockImplementation(() => { - throw new Error('always fails') - }) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS * (VIEWPORT_REFLOW_RESTORE_ATTEMPTS + 4)) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes( - VIEWPORT_REFLOW_RESTORE_ATTEMPTS + 1 - ) - - // Why: an unbounded retry would pin the latch and silently kill every later reflow. - vi.mocked(window.webContents.disableDeviceEmulation).mockImplementation(() => undefined) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.enableDeviceEmulation).toHaveBeenCalledTimes(2) - }) - - it('leaves evidence when it gives up with the viewport still emulated', () => { - const window = createWindow() - vi.mocked(window.webContents.disableDeviceEmulation).mockImplementation(() => { - throw new Error('always fails') - }) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS * (VIEWPORT_REFLOW_RESTORE_ATTEMPTS + 4)) - - expect(getCrashBreadcrumbSnapshot().map((crumb) => crumb.name)).toEqual([ - 'viewport_reflow_restore_failed' - ]) - }) - - it('records nothing when the restore succeeds', () => { - const window = createWindow() - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS * (VIEWPORT_REFLOW_RESTORE_ATTEMPTS + 4)) - - expect(getCrashBreadcrumbSnapshot()).toEqual([]) - }) - - it('abandons the restore once the webContents is gone', () => { - const window = createWindow() - vi.mocked(window.webContents.disableDeviceEmulation).mockImplementation(() => { - throw new Error('always fails') - }) - reflowRendererViewport(window) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS + 1) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(1) - - // Why: a destroyed renderer takes its emulated viewport with it; retrying is pointless. - vi.mocked(window.webContents.isDestroyed).mockReturnValue(true) - vi.advanceTimersByTime(VIEWPORT_REFLOW_SETTLE_MS * 5) - expect(window.webContents.disableDeviceEmulation).toHaveBeenCalledTimes(1) - }) -}) diff --git a/src/main/window/renderer-viewport-reflow.ts b/src/main/window/renderer-viewport-reflow.ts deleted file mode 100644 index e4d7450229f..00000000000 --- a/src/main/window/renderer-viewport-reflow.ts +++ /dev/null @@ -1,107 +0,0 @@ -import { screen } from 'electron' -import type { BrowserWindow } from 'electron' -import { recordCrashBreadcrumb } from '../crash-reporting/crash-breadcrumb-store' - -// Why: long enough for the emulated scale factor to reach the renderer and drive a relayout. -export const VIEWPORT_REFLOW_SETTLE_MS = 32 - -// Why: any delta re-runs layout; 0.25 stays clear of float-compare noise against the real factor. -const VIEWPORT_REFLOW_SCALE_DELTA = 0.25 - -// Why: a restore that keeps failing on a live webContents would loop forever; give up and -// release the latch so a later reveal can try a fresh cycle. -export const VIEWPORT_REFLOW_RESTORE_ATTEMPTS = 3 - -// Why: overlapping reveals must not stack emulation calls; the outer one owns the restore. -const activeViewportReflows = new WeakSet() - -function isWindowGone(window: BrowserWindow): boolean { - return window.isDestroyed() || window.webContents.isDestroyed() -} - -/** - * Force the renderer to recompute its viewport without touching the native window frame. - * - * Why: `webContents.invalidate()` repaints but never reflows, so a stale `dvh` root keeps the - * status bar clipped off-screen (STA-2383). The frame jiggle that used to fix that mutates - * NSWindow, which self-deadlocks the main thread on macOS 26's scene-backed windows. Device - * emulation drives the same resize through the compositor instead — real reflow, no scene update. - * - * Why scale factor and not a +1px viewport: a one-pixel height delta changes the CSS box, so a - * terminal sitting just under an xterm row boundary gains a row, and the pane fit observer reads - * that transient grid as stable and forwards a real PTY resize — then reverses it on restore. - * Measured 3/54 window heights SIGWINCHing twice per reveal. A scale-factor delta re-runs layout - * with byte-identical CSS geometry: 0/54, with no WebGL atlas rebuild or context loss. - */ -export function reflowRendererViewport(window: BrowserWindow): void { - if (activeViewportReflows.has(window) || isWindowGone(window)) { - return - } - activeViewportReflows.add(window) - // Why: reveal/resume fire from inside AppKit's dispatch; start on a fresh turn so nothing - // native runs while that callout frame is still on the stack. - setTimeout(() => { - if (isWindowGone(window)) { - activeViewportReflows.delete(window) - return - } - let emulating = false - try { - const [width, height] = window.getContentSize() - // Why: the real viewport, unchanged — only the scale factor moves. - const viewSize = { width, height } - // Why: emulating the current factor is a no-op, so offset from this window's own display - // rather than a constant, which would match on a 1.25x screen and reflow nothing. - const realScaleFactor = screen.getDisplayMatching(window.getBounds()).scaleFactor || 1 - // Why: Blink reads screenSize/viewPosition before the desktop branch, so 'desktop' does - // not make them inert despite the mobile-only docs. Passing the content size and 0,0 - // moved screen.width and window.screenX for the 32ms hold, misplacing anything that - // translates screen coords (the BrowserPane context menu). Empty screenSize means "no - // override", and an omitted viewPosition stays nullopt so the real position survives -- - // Electron's types require the key, but its converter only assigns when present. - window.webContents.enableDeviceEmulation({ - screenPosition: 'desktop', - screenSize: { width: 0, height: 0 }, - deviceScaleFactor: realScaleFactor + VIEWPORT_REFLOW_SCALE_DELTA, - viewSize, - scale: 1 - } as Parameters[0]) - emulating = true - } catch { - // Why: a teardown race can destroy the webContents mid-call; nothing was applied. - } - if (!emulating) { - activeViewportReflows.delete(window) - return - } - // Why: emulation must always be undone — leaving it on strands the renderer at the wrong - // scale factor for the rest of the window's life. - restoreRealViewport(window, 0) - }, 0) -} - -function restoreRealViewport(window: BrowserWindow, attempt: number): void { - setTimeout(() => { - if (isWindowGone(window)) { - // Why: the emulated viewport dies with the webContents, so there is nothing to restore. - activeViewportReflows.delete(window) - return - } - try { - window.webContents.disableDeviceEmulation() - activeViewportReflows.delete(window) - } catch { - // Why: the webContents is still alive, so the renderer is stuck at the emulated scale; - // keep retrying and hold the latch so no second cycle stacks on the unrestored state. - if (attempt >= VIEWPORT_REFLOW_RESTORE_ATTEMPTS) { - // Why: releasing the latch beats pinning it — a later reveal can still restore. But the - // renderer is stranded at the wrong scale factor until one happens, so leave evidence - // rather than let a permanently-emulated viewport look like a rendering bug. - recordCrashBreadcrumb('viewport_reflow_restore_failed', { attempts: attempt + 1 }) - activeViewportReflows.delete(window) - return - } - restoreRealViewport(window, attempt + 1) - } - }, VIEWPORT_REFLOW_SETTLE_MS) -} diff --git a/src/renderer/index.html b/src/renderer/index.html index ba84d774e10..a481d0e1443 100644 --- a/src/renderer/index.html +++ b/src/renderer/index.html @@ -1,5 +1,5 @@ - + Orca diff --git a/src/renderer/popout.html b/src/renderer/popout.html index 944bcd23b30..a67accd3fc3 100644 --- a/src/renderer/popout.html +++ b/src/renderer/popout.html @@ -1,5 +1,5 @@ - + Orca Agent Dashboard diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 6ee3e79eaa9..26a434525a4 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -2193,7 +2193,7 @@ function App(): React.JSX.Element { return (
{ - it('uses dynamic viewport height for document and app shell containers', () => { + it('keeps dynamic viewport height as the web default', () => { const css = readSource('src/renderer/src/assets/main.css') for (const selector of ['body', '#root', '.app-layout']) { @@ -22,14 +22,29 @@ describe('web viewport shell', () => { } }) + it('uses a percentage height chain only for native Electron shells', () => { + const css = readSource('src/renderer/src/assets/main.css') + const nativeIndex = readSource('src/renderer/index.html') + const popoutIndex = readSource('src/renderer/popout.html') + const webIndex = readSource('src/renderer/web-index.html') + const appSource = readSource('src/renderer/src/App.tsx') + + expect(css).toMatch( + /html\.native-shell,\s*html\.native-shell body,\s*html\.native-shell #root,\s*html\.native-shell \.app-layout\s*\{\s*height: 100%;\s*\}/ + ) + expect(nativeIndex).toContain('') + expect(popoutIndex).toContain('') + expect(webIndex).not.toContain('native-shell') + expect(appSource).toContain('className="app-layout"') + expect(appSource).not.toContain('h-dvh') + }) + it('uses dynamic viewport Tailwind utilities for web shell entry points', () => { const source = [ - readSource('src/renderer/src/App.tsx'), readSource('src/renderer/src/web/main.tsx'), readSource('src/renderer/src/web/WebConnect.tsx') ].join('\n') - expect(source).toContain('h-dvh') expect(source).toContain('min-h-dvh') expect(source).not.toMatch(/\b(?:min-)?h-screen\b/) }) From 747b241145cf02c48819d7508015461143240b63 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:43:30 -0700 Subject: [PATCH 3/7] feat(main): record main-thread hangs so we can measure them (#10256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deadlocked main thread never crashes, so it leaves no crash report and no artifact — incidence has been unmeasurable (n=1 confirmed, macOS 26.5.1, FB24004458 / electron#52437). This forks a plain-Node watchdog sibling under ELECTRON_RUN_AS_NODE that survives the deadlock, listens for a 2s heartbeat, and after 45s of silence writes a marker to userData. The next launch consumes it, records a durable crash breadcrumb, and emits a main_thread_hang_detected telemetry event carrying unresponsive_ms and self_recovered. Observes only — it never kills or relaunches the parent. A true positive recovers nothing force-quitting wouldn't, while a false positive would SIGKILL a live main thread mid-write. self_recovered counts exactly the stalls such a killer would have gotten wrong, so recovery can be built on evidence if the field numbers justify it. macOS-only, packaged-only (ORCA_HANG_WATCHDOG_FORCE=1 to test), with sleep-gap suppression and idempotent shutdown on will-quit. --- build-plugins/plain-node-entry-guard.ts | 1 + config/electron-builder.config.cjs | 1 + .../scripts/electron-builder-config.test.mjs | 8 ++ electron.vite.config.ts | 6 + .../hang-detection-marker.test.ts | 76 ++++++++++ .../hang-watchdog/hang-detection-marker.ts | 54 +++++++ .../hang-watchdog-child-loop.test.ts | 131 +++++++++++++++++ .../hang-watchdog/hang-watchdog-child-loop.ts | 51 +++++++ .../hang-watchdog/hang-watchdog-entry-path.ts | 17 +++ .../main-thread-hang-telemetry.test.ts | 39 ++++++ .../main-thread-hang-watchdog-entry.test.ts | 106 ++++++++++++++ .../main-thread-hang-watchdog-entry.ts | 72 ++++++++++ .../main-thread-hang-watchdog.test.ts | 132 ++++++++++++++++++ .../main-thread-hang-watchdog.ts | 81 +++++++++++ src/main/index.ts | 26 ++++ src/shared/telemetry-events.ts | 13 ++ 16 files changed, 814 insertions(+) create mode 100644 src/main/hang-watchdog/hang-detection-marker.test.ts create mode 100644 src/main/hang-watchdog/hang-detection-marker.ts create mode 100644 src/main/hang-watchdog/hang-watchdog-child-loop.test.ts create mode 100644 src/main/hang-watchdog/hang-watchdog-child-loop.ts create mode 100644 src/main/hang-watchdog/hang-watchdog-entry-path.ts create mode 100644 src/main/hang-watchdog/main-thread-hang-telemetry.test.ts create mode 100644 src/main/hang-watchdog/main-thread-hang-watchdog-entry.test.ts create mode 100644 src/main/hang-watchdog/main-thread-hang-watchdog-entry.ts create mode 100644 src/main/hang-watchdog/main-thread-hang-watchdog.test.ts create mode 100644 src/main/hang-watchdog/main-thread-hang-watchdog.ts diff --git a/build-plugins/plain-node-entry-guard.ts b/build-plugins/plain-node-entry-guard.ts index a7ad171a402..6638cca97c5 100644 --- a/build-plugins/plain-node-entry-guard.ts +++ b/build-plugins/plain-node-entry-guard.ts @@ -21,6 +21,7 @@ type OutputChunk = Rollup.OutputChunk const PLAIN_NODE_ENTRY_NAMES = [ 'daemon-entry', 'parcel-watcher-process-entry', + 'main-thread-hang-watchdog-entry', 'computer-sidecar', 'agent-hooks/managed-agent-hook-controls', 'codex/codex-app-server-grant-entry' diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 0a96abb6d24..91cb45da285 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -153,6 +153,7 @@ module.exports = { 'out/main/plugin-host-entry.js', 'out/main/computer-sidecar.js', 'out/main/parcel-watcher-process-entry.js', + 'out/main/main-thread-hang-watchdog-entry.js', 'out/main/chunks/**', 'resources/**', 'node_modules/ws/**', diff --git a/config/scripts/electron-builder-config.test.mjs b/config/scripts/electron-builder-config.test.mjs index 442b7bd84f7..9712e5edace 100644 --- a/config/scripts/electron-builder-config.test.mjs +++ b/config/scripts/electron-builder-config.test.mjs @@ -166,6 +166,14 @@ describe('electron-builder config', () => { ) }) + // Why: the watchdog only arms in packaged builds, and its ELECTRON_RUN_AS_NODE + // fork resolves the entry from app.asar.unpacked — inside the asar it never runs. + it('unpacks the forked main-thread hang-watchdog entry', () => { + expect(electronBuilderConfig.asarUnpack).toEqual( + expect.arrayContaining(['out/main/main-thread-hang-watchdog-entry.js']) + ) + }) + it('uses the multi-size icon source for Linux packages', () => { expect(electronBuilderConfig.linux.icon).toBe('resources/build/icon.icns') }) diff --git a/electron.vite.config.ts b/electron.vite.config.ts index d5fb6ebd1dc..7388211487a 100644 --- a/electron.vite.config.ts +++ b/electron.vite.config.ts @@ -208,6 +208,12 @@ export const electronViteConfig: UserConfig = { // Why: forked with ELECTRON_RUN_AS_NODE so @parcel/watcher faults // can't take down the main process (issue #7547). 'parcel-watcher-process-entry': resolve('src/main/ipc/parcel-watcher-process-entry.ts'), + // Why: forked with ELECTRON_RUN_AS_NODE so it survives a deadlocked + // main thread (macOS 26 AppKit scene-update deadlock) and can record + // the stall for the next launch to report. + 'main-thread-hang-watchdog-entry': resolve( + 'src/main/hang-watchdog/main-thread-hang-watchdog-entry.ts' + ), // Why: run under ELECTRON_RUN_AS_NODE while the caller blocks on // spawnSync — codex app-server trust grants need a live event loop // but must finish before a Codex pane launch proceeds. diff --git a/src/main/hang-watchdog/hang-detection-marker.test.ts b/src/main/hang-watchdog/hang-detection-marker.test.ts new file mode 100644 index 00000000000..17dbd3716c3 --- /dev/null +++ b/src/main/hang-watchdog/hang-detection-marker.test.ts @@ -0,0 +1,76 @@ +import { mkdtempSync, rmSync, writeFileSync, existsSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + consumeHangDetectionMarker, + hangDetectionMarkerPath, + writeHangDetectionMarker +} from './hang-detection-marker' + +describe('hang detection marker', () => { + let dir: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'hang-marker-')) + }) + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }) + }) + + it('round-trips a marker and deletes it on consume', () => { + const markerPath = hangDetectionMarkerPath(dir) + writeHangDetectionMarker(markerPath, { + detectedAt: 123, + parentPid: 456, + unresponsiveMs: 45000, + selfRecovered: false + }) + expect(consumeHangDetectionMarker(markerPath)).toEqual({ + detectedAt: 123, + parentPid: 456, + unresponsiveMs: 45000, + selfRecovered: false + }) + expect(existsSync(markerPath)).toBe(false) + expect(consumeHangDetectionMarker(markerPath)).toBeNull() + }) + + it('round-trips a self-recovered marker', () => { + const markerPath = hangDetectionMarkerPath(dir) + writeHangDetectionMarker(markerPath, { + detectedAt: 1, + parentPid: 2, + unresponsiveMs: 61000, + selfRecovered: true + }) + expect(consumeHangDetectionMarker(markerPath)?.selfRecovered).toBe(true) + }) + + it('returns null for a missing marker', () => { + expect(consumeHangDetectionMarker(hangDetectionMarkerPath(dir))).toBeNull() + }) + + // Why: a marker written by the detect leg has no selfRecovered field until the resolve leg + // rewrites it, and "never resolved" is the conservative reading of its absence. + it('treats a missing selfRecovered flag as an unresolved hang', () => { + const markerPath = hangDetectionMarkerPath(dir) + writeFileSync( + markerPath, + JSON.stringify({ detectedAt: 1, parentPid: 2, unresponsiveMs: 45000 }) + ) + expect(consumeHangDetectionMarker(markerPath)?.selfRecovered).toBe(false) + }) + + it('returns null for corrupted or incomplete markers and still deletes them', () => { + const markerPath = hangDetectionMarkerPath(dir) + writeFileSync(markerPath, 'not json') + expect(consumeHangDetectionMarker(markerPath)).toBeNull() + expect(existsSync(markerPath)).toBe(false) + + writeFileSync(markerPath, JSON.stringify({ detectedAt: 1 })) + expect(consumeHangDetectionMarker(markerPath)).toBeNull() + expect(existsSync(markerPath)).toBe(false) + }) +}) diff --git a/src/main/hang-watchdog/hang-detection-marker.ts b/src/main/hang-watchdog/hang-detection-marker.ts new file mode 100644 index 00000000000..a6e6fbddd08 --- /dev/null +++ b/src/main/hang-watchdog/hang-detection-marker.ts @@ -0,0 +1,54 @@ +import { readFileSync, rmSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' + +// Why: written by the plain-Node watchdog child when main-thread heartbeats stop, and rewritten if +// they resume; consumed on the next launch to report how long the stall lasted and whether it ever +// cleared. `selfRecovered` separates a real deadlock from a long-but-survivable stall — the two are +// indistinguishable at detection time, and only the latter would have been a destructive kill. +export type HangDetectionMarker = { + detectedAt: number + parentPid: number + unresponsiveMs: number + selfRecovered: boolean +} + +export function hangDetectionMarkerPath(userDataPath: string): string { + return join(userDataPath, 'main-thread-hang.json') +} + +export function writeHangDetectionMarker(markerPath: string, marker: HangDetectionMarker): void { + writeFileSync(markerPath, JSON.stringify(marker)) +} + +export function consumeHangDetectionMarker(markerPath: string): HangDetectionMarker | null { + let raw: string + try { + raw = readFileSync(markerPath, 'utf8') + } catch { + return null + } + try { + rmSync(markerPath, { force: true }) + } catch { + // Why: a marker that cannot be deleted must not block startup; worst case is one duplicate breadcrumb. + } + try { + const parsed = JSON.parse(raw) as Partial + if ( + typeof parsed.detectedAt !== 'number' || + typeof parsed.parentPid !== 'number' || + typeof parsed.unresponsiveMs !== 'number' + ) { + return null + } + return { + detectedAt: parsed.detectedAt, + parentPid: parsed.parentPid, + unresponsiveMs: parsed.unresponsiveMs, + // Why: a marker left by the detect leg and never rewritten means the stall never cleared. + selfRecovered: parsed.selfRecovered === true + } + } catch { + return null + } +} diff --git a/src/main/hang-watchdog/hang-watchdog-child-loop.test.ts b/src/main/hang-watchdog/hang-watchdog-child-loop.test.ts new file mode 100644 index 00000000000..8d390c04e8e --- /dev/null +++ b/src/main/hang-watchdog/hang-watchdog-child-loop.test.ts @@ -0,0 +1,131 @@ +import { describe, expect, it, vi } from 'vitest' +import { createHangWatchdogChildLoop } from './hang-watchdog-child-loop' + +const TIMEOUT_MS = 45_000 +const CHECK_INTERVAL_MS = 5_000 + +function loopWithClock(startAt = 0) { + let now = startAt + const onHangDetected = vi.fn() + const onHangResolved = vi.fn() + const loop = createHangWatchdogChildLoop({ + timeoutMs: TIMEOUT_MS, + checkIntervalMs: CHECK_INTERVAL_MS, + now: () => now, + onHangDetected, + onHangResolved + }) + return { loop, onHangDetected, onHangResolved, advance: (ms: number) => (now += ms) } +} + +describe('createHangWatchdogChildLoop', () => { + it('does not fire while heartbeats keep arriving', () => { + const { loop, onHangDetected, advance } = loopWithClock() + for (let i = 0; i < 100; i++) { + advance(CHECK_INTERVAL_MS) + loop.recordHeartbeat() + loop.tick() + } + expect(onHangDetected).not.toHaveBeenCalled() + }) + + it('fires once when heartbeats stop for longer than the timeout', () => { + const { loop, onHangDetected, advance } = loopWithClock() + loop.recordHeartbeat() + for (let i = 0; i < 12; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).toHaveBeenCalledTimes(1) + advance(CHECK_INTERVAL_MS) + loop.tick() + expect(onHangDetected).toHaveBeenCalledTimes(1) + }) + + // Why: the breadcrumb reports observed silence, so it must be the measured gap at the firing + // tick (the first one strictly past the timeout), not the timeout constant. + it('reports the measured stall duration, not the timeout', () => { + const { loop, onHangDetected, advance } = loopWithClock() + loop.recordHeartbeat() + for (let i = 0; i < 12; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).toHaveBeenCalledWith(50_000) + }) + + it('does not fire at exactly the timeout boundary', () => { + const { loop, onHangDetected, advance } = loopWithClock() + loop.recordHeartbeat() + for (let i = 0; i < 9; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).not.toHaveBeenCalled() + }) + + it('treats a large tick gap as system sleep and restarts the wait', () => { + const { loop, onHangDetected, advance } = loopWithClock() + loop.recordHeartbeat() + // Simulate suspension: the check timer did not run for far longer than the timeout. + advance(TIMEOUT_MS * 4) + loop.tick() + expect(onHangDetected).not.toHaveBeenCalled() + // A responsive parent resumes heartbeats after wake; the loop must fire only after a fresh full timeout of silence. + for (let i = 0; i < 9; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).not.toHaveBeenCalled() + for (let i = 0; i < 3; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).toHaveBeenCalledTimes(1) + }) + + // Why: this is the measurement the whole PR exists for — a stall that clears would have been a + // destructive kill under the SIGKILL design, so it has to be counted separately. + it('reports resolution when heartbeats resume after a detected hang', () => { + const { loop, onHangDetected, onHangResolved, advance } = loopWithClock() + loop.recordHeartbeat() + for (let i = 0; i < 12; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).toHaveBeenCalledTimes(1) + advance(CHECK_INTERVAL_MS) + loop.recordHeartbeat() + expect(onHangResolved).toHaveBeenCalledTimes(1) + expect(onHangResolved).toHaveBeenCalledWith(13 * CHECK_INTERVAL_MS) + }) + + it('does not report resolution when no hang was ever detected', () => { + const { loop, onHangResolved, advance } = loopWithClock() + for (let i = 0; i < 5; i++) { + advance(CHECK_INTERVAL_MS) + loop.recordHeartbeat() + loop.tick() + } + expect(onHangResolved).not.toHaveBeenCalled() + }) + + it('can detect a second hang after the first one resolved', () => { + const { loop, onHangDetected, onHangResolved, advance } = loopWithClock() + loop.recordHeartbeat() + for (let i = 0; i < 12; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + advance(CHECK_INTERVAL_MS) + loop.recordHeartbeat() + expect(onHangResolved).toHaveBeenCalledTimes(1) + // Why: the tick clock must keep advancing during the first hang, or this first tick reads as a + // sleep gap and silently restarts the wait instead of arming it. + for (let i = 0; i < 12; i++) { + advance(CHECK_INTERVAL_MS) + loop.tick() + } + expect(onHangDetected).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/hang-watchdog/hang-watchdog-child-loop.ts b/src/main/hang-watchdog/hang-watchdog-child-loop.ts new file mode 100644 index 00000000000..3a8ebb34fdf --- /dev/null +++ b/src/main/hang-watchdog/hang-watchdog-child-loop.ts @@ -0,0 +1,51 @@ +export type HangWatchdogChildLoopConfig = { + timeoutMs: number + checkIntervalMs: number + now: () => number + onHangDetected: (unresponsiveMs: number) => void + /** Heartbeats resumed after a detected hang — the main thread was stalled, not deadlocked. */ + onHangResolved: (unresponsiveMs: number) => void +} + +export type HangWatchdogChildLoop = { + recordHeartbeat: () => void + tick: () => void +} + +export function createHangWatchdogChildLoop( + config: HangWatchdogChildLoopConfig +): HangWatchdogChildLoop { + let lastHeartbeatAt = config.now() + let lastTickAt = config.now() + let detected = false + return { + recordHeartbeat: () => { + const now = config.now() + if (detected) { + detected = false + config.onHangResolved(now - lastHeartbeatAt) + } + lastHeartbeatAt = now + }, + tick: () => { + const now = config.now() + const tickGap = now - lastTickAt + // Why: advance the tick clock even while a hang is outstanding, or the first tick after the + // stall clears reads as a huge gap and gets misread as system sleep. + lastTickAt = now + if (detected) { + return + } + // Why: system sleep suspends this process too; a huge tick gap means suspension, not a parent hang, so restart the wait from scratch. + if (tickGap > config.checkIntervalMs * 3) { + lastHeartbeatAt = now + return + } + const unresponsiveMs = now - lastHeartbeatAt + if (unresponsiveMs > config.timeoutMs) { + detected = true + config.onHangDetected(unresponsiveMs) + } + } + } +} diff --git a/src/main/hang-watchdog/hang-watchdog-entry-path.ts b/src/main/hang-watchdog/hang-watchdog-entry-path.ts new file mode 100644 index 00000000000..74f8b1d0089 --- /dev/null +++ b/src/main/hang-watchdog/hang-watchdog-entry-path.ts @@ -0,0 +1,17 @@ +import { existsSync } from 'node:fs' +import { join } from 'node:path' + +export function resolveHangWatchdogEntryPath( + appPath: string, + isPackaged: boolean, + pathExists: (candidate: string) => boolean = existsSync +): string { + // Why: ELECTRON_RUN_AS_NODE bypasses asar integration, so the packaged entry must be forked from app.asar.unpacked. + const basePath = isPackaged ? appPath.replace('app.asar', 'app.asar.unpacked') : appPath + const adjacentBuildEntry = join(basePath, 'main-thread-hang-watchdog-entry.js') + // Why: electron-vite's unpackaged appPath is already out/main; appending out/main again would silently disable the watchdog in dev and E2E builds. + if (!isPackaged && pathExists(adjacentBuildEntry)) { + return adjacentBuildEntry + } + return join(basePath, 'out', 'main', 'main-thread-hang-watchdog-entry.js') +} diff --git a/src/main/hang-watchdog/main-thread-hang-telemetry.test.ts b/src/main/hang-watchdog/main-thread-hang-telemetry.test.ts new file mode 100644 index 00000000000..1ef4d0536c4 --- /dev/null +++ b/src/main/hang-watchdog/main-thread-hang-telemetry.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest' +import { eventSchemas } from '../../shared/telemetry-events' + +describe('main_thread_hang_detected telemetry schema', () => { + it('accepts an unresolved hang', () => { + expect( + eventSchemas.main_thread_hang_detected.safeParse({ + unresponsive_ms: 50_000, + self_recovered: false + }).success + ).toBe(true) + }) + + it('accepts a self-recovered stall', () => { + expect( + eventSchemas.main_thread_hang_detected.safeParse({ + unresponsive_ms: 61_000, + self_recovered: true + }).success + ).toBe(true) + }) + + // Why: strict() keeps unplanned fields (paths, pids, window titles) off the wire. + it('rejects unknown fields and non-integer durations', () => { + expect( + eventSchemas.main_thread_hang_detected.safeParse({ + unresponsive_ms: 50_000, + self_recovered: false, + parent_pid: 4242 + }).success + ).toBe(false) + expect( + eventSchemas.main_thread_hang_detected.safeParse({ + unresponsive_ms: 50_000.5, + self_recovered: false + }).success + ).toBe(false) + }) +}) diff --git a/src/main/hang-watchdog/main-thread-hang-watchdog-entry.test.ts b/src/main/hang-watchdog/main-thread-hang-watchdog-entry.test.ts new file mode 100644 index 00000000000..0f36e8acad9 --- /dev/null +++ b/src/main/hang-watchdog/main-thread-hang-watchdog-entry.test.ts @@ -0,0 +1,106 @@ +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { consumeHangDetectionMarker } from './hang-detection-marker' + +const { spawnMock } = vi.hoisted(() => ({ + spawnMock: vi.fn(() => ({ unref: vi.fn() })) +})) + +vi.mock('node:child_process', () => ({ + spawn: spawnMock +})) + +import { recordHangObservation } from './main-thread-hang-watchdog-entry' + +describe('recordHangObservation', () => { + let dir: string + let killSpy: ReturnType + let exitSpy: ReturnType + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'hang-detect-')) + spawnMock.mockClear() + killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) + }) + + afterEach(() => { + killSpy.mockRestore() + exitSpy.mockRestore() + rmSync(dir, { recursive: true, force: true }) + }) + + it('records an unresolved hang', () => { + const markerPath = join(dir, 'marker.json') + recordHangObservation({ + parentPid: 4242, + markerPath, + unresponsiveMs: 47_000, + selfRecovered: false + }) + expect(consumeHangDetectionMarker(markerPath)).toMatchObject({ + parentPid: 4242, + unresponsiveMs: 47_000, + selfRecovered: false + }) + }) + + it('overwrites the marker when the stall clears, keeping one observation per stall', () => { + const markerPath = join(dir, 'marker.json') + recordHangObservation({ + parentPid: 4242, + markerPath, + unresponsiveMs: 47_000, + selfRecovered: false + }) + recordHangObservation({ + parentPid: 4242, + markerPath, + unresponsiveMs: 62_000, + selfRecovered: true + }) + expect(consumeHangDetectionMarker(markerPath)).toMatchObject({ + unresponsiveMs: 62_000, + selfRecovered: true + }) + }) + + // Why: this is the safety contract of the whole PR. Observing a hang must never kill, relaunch, + // or exit — a false positive would SIGKILL a live main thread mid-write. If a future change + // reintroduces recovery, this test must fail loudly rather than ship silently. + it('never spawns, signals, or exits', () => { + recordHangObservation({ + parentPid: 4242, + markerPath: join(dir, 'marker.json'), + unresponsiveMs: 45_000, + selfRecovered: false + }) + expect(spawnMock).not.toHaveBeenCalled() + expect(killSpy).not.toHaveBeenCalled() + expect(exitSpy).not.toHaveBeenCalled() + }) + + it('survives an unwritable marker path', () => { + expect(() => + recordHangObservation({ + parentPid: 4242, + markerPath: join(dir, 'missing-subdir', 'marker.json'), + unresponsiveMs: 45_000, + selfRecovered: false + }) + ).not.toThrow() + }) + + it('is a no-op when no marker path is configured', () => { + expect(() => + recordHangObservation({ + parentPid: 4242, + markerPath: '', + unresponsiveMs: 45_000, + selfRecovered: false + }) + ).not.toThrow() + }) +}) diff --git a/src/main/hang-watchdog/main-thread-hang-watchdog-entry.ts b/src/main/hang-watchdog/main-thread-hang-watchdog-entry.ts new file mode 100644 index 00000000000..8d45c725a72 --- /dev/null +++ b/src/main/hang-watchdog/main-thread-hang-watchdog-entry.ts @@ -0,0 +1,72 @@ +// Forked with ELECTRON_RUN_AS_NODE from the main process. Watches heartbeats from the main +// thread; if they stop (e.g. the macOS 26 AppKit scene-update deadlock) it records a marker so +// the next launch can report the stall, and rewrites it if the heartbeats come back. +// +// Why no kill: a deadlocked main thread has already lost whatever sat in the persistence debounce +// window, so killing it recovers nothing the user could not recover by force-quitting. A false +// positive, though, would SIGKILL a live main thread that was merely blocked — most plausibly on +// I/O, i.e. exactly when writes are in flight. All of the downside sits in the misfire, so this +// measures first: `selfRecovered` counts the stalls a killer would have gotten wrong. +// +// Must never import electron. +import { createHangWatchdogChildLoop } from './hang-watchdog-child-loop' +import { writeHangDetectionMarker } from './hang-detection-marker' + +const DEFAULT_TIMEOUT_MS = 45_000 +const DEFAULT_CHECK_INTERVAL_MS = 5_000 + +export function recordHangObservation(options: { + parentPid: number + markerPath: string + unresponsiveMs: number + selfRecovered: boolean +}): void { + if (!options.markerPath) { + return + } + try { + writeHangDetectionMarker(options.markerPath, { + detectedAt: Date.now(), + parentPid: options.parentPid, + unresponsiveMs: options.unresponsiveMs, + selfRecovered: options.selfRecovered + }) + } catch { + // Why: telemetry is best-effort; a marker that cannot be written must not take down the watchdog. + } +} + +function runWatchdog(parentPid: number): void { + const markerPath = process.env.ORCA_HANG_WATCHDOG_MARKER_PATH ?? '' + const timeoutMs = Number(process.env.ORCA_HANG_WATCHDOG_TIMEOUT_MS) || DEFAULT_TIMEOUT_MS + const checkIntervalMs = + Number(process.env.ORCA_HANG_WATCHDOG_CHECK_INTERVAL_MS) || DEFAULT_CHECK_INTERVAL_MS + + const loop = createHangWatchdogChildLoop({ + timeoutMs, + checkIntervalMs, + now: () => Date.now(), + onHangDetected: (unresponsiveMs) => + recordHangObservation({ parentPid, markerPath, unresponsiveMs, selfRecovered: false }), + // Why: rewriting the marker keeps one observation per stall rather than two rows to reconcile. + onHangResolved: (unresponsiveMs) => + recordHangObservation({ parentPid, markerPath, unresponsiveMs, selfRecovered: true }) + }) + + process.on('message', (message) => { + const type = (message as { type?: string } | null)?.type + if (type === 'heartbeat') { + loop.recordHeartbeat() + } else if (type === 'shutdown') { + process.exit(0) + } + }) + // Why: a normal parent exit closes the IPC channel; the watchdog must not outlive it and misfire. + process.on('disconnect', () => process.exit(0)) + setInterval(() => loop.tick(), checkIntervalMs) +} + +const configuredParentPid = Number(process.env.ORCA_HANG_WATCHDOG_PARENT_PID) +if (Number.isInteger(configuredParentPid) && configuredParentPid > 0) { + runWatchdog(configuredParentPid) +} diff --git a/src/main/hang-watchdog/main-thread-hang-watchdog.test.ts b/src/main/hang-watchdog/main-thread-hang-watchdog.test.ts new file mode 100644 index 00000000000..b5c1063a915 --- /dev/null +++ b/src/main/hang-watchdog/main-thread-hang-watchdog.test.ts @@ -0,0 +1,132 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { forkMock, appMock } = vi.hoisted(() => ({ + forkMock: vi.fn(), + appMock: { + isPackaged: true, + getAppPath: vi.fn(() => '/apps/orca/app.asar'), + on: vi.fn() + } +})) + +vi.mock('node:child_process', () => ({ + fork: forkMock +})) + +vi.mock('electron', () => ({ + app: appMock +})) + +import { installMainThreadHangWatchdog } from './main-thread-hang-watchdog' + +function withPlatform(platform: NodeJS.Platform, run: () => T): T { + const original = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + try { + return run() + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: original }) + } +} + +function fakeChild() { + return { + connected: true, + stderr: { on: vi.fn() }, + on: vi.fn(), + send: vi.fn(), + disconnect: vi.fn(), + kill: vi.fn() + } +} + +describe('installMainThreadHangWatchdog', () => { + beforeEach(() => { + vi.useFakeTimers() + forkMock.mockReset() + appMock.on.mockReset() + appMock.isPackaged = true + delete process.env.ORCA_HANG_WATCHDOG_FORCE + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('is a no-op off macOS', () => { + expect( + withPlatform('win32', () => installMainThreadHangWatchdog({ userDataPath: '/ud' })) + ).toBeNull() + expect( + withPlatform('linux', () => installMainThreadHangWatchdog({ userDataPath: '/ud' })) + ).toBeNull() + expect(forkMock).not.toHaveBeenCalled() + }) + + it('is a no-op in unpackaged builds unless forced', () => { + appMock.isPackaged = false + expect( + withPlatform('darwin', () => installMainThreadHangWatchdog({ userDataPath: '/ud' })) + ).toBeNull() + process.env.ORCA_HANG_WATCHDOG_FORCE = '1' + const child = fakeChild() + forkMock.mockReturnValue(child) + expect( + withPlatform('darwin', () => installMainThreadHangWatchdog({ userDataPath: '/ud' })) + ).not.toBeNull() + delete process.env.ORCA_HANG_WATCHDOG_FORCE + }) + + it('forks the watchdog as plain Node with pid, bundle, and marker config', () => { + const child = fakeChild() + forkMock.mockReturnValue(child) + const handle = withPlatform('darwin', () => + installMainThreadHangWatchdog({ userDataPath: '/ud' }) + ) + expect(handle).not.toBeNull() + const [, , options] = forkMock.mock.calls[0] + expect(options.env.ELECTRON_RUN_AS_NODE).toBe('1') + expect(options.env.ORCA_HANG_WATCHDOG_PARENT_PID).toBe(String(process.pid)) + expect(options.env.ORCA_HANG_WATCHDOG_MARKER_PATH).toContain('/ud') + }) + + it('sends heartbeats on an interval and shutdown+disconnect on stop', () => { + const child = fakeChild() + forkMock.mockReturnValue(child) + const handle = withPlatform('darwin', () => + installMainThreadHangWatchdog({ userDataPath: '/ud' }) + ) + vi.advanceTimersByTime(6_000) + const heartbeats = child.send.mock.calls.filter(([m]) => m.type === 'heartbeat') + expect(heartbeats.length).toBe(3) + + handle?.stop() + expect(child.send.mock.calls.some(([m]) => m.type === 'shutdown')).toBe(true) + expect(child.disconnect).toHaveBeenCalled() + + // Why: quit fires will-quit twice; a second stop must not resend or throw. + handle?.stop() + const shutdowns = child.send.mock.calls.filter(([m]) => m.type === 'shutdown') + expect(shutdowns.length).toBe(1) + + vi.advanceTimersByTime(10_000) + const heartbeatsAfterStop = child.send.mock.calls.filter(([m]) => m.type === 'heartbeat') + expect(heartbeatsAfterStop.length).toBe(3) + }) + + it('registers stop on will-quit', () => { + const child = fakeChild() + forkMock.mockReturnValue(child) + withPlatform('darwin', () => installMainThreadHangWatchdog({ userDataPath: '/ud' })) + expect(appMock.on).toHaveBeenCalledWith('will-quit', expect.any(Function)) + }) + + it('returns null and stays inert when the fork itself fails', () => { + forkMock.mockImplementation(() => { + throw new Error('spawn failure') + }) + expect( + withPlatform('darwin', () => installMainThreadHangWatchdog({ userDataPath: '/ud' })) + ).toBeNull() + }) +}) diff --git a/src/main/hang-watchdog/main-thread-hang-watchdog.ts b/src/main/hang-watchdog/main-thread-hang-watchdog.ts new file mode 100644 index 00000000000..f859f8b5745 --- /dev/null +++ b/src/main/hang-watchdog/main-thread-hang-watchdog.ts @@ -0,0 +1,81 @@ +import { fork, type ChildProcess } from 'node:child_process' +import { app } from 'electron' +import { resolveHangWatchdogEntryPath } from './hang-watchdog-entry-path' +import { hangDetectionMarkerPath } from './hang-detection-marker' + +const HEARTBEAT_INTERVAL_MS = 2_000 + +export type MainThreadHangWatchdogHandle = { + stop: () => void + child: ChildProcess +} + +// Why: macOS 26 scene-backed AppKit windows can deadlock the main thread inside +// FrontBoardServices with no crash and no event loop left to self-report. A plain-Node sibling +// process watches heartbeats and records the stall so the next launch can report it. It observes +// only — see main-thread-hang-watchdog-entry.ts for why it does not kill the parent. +export function installMainThreadHangWatchdog(options: { + userDataPath: string +}): MainThreadHangWatchdogHandle | null { + if (process.platform !== 'darwin') { + return null + } + // Why: dev main threads pause in debuggers routinely; watch packaged builds only unless forced. + if (!app.isPackaged && process.env.ORCA_HANG_WATCHDOG_FORCE !== '1') { + return null + } + const entryPath = resolveHangWatchdogEntryPath(app.getAppPath(), app.isPackaged) + let child: ChildProcess + try { + child = fork(entryPath, [], { + stdio: ['ignore', 'ignore', 'pipe', 'ipc'], + env: { + ...process.env, + ELECTRON_RUN_AS_NODE: '1', + ORCA_HANG_WATCHDOG_PARENT_PID: String(process.pid), + ORCA_HANG_WATCHDOG_MARKER_PATH: hangDetectionMarkerPath(options.userDataPath) + } + }) + } catch (error) { + console.error('[hang-watchdog] failed to fork watchdog process:', error) + return null + } + child.stderr?.on('data', (chunk: Buffer) => { + console.error('[hang-watchdog]', String(chunk).trimEnd()) + }) + // Why: the watchdog is a safety net — if it dies, run without it; a restart loop here must never affect the app. + child.on('error', () => {}) + const heartbeatTimer = setInterval(() => { + if (child.connected) { + try { + child.send({ type: 'heartbeat' }, () => {}) + } catch { + // Channel raced closed between the check and the send. + } + } + }, HEARTBEAT_INTERVAL_MS) + let stopped = false + const stop = (): void => { + if (stopped) { + return + } + stopped = true + clearInterval(heartbeatTimer) + if (child.connected) { + try { + child.send({ type: 'shutdown' }, () => {}) + } catch { + // Already disconnecting. + } + } + // Why: disconnect guarantees the child observes parent shutdown and exits instead of misreading quit as a hang. + try { + child.disconnect() + } catch { + // Channel already closed. + } + } + // Why: will-quit fires twice during quit; stop is idempotent. + app.on('will-quit', stop) + return { stop, child } +} diff --git a/src/main/index.ts b/src/main/index.ts index e0930ae909c..95c214cf886 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -248,6 +248,11 @@ import { recordCrashBreadcrumb } from './crash-reporting/crash-breadcrumb-store' import { recordDurableCrashBreadcrumb } from './crash-reporting/durable-crash-breadcrumb' +import { installMainThreadHangWatchdog } from './hang-watchdog/main-thread-hang-watchdog' +import { + consumeHangDetectionMarker, + hangDetectionMarkerPath +} from './hang-watchdog/hang-detection-marker' import { getMainProcessLifecycleIdentity } from './crash-reporting/main-process-lifecycle-identity' import { CrashReportStore } from './crash-reporting/crash-report-store' import { @@ -1860,6 +1865,17 @@ function shouldSuppressCodexAutoApprovalSyntheticTitleFromHook(args: { void app.whenReady().then(async () => { logStartupMilestone('app-ready') + installMainThreadHangWatchdog({ userDataPath: getCanonicalUserDataPath() }) + const hangDetection = consumeHangDetectionMarker( + hangDetectionMarkerPath(getCanonicalUserDataPath()) + ) + if (hangDetection) { + recordDurableCrashBreadcrumb('main_thread_hang_detected', { + unresponsiveMs: hangDetection.unresponsiveMs, + previousPid: hangDetection.parentPid, + selfRecovered: hangDetection.selfRecovered + }) + } // Why: install certificate decisions before any webview or headless window issues its first TLS request. app.on( 'certificate-error', @@ -1987,6 +2003,16 @@ void app.whenReady().then(async () => { } // Why: telemetry must init before any IPC handler/renderer can call track(); it's a no-op in dev and while TELEMETRY_ENABLED is false, so it's safe early. initTelemetry(store) + // Why: the breadcrumb alone never leaves the machine — it rides crash reports, and a hang is not + // a crash (the app is force-quit, so no report is ever generated). Without this the incidence + // number the watchdog exists to produce would sit unread on the user's disk. Must run after + // initTelemetry: track() drops silently until the client and store are wired. + if (hangDetection) { + track('main_thread_hang_detected', { + unresponsive_ms: Math.round(hangDetection.unresponsiveMs), + self_recovered: hangDetection.selfRecovered + }) + } // Why: the trust-grant module is bundled into plain-node CLI entries where // the telemetry client cannot load, so the tracker is injected here instead // of imported there. diff --git a/src/shared/telemetry-events.ts b/src/shared/telemetry-events.ts index a98ece28df6..4428bdc9541 100644 --- a/src/shared/telemetry-events.ts +++ b/src/shared/telemetry-events.ts @@ -385,6 +385,18 @@ const runtimeRpcStartFailedSchema = z .object({ error_class: runtimeRpcStartErrorClassSchema }) .strict() +// Why: a deadlocked main thread never crashes, so it produces no crash report and no user report +// beyond "it froze" — incidence has been unmeasurable. `self_recovered` splits stalls that cleared +// from ones that never did, which is the number that decides whether auto-recovery is ever safe to +// build: every self-recovered stall is a kill that design would have gotten wrong. `unresponsive_ms` +// is the observed silence, kept raw so the 45s threshold can be calibrated against real tails. +const mainThreadHangDetectedSchema = z + .object({ + unresponsive_ms: z.number().int().nonnegative(), + self_recovered: z.boolean() + }) + .strict() + // Why: daemon replace/retire lifecycle signal — issue #7936 was undiagnosable without asking a user for daemon.log. // Enum-only + bucketed session count so no paths, raw versions, or exact counts reach the wire. // The union keeps each reason pinned to its transition, so a death can't be reported as a replace. @@ -1397,6 +1409,7 @@ export const eventSchemas = { agent_hook_unattributed: agentHookUnattributedSchema, daemon_start_failed: daemonStartFailedSchema, + main_thread_hang_detected: mainThreadHangDetectedSchema, daemon_lifecycle: daemonLifecycleSchema, runtime_rpc_start_failed: runtimeRpcStartFailedSchema, From f790d9cbe851bc3e8782cfeec442d53ec676f198 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:49:18 -0700 Subject: [PATCH 4/7] fix(skills): stop the skill review dialog contradicting the badge that opens it (#11128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): stop the skill review dialog contradicting the badge that opens it A skill whose only fault was an edited copy or one Orca could not read turned the setup-rail badge amber and offered Details — and Details opened a dialog headlined "All installed Orca skills are up to date." over an empty list. The badge says something is wrong, the dialog it points at says nothing is. The grouping only returned skills with an out-of-date copy, so those two states produced no row and the summary fell through to the all-clear headline. Include a skill when a copy needs attention as well, using one shared predicate so the badge and the dialog cannot disagree again. A plugin's own copy of a same-named skill stays out: that is the vendor's, not the user's drift. * test(skills): pin that a routine outdated copy raises no attention marker --- .../skills/skill-freshness-grouping.test.ts | 52 ++++++++++++++++++- .../skills/skill-freshness-grouping.ts | 23 +++++--- .../skill-freshness-display-status.test.ts | 8 +++ .../src/lib/skill-freshness-display-status.ts | 13 +---- src/shared/skill-freshness.ts | 20 +++++++ 5 files changed, 97 insertions(+), 19 deletions(-) diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts index d305353d950..c4913f678da 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -39,16 +39,64 @@ describe('groupSkillFreshness', () => { ]) }) - it('hides skills with nothing out of date (current, unrecognized-only, unreadable-only)', () => { + it('hides skills whose every copy is current', () => { const groups = groupSkillFreshness( [ placement('orca-cli', { status: 'current' }), + placement('orca-cli', { status: 'current', topology: 'provider-alias' }) + ], + [] + ) + expect(groups).toEqual([]) + }) + + it('keeps a plugin-managed copy of a same-named skill out of the list', () => { + // Why: the vendor owns that copy, so it is not drift the user can act on — + // reporting it put permanent amber on any ordinary plugin install. + const groups = groupSkillFreshness( + [placement('dataviz', { status: 'unrecognized', topology: 'plugin-cache' })], + [] + ) + expect(groups).toEqual([]) + }) + + it('lists a skill whose only fault cannot be fixed by the update, so Details explains it', () => { + // Why: these turn the setup-rail badge amber and its Details opens this list. Omitting + // them left that list headlined "all up to date" over nothing, contradicting the badge. + const groups = groupSkillFreshness( + [ placement('dataviz', { status: 'unrecognized', topology: 'independent-copy' }), placement('linear-tickets', { status: 'inaccessible' }) ], [] ) - expect(groups).toEqual([]) + expect(groups).toEqual([ + { + name: 'dataviz', + status: 'cannot-update', + locations: [ + { id: expect.any(String), path: '/home/.agents/skills/dataviz', chip: 'unrecognized' } + ] + }, + { + name: 'linear-tickets', + status: 'cannot-update', + locations: [ + { + id: expect.any(String), + path: '/home/.agents/skills/linear-tickets', + chip: 'inaccessible' + } + ] + } + ]) + }) + + it('lists an edited canonical copy, which is what an update would overwrite', () => { + const groups = groupSkillFreshness([placement('orchestration', { status: 'unrecognized' })], []) + expect(groups).toHaveLength(1) + expect(groups[0]?.status).toBe('cannot-update') + expect(groups[0]?.locations[0]?.chip).toBe('unrecognized') }) it('groups a blocked skill and flags the culprit location, not the main copy', () => { diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index ee2ebbbf3e0..2a306b44c72 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.ts @@ -1,4 +1,7 @@ -import type { SkillFreshnessInstallation } from '../../../../shared/skill-freshness' +import { + isSkillCopyNeedingAttention, + type SkillFreshnessInstallation +} from '../../../../shared/skill-freshness' export type SkillGroupStatus = 'update-available' | 'cannot-update' @@ -57,10 +60,15 @@ export function locationChip(installation: SkillFreshnessInstallation): SkillLoc } /** - * Groups installations by skill for the update modal and derives each skill's - * update disposition. Only skills with an out-of-date official copy are returned — - * up-to-date, unrecognized-only, and unreadable-only skills have nothing to change - * here, so they are omitted entirely. + * Groups installations by skill for the review modal and derives each skill's + * update disposition. A skill appears when a copy is out of date, or when a copy is + * wrong in a way the update cannot fix — an edited copy, one Orca could not read. + * + * That second half is what the badge already reports: it turns amber and offers + * Details, and Details opens this modal. Returning only out-of-date skills left that + * modal saying every skill was up to date over an empty list, contradicting the badge + * that sent the user there. Skills where every copy is current are still omitted, as + * is a plugin's own copy of a same-named skill, which is not the user's to fix. * * `alwaysIncludeNames` overrides that filter. A successful update makes every * targeted skill current, which would otherwise drop its row the instant the @@ -82,7 +90,10 @@ export function groupSkillFreshness( } const groups: SkillFreshnessGroupModel[] = [] for (const [name, entries] of byName) { - if (!pinned.has(name) && !entries.some((entry) => entry.status === 'outdated')) { + if ( + !pinned.has(name) && + !entries.some((entry) => entry.status === 'outdated' || isSkillCopyNeedingAttention(entry)) + ) { continue } const locations = entries diff --git a/src/renderer/src/lib/skill-freshness-display-status.test.ts b/src/renderer/src/lib/skill-freshness-display-status.test.ts index b619e59e854..f8bdf54d1b1 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.test.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.test.ts @@ -167,6 +167,14 @@ describe('getSkillFreshnessDisplayStatus', () => { }) describe('hasSkillCopyNeedingAttention', () => { + it('raises no attention marker over a routine outdated copy the update converges', () => { + // Why: an out-of-date convergent copy is ordinary work for the update command. + // Amber on the review affordance there would cry wolf on every routine update. + expect( + hasSkillCopyNeedingAttention(inventory([placement('outdated')], [SKILL_NAME]), SKILL_NAME) + ).toBe(false) + }) + it('ignores a traversal bound but keeps a real scan fault', () => { expect( hasSkillCopyNeedingAttention( diff --git a/src/renderer/src/lib/skill-freshness-display-status.ts b/src/renderer/src/lib/skill-freshness-display-status.ts index 0da1e08cb52..029725106a8 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.ts @@ -1,6 +1,6 @@ import { + isSkillCopyNeedingAttention, isSkillScanIssueNeedingAttention, - SUPPORTED_GLOBAL_SKILL_TOPOLOGIES, type SkillFreshnessInventory } from '../../../shared/skill-freshness' @@ -63,15 +63,6 @@ export function hasSkillCopyNeedingAttention( return ( (placements.length > 0 && Boolean(inventory?.scanIssues.some(isSkillScanIssueNeedingAttention))) || - placements.some( - (installation) => - installation.status !== 'current' && - !(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') && - // Why: an out-of-date copy the command converges is ordinary work, not a problem. - !( - SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) && - installation.status === 'outdated' - ) - ) + placements.some(isSkillCopyNeedingAttention) ) } diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 0414a1823c0..847988ca463 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -86,6 +86,26 @@ export type SkillFreshnessInstallation = { errorCategory: string | null } +/** + * Whether a copy is wrong in a way running the update would not resolve. + * + * Shared so the badge and the review dialog cannot disagree about what counts: the + * badge points at the dialog for the explanation, so a copy that turns the badge + * amber must also produce a row there. An out-of-date copy the command converges is + * ordinary work, not a problem, and a plugin's own copy of a same-named skill is the + * vendor's business rather than the user's drift. + */ +export function isSkillCopyNeedingAttention(installation: SkillFreshnessInstallation): boolean { + return ( + installation.status !== 'current' && + !(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') && + !( + SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) && + installation.status === 'outdated' + ) + ) +} + export type SkillFreshnessScanIssueReason = | 'depth-limit' | 'entry-limit' From bb9ae7813697b61b39080a001dec6c326c14846f Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:51:59 -0700 Subject: [PATCH 5/7] fix(macos): show TCC notice after first prompt (#11243) --- src/main/macos-tcc-prompt-notice.test.ts | 41 ++++++++++++++---------- src/main/macos-tcc-prompt-notice.ts | 6 ++-- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/src/main/macos-tcc-prompt-notice.test.ts b/src/main/macos-tcc-prompt-notice.test.ts index 9ee688cea4b..764ca153943 100644 --- a/src/main/macos-tcc-prompt-notice.test.ts +++ b/src/main/macos-tcc-prompt-notice.test.ts @@ -54,16 +54,11 @@ beforeEach(() => { }) describe('tcc prompt notice threshold', () => { - it('stays silent until the third dialog, then fires exactly once', () => { - expect(handleTccPromptForTests()).toBeNull() - expect(handleTccPromptForTests()).toBeNull() - - const payload = handleTccPromptForTests() - expect(payload).toEqual({ + it('fires on the first dialog, then stays silent', () => { + expect(handleTccPromptForTests()).toEqual({ promptCount: TCC_PROMPT_NOTICE_THRESHOLD }) - // Why: a recurring prompt loop must not produce a toast per dialog. expect(handleTccPromptForTests()).toBeNull() expect(handleTccPromptForTests()).toBeNull() }) @@ -109,7 +104,10 @@ describe('tcc prompt notice threshold', () => { expect(consumePendingTccPromptNotice(2)).toBeNull() expect(writeFileAtomically).toHaveBeenCalledOnce() const [, contents] = writeFileAtomically.mock.calls.at(-1) as [string, string] - expect(JSON.parse(contents)).toMatchObject({ promptCount: 3, notified: true }) + expect(JSON.parse(contents)).toMatchObject({ + promptCount: TCC_PROMPT_NOTICE_THRESHOLD, + notified: true + }) }) it('releases an unacknowledged claim for a replacement renderer', () => { @@ -123,11 +121,14 @@ describe('tcc prompt notice threshold', () => { releasePendingTccPromptNotice(1, oldClaim!.claimId) const replacementClaim = consumePendingTccPromptNotice(2) - expect(oldClaim).toEqual({ claimId: 1, promptCount: 3 }) - expect(replacementClaim).toEqual({ claimId: 2, promptCount: 3 }) + expect(oldClaim).toEqual({ claimId: 1, promptCount: TCC_PROMPT_NOTICE_THRESHOLD }) + expect(replacementClaim).toEqual({ claimId: 2, promptCount: TCC_PROMPT_NOTICE_THRESHOLD }) acknowledgePendingTccPromptNotice(1, oldClaim!.claimId) releasePendingTccPromptNotice(2) - expect(consumePendingTccPromptNotice(3)).toEqual({ claimId: 3, promptCount: 3 }) + expect(consumePendingTccPromptNotice(3)).toEqual({ + claimId: 3, + promptCount: TCC_PROMPT_NOTICE_THRESHOLD + }) }) it('invalidates an outstanding claim when the user dismisses the notice', () => { @@ -262,7 +263,10 @@ describe('tcc prompt notice threshold', () => { }).not.toThrow() expect(mainWindow.webContents.send).not.toHaveBeenCalled() expect(watchStop).toHaveBeenCalledTimes(1) - expect(consumePendingTccPromptNotice(1)).toEqual({ claimId: 1, promptCount: 3 }) + expect(consumePendingTccPromptNotice(1)).toEqual({ + claimId: 1, + promptCount: TCC_PROMPT_NOTICE_THRESHOLD + }) } finally { Object.defineProperty(process, 'platform', platform!) } @@ -284,17 +288,20 @@ describe('tcc prompt notice threshold', () => { }).not.toThrow() expect(watchStop).toHaveBeenCalledOnce() - expect(consumePendingTccPromptNotice(1)).toEqual({ claimId: 1, promptCount: 3 }) + expect(consumePendingTccPromptNotice(1)).toEqual({ + claimId: 1, + promptCount: TCC_PROMPT_NOTICE_THRESHOLD + }) } finally { Object.defineProperty(process, 'platform', platform!) } }) - it('does not respawn the watcher for a persisted pending threshold', () => { + it('delivers a tally persisted below the old threshold without respawning the watcher', () => { const platform = Object.getOwnPropertyDescriptor(process, 'platform') Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' }) readTallyFile.mockReturnValue( - JSON.stringify({ promptCount: 3, notified: false, dismissed: false }) + JSON.stringify({ promptCount: 2, notified: false, dismissed: false }) ) try { const mainWindow = createWindowStub() @@ -302,10 +309,10 @@ describe('tcc prompt notice threshold', () => { expect(watchStart).not.toHaveBeenCalled() expect(mainWindow.webContents.send).toHaveBeenCalledWith('macosTccPrompts:threshold', { - promptCount: 3 + promptCount: 2 }) expect(mainWindow.once).not.toHaveBeenCalled() - expect(consumePendingTccPromptNotice(1)).toEqual({ claimId: 1, promptCount: 3 }) + expect(consumePendingTccPromptNotice(1)).toEqual({ claimId: 1, promptCount: 2 }) } finally { Object.defineProperty(process, 'platform', platform!) } diff --git a/src/main/macos-tcc-prompt-notice.ts b/src/main/macos-tcc-prompt-notice.ts index 30b96f36c39..d76b0470453 100644 --- a/src/main/macos-tcc-prompt-notice.ts +++ b/src/main/macos-tcc-prompt-notice.ts @@ -8,11 +8,11 @@ import { MacosTccPromptWatch } from './macos-tcc-prompt-watch' /** * Surfaces Full Disk Access guidance only to users macOS is actually prompting * (#9756), instead of nudging every Mac user. Counts Orca-attributed TCC - * dialogs across launches and tells the renderer once the third one lands. + * dialogs across launches and tells the renderer when the first one lands. */ -/** Why: one dialog is normal and two is bad luck; three means it is recurring for this user. */ -export const TCC_PROMPT_NOTICE_THRESHOLD = 3 +/** Why: the first detected dialog identifies an affected user; the notice remains one-time. */ +export const TCC_PROMPT_NOTICE_THRESHOLD = 1 /** Why: preserve detection when Electron never emits `ready-to-show` without competing with startup. */ export const TCC_PROMPT_WATCH_START_FALLBACK_MS = 10_000 From 2db02562b6c0ce7ee38533d036f8207088485d76 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:57:51 -0700 Subject: [PATCH 6/7] fix(skills): tell the user how to fix a skill the updater cannot converge (#11248) Re-lands #11129, which was merged into #11128's branch rather than main and so never reached main. Content is identical to the reviewed and live-QA'd head ac5ec5b0b0 (1775d83cf6 + ac5ec5b0b0, minus the intermediate merge). --- .../SkillFreshnessUpdateDialog.test.tsx | 19 ++++++++++ .../src/components/skills/SkillUpdateRow.tsx | 2 +- .../skills/skill-freshness-grouping.test.ts | 3 ++ .../skills/skill-freshness-grouping.ts | 12 +++++-- .../skill-freshness-skipped-reason.test.ts | 26 +++++++++++++- .../skills/skill-freshness-skipped-reason.ts | 35 ++++++++++++++++--- .../skills/skill-location-chip-copy.ts | 7 ++++ src/renderer/src/i18n/locales/en.json | 6 +++- src/renderer/src/i18n/locales/es.json | 6 +++- src/renderer/src/i18n/locales/ja.json | 6 +++- src/renderer/src/i18n/locales/ko.json | 6 +++- src/renderer/src/i18n/locales/zh.json | 6 +++- 12 files changed, 119 insertions(+), 15 deletions(-) diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx index 0733df60470..7c00fb26d53 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx @@ -423,6 +423,25 @@ describe('SkillFreshnessUpdateDialog', () => { ) }) + it('names the skill in the stale-record remedy so the command is runnable as-is', async () => { + // Why: the reinstall advice only fires when the row hands its group name through + // to skippedReason — dropping that argument silently degrades every stale-record + // row to the generic sentence with no command to run. + mocks.inventory = { + schemaVersion: 1, + installations: [placement('orchestration')], + eligibleUpdateNames: [], + scanIssues: [], + scannedAt: 3 + } + await renderDialog() + await openViaRequest() + + expect(container?.textContent).toContain( + 'npx skills add https://github.com/stablyai/orca --skill orchestration --global' + ) + }) + it('stays coherent while an idle re-scan is in flight', async () => { await renderDialog() await openViaRequest() diff --git a/src/renderer/src/components/skills/SkillUpdateRow.tsx b/src/renderer/src/components/skills/SkillUpdateRow.tsx index 3f069be5b54..d6d95f34783 100644 --- a/src/renderer/src/components/skills/SkillUpdateRow.tsx +++ b/src/renderer/src/components/skills/SkillUpdateRow.tsx @@ -118,7 +118,7 @@ export function SkillUpdateRow({ nor on a mount-time `defaultOpen` that a re-scan can't re-fire. */} {state === 'blocked' ? (

- {skippedReason(group.locations)} + {skippedReason(group.locations, group.name)}

) : null} diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts index c4913f678da..39bb58cc4cb 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -164,6 +164,9 @@ describe('groupSkillFreshness', () => { expect(chipFor(at('g', { topology: 'repo-scope' }))).toBe('in-a-repo') expect(chipFor(at('h', { topology: 'plugin-cache' }))).toBe('plugin-cache') expect(chipFor(at('i', { status: 'current', topology: 'provider-alias' }))).toBe('current') + // Why: a bare chip is what the reason copy reads as "behind, and a reinstall fixes + // it". A copy that is ahead must be told apart, or that advice rolls it back. + expect(chipFor(at('k', { status: 'newer-known', topology: 'canonical-copy' }))).toBe('newer') expect(chipFor(at('j', { status: 'unrecognized', topology: 'plugin-cache' }))).toBe( 'plugin-cache' ) diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index 2a306b44c72..79d39b31aff 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.ts @@ -7,6 +7,7 @@ export type SkillGroupStatus = 'update-available' | 'cannot-update' export type SkillLocationChip = | 'current' + | 'newer' | 'unrecognized' | 'inaccessible' | 'duplicate' @@ -53,9 +54,14 @@ export function locationChip(installation: SkillFreshnessInstallation): SkillLoc return 'plugin-cache' case 'canonical-copy': case 'provider-alias': - // Why: a supported location only needs a chip when it's already up to date, - // to explain why the update won't touch it; the out-of-date main copy is bare. - return installation.status === 'current' ? 'current' : null + // Why: a supported location only needs a chip when the update won't touch it — + // already current, or ahead of what this build knows. The out-of-date main copy + // is bare, and 'newer' is what keeps a bare chip meaning "behind, and fixable": + // reinstalling a copy that is ahead would quietly roll the user back. + if (installation.status === 'current') { + return 'current' + } + return installation.status === 'newer-known' ? 'newer' : null } } diff --git a/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts index 16ef8cb0876..664a4883f84 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts @@ -25,7 +25,31 @@ describe('skippedReason', () => { expect(skippedReason([row('duplicate'), row('read-only')])).toContain('read-only location') }) - it('falls back to the generic sentence when nothing is blocking', () => { + it('hands over the reinstall command when no location is at fault', () => { + // Why: the only way to reach this with a bare out-of-date copy is the updater's own + // record, which `skills update` can never converge — so the sentence has to give the + // one command that does, not report a skip the user cannot act on. + const reason = skippedReason([row(null)], 'orchestration') + expect(reason).toContain('reports the skill as already up to date') + expect(reason).toContain( + 'npx skills add https://github.com/stablyai/orca --skill orchestration --global' + ) + }) + + it('never offers the reinstall for a copy that is ahead of this build', () => { + // Why: reinstalling a newer copy rolls the user back to what this build ships. + const reason = skippedReason([row('newer')], 'orchestration') + expect(reason).toContain('later version') + expect(reason).not.toContain('skills add') + }) + + it('keeps a placement blocker ahead of the record advice', () => { + expect(skippedReason([row(null), row('unrecognized')], 'orchestration')).toContain( + 'doesn’t match the official version' + ) + }) + + it('falls back to the generic sentence when no skill name is available', () => { expect(skippedReason([row('current')])).toContain('left this skill out of the update') expect(skippedReason([])).toContain('left this skill out of the update') }) diff --git a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts index 05af44e1a63..0e9c01c01f1 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts @@ -1,3 +1,4 @@ +import { buildAgentFeatureSkillInstallCommand } from '../../../../shared/agent-feature-install-commands' import type { SkillLocationChip, SkillLocationRow } from './skill-freshness-grouping' import { translate } from '@/i18n/i18n' @@ -8,6 +9,9 @@ const SKIPPED_REASON_PRIORITY: SkillLocationChip[] = [ 'unrecognized', 'read-only', 'inaccessible', + // Why: above the placement chips — a copy that is ahead of this build must never + // fall through to the reinstall advice below, which would roll it back. + 'newer', 'in-a-repo', 'plugin-cache', 'external-link', @@ -29,10 +33,18 @@ function blockingChip(locations: readonly SkillLocationRow[]): SkillLocationChip * The wording is deictic ("this copy") on purpose: it is only ever rendered beside the * location rows it describes, which is why the setup rails link into the dialog rather * than repeating a sentence that would have nothing to point at. + * + * `skillName` is only used for the no-chip case, where the fault is not a placement at + * all but the updater's own record of this skill, and the remedy has to name it. */ -export function skippedReason(locations: readonly SkillLocationRow[]): string { +export function skippedReason(locations: readonly SkillLocationRow[], skillName?: string): string { const chip = blockingChip(locations) switch (chip) { + case 'newer': + return translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonNewer', + 'This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.' + ) case 'unrecognized': return translate( 'auto.components.skills.SkillFreshnessRow.skippedReasonUnrecognized', @@ -75,9 +87,22 @@ export function skippedReason(locations: readonly SkillLocationRow[]): string { ) case 'current': case undefined: - return translate( - 'auto.components.skills.SkillFreshnessRow.cantUpdateReason', - 'Orca left this skill out of the update command.' - ) + // Why: no location is at fault here — the copy is an ordinary out-of-date one the + // update would happily write. What is wrong is the updater's own record of it: + // `skills update` decides what to do by comparing that record against the source + // and never reads disk, so when the record is missing or already names the version + // it was meant to fetch, the command reports "up to date" and writes nothing. No + // retry converges it; only a reinstall rewrites the record, which is why the + // sentence has to hand over the command rather than say the update was skipped. + return skillName + ? translate( + 'auto.components.skills.SkillFreshnessRow.skippedReasonStaleRecord', + 'The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}', + { value0: buildAgentFeatureSkillInstallCommand([skillName]) } + ) + : translate( + 'auto.components.skills.SkillFreshnessRow.cantUpdateReason', + 'Orca left this skill out of the update command.' + ) } } diff --git a/src/renderer/src/components/skills/skill-location-chip-copy.ts b/src/renderer/src/components/skills/skill-location-chip-copy.ts index 82e709665cc..fdee4dce2a8 100644 --- a/src/renderer/src/components/skills/skill-location-chip-copy.ts +++ b/src/renderer/src/components/skills/skill-location-chip-copy.ts @@ -5,6 +5,8 @@ export function chipLabel(chip: SkillLocationChip): string { switch (chip) { case 'current': return translate('auto.components.skills.SkillFreshnessRow.chipCurrent', 'Current') + case 'newer': + return translate('auto.components.skills.SkillFreshnessRow.chipNewer', 'Newer') case 'unrecognized': return translate('auto.components.skills.SkillFreshnessRow.chipUnrecognized', 'Unrecognized') case 'inaccessible': @@ -33,6 +35,11 @@ export function chipTooltip(chip: SkillLocationChip): string { 'auto.components.skills.SkillFreshnessRow.tipCurrent', 'This copy matches the current official version.' ) + case 'newer': + return translate( + 'auto.components.skills.SkillFreshnessRow.tipNewer', + 'This copy is a later version than the one this build of Orca ships.' + ) case 'unrecognized': return translate( 'auto.components.skills.SkillFreshnessRow.tipUnrecognized', diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 778e78b8443..6b516174191 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3828,7 +3828,11 @@ "tipReadOnly": "This copy is in a read-only location.", "tipInRepo": "This copy lives inside a project, not your global skills.", "tipPluginCache": "This copy is managed by a plugin.", - "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "Update skills", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 3caa4065ac4..f5025910756 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -3805,7 +3805,11 @@ "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", - "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "Actualizar skills", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 6a78bc44545..5eb25d00436 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -3805,7 +3805,11 @@ "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", - "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "スキルを更新", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index cea722e067a..2941dbf9575 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -3805,7 +3805,11 @@ "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", - "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "스킬 업데이트", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 1b39305661b..f50268ab4fa 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -3805,7 +3805,11 @@ "skippedReasonPluginCache": "A plugin manages this skill, so Orca left it out of the update — update the plugin instead.", "skippedReasonExternalLink": "This copy is a shortcut pointing outside Orca’s skill folders, so Orca left it out of the update.", "skippedReasonBrokenLink": "This copy is a shortcut to something that no longer exists, so Orca left it out — you can safely delete it.", - "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one." + "skippedReasonDuplicate": "This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.", + "skippedReasonNewer": "This copy is a later version than the one this build of Orca ships, so Orca left it alone rather than roll it back. Updating Orca will bring the two back in line.", + "skippedReasonStaleRecord": "The skills updater has no usable record of this copy, so it reports the skill as already up to date and changes nothing. Reinstall it to bring the record back in line: {{value0}}", + "chipNewer": "Newer", + "tipNewer": "This copy is a later version than the one this build of Orca ships." }, "SkillFreshnessUpdateDialog": { "title": "更新技能", From a721125d06ada12a8ad7ea729a42160ebd95b29a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:08:14 -0700 Subject: [PATCH 7/7] fix(perf): correct three 07-27 perf regressions (#11234) * fix(perf): correct three 07-27 perf regressions Traversal capacity cap no longer scales with worker concurrency (#11026). retainWorkspaceSpaceScanEntry charged a traversal-wide entry counter, so N workers each holding a listing multiplied the live charge. At concurrency 48 a 48x2,100 tree (100,848 entries) hit the 100,000 cap while 100x1,500 (150,100 entries, 50% more) passed, and scanLocalWorktree treats the capacity error as terminal, reporting an intact worktree as "Unavailable" with sizeBytes 0. The cap is now per directory listing -- the only quantity fixed by directory shape -- restoring the invariant docs/workspace-space-scan-resource-bounds.md already states. Aggregate live retention stays bounded by the unchanged 64 MiB byte cap. Note: releasing each entry's charge at dispatch (the originally suggested fix) was measured and does not help; the peak is set at admission, before any entry is dispatched. Repo image icons are no longer fully base64-decoded on every snapshot publish (#11012). sanitizeRepoIcon reached decodeBase64Prefix, which sized its buffer to the whole payload to read a 24-byte header, running synchronously inside ipcMain.handle at a 250 ms throttle. Validation is now memoized on source+src in a BoundedMap. Measured for 10 icons x 256 KB: 37.34 ms -> 0.67 ms per publish. One over-long card label no longer discards the entire snapshot (#11012). isDashboardSnapshot was all-or-nothing and dashboard-popout returned early with no log while replaying lastSnapshot, so `orca terminal rename --title "<1025+ chars>"` froze the pop-out board on its last good paint with nothing surfaced. Labels are truncated at the producer, the validator drops only the offending card, and both the rejection and the drop are logged. The bound now lives in the shared snapshot contract so producer and validator cannot drift. Co-authored-by: Orca * fix(perf): charge a scan listing's parent path once, not per entry The 4.1 fix made the entry cap per-listing but left the 64 MiB byte cap charging parentPath.length for every entry in a listing. Because a listing's entries all share one parent-path string, that multiplied the path by the directory's width, so the byte cap measured checkout depth rather than live heap. The reported symptom therefore still reproduced at the production default limits: 48 x 2,100 @ concurrency 48 raised a capacity error once the worktree path passed ~58 characters, while the same layout at concurrency 1 succeeded. The shipped regression test could not see this because it passes maxRetainedBytes: Number.MAX_SAFE_INTEGER, disabling the only cap still in play. Measured at a real 65-char worktree root, 3 of the report's 4 documented layouts still failed. The parent path is now charged once per listing, with its first entry, so an empty listing strands no charge. Per-entry overhead is unchanged at 512 B + name, which still dominates the estimate, so the OOM protection the original PR added is preserved. Adds a production-default-limits case covering the report's layouts under a deep root, plus an assertion that a short and a deep root reach the same verdict -- the path independence docs/workspace-space-scan-resource-bounds.md requires and which no existing test enforced. Co-authored-by: Orca * fix(perf): prove the icon cache by decode count, not wall clock The caching test asserted a per-publish millisecond budget, which failed on CI at 5.64 ms against a 5 ms ceiling. Any threshold flakes on a loaded box, so count real sanitizeRepoIcon entries instead: 10 repos x 20 publishes is 200 icon checks against exactly 1 decode. Added cases pin the cache key (payload and source both re-decode; a cached image verdict never answers for an emoji) and that a rejection is cached too. Also drops budget.entries, which the per-listing cap left as a traversal-wide counter no check reads -- exactly the shape a future guard could reintroduce the concurrency bug from. Co-authored-by: Orca * fix(dashboard): bound the project filter label the whole board rides on #11042 added snapshot-level filterOptions whose project labels are repo.displayName -- the same unbounded source this PR already bounds for card.repoName, but one level up where dropping a card cannot recover it. An over-long project name would fail isDashboardFilterOptions and take the entire snapshot with it, which is the exact frozen-board failure the per-card drop was added to end. Workspace-status labels are already capped at 32 by workspace-statuses.ts, so only projects needed this. Co-authored-by: Orca * fix(dashboard): disambiguate the repo icon cache key The memoization key joined `source` and `src` with a space, but the sanitizer's base64 pattern admits whitespace inside a valid `src`. A rejected icon can therefore split the same concatenation differently and inherit an accepted icon's cached verdict, reaching the pop-out's `` without ever being sanitized. Length-prefix the source. Co-authored-by: Orca --------- Co-authored-by: Orca --- docs/workspace-space-scan-resource-bounds.md | 25 ++- .../ipc/dashboard-payload-validation.test.ts | 208 +++++++++++++++++- src/main/ipc/dashboard-payload-validation.ts | 88 +++++++- src/main/ipc/dashboard-popout.test.ts | 52 +++++ src/main/ipc/dashboard-popout.ts | 28 ++- .../build-dashboard-snapshot.test.ts | 30 +++ .../dashboard/build-dashboard-snapshot.ts | 35 ++- src/shared/dashboard-snapshot.ts | 5 + .../workspace-space-entry-traversal.test.ts | 127 ++++++++++- src/shared/workspace-space-entry-traversal.ts | 2 +- .../workspace-space-scan-budget.test.ts | 13 +- src/shared/workspace-space-scan-budget.ts | 53 +++-- 12 files changed, 598 insertions(+), 68 deletions(-) diff --git a/docs/workspace-space-scan-resource-bounds.md b/docs/workspace-space-scan-resource-bounds.md index de4c235d935..81510e3cfea 100644 --- a/docs/workspace-space-scan-resource-bounds.md +++ b/docs/workspace-space-scan-resource-bounds.md @@ -87,11 +87,21 @@ allocate one live operation per entry. Every portable entry is admitted through `WorkspaceSpaceScanBudget` before retention: -- maximum entries held at once per worktree traversal: 100,000; +- maximum entries in any one directory listing: 100,000; - maximum estimated live scan state per worktree traversal: 64 MiB. -The retained-byte estimate includes parent path and entry name UTF-16 storage plus conservative -per-entry object overhead. These are admission limits, not post-allocation observations. +The entry cap is per listing rather than per traversal because only a single directory's width is +fixed by directory shape. A traversal-wide entry counter is charged by every worker holding a +listing at once, so its verdict scales with the configured concurrency: at 48 workers, 48 × 2,100 +files (100,848 entries) was rejected while 100 × 1,500 (150,100 entries, 50% more) was admitted. +Aggregate live retention stays bounded by the 64 MiB byte cap, which all concurrent listings share. + +The retained-byte estimate includes entry name UTF-16 storage plus conservative per-entry object +overhead, and each listing's parent path once. The parent path is charged per listing rather than +per entry because a listing's entries all share a single parent-path string; charging it per entry +multiplied it by the directory's width, so the 64 MiB cap tracked checkout depth instead of live +heap and rejected the layouts above once the worktree path passed ~58 characters. These are +admission limits, not post-allocation observations. The budget measures what the traversal is holding **right now**, not what it has ever seen. A directory listing is charged when admitted and released once every one of its entries has been @@ -100,9 +110,10 @@ distinction decides real workspaces: a cumulative counter charged an ordinary 76 worktree 61.2 MiB of its 64 MiB cap — 4% headroom, and tipping over purely because a longer branch name lengthens every absolute path. The same worktree peaks between 4 MiB and 8 MiB of live state. -Because the estimate scales with absolute path length, a cumulative cap would make success depend on -where a user checks out their worktrees. A live cap depends only on directory shape, so it rejects -genuinely pathological layouts (one directory holding six figures of entries) and nothing else. +Neither cap may depend on where a user checks out their worktrees, so the estimate charges each +absolute path once per listing rather than once per entry. A live cap depends only on directory +shape, so it rejects genuinely pathological layouts (one directory holding six figures of entries) +and nothing else. Top-level directory enumeration for the `du` path uses the same budget. A capacity error must not fall through into the portable walker, because that would repeat an already rejected traversal. @@ -169,7 +180,7 @@ The deterministic regression measurements are: - peak simultaneous local `du` calls across repositories: exactly one; - peak simultaneous desktop-side SSH fallback traversals across repositories: at most two; - portable traversal live entry jobs: no more than its configured worker count; -- portable entries held at once: at most 100,000 per worktree; +- portable entries in any one directory listing: at most 100,000; - estimated live portable state: at most 64 MiB per worktree; an ordinary 76,788-entry worktree peaks between 4 MiB and 8 MiB; - progress and final row counts remain unchanged for scans below the limits. diff --git a/src/main/ipc/dashboard-payload-validation.test.ts b/src/main/ipc/dashboard-payload-validation.test.ts index edc20d28e5a..7baed3adbb8 100644 --- a/src/main/ipc/dashboard-payload-validation.test.ts +++ b/src/main/ipc/dashboard-payload-validation.test.ts @@ -1,6 +1,26 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import type { DashboardSnapshot } from '../../shared/dashboard-snapshot' -import { isDashboardRevealAgentArgs, isDashboardSnapshot } from './dashboard-payload-validation' +import type * as RepoIconModule from '../../shared/repo-icon' + +// Counts real sanitizer entries so the icon cache is proven by decode count +// rather than by wall clock, which is unfalsifiable on a loaded CI box. +const sanitizeRepoIconCalls = vi.hoisted(() => vi.fn()) +vi.mock('../../shared/repo-icon', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + sanitizeRepoIcon: (value: unknown) => { + sanitizeRepoIconCalls(value) + return actual.sanitizeRepoIcon(value) + } + } +}) + +import { + admitDashboardSnapshot, + isDashboardRevealAgentArgs, + isDashboardSnapshot +} from './dashboard-payload-validation' const SNAPSHOT = { generatedAt: 1_700_000_000_000, @@ -40,6 +60,17 @@ const SNAPSHOT = { } } satisfies DashboardSnapshot +/** A real PNG header plus `bodyBytes` of filler, so sanitizing actually decodes. */ +function imageIconSrc(bodyBytes: number, withWhitespace = false): string { + const header = Buffer.from([ + 137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0, 0, 0, 64, 0, 0, 0, 64, 8, 6, 0, + 0, 0 + ]) + const body = Buffer.concat([header, Buffer.alloc(bodyBytes, bodyBytes % 251)]).toString('base64') + // The sanitizer's base64 pattern admits whitespace, so a real src can hold it. + return `data:image/png;base64,${withWhitespace ? `${body.slice(0, 20)} ${body.slice(20)}` : body}` +} + describe('dashboard payload validation', () => { it('accepts a complete dashboard snapshot', () => { expect(isDashboardSnapshot(SNAPSHOT)).toBe(true) @@ -146,6 +177,179 @@ describe('dashboard payload validation', () => { ).toBe(false) }) + // Why: the pop-out replays the last accepted snapshot, so rejecting the whole + // board over one card froze every other agent's status until it was renamed. + describe('admitDashboardSnapshot', () => { + it('drops only the offending card and keeps the rest of the board', () => { + const good = SNAPSHOT.cards[0] + const bad = { ...good, paneKey: 'tab-2:leaf-2', conversationName: 'x'.repeat(1_025) } + + const admitted = admitDashboardSnapshot({ ...SNAPSHOT, cards: [good, bad] }) + + expect(admitted?.droppedCardCount).toBe(1) + expect(admitted?.snapshot.cards.map((card) => card.paneKey)).toEqual(['tab-1:leaf-1']) + }) + + it('reports nothing dropped for a fully valid snapshot', () => { + const admitted = admitDashboardSnapshot(SNAPSHOT) + + expect(admitted?.droppedCardCount).toBe(0) + expect(admitted?.snapshot.cards).toHaveLength(1) + }) + + it('drops a card that fails only the search-board fields', () => { + const good = SNAPSHOT.cards[0] + const badReview = { ...good, paneKey: 'p2', review: { number: 0, state: 'open' } } + const badSubagent = { + ...good, + paneKey: 'p3', + subagents: [{ id: '', name: 'x', dotState: 'idle' }] + } + const badBucket = { ...good, paneKey: 'p4', bucket: 'archived' } + + const admitted = admitDashboardSnapshot({ + ...SNAPSHOT, + cards: [good, badReview, badSubagent, badBucket] + }) + + expect(admitted?.droppedCardCount).toBe(3) + expect(admitted?.snapshot.cards.map((card) => card.paneKey)).toEqual(['tab-1:leaf-1']) + }) + + it('keeps a done-bucket card the search board produces', () => { + const admitted = admitDashboardSnapshot({ + ...SNAPSHOT, + cards: [{ ...SNAPSHOT.cards[0], bucket: 'done', dotState: 'done' }] + }) + + expect(admitted?.droppedCardCount).toBe(0) + }) + + it('still rejects a snapshot whose own shape is unusable', () => { + expect(admitDashboardSnapshot({ ...SNAPSHOT, generatedAt: Number.NaN })).toBeNull() + expect(admitDashboardSnapshot({ ...SNAPSHOT, cards: 'nope' })).toBeNull() + expect(admitDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: [] })).toBeNull() + // Why: showIdle and filterOptions describe the frame, not one card, so a + // bad value there has no card to drop and must fail the whole snapshot. + expect(admitDashboardSnapshot({ ...SNAPSHOT, showIdle: 'yes' })).toBeNull() + expect( + admitDashboardSnapshot({ + ...SNAPSHOT, + filterOptions: { ...SNAPSHOT.filterOptions, projects: [{ id: '', label: 'Invalid' }] } + }) + ).toBeNull() + }) + + it('mirrors isDashboardSnapshot on every snapshot-level rejection', () => { + const cases: unknown[] = [ + { ...SNAPSHOT, generatedAt: Number.NaN }, + { ...SNAPSHOT, cards: 'nope' }, + { ...SNAPSHOT, repoIconsByRepoId: [] }, + { ...SNAPSHOT, showIdle: 'yes' }, + { ...SNAPSHOT, filterOptions: { projects: [], workspaceStatuses: 'nope' } }, + null, + [] + ] + for (const value of cases) { + expect(isDashboardSnapshot(value)).toBe(false) + expect(admitDashboardSnapshot(value)).toBeNull() + } + }) + }) + + // Why: sanitizing an image icon decodes the whole payload to read a 24-byte + // header, and the renderer republishes the same icons every 250 ms. + it('validates a repeated image icon without re-decoding it every publish', () => { + const src = imageIconSrc(256 * 1024) + const snapshot = { + ...SNAPSHOT, + repoIconsByRepoId: Object.fromEntries( + Array.from({ length: 10 }, (_, index) => [ + `repo-${index}`, + { type: 'image', src, source: 'upload' } + ]) + ) + } + sanitizeRepoIconCalls.mockClear() + + for (let publish = 0; publish < 20; publish += 1) { + expect(isDashboardSnapshot(snapshot)).toBe(true) + } + + // 10 repos x 20 publishes = 200 icon checks against ONE decode. + expect(sanitizeRepoIconCalls).toHaveBeenCalledTimes(1) + }) + + it('re-decodes when the icon payload or its source actually changes', () => { + const first = { type: 'image', src: imageIconSrc(1_024), source: 'upload' } + const second = { type: 'image', src: imageIconSrc(2_048), source: 'upload' } + // Same bytes, different source: `source` picks which src pattern is legal, + // so it must not collide with the first entry's cached verdict. + const rebranded = { ...first, source: 'file' } + sanitizeRepoIconCalls.mockClear() + + for (const icon of [first, first, second, second, rebranded, rebranded]) { + isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: { 'repo-1': icon } }) + } + + expect(sanitizeRepoIconCalls).toHaveBeenCalledTimes(3) + }) + + it('caches a rejection too, so a bad icon cannot be re-decoded every publish', () => { + const icon = { + type: 'image', + src: `${imageIconSrc(1_024)}`.replace('png', 'gif'), + source: 'upload' + } + sanitizeRepoIconCalls.mockClear() + + for (let publish = 0; publish < 5; publish += 1) { + expect(isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: { 'repo-1': icon } })).toBe( + false + ) + } + + expect(sanitizeRepoIconCalls).toHaveBeenCalledTimes(1) + }) + + it('does not let a split of one icon key inherit another icon verdict', () => { + // A valid base64 src may contain whitespace, so `source` and `src` must not + // be joined ambiguously: this pair concatenates identically. + const accepted = { type: 'image', src: imageIconSrc(1_024, true), source: 'upload' } + const joined = `upload ${accepted.src}` + const splitAt = joined.indexOf(' ', 'upload '.length) + const forged = { + type: 'image', + source: joined.slice(0, splitAt), + src: joined.slice(splitAt + 1) + } + sanitizeRepoIconCalls.mockClear() + + expect(isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: { 'repo-1': accepted } })).toBe( + true + ) + // `forged.source` is not a legal RepoIconImageSource, so it must be rejected + // no matter what the accepted icon left in the cache. + expect(isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: { 'repo-1': forged } })).toBe( + false + ) + expect(sanitizeRepoIconCalls).toHaveBeenCalledTimes(2) + }) + + it('does not let a cached image verdict answer for a non-image icon', () => { + const emoji = { type: 'emoji', emoji: '🦑' } + sanitizeRepoIconCalls.mockClear() + + for (let publish = 0; publish < 3; publish += 1) { + expect(isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: { 'repo-1': emoji } })).toBe( + true + ) + } + + // Cheap branches bypass the cache entirely rather than sharing its keyspace. + expect(sanitizeRepoIconCalls).toHaveBeenCalledTimes(3) + }) + it('requires complete bounded reveal routing', () => { expect( isDashboardRevealAgentArgs({ diff --git a/src/main/ipc/dashboard-payload-validation.ts b/src/main/ipc/dashboard-payload-validation.ts index add3e182856..00c62b10554 100644 --- a/src/main/ipc/dashboard-payload-validation.ts +++ b/src/main/ipc/dashboard-payload-validation.ts @@ -1,4 +1,9 @@ -import type { DashboardRevealAgentArgs, DashboardSnapshot } from '../../shared/dashboard-snapshot' +import { + DASHBOARD_MAX_LABEL_LENGTH, + type DashboardRevealAgentArgs, + type DashboardSnapshot +} from '../../shared/dashboard-snapshot' +import { BoundedMap } from '../../shared/bounded-map' import { sanitizeRepoIcon } from '../../shared/repo-icon' import { AGENT_STATUS_ASSISTANT_MESSAGE_MAX_LENGTH, @@ -11,8 +16,16 @@ const MAX_DASHBOARD_CARDS = 1_000 const MAX_DASHBOARD_SUBAGENTS = 100 const MAX_DASHBOARD_REPO_ICONS = 500 const MAX_DASHBOARD_FILTER_OPTIONS = 500 +// Why: sanitizing an image icon base64-decodes the whole data URI to read a +// 24-byte header, and the renderer republishes the same icons every 250 ms. +const MAX_CACHED_ICON_SRC_BYTES = 8 * 1024 * 1024 +const imageIconValidity = new BoundedMap({ + maxEntries: MAX_DASHBOARD_REPO_ICONS, + maxBytes: MAX_CACHED_ICON_SRC_BYTES, + sizeOf: (_valid, key) => key.length * 2 +}) const MAX_ID_LENGTH = 4_096 -const MAX_LABEL_LENGTH = 1_024 +const MAX_LABEL_LENGTH = DASHBOARD_MAX_LABEL_LENGTH const DASHBOARD_BUCKETS = new Set(['attention', 'working', 'done', 'idle']) const DASHBOARD_DOT_STATES = new Set(['working', 'blocked', 'waiting', 'done', 'idle']) const DASHBOARD_REVIEW_STATES = new Set(['open', 'closed', 'merged', 'draft']) @@ -62,6 +75,41 @@ export function isDashboardSnapshot(value: unknown): value is DashboardSnapshot ) } +export type DashboardSnapshotAdmission = { + snapshot: DashboardSnapshot + /** Cards dropped for failing validation; the rest of the board still paints. */ + droppedCardCount: number +} + +/** + * Why: one malformed card used to reject the whole snapshot, and the pop-out + * then replayed its last good board forever with nothing logged. A single + * over-long label must cost that card, not every other agent's live status. + * Snapshot-level fields still reject outright — there is no partial board to + * salvage when the frame itself is unusable. + */ +export function admitDashboardSnapshot(value: unknown): DashboardSnapshotAdmission | null { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + return null + } + const snapshot = value as Record + if ( + !isFiniteNumber(snapshot.generatedAt) || + !Array.isArray(snapshot.cards) || + snapshot.cards.length > MAX_DASHBOARD_CARDS || + (snapshot.showIdle !== undefined && typeof snapshot.showIdle !== 'boolean') || + !isDashboardFilterOptions(snapshot.filterOptions) || + !isDashboardRepoIcons(snapshot.repoIconsByRepoId) + ) { + return null + } + const cards = snapshot.cards.filter(isDashboardCard) + return { + snapshot: { ...(snapshot as unknown as DashboardSnapshot), cards }, + droppedCardCount: snapshot.cards.length - cards.length + } +} + function isDashboardFilterOptions(value: unknown): boolean { if (value === undefined) { return true @@ -107,9 +155,7 @@ function isDashboardRepoIcons(value: unknown): boolean { return ( entries.length <= MAX_DASHBOARD_REPO_ICONS && entries.every( - ([repoId, icon]) => - isBoundedString(repoId, MAX_ID_LENGTH) && - (icon === null || sanitizeRepoIcon(icon) !== undefined) + ([repoId, icon]) => isBoundedString(repoId, MAX_ID_LENGTH) && (icon === null || isIcon(icon)) ) ) } @@ -152,6 +198,38 @@ function isDashboardSubagents(value: unknown): boolean { ) } +/** Memoizes only the image branch, whose cost is proportional to payload size. */ +function isIcon(icon: unknown): boolean { + const key = imageIconCacheKey(icon) + if (key === null) { + return sanitizeRepoIcon(icon) !== undefined + } + const cached = imageIconValidity.get(key) + if (cached !== undefined) { + return cached + } + const valid = sanitizeRepoIcon(icon) !== undefined + imageIconValidity.set(key, valid) + return valid +} + +/** Cache key for an image icon; null when the verdict is already cheap. */ +function imageIconCacheKey(icon: unknown): string | null { + if (!icon || typeof icon !== 'object' || Array.isArray(icon)) { + return null + } + const candidate = icon as Record + // Why: `source` and `src` are the only fields an image icon can be rejected + // on — `label` is normalized rather than rejected — so they alone key it. + // Length-prefixed because a valid `src` may contain whitespace, so a plain + // separator would let a rejected icon collide with an accepted one's verdict. + return candidate.type === 'image' && + typeof candidate.src === 'string' && + typeof candidate.source === 'string' + ? `${candidate.source.length}:${candidate.source}${candidate.src}` + : null +} + function isDashboardCard(value: unknown): boolean { if (!value || typeof value !== 'object' || Array.isArray(value)) { return false diff --git a/src/main/ipc/dashboard-popout.test.ts b/src/main/ipc/dashboard-popout.test.ts index 96631d2d6b3..d516faa4554 100644 --- a/src/main/ipc/dashboard-popout.test.ts +++ b/src/main/ipc/dashboard-popout.test.ts @@ -53,6 +53,24 @@ const mainSender = { id: 1, send: vi.fn() } const popoutSender = { id: 2, send: vi.fn() } const untrustedSender = { id: 3, send: vi.fn() } const SNAPSHOT = { generatedAt: 1, cards: [] } +const CARD = { + paneKey: 'tab-1:leaf-1', + ptyId: 'pty-1', + agentType: 'codex', + bucket: 'working', + dotState: 'working', + task: 'Ship it', + repoId: 'repo-1', + worktreeId: 'worktree-1', + tabId: 'tab-1', + leafId: 'leaf-1', + repoName: 'Orca', + worktreeName: 'Dashboard', + startedAt: 1, + finishedAt: null, + stateChangedAt: 1, + unseen: false +} function makeWindow(sender: typeof mainSender) { return { @@ -131,6 +149,40 @@ describe('registerDashboardPopoutHandlers', () => { expect(popoutSender.send).toHaveBeenCalledWith('dashboard:snapshot', SNAPSHOT) }) + // Why: an over-long label used to drop the whole snapshot silently, leaving + // the board frozen on its last good paint with nothing logged. + it('keeps publishing the board when one card is invalid, and says so', () => { + const popout = makeWindow(popoutSender) + getPopoutMock.mockReturnValue(popout) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + const good = { ...CARD, paneKey: 'tab-1:leaf-1' } + const bad = { ...CARD, paneKey: 'tab-2:leaf-2', conversationName: 'x'.repeat(1_025) } + + handlers.get('dashboard:publishSnapshot')!({ sender: mainSender } as never, { + generatedAt: 2, + cards: [good, bad] + }) + + expect(popoutSender.send).toHaveBeenCalledWith('dashboard:snapshot', { + generatedAt: 2, + cards: [good] + }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('dropped 1 invalid card')) + warn.mockRestore() + }) + + it('logs when a snapshot is rejected outright', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + + handlers.get('dashboard:publishSnapshot')!({ sender: mainSender } as never, { + generatedAt: Number.NaN, + cards: [] + }) + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('rejected malformed snapshot')) + warn.mockRestore() + }) + it('replays the cached snapshot only to the popout and nudges only the trusted main renderer', () => { const popout = makeWindow(popoutSender) getPopoutMock.mockReturnValue(popout) diff --git a/src/main/ipc/dashboard-popout.ts b/src/main/ipc/dashboard-popout.ts index 7c794041cf0..c7a26421878 100644 --- a/src/main/ipc/dashboard-popout.ts +++ b/src/main/ipc/dashboard-popout.ts @@ -12,9 +12,9 @@ import { import { safelyRevealWindow } from '../window/focus-existing-window' import { getTrustedUIRendererWindow, isTrustedUIRenderer, sendToTrustedUIRenderer } from './ui' import { + admitDashboardSnapshot, isDashboardPaneKey, - isDashboardRevealAgentArgs, - isDashboardSnapshot + isDashboardRevealAgentArgs } from './dashboard-payload-validation' // The most recent snapshot the main renderer published, replayed to the popout @@ -63,22 +63,28 @@ export function registerDashboardPopoutHandlers( // Relay: the main renderer publishes derived snapshots; forward to the popout. ipcMain.handle('dashboard:publishSnapshot', (event, snapshot: unknown): void => { - if ( - !isTrustedUIRenderer(event.sender) || - !isDashboardEnabled(store) || - !isDashboardSnapshot(snapshot) - ) { + if (!isTrustedUIRenderer(event.sender) || !isDashboardEnabled(store)) { return } + const admitted = admitDashboardSnapshot(snapshot) + // Why: silently dropping left the pop-out replaying a stale board with no + // trace of why it stopped updating. + if (!admitted) { + console.warn('[dashboard] rejected malformed snapshot; pop-out keeps its previous board') + return + } + if (admitted.droppedCardCount > 0) { + console.warn(`[dashboard] dropped ${admitted.droppedCardCount} invalid card(s) from snapshot`) + } // The renderer omits repoIconsByRepoId once it is unchanged, so carry the // last map into the cache — a popout mounting mid-session is replayed this // and has no icons of its own to retain. The live popout does, so what is // forwarded stays as slim as the renderer sent it. lastSnapshot = - snapshot.repoIconsByRepoId === undefined && lastSnapshot?.repoIconsByRepoId - ? { ...snapshot, repoIconsByRepoId: lastSnapshot.repoIconsByRepoId } - : snapshot - getDashboardPopoutWindow()?.webContents.send('dashboard:snapshot', snapshot) + admitted.snapshot.repoIconsByRepoId === undefined && lastSnapshot?.repoIconsByRepoId + ? { ...admitted.snapshot, repoIconsByRepoId: lastSnapshot.repoIconsByRepoId } + : admitted.snapshot + getDashboardPopoutWindow()?.webContents.send('dashboard:snapshot', admitted.snapshot) }) // The popout asks for a snapshot on mount: replay the cache immediately, then diff --git a/src/renderer/src/components/dashboard/build-dashboard-snapshot.test.ts b/src/renderer/src/components/dashboard/build-dashboard-snapshot.test.ts index c72e1dc4ac2..a2821b094c8 100644 --- a/src/renderer/src/components/dashboard/build-dashboard-snapshot.test.ts +++ b/src/renderer/src/components/dashboard/build-dashboard-snapshot.test.ts @@ -5,6 +5,7 @@ import { type AgentStatusEntry, type AgentStatusOrchestrationContext } from '../../../../shared/agent-status-types' +import { DASHBOARD_MAX_LABEL_LENGTH } from '../../../../shared/dashboard-snapshot' import { makePaneKey } from '../../../../shared/stable-pane-id' import type { TerminalTab, Worktree } from '../../../../shared/types' import { selectRuntimeAgentOrchestrationBatch } from '../sidebar/worktree-agent-orchestration-batch' @@ -164,6 +165,35 @@ describe('buildDashboardSnapshot', () => { expect(unnamed.cards[0].conversationName).toBeUndefined() }) + // Why: `orca terminal rename --title` is unbounded, and the main-process + // validator drops any card whose label exceeds the shared bound. + it('truncates labels to the length the snapshot validator accepts', () => { + const snapshot = buildDashboardSnapshot( + baseState({ + agentStatusByPaneKey: { [PANE_KEY]: entry({}) }, + tabsByWorktree: { w1: [{ ...tab(), customTitle: 'x'.repeat(5_000) }] } + }), + NOW + ) + + expect(snapshot.cards[0].conversationName).toHaveLength(DASHBOARD_MAX_LABEL_LENGTH) + }) + + // Why: filterOptions is snapshot-level, so an over-long project label is not + // recoverable by dropping a card — it invalidates the entire board. + it('truncates the project filter label the whole snapshot rides on', () => { + const snapshot = buildDashboardSnapshot( + baseState({ + repos: [ + { id: 'r1', path: '/r1', displayName: 'x'.repeat(5_000), badgeColor: '#000', addedAt: 0 } + ] + }), + NOW + ) + + expect(snapshot.filterOptions?.projects[0].label).toHaveLength(DASHBOARD_MAX_LABEL_LENGTH) + }) + it('withholds generated titles until the setting enables them', () => { const tabs = { w1: [{ ...tab(), generatedTitle: 'Fix the flaky pty test' }] } const off = buildDashboardSnapshot( diff --git a/src/renderer/src/components/dashboard/build-dashboard-snapshot.ts b/src/renderer/src/components/dashboard/build-dashboard-snapshot.ts index aef1ea24d48..e51313ad16c 100644 --- a/src/renderer/src/components/dashboard/build-dashboard-snapshot.ts +++ b/src/renderer/src/components/dashboard/build-dashboard-snapshot.ts @@ -1,10 +1,11 @@ import type { AppState } from '@/store/types' -import type { - DashboardBucket, - DashboardCard, - DashboardCardDotState, - DashboardCardSubagent, - DashboardSnapshot +import { + DASHBOARD_MAX_LABEL_LENGTH, + type DashboardBucket, + type DashboardCard, + type DashboardCardDotState, + type DashboardCardSubagent, + type DashboardSnapshot } from '../../../../shared/dashboard-snapshot' import type { RepoIcon } from '../../../../shared/repo-icon' import { DEFAULT_WORKSPACE_STATUSES } from '../../../../shared/workspace-statuses' @@ -79,6 +80,18 @@ function nonEmpty(value: string | undefined): string | undefined { return trimmed.length > 0 ? trimmed : undefined } +/** Why: these labels come from unbounded sources (`terminal rename`, OSC titles, + * display names). Over the validator's bound the card would be dropped. */ +function boundedLabel(value: string): string { + return value.length > DASHBOARD_MAX_LABEL_LENGTH + ? value.slice(0, DASHBOARD_MAX_LABEL_LENGTH) + : value +} + +function boundedLabelOrUndefined(value: string | undefined): string | undefined { + return value === undefined ? undefined : boundedLabel(value) +} + /** Mirrors useAgentRowConversationName so the board and the sidebar label the * same agent with the same name. */ function rowConversationName( @@ -129,8 +142,10 @@ export function buildDashboardSnapshot( options.includeFilterOptions === false ? undefined : { + // Why: filterOptions is snapshot-level, so an over-long project label + // costs the WHOLE board, not one card. Bound it at the producer. projects: [...new Map(activeWorktrees.map(({ repo }) => [repo.id, repo])).values()].map( - (repo) => ({ id: repo.id, label: repo.displayName }) + (repo) => ({ id: repo.id, label: boundedLabel(repo.displayName) }) ), workspaceStatuses: (state.workspaceStatuses && state.workspaceStatuses.length > 0 ? state.workspaceStatuses @@ -261,8 +276,8 @@ export function buildDashboardSnapshot( worktreeId, tabId, leafId, - repoName: repo.displayName, - worktreeName: worktree.displayName, + repoName: boundedLabel(repo.displayName), + worktreeName: boundedLabel(worktree.displayName), workspaceStatusId: context?.workspaceStatus.id, workspaceStatusLabel: context?.workspaceStatus.label, workspaceStatusColor: context?.workspaceStatus.color, @@ -280,7 +295,7 @@ export function buildDashboardSnapshot( !isTitleDerived && (state.acknowledgedAgentsByPaneKey?.[row.paneKey] ?? 0) < row.entry.stateStartedAt, askSummary: bucket === 'attention' ? (row.entry.interactivePrompt ?? undefined) : undefined, - conversationName: rowConversationName(row, generatedTitlesEnabled) + conversationName: boundedLabelOrUndefined(rowConversationName(row, generatedTitlesEnabled)) }) } } diff --git a/src/shared/dashboard-snapshot.ts b/src/shared/dashboard-snapshot.ts index 376760bff73..5b70bf35802 100644 --- a/src/shared/dashboard-snapshot.ts +++ b/src/shared/dashboard-snapshot.ts @@ -19,6 +19,11 @@ export const DASHBOARD_BUCKET_ORDER: readonly DashboardBucket[] = [ 'idle' ] +/** Max length of a card's display labels. The producer truncates to this and + * the main-process validator enforces it, so an unbounded name (a long + * `terminal rename`, an OSC title) cannot cost the card its place on the board. */ +export const DASHBOARD_MAX_LABEL_LENGTH = 1_024 + /** Kept distinct from `bucket` so attention cards retain their precise dot state. */ export type DashboardCardDotState = 'working' | 'blocked' | 'waiting' | 'done' | 'idle' diff --git a/src/shared/workspace-space-entry-traversal.test.ts b/src/shared/workspace-space-entry-traversal.test.ts index af57130f2e3..38019cb94d0 100644 --- a/src/shared/workspace-space-entry-traversal.test.ts +++ b/src/shared/workspace-space-entry-traversal.test.ts @@ -10,12 +10,13 @@ function makeTraversal( kind: 'directory' | 'file' | 'symlink' sizeBytes: number }>, - limits?: { maxEntries?: number; maxRetainedBytes?: number } + limits?: { maxEntries?: number; maxRetainedBytes?: number }, + concurrency = 5 ) { return scanWorkspaceSpaceEntryTree({ rootPath: '/root', rootName: 'root', - concurrency: 5, + concurrency, entryName: (entry: Entry) => entry.name, joinPath: (parent, child) => `${parent}/${child}`, classifyEntry: (path) => classifyEntry(path), @@ -157,8 +158,6 @@ describe('scanWorkspaceSpaceEntryTree', () => { ]) }) - // Why: the cap bounds entries held at once. A deep walk retires each listing - // as it descends, so total tree size must not decide the outcome. it('scans a chain far longer than the cap because listings are released', async () => { const depth = 50 const directories = new Map() @@ -189,4 +188,124 @@ describe('scanWorkspaceSpaceEntryTree', () => { await expect(scan).rejects.toBeInstanceOf(WorkspaceSpaceScanCapacityError) }) + + // Why: a cap N workers can each charge scales the verdict with concurrency, + // which docs/workspace-space-scan-resource-bounds.md forbids. + describe('capacity depends on directory shape, not tree size or concurrency', () => { + function buildWideTree(dirCount: number, filesPerDir: number) { + const directories = new Map() + directories.set( + '/root', + Array.from({ length: dirCount }, (_, index) => ({ name: `dir-${index}` })) + ) + for (let index = 0; index < dirCount; index += 1) { + directories.set( + `/root/dir-${index}`, + Array.from({ length: filesPerDir }, (_, file) => ({ name: `file-${file}` })) + ) + } + return directories + } + + function scanWideTree(dirCount: number, filesPerDir: number, concurrency: number) { + const directories = buildWideTree(dirCount, filesPerDir) + return makeTraversal( + directories, + async (path) => ({ + kind: directories.has(path) ? 'directory' : 'file', + sizeBytes: 1 + }), + { maxEntries: 100_000, maxRetainedBytes: Number.MAX_SAFE_INTEGER }, + concurrency + ) + } + + it.each([ + { dirCount: 48, filesPerDir: 2_100, concurrency: 48 }, + { dirCount: 100, filesPerDir: 1_500, concurrency: 48 }, + { dirCount: 10, filesPerDir: 10_001, concurrency: 10 }, + { dirCount: 40, filesPerDir: 2_500, concurrency: 48 } + ])( + 'scans $dirCount x $filesPerDir at concurrency $concurrency', + async ({ dirCount, filesPerDir, concurrency }) => { + const result = await scanWideTree(dirCount, filesPerDir, concurrency) + expect(result.sizeBytes).toBe(dirCount * filesPerDir + dirCount + 1) + expect(result.skippedEntryCount).toBe(0) + }, + 30_000 + ) + + it('still rejects a single directory above the cap', async () => { + await expect(scanWideTree(1, 100_001, 48)).rejects.toBeInstanceOf( + WorkspaceSpaceScanCapacityError + ) + }, 30_000) + }) + + // Why: the cases above pin maxRetainedBytes open, so only these see the byte + // cap — and its verdict must not depend on where the worktree is checked out. + describe('capacity at the production default limits', () => { + // A real worktree checkout path; the bug appeared above ~58 characters. + const DEEP_ROOT = '/Users/octocat/projects/orca/.claude/worktrees/wf_d39acf3c-e7d-2' + + function scanAtDefaults( + rootPath: string, + dirCount: number, + filesPerDir: number, + concurrency: number + ) { + const directories = new Map() + directories.set( + rootPath, + Array.from({ length: dirCount }, (_, index) => ({ name: `dir-${index}` })) + ) + for (let index = 0; index < dirCount; index += 1) { + directories.set( + `${rootPath}/dir-${index}`, + Array.from({ length: filesPerDir }, (_, file) => ({ name: `file-${file}.ts` })) + ) + } + return scanWorkspaceSpaceEntryTree({ + rootPath, + rootName: 'root', + concurrency, + entryName: (entry: Entry) => entry.name, + joinPath: (parent, child) => `${parent}/${child}`, + classifyEntry: async (path) => ({ + kind: directories.has(path) ? ('directory' as const) : ('file' as const), + sizeBytes: 1 + }), + readDirectory: async (path) => { + const entries = directories.get(path) + if (!entries) { + throw new Error(`unreadable ${path}`) + } + return entries + }, + checkCancelled: () => undefined, + createCancellationError: () => new Error('cancelled'), + isCancellationError: (error) => error instanceof Error && error.message === 'cancelled' + }) + } + + it.each([ + { dirCount: 48, filesPerDir: 2_100, concurrency: 48 }, + { dirCount: 10, filesPerDir: 10_001, concurrency: 10 }, + { dirCount: 40, filesPerDir: 2_500, concurrency: 48 } + ])( + 'scans $dirCount x $filesPerDir at concurrency $concurrency under a deep root', + async ({ dirCount, filesPerDir, concurrency }) => { + const result = await scanAtDefaults(DEEP_ROOT, dirCount, filesPerDir, concurrency) + expect(result.sizeBytes).toBe(dirCount * filesPerDir + dirCount + 1) + }, + 30_000 + ) + + it('reaches the same verdict under a short root as under a deep one', async () => { + const shallow = await scanAtDefaults('/w', 48, 2_100, 48) + const deep = await scanAtDefaults(DEEP_ROOT, 48, 2_100, 48) + + expect(deep.sizeBytes).toBe(shallow.sizeBytes) + }, 30_000) + }) }) diff --git a/src/shared/workspace-space-entry-traversal.ts b/src/shared/workspace-space-entry-traversal.ts index ba92d6ecd82..fe0695176e6 100644 --- a/src/shared/workspace-space-entry-traversal.ts +++ b/src/shared/workspace-space-entry-traversal.ts @@ -120,7 +120,7 @@ export async function scanWorkspaceSpaceEntryTree( return } frame.retired = true - releaseWorkspaceSpaceScanEntries(budget, frame.entries.length, frame.retainedBytes) + releaseWorkspaceSpaceScanEntries(budget, frame.retainedBytes) frame.entries = [] frame.retainedBytes = 0 } diff --git a/src/shared/workspace-space-scan-budget.test.ts b/src/shared/workspace-space-scan-budget.test.ts index 71cbe651f36..2cd3ac61892 100644 --- a/src/shared/workspace-space-scan-budget.test.ts +++ b/src/shared/workspace-space-scan-budget.test.ts @@ -3,6 +3,7 @@ import { collectWorkspaceSpaceDirectoryEntries, createWorkspaceSpaceScanBudget, estimateWorkspaceSpaceEntryRetainedBytes, + estimateWorkspaceSpaceListingRetainedBytes, releaseWorkspaceSpaceScanEntries, WorkspaceSpaceScanCapacityError } from './workspace-space-scan-budget' @@ -12,8 +13,8 @@ describe('workspace space scan budget', () => { const entries = [{ name: 'first' }, { name: 'second' }] const parentPath = '/workspace' const exactBytes = entries.reduce( - (total, entry) => total + estimateWorkspaceSpaceEntryRetainedBytes(parentPath, entry.name), - 0 + (total, entry) => total + estimateWorkspaceSpaceEntryRetainedBytes(entry.name), + estimateWorkspaceSpaceListingRetainedBytes(parentPath) ) await expect( @@ -43,15 +44,15 @@ describe('workspace space scan budget', () => { () => undefined ) ).rejects.toThrow('readdir exploded') - expect(budget).toMatchObject({ entries: 0, retainedBytes: 0 }) + expect(budget).toMatchObject({ retainedBytes: 0 }) }) it('frees capacity for later directories once a listing is released', async () => { const parentPath = '/workspace' const entries = [{ name: 'first' }, { name: 'second' }] const exactBytes = entries.reduce( - (total, entry) => total + estimateWorkspaceSpaceEntryRetainedBytes(parentPath, entry.name), - 0 + (total, entry) => total + estimateWorkspaceSpaceEntryRetainedBytes(entry.name), + estimateWorkspaceSpaceListingRetainedBytes(parentPath) ) const budget = createWorkspaceSpaceScanBudget({ maxRetainedBytes: exactBytes }) @@ -63,7 +64,7 @@ describe('workspace space scan budget', () => { () => undefined ) // Why: a cumulative counter would reject the identical second listing here. - releaseWorkspaceSpaceScanEntries(budget, first.entries.length, first.retainedBytes) + releaseWorkspaceSpaceScanEntries(budget, first.retainedBytes) await expect( collectWorkspaceSpaceDirectoryEntries( diff --git a/src/shared/workspace-space-scan-budget.ts b/src/shared/workspace-space-scan-budget.ts index 65027f917cb..cef579a88cd 100644 --- a/src/shared/workspace-space-scan-budget.ts +++ b/src/shared/workspace-space-scan-budget.ts @@ -9,12 +9,17 @@ export type WorkspaceSpaceScanLimits = { } /** - * Tracks entries the traversal is holding right now, not entries it has ever - * seen. Counters fall again as directory listings are dispatched and dropped, - * so the caps bound live heap rather than total tree size. + * Tracks what the traversal is holding right now, not what it has ever seen: + * `retainedBytes` falls again as listings are dispatched and dropped, so the + * cap bounds live heap rather than total tree size. + * + * `maxEntries` bounds ONE listing, not the traversal — a directory's width is + * the only entry count fixed by shape. A traversal-wide entry counter is + * charged by every worker holding a listing at once, so its verdict scaled with + * concurrency and rejected intact worktrees; aggregate live cost is bounded by + * `maxRetainedBytes` instead. Do not reintroduce a traversal-wide entry count. */ export type WorkspaceSpaceScanBudget = { - entries: number retainedBytes: number limits: WorkspaceSpaceScanLimits } @@ -39,7 +44,6 @@ export function createWorkspaceSpaceScanBudget( requested?: Partial ): WorkspaceSpaceScanBudget { return { - entries: 0, retainedBytes: 0, limits: { maxEntries: clampLimit(requested?.maxEntries, WORKSPACE_SPACE_MAX_SCANNED_ENTRIES), @@ -51,38 +55,39 @@ export function createWorkspaceSpaceScanBudget( } } -export function estimateWorkspaceSpaceEntryRetainedBytes( - parentPath: string, - entryName: string -): number { - return (parentPath.length + entryName.length) * 2 + WORKSPACE_SPACE_ENTRY_OVERHEAD_BYTES +export function estimateWorkspaceSpaceEntryRetainedBytes(entryName: string): number { + return entryName.length * 2 + WORKSPACE_SPACE_ENTRY_OVERHEAD_BYTES +} + +/** Why: a listing's entries share one parent-path string, so charging it per + * entry scaled the estimate by checkout depth rather than live heap. */ +export function estimateWorkspaceSpaceListingRetainedBytes(parentPath: string): number { + return parentPath.length * 2 } export function retainWorkspaceSpaceScanEntry( budget: WorkspaceSpaceScanBudget, - parentPath: string, - entryName: string + entryName: string, + listingEntryCount: number, + additionalBytes = 0 ): void { const retainedBytes = - budget.retainedBytes + estimateWorkspaceSpaceEntryRetainedBytes(parentPath, entryName) + budget.retainedBytes + estimateWorkspaceSpaceEntryRetainedBytes(entryName) + additionalBytes if ( - budget.entries >= budget.limits.maxEntries || + listingEntryCount >= budget.limits.maxEntries || retainedBytes > budget.limits.maxRetainedBytes ) { throw new WorkspaceSpaceScanCapacityError(budget.limits) } - budget.entries += 1 budget.retainedBytes = retainedBytes } -// Why: callers must return a listing's charge once they drop it, so the caps -// track live retention instead of accumulating across the whole traversal. +// Why: callers must return a listing's charge once they drop it, so the cap +// tracks live retention instead of accumulating across the whole traversal. export function releaseWorkspaceSpaceScanEntries( budget: WorkspaceSpaceScanBudget, - entryCount: number, retainedBytes: number ): void { - budget.entries = Math.max(0, budget.entries - entryCount) budget.retainedBytes = Math.max(0, budget.retainedBytes - retainedBytes) } @@ -105,14 +110,18 @@ export async function collectWorkspaceSpaceDirectoryEntries( for await (const entry of directory) { checkCancelled() const name = entryName(entry) - retainWorkspaceSpaceScanEntry(budget, parentPath, name) - retainedBytes += estimateWorkspaceSpaceEntryRetainedBytes(parentPath, name) + // The listing's shared parent path is charged once, with its first entry, + // so an empty listing holds no charge for the caller to release. + const listingBytes = + entries.length === 0 ? estimateWorkspaceSpaceListingRetainedBytes(parentPath) : 0 + retainWorkspaceSpaceScanEntry(budget, name, entries.length, listingBytes) + retainedBytes += estimateWorkspaceSpaceEntryRetainedBytes(name) + listingBytes entries.push(entry) } } catch (error) { // Why: a rejected or cancelled listing is never handed to the caller, so // nothing would otherwise return the charge already taken for it. - releaseWorkspaceSpaceScanEntries(budget, entries.length, retainedBytes) + releaseWorkspaceSpaceScanEntries(budget, retainedBytes) throw error } return { entries, retainedBytes }