diff --git a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts index 83aab9d2fab..f1d1e24dc44 100644 --- a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts +++ b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts @@ -298,6 +298,49 @@ describe('renderer breadcrumb IPC routing', () => { ]) }) + // Why: the renderer guard is once-per-tab-id, so one stale worktree map emits + // a crumb per duplicated tab and would evict the pre-crash trail. + it('coalesces duplicate-tab-owner notices across tabs', () => { + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: true } + }) + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 3, resolvedToActiveWorktree: true } + }) + + expect(recordCrashBreadcrumbMock).not.toHaveBeenCalled() + expect(recordCoalescedCrashBreadcrumbMock).toHaveBeenCalledTimes(2) + for (const call of recordCoalescedCrashBreadcrumbMock.mock.calls) { + expect(call[0]).toMatchObject({ + coalesceKey: 'terminal_tab_id_owned_by_multiple_worktrees:true' + }) + } + }) + + // Why flag-scoped: coalescing keeps only the newest payload, and `false` is + // the only value saying the activation still could not converge activeTabId. + it('keeps a non-converging duplicate-tab-owner notice out of the converging one', () => { + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: false } + }) + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: true } + }) + + expect( + recordCoalescedCrashBreadcrumbMock.mock.calls.map( + (call) => (call[0] as { coalesceKey: string }).coalesceKey + ) + ).toEqual([ + 'terminal_tab_id_owned_by_multiple_worktrees:false', + 'terminal_tab_id_owned_by_multiple_worktrees:true' + ]) + }) + it('records non-error renderer breadcrumbs without coalescing', () => { emitRendererBreadcrumb({ name: 'renderer_bootstrap_started', data: { dev: true } }) diff --git a/src/main/ipc/crash-reporting.ts b/src/main/ipc/crash-reporting.ts index 5a00669176c..65cc2a129c2 100644 --- a/src/main/ipc/crash-reporting.ts +++ b/src/main/ipc/crash-reporting.ts @@ -326,12 +326,13 @@ function buildUncapturedCrashReportText( // storm, #8260) can flush the whole fixed-size breadcrumb ring in seconds, // erasing the pre-crash trail. Coalesce repeats into one entry that carries a // suppressed count instead. +const DUPLICATE_TAB_OWNER_BREADCRUMB = 'terminal_tab_id_owned_by_multiple_worktrees' const COALESCED_RENDERER_BREADCRUMB_NAMES = new Set([ 'renderer_error', 'renderer_unhandled_rejection', 'terminal_park_verdict_churn', 'terminal_safe_fit_retry_exhausted', - 'terminal_tab_id_owned_by_multiple_worktrees', + DUPLICATE_TAB_OWNER_BREADCRUMB, TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB ]) const RENDERER_BREADCRUMB_COALESCE_MS = 30_000 @@ -344,14 +345,9 @@ const RENDERER_BREADCRUMB_COALESCE_MS = 30_000 // mounted pane within ~60ms. Windows crash F0BKR84AHEH lost 26-90% of its // 30-entry ring to two such bursts. `suppressedSinceLast` keeps the pane count // — the only signal these carry — in one slot. -// -// terminal_tab_id_owned_by_multiple_worktrees: its renderer-side guard is -// once-per-tab-id, not once-per-session, so one stale worktree map duplicates -// every tab id at once and emits a crumb per tab. const NAME_ONLY_COALESCED_BREADCRUMB_NAMES = new Set([ 'terminal_park_verdict_churn', - 'terminal_safe_fit_retry_exhausted', - 'terminal_tab_id_owned_by_multiple_worktrees' + 'terminal_safe_fit_retry_exhausted' ]) function rendererBreadcrumbCoalesceKey( @@ -368,6 +364,16 @@ function rendererBreadcrumbCoalesceKey( if (name === TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB) { return `${name}:${String(data?.kind ?? '')}` } + // Why coalesce at all: the renderer guard is once-per-tab-id, not + // once-per-session, so one stale worktree map duplicates every tab id at once + // and emits a crumb per tab. Why keyed on the flag rather than name alone: + // `false` is the only value that says the activation still could not converge + // activeTabId, and coalescing keeps just the newest payload — a later benign + // `true` would erase the sample the crumb exists to capture. Two keys still + // bound the storm. + if (name === DUPLICATE_TAB_OWNER_BREADCRUMB) { + return `${name}:${String(data?.resolvedToActiveWorktree ?? '')}` + } const primaryMessage = name === 'renderer_error' ? data?.message : data?.reasonMessage const fallbackMessage = name === 'renderer_error' ? data?.errorMessage : undefined const message = diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts index ae1e78459bd..d31e037e8fc 100644 --- a/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts @@ -6,9 +6,8 @@ vi.mock('../../lib/crash-breadcrumb-recorder', () => ({ recordRendererCrashBreadcrumb: (...args: unknown[]) => recordRendererCrashBreadcrumb(...args) })) -const { resolveActiveTabOwnerWorktreeId, _resetDuplicateTabOwnerBreadcrumbsForTests } = await import( - './active-tab-owner-worktree' -) +const { resolveActiveTabOwnerWorktreeId, _resetDuplicateTabOwnerBreadcrumbsForTests } = + await import('./active-tab-owner-worktree') function tab(id: string, worktreeId: string): TerminalTab { return { id, worktreeId, title: id, createdAt: 0, sortOrder: 0 } as unknown as TerminalTab @@ -62,6 +61,16 @@ describe('resolveActiveTabOwnerWorktreeId', () => { ) }) + // Why: a truthiness guard on the active id would drop this back to first-match. + it('prefers a falsy-but-valid active worktree id', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-other': [tab('t1', 'wt-other')], '': [tab('t1', '')] }, + '', + 't1' + ) + expect(owner).toBe('') + }) + it('breadcrumbs a given tab id only once so it cannot flood the ring', () => { const maps = { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t1', 'wt-b')] } for (let i = 0; i < 5; i += 1) { diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.ts index 3d188ef9870..d2bdc7dd043 100644 --- a/src/renderer/src/store/slices/active-tab-owner-worktree.ts +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.ts @@ -25,7 +25,10 @@ export function resolveActiveTabOwnerWorktreeId( let ownerCount = 0 // Why tracked in-loop rather than re-read by key: `tabsByWorktree[activeWorktreeId]` // resolves inherited members for ids like `toString`, and `?.some` would then throw. - let activeWorktreeOwnsTab = false + // Why the id and not a boolean: a falsy-but-valid active id ('') would fail a + // truthiness guard below and silently fall back to the first match — the very + // misattribution this function exists to remove. + let activeOwnerId: string | null = null for (const [worktreeId, tabs] of Object.entries(tabsByWorktree)) { if (!tabs.some((tab) => tab.id === tabId)) { continue @@ -35,7 +38,7 @@ export function resolveActiveTabOwnerWorktreeId( firstOwnerId = worktreeId } if (worktreeId === activeWorktreeId) { - activeWorktreeOwnsTab = true + activeOwnerId = worktreeId } } @@ -45,12 +48,12 @@ export function resolveActiveTabOwnerWorktreeId( reportedDuplicateTabIds.add(tabId) recordRendererCrashBreadcrumb('terminal_tab_id_owned_by_multiple_worktrees', { ownerCount, - resolvedToActiveWorktree: activeWorktreeOwnsTab + resolvedToActiveWorktree: activeOwnerId !== null }) } - if (ownerCount > 1 && activeWorktreeId && activeWorktreeOwnsTab) { - return activeWorktreeId + if (ownerCount > 1 && activeOwnerId !== null) { + return activeOwnerId } return firstOwnerId }