fix(sentry): silence expected-condition telemetry noise on rc#1141
fix(sentry): silence expected-condition telemetry noise on rc#1141pedramamini wants to merge 1 commit into
Conversation
Three field-crash issues on the rc channel were best-effort/recoverable conditions being reported to Sentry as if they were bugs: - MAESTRO-M9 (regression): getGlobalStats lost the RangeError carve-out when it was refactored onto statsCache. A session file too big to read into one V8 string throws `RangeError: Invalid string length`; both the Claude and Codex read loops captured it. Re-add the guard, mirroring the storage-layer pattern in claude-/codex-session-storage.ts. - MAESTRO-FM (553 occ): writeEntryLocal is a best-effort cross-host history sync write. When the project lives in a permission-restricted or non-existent dir (read-only Dropbox/CloudStorage team folders, /home/.maestro/history), mkdir throws EACCES/ENOENT. The primary history store is unaffected, so skip Sentry for expected fs error codes. - MAESTRO-JB: groupChat resetContext already recovers from a grooming failure by falling back to a fresh session. When the failure is "Session not found" (the participant's session was deleted), that's an expected, fully-recovered condition - skip the Sentry report. Adds regression tests for the FM carve-out.
📝 WalkthroughWalkthroughThree error handlers are updated to skip ChangesSelective Sentry Exception Reporting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR reduces Sentry noise for recovered or best-effort error paths.
Confidence Score: 4/5The changed flow looks mergeable after narrowing two diagnostic filters.
src/main/ipc/handlers/agentSessions.ts, src/main/ipc/handlers/groupChat.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(sentry): silence expected-condition ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (error instanceof RangeError) { | ||
| logger.warn(`Claude session file too large to parse: ${file.sessionKey}`, LOG_CONTEXT); | ||
| } else { | ||
| void captureException(error); | ||
| logger.warn(`Failed to parse Claude session: ${file.sessionKey}`, LOG_CONTEXT, { | ||
| error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This catch block now treats every RangeError from the full read, stat, and parse block as an oversized session file. If parsing or future stats logic throws a different RangeError, that session is skipped from global stats and the bug is not reported to Sentry. Narrowing this to the known Invalid string length condition keeps the expected noisy case quiet without hiding unrelated failures.
Context Used: CLAUDE.md (source)
| if (error instanceof RangeError) { | ||
| logger.warn(`Codex session file too large to parse: ${file.sessionKey}`, LOG_CONTEXT); | ||
| } else { | ||
| void captureException(error); | ||
| logger.warn(`Failed to parse Codex session: ${file.sessionKey}`, LOG_CONTEXT, { | ||
| error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This branch suppresses every RangeError raised while reading, statting, or parsing a Codex session. A non-size RangeError in the parser or aggregation path would drop that session from global stats and skip Sentry, even though only V8's oversized string error is expected here.
Context Used: CLAUDE.md (source)
| const message = error instanceof Error ? error.message : String(error); | ||
| if (!/Session not found/i.test(message)) { | ||
| void captureException(error); | ||
| } |
There was a problem hiding this comment.
groomContext wraps process errors as Grooming error: <message>, and unknown provider or tool errors can preserve raw text. If an unexpected grooming failure contains Session not found in its message, this substring check suppresses Sentry even though the failure is not the recovered deleted-session case.
Context Used: CLAUDE.md (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/ipc/handlers/agentSessions.ts`:
- Around line 1002-1014: Narrow the error carve-out in agentSessions handling so
only the expected oversized-session case is skipped. Update the logic around the
session parsing flow to check for the specific “Invalid string length”
RangeError signature (or a shared helper used by the session storage code)
instead of using `error instanceof RangeError`, and keep all other RangeErrors
flowing through `captureException` and the existing `logger.warn` path in the
relevant session aggregation code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52798030-b1ac-4805-90da-1ddc86a4ee29
📒 Files selected for processing (4)
src/__tests__/main/shared-history-manager.test.tssrc/main/ipc/handlers/agentSessions.tssrc/main/ipc/handlers/groupChat.tssrc/main/shared-history-manager.ts
| // A session file too large to read into a single V8 string throws | ||
| // `RangeError: Invalid string length` (MAESTRO-M9). That's an expected | ||
| // boundary for huge sessions, not a bug - skip it and keep aggregating | ||
| // the rest. Mirrors the storage-layer carve-out in | ||
| // claude-/codex-session-storage.ts. | ||
| if (error instanceof RangeError) { | ||
| logger.warn(`Claude session file too large to parse: ${file.sessionKey}`, LOG_CONTEXT); | ||
| } else { | ||
| void captureException(error); | ||
| logger.warn(`Failed to parse Claude session: ${file.sessionKey}`, LOG_CONTEXT, { | ||
| error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow the carve-out to the oversized-file RangeError.
error instanceof RangeError suppresses all RangeErrors from fs.readFile, fs.stat, and the parsers here, so unrelated bugs stop reaching Sentry too. The documented expected case is specifically RangeError: Invalid string length; please gate on that signature (or a shared helper) and keep other RangeErrors reportable.
Suggested fix
- if (error instanceof RangeError) {
+ const isExpectedOversizeError =
+ error instanceof RangeError && /Invalid string length/i.test(error.message);
+ if (isExpectedOversizeError) {
logger.warn(`Claude session file too large to parse: ${file.sessionKey}`, LOG_CONTEXT);
} else {
void captureException(error);
logger.warn(`Failed to parse Claude session: ${file.sessionKey}`, LOG_CONTEXT, {
error,
@@
- if (error instanceof RangeError) {
+ const isExpectedOversizeError =
+ error instanceof RangeError && /Invalid string length/i.test(error.message);
+ if (isExpectedOversizeError) {
logger.warn(`Codex session file too large to parse: ${file.sessionKey}`, LOG_CONTEXT);
} else {
void captureException(error);
logger.warn(`Failed to parse Codex session: ${file.sessionKey}`, LOG_CONTEXT, {
error,As per coding guidelines, "Handle only expected/recoverable errors explicitly."
Also applies to: 1038-1048
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/handlers/agentSessions.ts` around lines 1002 - 1014, Narrow the
error carve-out in agentSessions handling so only the expected oversized-session
case is skipped. Update the logic around the session parsing flow to check for
the specific “Invalid string length” RangeError signature (or a shared helper
used by the session storage code) instead of using `error instanceof
RangeError`, and keep all other RangeErrors flowing through `captureException`
and the existing `logger.warn` path in the relevant session aggregation code
paths.
Source: Coding guidelines
…oom/oversized-session telemetry Three stable-channel (main) field-crash fixes, validated against Sentry (smash-labs/maestro) on releases 0.17.1/0.17.2 (channel:stable). Fixes 2 and 3 mirror rc PR #1141 exactly so the rc->main merge converges with no conflict. 1. Native bindings packaging (MAESTRO-TE, MAESTRO-Q3, MAESTRO-JV, plus the MAESTRO-TC/TD "Could not locate the bindings file" cluster). better-sqlite3 depends on bindings, which depends on file-uri-to-path, but asarUnpack only unpacked better-sqlite3 and node-pty. When the unpacked bindings.js required file-uri-to-path (still inside app.asar), resolution could not cross back into the archive ("Cannot find module 'file-uri-to-path'"); the mixed packed/unpacked layout also produced "Could not locate the bindings file" when bindings computed an in-archive search root for the .node. Unpack the full native dependency closure (bindings + file-uri-to-path) so the whole chain resolves from app.asar.unpacked. 2. Recoverable grooming session loss (MAESTRO-JB, 65 events). Group-chat context summary spawns a batch agent; if the participant's provider session was deleted mid-summary the agent emits a recoverable "Session not found" error (error-patterns.ts session_not_found) and we already fall back to a fresh session. Skip captureException for that case in groupChat.ts; real summary failures still report. 3. Oversized session files (MAESTRO-M9, regressed onto stable). The getGlobalStats parse loops in agentSessions.ts read each session file into a single string; a file too large throws "RangeError: Invalid string length". The #1115 carve-out was lost in the statsCache refactor that reached main. Re-skip the expected RangeError in both the Claude and Codex loops, mirroring the storage-layer pattern.
1 participant
Summary
Triage of Sentry field crashes on the rc channel. Three issues were best-effort or fully-recovered conditions being reported to Sentry as if they were bugs (the same telemetry-noise-on-expected-condition family addressed in prior triages). Fixed in one branch:
MAESTRO-M9 -
RangeError: Invalid string length(regression)agentSessions.getGlobalStatslost itsRangeErrorcarve-out when it was refactored ontostatsCache. A session file too large to read into a single V8 string throwsRangeError: Invalid string length, and both the Claude and Codex incremental read loops captured it unconditionally. Re-added the guard, mirroring the storage-layer pattern already present inclaude-session-storage.ts/codex-session-storage.ts. Still live on0.18.2-RC(12 recent events).MAESTRO-FM -
EACCES/ENOENTmkdir.maestro/history(553 occ)writeEntryLocalis a best-effort cross-host history sync write. When the project lives in a permission-restricted or non-existent directory (read-only Dropbox / CloudStorage team folders,/home/.maestro/history),mkdirthrowsEACCES/ENOENT. The primary history store is unaffected, so we now skip Sentry for expected filesystem error codes and only report genuinely unexpected failures. Added regression tests.MAESTRO-JB - "Grooming error: Session not found" (65 occ)
groupChatresetContextalready recovers from a grooming failure by falling back to a fresh session. When the failure is "Session not found" (the participant's session was deleted mid-summary), that's an expected, fully-recovered condition - skip the Sentry report; unexpected grooming failures still surface.Validation
tsc -p tsconfig.main.jsoncleanvitestgreen: shared-history-manager (12, +2 new), agentSessions + groupChat (74)Deliberately not included
maestro-p --status sample failed, ~5.7k occ): a periodic best-effort sampler reporting every failure mode as a Sentry warning. Deferred in two prior triages pending a human call onmaestro-pexit semantics; thereason/stagecontext isn't indexed as queryable tags, so there's no data-backed clean expected-boundary to carve out. Flagging for a decision rather than guessing.No handler registered for 'pianola:get-rules'):pianolahas zero references in the codebase (unmerged dev-environment feature) - no handler or caller to fix here.Note: merged into
rc, so GitHub will reference but not auto-close the issues untilrcreachesmain. The M9/FM/JB code also exists onmain; this PR is scoped torcper request.Summary by CodeRabbit