From 6f5b619ccf99ddbb2b1b71bc404e02b02e6b4227 Mon Sep 17 00:00:00 2001 From: m4air Date: Mon, 14 Sep 2026 07:17:42 -0700 Subject: [PATCH] fix(crash-reporting): stop a once-a-minute sampler from evicting the crash trail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The breadcrumb ring is 30 entries and evicts oldest-first, so any emitter that repeats outlasts the whole lifecycle trail. Across 293 field reports three periodic emitters hold 77% of every slot ever shipped and 39% of reports arrive with no lifecycle crumb at all — the "Recent activity" section cannot say what the app was doing. Charge the overflow to the most crowded name instead of the oldest event, so a series is thinned from its oldest end and singletons survive. No allowlist, so a new periodic emitter cannot reopen the hole. --- .../crash-breadcrumb-store.test.ts | 68 ++++++++++++++++++- .../crash-reporting/crash-breadcrumb-store.ts | 38 ++++++++++- 2 files changed, 102 insertions(+), 4 deletions(-) diff --git a/src/main/crash-reporting/crash-breadcrumb-store.test.ts b/src/main/crash-reporting/crash-breadcrumb-store.test.ts index 9c5a9dcce4d..df1bcf20b98 100644 --- a/src/main/crash-reporting/crash-breadcrumb-store.test.ts +++ b/src/main/crash-reporting/crash-breadcrumb-store.test.ts @@ -24,6 +24,65 @@ describe('crash breadcrumb store', () => { expect(snapshot[29].name).toBe('event_31') }) + describe('fair-share eviction', () => { + it('spends the overflow on the most repeated series, not the oldest event', () => { + recordCrashBreadcrumb('app_started', { packaged: true }) + recordCrashBreadcrumb('main_window_created') + recordCrashBreadcrumb('main_window_loaded') + for (let sample = 0; sample < 200; sample += 1) { + recordCrashBreadcrumb('renderer_memory', { sample }) + } + + const snapshot = getCrashBreadcrumbSnapshot() + + expect(snapshot.map((entry) => entry.name).slice(0, 3)).toEqual([ + 'app_started', + 'main_window_created', + 'main_window_loaded' + ]) + expect(snapshot.filter((entry) => entry.name === 'renderer_memory')).toHaveLength(27) + }) + + it('thins the crowded series from its oldest end, keeping the run before the crash', () => { + recordCrashBreadcrumb('app_started') + for (let sample = 0; sample < 200; sample += 1) { + recordCrashBreadcrumb('renderer_memory', { sample }) + } + + const samples = getCrashBreadcrumbSnapshot() + .filter((entry) => entry.name === 'renderer_memory') + .map((entry) => entry.data?.sample) + + expect(samples.at(-1)).toBe(199) + expect(samples).toEqual( + Array.from({ length: samples.length }, (_, i) => 200 - samples.length + i) + ) + }) + + it('splits the ring between two competing series', () => { + for (let round = 0; round < 100; round += 1) { + recordCrashBreadcrumb('renderer_memory', { round }) + recordCrashBreadcrumb('pr_refresh_queue', { round }) + } + + const snapshot = getCrashBreadcrumbSnapshot() + + expect(snapshot.filter((entry) => entry.name === 'renderer_memory')).toHaveLength(15) + expect(snapshot.filter((entry) => entry.name === 'pr_refresh_queue')).toHaveLength(15) + }) + + it('degenerates to oldest-first when no name repeats', () => { + for (let index = 0; index < 40; index += 1) { + recordCrashBreadcrumb(`event_${index}`) + } + + const snapshot = getCrashBreadcrumbSnapshot() + + expect(snapshot[0].name).toBe('event_10') + expect(snapshot[29].name).toBe('event_39') + }) + }) + it('retains bounded renderer high-water profiles across later activity', () => { vi.useFakeTimers() vi.setSystemTime(new Date('2026-07-22T12:00:00.000Z')) @@ -235,7 +294,10 @@ describe('crash breadcrumb store', () => { } const burstSize = 34 - it('erases the entire pre-crash trail when uncoalesced', () => { + // Fair-share eviction spares the one-off trail, but the burst still takes + // two thirds of the ring — enough to starve any *other* series and to lose + // the pane count entirely. Coalescing is still the right answer for bursts. + it('takes most of the ring when uncoalesced, but no longer erases the trail', () => { recordPreCrashTrail() for (let pane = 0; pane < burstSize; pane += 1) { recordCrashBreadcrumb('terminal_safe_fit_retry_exhausted', { paneId: 1 }) @@ -244,11 +306,11 @@ describe('crash breadcrumb store', () => { const snapshot = getCrashBreadcrumbSnapshot() expect(snapshot.filter((entry) => entry.name.startsWith('pre_crash_evidence_'))).toHaveLength( - 0 + 10 ) expect( snapshot.filter((entry) => entry.name === 'terminal_safe_fit_retry_exhausted') - ).toHaveLength(30) + ).toHaveLength(20) }) it('costs one slot when coalesced, and keeps the pane count on the payload', () => { diff --git a/src/main/crash-reporting/crash-breadcrumb-store.ts b/src/main/crash-reporting/crash-breadcrumb-store.ts index 2c0b68b043e..238618b570d 100644 --- a/src/main/crash-reporting/crash-breadcrumb-store.ts +++ b/src/main/crash-reporting/crash-breadcrumb-store.ts @@ -85,11 +85,47 @@ export function recordCrashBreadcrumb( } breadcrumbs.push(breadcrumb) if (breadcrumbs.length > MAX_BREADCRUMBS) { - breadcrumbs.shift() + breadcrumbs.splice(evictionIndex(breadcrumbs), 1) } return breadcrumb } +/** + * Index of the entry to drop when the ring overflows: the oldest entry of + * whichever name currently occupies the most slots. + * + * Why not the oldest overall: a once-a-minute sampler outnumbers the whole + * lifecycle trail within the hour, so plain FIFO spends the ring on the one + * series that repeats and evicts the singletons that explain the death. Across + * 293 field reports, `renderer_memory`, `agent_state_changed` and + * `pr_refresh_queue` held 77% of every slot ever shipped and 39% of reports + * arrived with no lifecycle crumb at all. Charging the overflow to the most + * redundant name instead bounds any series without naming it, so a new periodic + * emitter cannot reopen the hole the way an allowlist lets it. + * + * Every name appearing once degenerates to the oldest entry, i.e. plain FIFO. + */ +function evictionIndex(ring: CrashReportBreadcrumb[]): number { + const counts = new Map() + for (const entry of ring) { + counts.set(entry.name, (counts.get(entry.name) ?? 0) + 1) + } + let crowdedIndex = 0 + let crowdedCount = 0 + for (let index = 0; index < ring.length; index += 1) { + const count = counts.get(ring[index].name) ?? 0 + // Why strictly greater: `ring` is oldest-first, so the first index holding + // the maximum is the OLDEST entry of the most crowded name. Accepting ties + // would walk to that name's newest entry and thin the series from the wrong + // end, leaving a stale head instead of the minutes before the crash. + if (count > crowdedCount) { + crowdedIndex = index + crowdedCount = count + } + } + return crowdedIndex +} + export function recordCoalescedCrashBreadcrumb({ name, data,