mirror of
https://github.com/stablyai/orca.git
synced 2026-10-02 00:02:05 +00:00
fix(crash-reporting): scope eviction per origin and spare live coalescing owners
Round-1 review found two ways the name-only policy was worse than plain FIFO: - Counting ignored `origin` while the snapshot filters by it, so a busy popout's samples made the main window's singleton look redundant and deleted it. - Names like `renderer_error` carry many independent coalesce keys, so the name became "crowded" out of genuinely distinct errors — and the entry taken was the oldest, i.e. a key still accumulating `suppressedSinceLast`. A crash report is the last snapshot, so an orphaned owner is never re-claimed and the burst count simply vanished. Group by (name, origin), skip an entry a coalesce key still owns unless every candidate is owned, and never consider the crumb that just arrived — its coalesce state is linked after the push, so it would always look unowned.
This commit is contained in:
@@ -71,13 +71,11 @@ describe('crash breadcrumb store', () => {
|
||||
expect(snapshot.filter((entry) => entry.name === 'pr_refresh_queue')).toHaveLength(15)
|
||||
})
|
||||
|
||||
// The one interaction fair-share eviction could plausibly break: a coalesced key
|
||||
// owns a ring entry by reference, and `isCoalescedCrumbStillInEvidence` was written
|
||||
// when eviction only ever removed from the FRONT. Fair share can remove from the
|
||||
// middle, so pin that the accounting still holds — the burst is materialized exactly
|
||||
// once when the window expires, never folded into an entry no snapshot can see and
|
||||
// never claimed twice.
|
||||
it('accounts a coalesced burst exactly once when its owned entry is evicted mid-ring', () => {
|
||||
// The interaction fair-share eviction could break, and the reason `ownsUnresolvedRepeats`
|
||||
// exists: a coalesce key owns a ring entry by reference and carries its running
|
||||
// suppressed count there. A crash report is the LAST snapshot, so an entry orphaned by
|
||||
// eviction never gets re-claimed — the burst would simply vanish from the report.
|
||||
it('does not evict a coalescing owner that still holds unfolded repeats', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-09-14T12:00:00.000Z'))
|
||||
const hit = (key: string): void => {
|
||||
@@ -89,31 +87,128 @@ describe('crash breadcrumb store', () => {
|
||||
})
|
||||
}
|
||||
|
||||
recordCrashBreadcrumb('app_started')
|
||||
hit('hot')
|
||||
vi.advanceTimersByTime(10)
|
||||
for (let repeat = 0; repeat < 5; repeat += 1) {
|
||||
hit('hot')
|
||||
}
|
||||
// Churn on the SAME name makes it the crowded one, so fair share evicts the oldest
|
||||
// `renderer_error` — the hot key's own entry — from the middle of the ring.
|
||||
recordCrashBreadcrumb('app_started')
|
||||
// Distinct messages make `renderer_error` the crowded group even though each entry
|
||||
// is a different error — so the naive "oldest of the crowded name" would take the
|
||||
// hot key's own crumb, which is the one carrying the count.
|
||||
for (let index = 0; index < 40; index += 1) {
|
||||
vi.advanceTimersByTime(10)
|
||||
hit(`cold_${index}`)
|
||||
}
|
||||
|
||||
// Plain FIFO loses this; fair share is why the one-off survives 41 same-name crumbs.
|
||||
expect(getCrashBreadcrumbSnapshot().some((entry) => entry.name === 'app_started')).toBe(true)
|
||||
const snapshot = getCrashBreadcrumbSnapshot()
|
||||
const hotCrumb = snapshot.find((entry) => entry.data?.key === 'hot')
|
||||
|
||||
// Expiring the window is what surrenders an orphaned owner's unclaimed repeats.
|
||||
vi.advanceTimersByTime(30_000)
|
||||
hit('hot')
|
||||
// Plain FIFO loses this singleton; fair share is why it survives 41 same-name crumbs.
|
||||
expect(snapshot.some((entry) => entry.name === 'app_started')).toBe(true)
|
||||
expect(hotCrumb?.data?.suppressedSinceLast).toBe(5)
|
||||
})
|
||||
|
||||
const claimed = getCrashBreadcrumbSnapshot()
|
||||
.filter((entry) => entry.name === 'renderer_error')
|
||||
.reduce((total, entry) => total + Number(entry.data?.suppressedSinceLast ?? 0), 0)
|
||||
// The real field shape: THREE periodic emitters at roughly a quarter of the ring each,
|
||||
// none of them past half. A policy that only engages once one name owns a majority
|
||||
// reproduces the original bug exactly while every other test stays green.
|
||||
it('protects the trail when three series share the ring, none holding a majority', () => {
|
||||
recordCrashBreadcrumb('app_started')
|
||||
recordCrashBreadcrumb('main_window_created')
|
||||
recordCrashBreadcrumb('main_window_loaded')
|
||||
for (let round = 0; round < 100; round += 1) {
|
||||
recordCrashBreadcrumb('renderer_memory', { round })
|
||||
recordCrashBreadcrumb('agent_state_changed', { round })
|
||||
recordCrashBreadcrumb('pr_refresh_queue', { round })
|
||||
}
|
||||
|
||||
expect(claimed).toBe(5)
|
||||
const snapshot = getCrashBreadcrumbSnapshot()
|
||||
|
||||
expect(snapshot.slice(0, 3).map((entry) => entry.name)).toEqual([
|
||||
'app_started',
|
||||
'main_window_created',
|
||||
'main_window_loaded'
|
||||
])
|
||||
})
|
||||
|
||||
// Engagement threshold: two slots is already enough redundancy to charge the overflow to.
|
||||
it('charges the overflow to a name holding only two slots', () => {
|
||||
for (let index = 0; index < 15; index += 1) {
|
||||
recordCrashBreadcrumb(`single_${index}`)
|
||||
}
|
||||
recordCrashBreadcrumb('duplicated', { first: true })
|
||||
for (let index = 15; index < 29; index += 1) {
|
||||
recordCrashBreadcrumb(`single_${index}`)
|
||||
}
|
||||
recordCrashBreadcrumb('duplicated', { first: false })
|
||||
|
||||
const snapshot = getCrashBreadcrumbSnapshot()
|
||||
|
||||
expect(snapshot[0].name).toBe('single_0')
|
||||
expect(snapshot.filter((entry) => entry.name === 'duplicated')).toHaveLength(1)
|
||||
})
|
||||
|
||||
// The newest entry must be counted, or a near-tie is resolved against the wrong series.
|
||||
it('counts the entry that just arrived when two series are tied', () => {
|
||||
recordCrashBreadcrumb('lifecycle_a')
|
||||
recordCrashBreadcrumb('lifecycle_b')
|
||||
for (let index = 0; index < 14; index += 1) {
|
||||
recordCrashBreadcrumb('series_b', { index })
|
||||
}
|
||||
for (let index = 0; index < 14; index += 1) {
|
||||
recordCrashBreadcrumb('series_a', { index })
|
||||
}
|
||||
recordCrashBreadcrumb('series_a', { index: 14 })
|
||||
|
||||
const snapshot = getCrashBreadcrumbSnapshot()
|
||||
|
||||
expect(snapshot.filter((entry) => entry.name === 'series_a')).toHaveLength(14)
|
||||
expect(snapshot.filter((entry) => entry.name === 'series_b')).toHaveLength(14)
|
||||
})
|
||||
|
||||
// Eviction counts per (name, origin); the snapshot is filtered per reporter, so one
|
||||
// surface's sample must not make another surface's singleton look redundant.
|
||||
it("does not let one renderer surface evict another surface's only sample", () => {
|
||||
for (let index = 0; index < 15; index += 1) {
|
||||
recordCrashBreadcrumb(`lifecycle_${index}`, undefined, 'main')
|
||||
}
|
||||
recordCrashBreadcrumb('renderer_memory', { surface: 'main' }, 'main')
|
||||
for (let index = 15; index < 29; index += 1) {
|
||||
recordCrashBreadcrumb(`lifecycle_${index}`, undefined, 'main')
|
||||
}
|
||||
recordCrashBreadcrumb('renderer_memory', { surface: 'popout' }, 'popout')
|
||||
|
||||
const mainSnapshot = getCrashBreadcrumbSnapshot('main')
|
||||
|
||||
expect(mainSnapshot.filter((entry) => entry.name === 'renderer_memory')).toHaveLength(1)
|
||||
})
|
||||
|
||||
// Fallback path: when EVERY entry of the crowded group is a live owner there is no
|
||||
// unowned candidate, and the overflow must still be charged to that group rather than
|
||||
// to the oldest entry in the ring — which is the one-off the whole policy protects.
|
||||
it('charges the crowded group even when all of its entries are live owners', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-09-14T12:00:00.000Z'))
|
||||
recordCrashBreadcrumb('app_started')
|
||||
for (let index = 0; index < 30; index += 1) {
|
||||
const hit = (): void => {
|
||||
recordCoalescedCrashBreadcrumb({
|
||||
name: 'renderer_error',
|
||||
data: { index },
|
||||
coalesceKey: `key_${index}`,
|
||||
minIntervalMs: 30_000
|
||||
})
|
||||
}
|
||||
hit()
|
||||
hit()
|
||||
}
|
||||
|
||||
const snapshot = getCrashBreadcrumbSnapshot()
|
||||
|
||||
expect(snapshot.some((entry) => entry.name === 'app_started')).toBe(true)
|
||||
// And the crumb that just arrived is kept: its coalesce state is linked only after
|
||||
// the push, so treating it as a candidate would always discard the newest evidence.
|
||||
expect(snapshot.some((entry) => entry.data?.index === 29)).toBe(true)
|
||||
})
|
||||
|
||||
it('degenerates to oldest-first when no name repeats', () => {
|
||||
|
||||
@@ -105,25 +105,62 @@ export function recordCrashBreadcrumb(
|
||||
*
|
||||
* Every name appearing once degenerates to the oldest entry, i.e. plain FIFO.
|
||||
*/
|
||||
function evictionGroupKey(entry: CrashReportBreadcrumb): string {
|
||||
// Why origin is part of the group: the snapshot is filtered per reporter, so a name that
|
||||
// is a singleton on THIS surface is not redundant just because a busy popout also emits
|
||||
// it. Counting them together let one surface delete the other's trail.
|
||||
return `${entry.name}\u0000${entry.origin ?? ''}`
|
||||
}
|
||||
|
||||
/** Whether a coalesce key still owns this entry and has repeats it has not folded in. */
|
||||
function ownsUnresolvedRepeats(entry: CrashReportBreadcrumb): boolean {
|
||||
for (const state of coalescedBreadcrumbs.values()) {
|
||||
if (state.emitted === entry && state.suppressed > state.resolved) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
function evictionIndex(ring: CrashReportBreadcrumb[]): number {
|
||||
const counts = new Map<string, number>()
|
||||
for (const entry of ring) {
|
||||
counts.set(entry.name, (counts.get(entry.name) ?? 0) + 1)
|
||||
const key = evictionGroupKey(entry)
|
||||
counts.set(key, (counts.get(key) ?? 0) + 1)
|
||||
}
|
||||
let crowdedIndex = 0
|
||||
let crowdedKey = ''
|
||||
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.
|
||||
for (const entry of ring) {
|
||||
const key = evictionGroupKey(entry)
|
||||
const count = counts.get(key) ?? 0
|
||||
// Why strictly greater: `ring` is oldest-first, so the first group to reach the
|
||||
// maximum is the one whose oldest entry is oldest. Accepting ties walks to a later
|
||||
// group and thins the wrong series.
|
||||
if (count > crowdedCount) {
|
||||
crowdedIndex = index
|
||||
crowdedKey = key
|
||||
crowdedCount = count
|
||||
}
|
||||
}
|
||||
return crowdedIndex
|
||||
let oldestOfGroup = 0
|
||||
let foundGroup = false
|
||||
// Why the newest entry is never a candidate: it is the crumb that just arrived, and its
|
||||
// coalesce state has not been linked to it yet, so it would always look unowned.
|
||||
for (let index = 0; index < ring.length - 1; index += 1) {
|
||||
if (evictionGroupKey(ring[index]) !== crowdedKey) {
|
||||
continue
|
||||
}
|
||||
if (!foundGroup) {
|
||||
oldestOfGroup = index
|
||||
foundGroup = true
|
||||
}
|
||||
// Why skip a live owner: that entry carries its key's running suppressed count, and a
|
||||
// crash report is the LAST snapshot — "the next emit re-claims it" never happens. Take
|
||||
// the next entry in the same group instead; fall back only if every one is owned.
|
||||
if (!ownsUnresolvedRepeats(ring[index])) {
|
||||
return index
|
||||
}
|
||||
}
|
||||
return oldestOfGroup
|
||||
}
|
||||
|
||||
export function recordCoalescedCrashBreadcrumb({
|
||||
|
||||
Reference in New Issue
Block a user