Repository navigation
fix(crash-reporting): bound coalescedBreadcrumbs with TTL prune + LRU cap - #5818
Merged
Merged
Conversation
… cap
recordCoalescedCrashBreadcrumb set entries keyed by agent:${agentType}:${state}
with no prune and no cap. agentType is an open string (length-trimmed only,
never enum-checked) over the relay/SSH/OSC ingest paths, so the map grew
unbounded over a long multi-agent session — the sibling breadcrumbs array
is capped at 30, but this map was not.
Prune entries past their suppression window and LRU-cap to 128, mirroring
ProcessGoneDedupe's prune/maxKeys idiom.
Regression test fails before the fix (1000 keys) and passes after.
Co-authored-by: Orca <help@stably.ai>
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit ad4d608 on Jun 19, 2026 6:46pm UTC. |
Contributor
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d4a9f51-2c24-4caa-81ef-3e309db08df1
📒 Files selected for processing (2)
src/main/crash-reporting/crash-breadcrumb-store-leak.test.tssrc/main/crash-reporting/crash-breadcrumb-store.ts
Comment on lines
+52
to
+57
| // Drop entries past their suppression window (they can no longer coalesce | ||
| // anything) and LRU-cap the rest. delete-then-set keeps insertion order = | ||
| // recency so only genuinely idle keys are evicted. | ||
| for (const [key, entry] of coalescedBreadcrumbs) { | ||
| if (now - entry.recordedAt >= minIntervalMs) { | ||
| coalescedBreadcrumbs.delete(key) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites for recordCoalescedCrashBreadcrumb:"
rg -n -C3 --type ts --type tsx '\brecordCoalescedCrashBreadcrumb\s*\('
echo
echo "minIntervalMs values passed at call sites:"
rg -n --type ts --type tsx 'minIntervalMs\s*:'Repository: stablyai/orca
Length of output: 150
Add per-entry interval tracking or document single-interval constraint.
Line 56 applies the current call's minIntervalMs to prune all entries. Since entries don't store per-interval metadata, callers with different minIntervalMs values would cause incorrect expiry. Currently all callers use 30_000 (including the only production caller at src/main/index.ts:421 and all tests), so there's no active bug. However, the function signature accepts variable intervals per call, making this a design fragility. Either store minIntervalMs per entry or document that all callers must use the same interval.
brennanb2025
added a commit
that referenced
this pull request
Aug 15, 2026
Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone.
brennanb2025
added a commit
that referenced
this pull request
Aug 15, 2026
Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone.
brennanb2025
added a commit
that referenced
this pull request
Aug 15, 2026
Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone.
brennanb2025
added a commit
that referenced
this pull request
Aug 15, 2026
Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone.
brennanb2025
added a commit
that referenced
this pull request
Aug 16, 2026
Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone.
brennanb2025
added a commit
that referenced
this pull request
Aug 16, 2026
* fix(crash-reporting): attribute coalesced repeats exactly once Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone. * fix(crash-reporting): preserve orphaned repeat debt on cleanup * fix(crash-reporting): preserve data-less repeat debt * fix(crash-reporting): make coalescing window monotonic
AmethystLiang
pushed a commit
that referenced
this pull request
Aug 17, 2026
* fix(crash-reporting): attribute coalesced repeats exactly once Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in #8800/#5818/#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone. * fix(crash-reporting): preserve orphaned repeat debt on cleanup * fix(crash-reporting): preserve data-less repeat debt * fix(crash-reporting): make coalescing window monotonic
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
… cap (stablyai#5818) recordCoalescedCrashBreadcrumb set entries keyed by agent:${agentType}:${state} with no prune and no cap. agentType is an open string (length-trimmed only, never enum-checked) over the relay/SSH/OSC ingest paths, so the map grew unbounded over a long multi-agent session — the sibling breadcrumbs array is capped at 30, but this map was not. Prune entries past their suppression window and LRU-cap to 128, mirroring ProcessGoneDedupe's prune/maxKeys idiom. Regression test fails before the fix (1000 keys) and passes after. Co-authored-by: Orca <help@stably.ai>
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…14666) * fix(crash-reporting): attribute coalesced repeats exactly once Two ways one burst's suppressed repeats were misattributed in forensics, found across two adversarial review loops on the sibling-correlation work but pre-dating it (the coalesce machinery shipped in stablyai#8800/stablyai#5818/stablyai#10729): - Double-claim: a crash report filed mid-window snapshots the ring, folding the suppressed repeats into the emitted crumb — but the next emit still re-claimed those repeats in its suppressedSinceLast, reporting one burst twice across two crumbs. - Mirror erasure: a fold resolving onto a re-emitted crumb overwrote the count that crumb was born carrying, deleting the previous window's repeats. Track what each crumb has claimed (carried at emit, resolved by folds) so every repeat is attributed exactly once. And never resolve into an evicted crumb: when a storm pushes the burst crumb out of the 30-entry ring — or past the retained-slot snapshot budget — mid-window, folding there would mark the repeats claimed by evidence no snapshot can see and the burst would vanish from the record entirely; drop the handle so the next emit claims them instead. Semantic note for trace-mirroring consumers: the returned suppressedSinceLast is now net of already-folded repeats, so exactly-once holds over the union of trace spans and report snapshots rather than within the trace stream alone. * fix(crash-reporting): preserve orphaned repeat debt on cleanup * fix(crash-reporting): preserve data-less repeat debt * fix(crash-reporting): make coalescing window monotonic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Leak
recordCoalescedCrashBreadcrumbdoescoalescedBreadcrumbs.set(coalesceKey, …)with no TTL prune and no size cap. Production keys areagent:${agentType}:${state}, andagentTypereaches this store (via the relay/SSHingestRemoteand OSC observed-status paths throughnormalizeAgentStatusPayload) as an open string — only length-trimmed to 40 chars, never enum-checked — so the key space is unbounded over a long multi-agent/SSH session. The only shrink path was the test-onlyclearCrashBreadcrumbsForTest. (The siblingbreadcrumbsarray is already bounded toMAX_BREADCRUMBS = 30— the unbounded map was the oversight.)Fix
In
recordCoalescedCrashBreadcrumb, after the suppression check: prune entries past their suppression window (now - recordedAt >= minIntervalMs— provably inert, can no longer coalesce anything), then enforce an LRU cap ofMAX_COALESCE_KEYS = 128(delete-then-setthe live key so insertion order tracks recency; evict the oldest while over the cap). This mirrors theprune()/maxKeysidiom already used byProcessGoneDedupein the same directory.Evidence of the leak (regression test FAILS before the fix)
crash-breadcrumb-store-leak.test.tsagainst the unfixed code (fake timers, no advance, so nothing coalesces — pure cap pressure):1000 distinct agentType keys → 1000 map entries.
Evidence the leak is resolved (test PASSES after the fix)
The map stays ≤ 128 under unbounded distinct keys, and still coalesces repeated breadcrumbs for the same key within the interval.
No functional regression
The TTL prune only deletes entries past
minIntervalMs(provably inert), so a still-live entry is never wrongly evicted — the existing "coalesces repeated breadcrumbs inside the interval" test guards within-window suppression. The cap only triggers above 128 live keys, far above any single agent's working set.🤖 Generated with Claude Code
Made with Orca 🐋