From f230c2d33df51eafb61cd616200ae98b7fc59db5 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 17 Jun 2026 13:29:39 -0700 Subject: [PATCH] Bound concurrent repo scans and trace renderer breadcrumbs (#5608) - Limit worktree refresh concurrency to 5 parallel workers to prevent launching too many concurrent git scans during startup and activation. - Record renderer breadcrumbs as tracer spans to provide durable pre-crash context for native main-process crashes. - Log a breadcrumb when activating a worktree from the sidebar. --- src/main/ipc/crash-reporting.test.ts | 55 ++++++++++++++--- src/main/ipc/crash-reporting.ts | 29 +++++++-- .../src/components/sidebar/WorktreeCard.tsx | 9 +++ .../src/store/slices/worktrees.test.ts | 61 +++++++++++++++++++ src/renderer/src/store/slices/worktrees.ts | 38 ++++++++++-- 5 files changed, 174 insertions(+), 18 deletions(-) diff --git a/src/main/ipc/crash-reporting.test.ts b/src/main/ipc/crash-reporting.test.ts index 4d3dd7228..1c2059bb7 100644 --- a/src/main/ipc/crash-reporting.test.ts +++ b/src/main/ipc/crash-reporting.test.ts @@ -10,17 +10,32 @@ const { getDiagnosticsStatusMock, recordCrashBreadcrumbMock, resolveDiagnosticOrcaChannelMock, + spanEndMock, + startSpanMock, submitFeedbackMock -} = vi.hoisted(() => ({ - handlers: new Map unknown>(), - listeners: new Map void>(), - clipboardWriteTextMock: vi.fn(), - collectDiagnosticBundleMock: vi.fn(), - getDiagnosticsStatusMock: vi.fn(), - recordCrashBreadcrumbMock: vi.fn(), - resolveDiagnosticOrcaChannelMock: vi.fn(), - submitFeedbackMock: vi.fn() -})) +} = vi.hoisted(() => { + const spanEndMock = vi.fn() + return { + handlers: new Map unknown>(), + listeners: new Map void>(), + clipboardWriteTextMock: vi.fn(), + collectDiagnosticBundleMock: vi.fn(), + getDiagnosticsStatusMock: vi.fn(), + recordCrashBreadcrumbMock: vi.fn(), + resolveDiagnosticOrcaChannelMock: vi.fn(), + spanEndMock, + startSpanMock: vi.fn(() => ({ + traceId: 'trace-id', + spanId: 'span-id', + setAttribute: vi.fn(), + addEvent: vi.fn(), + fail: vi.fn(), + interrupt: vi.fn(), + end: spanEndMock + })), + submitFeedbackMock: vi.fn() + } +}) vi.mock('electron', () => ({ app: { getVersion: () => '1.2.3-test' }, @@ -55,6 +70,10 @@ vi.mock('../observability/diagnostic-upload-endpoint', () => ({ resolveDiagnosticOrcaChannel: resolveDiagnosticOrcaChannelMock })) +vi.mock('../observability/tracer', () => ({ + startSpan: startSpanMock +})) + import { _getCrashReportingStateSizesForTests, _resetRendererErrorReportDedupeForTests, @@ -110,6 +129,8 @@ describe('registerCrashReportingHandlers', () => { }) resolveDiagnosticOrcaChannelMock.mockReset() resolveDiagnosticOrcaChannelMock.mockReturnValue('stable') + startSpanMock.mockClear() + spanEndMock.mockClear() submitFeedbackMock.mockReset() recordCrashBreadcrumbMock.mockReset() submitFeedbackMock.mockResolvedValue({ ok: true }) @@ -698,6 +719,19 @@ describe('registerCrashReportingHandlers', () => { ok: true, empty: null }) + expect(startSpanMock).toHaveBeenCalledWith('renderer.breadcrumb', { + attributes: { + kind: 'crash-breadcrumb', + 'breadcrumb.name': 'renderer_error', + 'breadcrumb.data': { + message: 'boom', + count: 2, + ok: true, + empty: null + } + } + }) + expect(spanEndMock).toHaveBeenCalledTimes(1) }) it('ignores renderer breadcrumbs without a string name', () => { @@ -717,5 +751,6 @@ describe('registerCrashReportingHandlers', () => { }) expect(recordCrashBreadcrumbMock).not.toHaveBeenCalled() + expect(startSpanMock).not.toHaveBeenCalled() }) }) diff --git a/src/main/ipc/crash-reporting.ts b/src/main/ipc/crash-reporting.ts index 6a20e2081..46b361d92 100644 --- a/src/main/ipc/crash-reporting.ts +++ b/src/main/ipc/crash-reporting.ts @@ -11,6 +11,7 @@ import { type CrashReportSubmitResult, formatCrashReportText, formatUncapturedCrashReportText, + sanitizeCrashReportDetails, sanitizeCrashReportString } from '../../shared/crash-reporting' import { submitFeedback } from './feedback' @@ -21,6 +22,7 @@ import { } from '../crash-reporting/crash-breadcrumb-store' import { collectDiagnosticBundle, getDiagnosticsStatus } from '../observability' import { resolveDiagnosticOrcaChannel } from '../observability/diagnostic-upload-endpoint' +import { startSpan } from '../observability/tracer' import type { FeedbackDiagnosticBundleAttachment } from './feedback' const inFlightSubmissions = new Set() @@ -268,17 +270,34 @@ function sanitizeRendererBreadcrumbData(value: unknown): CrashReportBreadcrumbDa if (!value || typeof value !== 'object' || Array.isArray(value)) { return undefined } - const sanitized: CrashReportBreadcrumbData = {} + const primitiveData: Record = {} for (const [key, entry] of Object.entries(value)) { if (typeof entry === 'string' || typeof entry === 'boolean' || entry === null) { - sanitized[key] = entry + primitiveData[key] = entry } else if (typeof entry === 'number' && Number.isFinite(entry)) { - sanitized[key] = entry + primitiveData[key] = entry } } + const sanitized = sanitizeCrashReportDetails(primitiveData) return Object.keys(sanitized).length > 0 ? sanitized : undefined } +function recordRendererBreadcrumbTrace( + name: string, + data: CrashReportBreadcrumbData | undefined +): void { + const span = startSpan('renderer.breadcrumb', { + attributes: { + kind: 'crash-breadcrumb', + 'breadcrumb.name': sanitizeCrashReportString(name), + ...(data ? { 'breadcrumb.data': data } : {}) + } + }) + // Why: main-process native crashes cannot persist memory-only breadcrumbs. + // A tiny trace span gives the next crash report durable pre-crash context. + span.end() +} + function formatUnknownError(error: unknown): string { return sanitizeCrashReportString(error instanceof Error ? error.message : String(error)) } @@ -380,7 +399,9 @@ export function registerCrashReportingHandlers(store: CrashReportStore): void { if (!args || typeof args.name !== 'string') { return } - recordCrashBreadcrumb(args.name, sanitizeRendererBreadcrumbData(args.data)) + const data = sanitizeRendererBreadcrumbData(args.data) + recordCrashBreadcrumb(args.name, data) + recordRendererBreadcrumbTrace(args.name, data) } ) diff --git a/src/renderer/src/components/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index e7ac25dd6..8213cda50 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -64,6 +64,7 @@ import { DetachedHeadBadge } from '@/components/DetachedHeadBadge' import { getWorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display' import { getFlushWorktreeCardPaddingLeft } from './worktree-list-indentation' import { translate } from '@/i18n/i18n' +import { recordRendererCrashBreadcrumb } from '@/lib/crash-diagnostics' import { folderWorkspaceKey, parseWorkspaceKey } from '../../../../shared/workspace-scope' type WorktreeRenameRequest = { @@ -578,6 +579,12 @@ const WorktreeCard = React.memo(function WorktreeCard({ // Why: route sidebar clicks through the shared activation path so the // back/forward stack stays complete for the primary worktree navigation // surface instead of only recording palette-driven switches. + recordRendererCrashBreadcrumb('sidebar_worktree_activate', { + worktreeId: worktree.id, + repoId: worktree.repoId, + wasActive: isActive, + sshDisconnected: isSshDisconnected + }) onImmediateActivate?.(worktree.id, activationRowKey) activateWorktreeFromSidebar(worktree.id) if (isSshDisconnected) { @@ -588,6 +595,8 @@ const WorktreeCard = React.memo(function WorktreeCard({ [ affiliateListMode, worktree.id, + worktree.repoId, + isActive, isDeleting, activationRowKey, isSshDisconnected, diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index 5b72bdd2d..abd54ff6e 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -3449,6 +3449,67 @@ describe('fetchAllWorktrees hydration-time purge (design ยง4.4)', () => { expect(store.getState().hasHydratedWorktreePurge).toBe(true) }) + it('bounds concurrent repo scans during hydration-time refresh', async () => { + const store = createTestStore() + const repos = Array.from({ length: 7 }, (_, index) => ({ + id: `repo-${index}`, + path: `/repos/${index}`, + displayName: `repo-${index}`, + badgeColor: '#000', + addedAt: 0 + })) + let activeScans = 0 + let maxActiveScans = 0 + + mockApi.worktrees.list.mockImplementation(async ({ repoId }: { repoId: string }) => { + activeScans += 1 + maxActiveScans = Math.max(maxActiveScans, activeScans) + await new Promise((resolve) => setTimeout(resolve, 5)) + activeScans -= 1 + return [makeWorktree({ id: `${repoId}::/wt`, repoId, path: `/wt/${repoId}` })] + }) + + store.setState({ repos } as unknown as Partial) + + await store.getState().fetchAllWorktrees() + + expect(maxActiveScans).toBeLessThanOrEqual(5) + expect(mockApi.worktrees.list).toHaveBeenCalledTimes(repos.length) + expect(store.getState().hasHydratedWorktreePurge).toBe(true) + }) + + it('bounds concurrent repo scans after the hydration purge has run', async () => { + const store = createTestStore() + const repos = Array.from({ length: 7 }, (_, index) => ({ + id: `repo-${index}`, + path: `/repos/${index}`, + displayName: `repo-${index}`, + badgeColor: '#000', + addedAt: 0 + })) + let activeScans = 0 + let maxActiveScans = 0 + + mockApi.worktrees.list.mockImplementation(async ({ repoId }: { repoId: string }) => { + activeScans += 1 + maxActiveScans = Math.max(maxActiveScans, activeScans) + await new Promise((resolve) => setTimeout(resolve, 5)) + activeScans -= 1 + return [makeWorktree({ id: `${repoId}::/wt`, repoId, path: `/wt/${repoId}` })] + }) + + store.setState({ + hasHydratedWorktreePurge: true, + repos + } as unknown as Partial) + + await store.getState().fetchAllWorktrees() + + expect(maxActiveScans).toBeLessThanOrEqual(5) + expect(mockApi.worktrees.list).toHaveBeenCalledTimes(repos.length) + expect(store.getState().hasHydratedWorktreePurge).toBe(true) + }) + it('preserves floating workspace state while purging a real stale worktree', async () => { const store = createTestStore() const wtA = makeWorktree({ id: 'repoA::/a/wt1', repoId: 'repoA', path: '/a/wt1' }) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 25c504628..c7fc1be86 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -63,10 +63,34 @@ const REMOTE_WORKTREE_LIST_PARITY_LIMIT = 10_000 const ACTIVE_WORKTREE_TERMINAL_PREP_DELAY_MS = 300 const ACTIVE_WORKTREE_TERMINAL_PREP_INPUT_QUIET_MS = 450 const ACTIVE_WORKTREE_TERMINAL_PREP_IDLE_TIMEOUT_MS = 180 +const WORKTREE_REFRESH_CONCURRENCY = 5 const pendingActivationTerminalPrepCancels = new Map void>() const detachedHeadAutoDerivedDisplayNames = new Map() const folderWorkspaceWorktreeCache = new WeakMap() +async function mapReposForWorktreeRefresh( + repos: readonly { id: string }[], + mapper: (repo: { id: string }) => Promise +): Promise { + const results = Array(repos.length) + let nextIndex = 0 + const workerCount = Math.min(WORKTREE_REFRESH_CONCURRENCY, repos.length) + + // Why: worktree refresh can be triggered during activation/startup. Keeping + // repo scans bounded avoids one UI moment launching every git probe at once. + await Promise.all( + Array.from({ length: workerCount }, async () => { + while (nextIndex < repos.length) { + const index = nextIndex + nextIndex += 1 + results[index] = await mapper(repos[index]) + } + }) + ) + + return results +} + function countTerminalLayoutLeaves(node: TerminalPaneLayoutNode | null | undefined): number { if (!node) { return 0 @@ -1293,7 +1317,7 @@ export const createWorktreeSlice: StateCreator // calls just need to refresh each repo's cached list. No need to // double-probe the IPC for the per-repo success signal. if (get().hasHydratedWorktreePurge) { - await Promise.all(repos.map((r) => get().fetchWorktrees(r.id))) + await mapReposForWorktreeRefresh(repos, (r) => get().fetchWorktrees(r.id)) return } @@ -1308,8 +1332,14 @@ export const createWorktreeSlice: StateCreator // empty-replace when cached data exists. Neither signal bubbles up to the // caller, so we probe the IPC directly to get the per-repo success signal, // then apply that same payload to state instead of listing each repo again. - const results = await Promise.all( - repos.map(async (r) => { + const results = await mapReposForWorktreeRefresh( + repos, + async ( + r + ): Promise< + | { repoId: string; ok: boolean; detected: DetectedWorktreeListResult } + | { repoId: string; ok: false } + > => { try { const detected = await listDetectedWorktreesForRepo( settingsForRepoOwner(get(), r.id), @@ -1336,7 +1366,7 @@ export const createWorktreeSlice: StateCreator console.error(`Failed to fetch worktrees for repo ${r.id}:`, err) return { repoId: r.id, ok: false as const } } - }) + } ) const hasAnyDetectedWorktree = results.some(