fix(telemetry): handle obvious main-branch field crashes#1115
fix(telemetry): handle obvious main-branch field crashes#1115pedramamini wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR suppresses Sentry noise from recoverable/expected failures across three subsystems: network-level fetch errors in the Codex usage sampler, recoverable disk errors in the renderer session persistence hook, and uninitialized-DB errors in the ChangesRecoverable Error Noise Reduction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 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
🧪 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 |
Addresses four stable-channel (main) Sentry issues seen on the current
0.17.x release, all where the bug and path to resolution are clear:
- MAESTRO-RR: the Codex usage sampler reported network "request: fetch
failed" errors to Sentry (the dominant cause, ~375 events). Offline /
DNS / TLS / abort-timeout failures are expected for a best-effort
background sampler, so skip them - the UI still surfaces the error
state. The existing 401/403 carve-out (un-logged-in CODEX_HOME) stays.
- MAESTRO-M9: the global-stats refresh reported the RangeError ("Invalid
string length") that V8 throws when a session log is too large to read
into a single string. Treat it as an expected, recoverable data
condition (skip the file, log a warning) instead of a Sentry crash -
mirrors the storage-layer pattern and the rc fix in #1114 so the two
converge cleanly on the next rc -> main merge.
- MAESTRO-SP: stats:record-shortcut-usage threw "Database not
initialized" when a shortcut fired before the stats DB finished
initializing. Skip the fire-and-forget analytics write when the DB
isn't ready yet (new StatsDB.isReady()).
- MAESTRO-QF: the debounced session-persistence flush reported the main
process's deliberate "recoverable disk error" (a false return thrown
only to preserve isPending for retry) to Sentry. Keep that expected
user-environment condition out of Sentry; genuine flush failures still
report.
Adds regression tests for RR, SP, and QF.
pedramamini
force-pushed
the
fix/sentry-main-telemetry-noise
branch
from
71f85e1 to
4aa9b1d
Compare
Greptile SummaryThis PR reduces Sentry noise by stopping four categories of expected, recoverable user-environment conditions from being reported as crashes in the
Confidence Score: 3/5Hold for the duplicate isReady() in stats-db.ts before merging — the other three fixes are clean and safe. Three of the four fixes are straightforward and well-tested. The MAESTRO-SP fix introduces a new isReady() in StatsDB at line 103, but an existing isReady() with stronger semantics already lives in the same class at line 188–190. TypeScript treats duplicate method definitions as a compile error (TS2300), and the weaker new implementation could permit shortcut-usage writes before schema migrations finish if the duplicate is resolved in favour of the new definition. src/main/stats/stats-db.ts — the duplicate isReady() needs to be resolved before this PR is merged. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Recoverable condition occurs] --> B{Which fix?}
B --> RR[MAESTRO-RR\nFetch throws in codex-usage-sampler]
RR --> RR2[Return error snapshot\nNo Sentry report]
B --> M9[MAESTRO-M9\nSession file >= 500 MB]
M9 --> M9b{fileStat.size > MAX?}
M9b -->|Yes| M9c[logger.warn + continue\nSkip file, no crash]
M9b -->|No| M9d[readFile + parse normally]
B --> SP[MAESTRO-SP\nShortcut fires before DB ready]
SP --> SP2{db.isReady?}
SP2 -->|false| SP3[return null silently]
SP2 -->|true| SP4[incrementShortcutUsage\nbroadcast stats:updated]
B --> QF[MAESTRO-QF\nsetAll/setMany returns false]
QF --> QF2[persistInternal throws\nrecoverable disk error]
QF2 --> QF3{message.includes\nrecoverable disk error?}
QF3 -->|Yes| QF4[Suppress Sentry\nisPending preserved for retry]
QF3 -->|No| QF5[captureException to Sentry]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Recoverable condition occurs] --> B{Which fix?}
B --> RR[MAESTRO-RR\nFetch throws in codex-usage-sampler]
RR --> RR2[Return error snapshot\nNo Sentry report]
B --> M9[MAESTRO-M9\nSession file >= 500 MB]
M9 --> M9b{fileStat.size > MAX?}
M9b -->|Yes| M9c[logger.warn + continue\nSkip file, no crash]
M9b -->|No| M9d[readFile + parse normally]
B --> SP[MAESTRO-SP\nShortcut fires before DB ready]
SP --> SP2{db.isReady?}
SP2 -->|false| SP3[return null silently]
SP2 -->|true| SP4[incrementShortcutUsage\nbroadcast stats:updated]
B --> QF[MAESTRO-QF\nsetAll/setMany returns false]
QF --> QF2[persistInternal throws\nrecoverable disk error]
QF2 --> QF3{message.includes\nrecoverable disk error?}
QF3 -->|Yes| QF4[Suppress Sentry\nisPending preserved for retry]
QF3 -->|No| QF5[captureException to Sentry]
Reviews (1): Last reviewed commit: "fix(telemetry): handle obvious main-bran..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| isReady(): boolean { | ||
| return this.db !== null; | ||
| } |
There was a problem hiding this comment.
isReady() definition — compile error and weaker semantics
This PR adds a second isReady() at line 103 that returns this.db !== null, but a pre-existing isReady() already exists in the same class at line 188–190 and returns this.initialized && this.db !== null. TypeScript will emit error TS2300: Duplicate identifier 'isReady'. Beyond the compile failure, the semantics differ: the pre-existing check requires both the DB handle to be open and initialize() to have run to completion (which includes running migrations); the new one only checks for the handle, meaning it could return true between the this.db = new Database(...) assignment (line 139) and runMigrations() finishing (line 149/151). The pre-existing method should be removed and the new one updated to preserve the stronger this.initialized && this.db !== null guard.
| // Maestro bug, so keep it out of Sentry. Genuine flush failures (real | ||
| // exceptions) still report. (MAESTRO-QF) | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| if (!message.includes('recoverable disk error')) { |
There was a problem hiding this comment.
The message.includes('recoverable disk error') guard couples the catch site (line 391) to the throw-site messages at lines 340 and 355 through a magic string. If either message is ever rephrased without updating this check, the recoverable error will silently start reaching Sentry (or, conversely, a new genuine error with that substring will be silently swallowed). A dedicated error subclass — e.g. class RecoverableDiskError extends Error {} — gives the same locality with a robust instanceof check that can't go stale.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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/stats/stats-db.ts`:
- Around line 96-105: The StatsDB class has duplicate isReady() method
definitions, with one at lines 96-105 and another at line 188. Remove the first
isReady() method implementation (the one shown in the diff at lines 96-105 with
the JSDoc comment about the database handle being open) to eliminate the
duplicate method declaration, as having two methods with the same name in the
same class will cause compilation errors.
🪄 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: d35e94b3-1578-40c6-a3a6-329271cc42ec
📒 Files selected for processing (8)
src/__tests__/main/agents/codex-usage-sampler.test.tssrc/__tests__/main/ipc/handlers/stats.test.tssrc/__tests__/renderer/hooks/utils/useDebouncedPersistence.test.tssrc/main/agents/codex-usage-sampler.tssrc/main/ipc/handlers/agentSessions.tssrc/main/ipc/handlers/stats.tssrc/main/stats/stats-db.tssrc/renderer/hooks/utils/useDebouncedPersistence.ts
| /** | ||
| * True once the underlying database handle is open and usable. Lets | ||
| * best-effort callers (e.g. fire-and-forget analytics writes) skip | ||
| * gracefully instead of throwing "Database not initialized" when a write | ||
| * races app startup (the handle is created in `initialize()`, called on | ||
| * app-ready). (MAESTRO-SP) | ||
| */ | ||
| isReady(): boolean { | ||
| return this.db !== null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify only one StatsDB.isReady() implementation exists after fix.
rg -nP '^\s*isReady\(\): boolean\s*\{' src/main/stats/stats-db.ts
# Expected: exactly one matchRepository: RunMaestro/Maestro
Length of output: 114
Remove the duplicate isReady() implementation at lines 96–105.
StatsDB defines isReady() twice: at line 103 (in the changed code) and line 188. Duplicate method declarations in a class are a critical error and will cause a compilation failure or unexpected behavior.
Suggested fix
- /**
- * True once the underlying database handle is open and usable. Lets
- * best-effort callers (e.g. fire-and-forget analytics writes) skip
- * gracefully instead of throwing "Database not initialized" when a write
- * races app startup (the handle is created in `initialize()`, called on
- * app-ready). (MAESTRO-SP)
- */
- isReady(): boolean {
- return this.db !== null;
- }🤖 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/stats/stats-db.ts` around lines 96 - 105, The StatsDB class has
duplicate isReady() method definitions, with one at lines 96-105 and another at
line 188. Remove the first isReady() method implementation (the one shown in the
diff at lines 96-105 with the JSDoc comment about the database handle being
open) to eliminate the duplicate method declaration, as having two methods with
the same name in the same class will cause compilation errors.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/stats/stats-db.ts (1)
96-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
isReady()method causes compilation failure.This is already flagged in a previous review. The class defines
isReady()twice:
- Lines 103-105:
return this.db !== null;- Lines 188-190:
return this.initialized && this.db !== null;The existing implementation at lines 188-190 is semantically correct—it ensures migrations have completed before allowing writes. Remove the new duplicate and update the existing method's JSDoc to document the MAESTRO-SP use case.
Suggested fix
Remove lines 96-105 entirely, then update the existing method's docstring:
/** - * Check if database is initialized and ready + * True once the underlying database handle is open and usable. Lets + * best-effort callers (e.g. fire-and-forget analytics writes) skip + * gracefully instead of throwing "Database not initialized" when a write + * races app startup (the handle is created in `initialize()`, called on + * app-ready). (MAESTRO-SP) */ isReady(): boolean { return this.initialized && this.db !== null; }🤖 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/stats/stats-db.ts` around lines 96 - 105, The isReady() method is defined twice in the class causing a compilation failure. Remove the duplicate isReady() method that returns this.db !== null along with its JSDoc comment block. Keep the existing implementation of isReady() at lines 188-190 that returns this.initialized && this.db !== null, and update its JSDoc to include the MAESTRO-SP use case documentation that was in the removed method's comment.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@src/main/stats/stats-db.ts`:
- Around line 96-105: The isReady() method is defined twice in the class causing
a compilation failure. Remove the duplicate isReady() method that returns
this.db !== null along with its JSDoc comment block. Keep the existing
implementation of isReady() at lines 188-190 that returns this.initialized &&
this.db !== null, and update its JSDoc to include the MAESTRO-SP use case
documentation that was in the removed method's comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5619ee3e-3450-432b-9847-09ad16d47417
📒 Files selected for processing (8)
src/__tests__/main/agents/codex-usage-sampler.test.tssrc/__tests__/main/ipc/handlers/stats.test.tssrc/__tests__/renderer/hooks/utils/useDebouncedPersistence.test.tssrc/main/agents/codex-usage-sampler.tssrc/main/ipc/handlers/agentSessions.tssrc/main/ipc/handlers/stats.tssrc/main/stats/stats-db.tssrc/renderer/hooks/utils/useDebouncedPersistence.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/tests/main/agents/codex-usage-sampler.test.ts
- src/renderer/hooks/utils/useDebouncedPersistence.ts
- src/main/agents/codex-usage-sampler.ts
- src/tests/renderer/hooks/utils/useDebouncedPersistence.test.ts
- src/main/ipc/handlers/stats.ts
- src/tests/main/ipc/handlers/stats.test.ts
1 participant
Summary
Triages the
channel:stable(=main) Sentry issues for Maestro and fixes the four that affect the current 0.17.x release and have an obvious bug + path to resolution. All four are the same family the prior triages addressed: expected/recoverable conditions being reported to Sentry as crashes.Validated each against current
maincode (notrc) before fixing. Issues that are build/packaging (MAESTRO-RSglibc), native Chromium crashes (SG/RB/SZ/Y), already-archived (1G), genuine spawn failures worth surfacing (R0/NM), or already-fixed-on-main but still trickling from old 0.15.x builds (4Y,BH) were deliberately left out of scope.Fixes
reason: request: fetch failedfetch(offline / DNS / TLS / abort-timeout) is expected. Stop reporting it to Sentry. The 401/403 carve-out and unexpected-HTTP reporting are unchanged.RangeError: Invalid string lengthinagentSessionsglobal-stats refreshrc→mainmerge.stats:record-shortcut-usage … Database not initializedStatsDB.isReady()).sessions:setMany returned false (recoverable disk error)isPendingfor retry) when the main process returns a recoverable disk error. Keep that expected user-environment condition out of Sentry; genuine flush failures still report.Tests
Adds regression tests for RR (network failure not reported), SP (skips silently when DB not ready; records + broadcasts when ready), and QF (recoverable disk error not reported; genuine failure still reported).
npm run lint(tsc) — no new errors in changed filesnpm run lint:eslint— cleanprettier --check— cleanSummary by CodeRabbit