From 9210db4bd32bbfb9d0d17c5f784977febc547a0d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:20:37 -0700 Subject: [PATCH] fix(crash-reporting): sample system memory before the renderer dies --- .../pre-gone-system-memory.test.ts | 79 +++++++++++++++++++ .../process-gone-diagnostics.ts | 16 +++- 2 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 src/main/crash-reporting/pre-gone-system-memory.test.ts diff --git a/src/main/crash-reporting/pre-gone-system-memory.test.ts b/src/main/crash-reporting/pre-gone-system-memory.test.ts new file mode 100644 index 00000000000..76ef1cc86e3 --- /dev/null +++ b/src/main/crash-reporting/pre-gone-system-memory.test.ts @@ -0,0 +1,79 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { setSystemMemoryInfoReaderForTest } from './gone-time-system-memory' +import { + buildProcessGoneCrashDetails, + resetPreGoneProcessMetricsSamplingForTest, + samplePreGoneProcessMetrics +} from './process-gone-diagnostics' + +type MetricFixture = { + pid: number + creationTime: number + type: string + memory: { workingSetSize: number; peakWorkingSetSize?: number; privateBytes?: number } +} + +const { appMetricsMock } = vi.hoisted(() => ({ + appMetricsMock: vi.fn<() => MetricFixture[]>(() => []) +})) + +vi.mock('electron', () => ({ app: { getAppMetrics: appMetricsMock } })) + +const BROWSER_AND_RENDERER: MetricFixture[] = [ + { pid: 10, creationTime: 1, type: 'Browser', memory: { workingSetSize: 1024 * 250 } }, + { + pid: 11, + creationTime: 2, + type: 'Tab', + memory: { workingSetSize: 1024 * 400, peakWorkingSetSize: 1024 * 420, privateBytes: 1024 * 260 } + } +] + +const BROWSER_ONLY: MetricFixture[] = [BROWSER_AND_RENDERER[0]] + +describe('pre-gone system memory', () => { + beforeEach(() => { + resetPreGoneProcessMetricsSamplingForTest() + setSystemMemoryInfoReaderForTest(null) + appMetricsMock.mockReturnValue(BROWSER_AND_RENDERER) + }) + + it('carries a pre-gone system-memory reading, not only the post-mortem one', () => { + // Windows commit pressure at sample time: 400 MB physical free, 200 MB commit left. + setSystemMemoryInfoReaderForTest(() => ({ + total: 16_000 * 1024, + free: 400 * 1024, + swapTotal: 48_000 * 1024, + swapFree: 200 * 1024 + })) + samplePreGoneProcessMetrics(Date.now() - 5_000) + + // The renderer dies; its ~400 MB returns to the OS, so the gone-time read + // now shows a much healthier machine than the one that refused the alloc. + setSystemMemoryInfoReaderForTest(() => ({ + total: 16_000 * 1024, + free: 3_000 * 1024, + swapTotal: 48_000 * 1024, + swapFree: 2_900 * 1024 + })) + appMetricsMock.mockReturnValue(BROWSER_ONLY) + + const details = buildProcessGoneCrashDetails({ processType: 'renderer' }, 'renderer') + + expect(details.systemMemorySwapFreeMB).toBe(2_900) + expect(details.processMetricsPreGoneSystemMemorySwapFreeMB).toBe(200) + expect(details.processMetricsPreGoneSystemMemoryFreeMB).toBe(400) + expect(details.processMetricsPreGoneSystemMemoryTotalMB).toBe(16_000) + }) + + it('leaves the pre-gone sample free of system-memory keys when the reading is unavailable', () => { + samplePreGoneProcessMetrics(Date.now() - 5_000) + + const details = buildProcessGoneCrashDetails({ processType: 'renderer' }, 'renderer') + + expect(details.processMetricsPreGoneRendererWorkingSetMB).toBe(400) + expect( + Object.keys(details).filter((key) => key.startsWith('processMetricsPreGoneSystemMemory')) + ).toEqual([]) + }) +}) diff --git a/src/main/crash-reporting/process-gone-diagnostics.ts b/src/main/crash-reporting/process-gone-diagnostics.ts index d0bb380a2b6..8070be3ae22 100644 --- a/src/main/crash-reporting/process-gone-diagnostics.ts +++ b/src/main/crash-reporting/process-gone-diagnostics.ts @@ -195,7 +195,9 @@ export function samplePreGoneProcessMetrics(nowMs: number = Date.now()): void { try { const metrics = app.getAppMetrics() preGoneSample = { - details: collectProcessGoneMetricDetails(metrics), + // Why: the gone-time system read happens after the corpse released its + // pages, so only a live sample can show the pressure that refused the alloc. + details: { ...collectProcessGoneMetricDetails(metrics), ...getSystemMemoryAtGoneDetails() }, processes: sampledProcessIdentities(metrics), sampledAtMs: nowMs } @@ -225,6 +227,15 @@ export function resetPreGoneProcessMetricsSamplingForTest(): void { const PROCESS_METRICS_KEY_PREFIX = 'processMetrics' +// Why: the sample now mixes processMetrics* and systemMemory* keys; only the +// former carries the prefix to strip, the latter just needs title-casing. +function preGoneDetailKey(key: string): string { + const suffix = key.startsWith(PROCESS_METRICS_KEY_PREFIX) + ? key.slice(PROCESS_METRICS_KEY_PREFIX.length) + : `${key.charAt(0).toUpperCase()}${key.slice(1)}` + return `${PROCESS_METRICS_KEY_PREFIX}PreGone${suffix}` +} + function preGoneSampleDetails( sample: PreGoneProcessMetricsSample, nowMs: number @@ -236,8 +247,7 @@ function preGoneSampleDetails( processMetricsPreGoneCrashedProcessAttributionAmbiguous: true } for (const [key, value] of Object.entries(sample.details)) { - details[`${PROCESS_METRICS_KEY_PREFIX}PreGone${key.slice(PROCESS_METRICS_KEY_PREFIX.length)}`] = - value + details[preGoneDetailKey(key)] = value } return details }