fix: silence main-process Sentry noise from FS watchers#879
Conversation
📝 WalkthroughWalkthroughThe changes introduce a centralized Windows system file ignore pattern for filesystem watchers and improve error handling across multiple watcher implementations to prevent unhandled rejections on Windows systems. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR fixes two classes of Windows-only Sentry noise: unhandled rejections from Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions with no functional impact. No P0 or P1 issues found. The regex is correct (case-insensitive, handles both / and \ separators, covers all documented locked filenames). Error handlers and try/catch patterns are logically sound. Tests cover the new code paths. The only findings are minor: a missing inline comment on the regex and a redundant ignore entry on the depth-0 git watcher. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Chokidar / fs.watch starts] --> B{Path contains locked\nWindows system file?}
B -- Yes --> C[WINDOWS_LOCKED_SYSTEM_FILES\nregex matches → skip]
B -- No --> D[Watch proceeds normally]
D --> E{Runtime error emitted\ne.g. EBUSY / UNKNOWN}
E -- fs.watch --> F[.on error handler\nlogs warn, swallows]
E -- chokidar --> G{Error matches lstat\n+ system filename?}
G -- Yes --> H[Sentry beforeSend\nreturns null → dropped]
G -- No --> I[Sent to Sentry normally]
C --> J[No EBUSY noise]
F --> J
H --> J
Reviews (1): Last reviewed commit: "test: update git watcher assertion for i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| /** Windows drive-root system files that always fail lstat with EBUSY. */ | ||
| export const WINDOWS_LOCKED_SYSTEM_FILES: RegExp = new RegExp( | ||
| '(^|[/\\\\])(pagefile\\.sys|hiberfil\\.sys|swapfile\\.sys|DumpStack\\.log(\\.tmp)?|System Volume Information)$', |
There was a problem hiding this comment.
System Volume Information not covered by dotfile ignore, but matched correctly
Minor readability note: System Volume Information contains a space and is not a file extension pattern, so it stands out from the other entries. A brief inline comment on that entry would help future readers understand why it's included alongside .sys files. The regex itself is correct.
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!
|
|
||
| // Start watching the directory (only top level, not recursive) | ||
| const watcher = chokidar.watch(worktreePath, { | ||
| ignored: /(^|[/\\])\../, // Ignore dotfiles | ||
| ignored: [ | ||
| /(^|[/\\])\../, // Ignore dotfiles | ||
| WINDOWS_LOCKED_SYSTEM_FILES, | ||
| ], |
There was a problem hiding this comment.
WINDOWS_LOCKED_SYSTEM_FILES applied to a depth: 0, non-drive-root watcher
The git worktree watcher is configured with depth: 0 and always targets a specific worktree directory (never a Windows drive root), so it can't actually encounter pagefile.sys/hiberfil.sys during its walk. Adding the ignore is harmless, but it inflates the ignored option unnecessarily. If someone later audits why locked-file ignores appear on a Git worktree watcher, it may cause confusion. Consider keeping it consistent with other chokidar watchers only if there's a real EBUSY risk at the worktree path.
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
🧹 Nitpick comments (2)
src/main/history-manager.ts (1)
496-516: LGTM — scoped try/catch +'error'listener addresses the Windows watcher noise.Wrapping
fs.watch()and installing an'error'listener matches the pattern used inmarketplace.tsand directly targets the MAESTRO-9A unhandled-rejection class. Settingthis.watcher = nullin the catch branch correctly preserves the "not watching" invariant sostopWatching()remains a no-op and a laterstartWatching()call will retry.Minor note (non-blocking):
fs.mkdirSyncon line 493 is still outside thetry/catch. A Windows directory-creation failure (e.g., EPERM on a locked/virtualized userData path) would propagate as before. If you want full symmetry with the "don't let watcher setup crash the app" goal, consider moving themkdirSynccall inside the sametryblock. Leaving as-is is also defensible since an unwritable userData dir is a genuinely unexpected condition worth surfacing.♻️ Optional: include mkdirSync in the guarded block
- // Ensure directory exists before watching - if (!fs.existsSync(this.historyDir)) { - fs.mkdirSync(this.historyDir, { recursive: true }); - } - try { + // Ensure directory exists before watching + if (!fs.existsSync(this.historyDir)) { + fs.mkdirSync(this.historyDir, { recursive: true }); + } + this.watcher = fs.watch(this.historyDir, (_eventType, filename) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/history-manager.ts` around lines 496 - 516, The mkdirSync that creates the historyDir is outside the guarded start-up block, so a Windows/permission error from fs.mkdirSync can still crash startup; move the fs.mkdirSync(...) call into the same try block that creates and assigns this.watcher (the block that calls fs.watch and sets this.watcher and installs the 'error' listener) so mkdir failures are caught, logged by the existing catch which sets this.watcher = null, and do not produce an unhandled exception—adjust the startWatching/init logic around fs.mkdirSync and this.watcher accordingly.src/__tests__/main/history-manager.test.ts (1)
1288-1310: LGTM — new tests cover both watcher-error paths.The two added tests correctly exercise (1)
watcher.on('error', ...)registration and (2) the synchronousfs.watchthrow →logger.warn+ no rethrow behavior added instartWatching(). Mock updates to includeon: vi.fn()across existing tests are consistent.One optional tightening: in the "fs.watch throws" test you could also assert that a subsequent
manager.stopWatching()is safe to call (sincethis.watcheris set tonullin the catch branch) and that a latermanager.startWatching(...)can recover by re-invokingfs.watch. Not required — the existing "safe to call stopWatching when not watching" test already covers the null case indirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/history-manager.test.ts` around lines 1288 - 1310, Add assertions to the "fs.watch itself throws" test to verify recovery: after mocking fs.watch to throw and calling manager.startWatching(...), call manager.stopWatching() and assert it does not throw (ensuring this.watcher is null/safe), then set mockWatch to a successful watcher mock and call manager.startWatching(...) again and assert mockWatch was invoked (or watcher.on('error') was registered) to confirm recovery; reference manager.startWatching, manager.stopWatching, and mockWatch in the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/index.ts`:
- Around line 217-220: Replace the loose substring check against exceptionValue
with logic that extracts the lstat path from the existing regex (the current
/^(EBUSY|EPERM): [^,]+, lstat /i.test(exceptionValue) usage), then test that
extracted path against the shared WINDOWS_LOCKED_SYSTEM_FILES matcher instead of
scanning the whole exception string; use the same extraction and test so false
positives like "my-pagefile.sys.bak" are not matched and the rule stays
consistent with the watcher ignore rule.
---
Nitpick comments:
In `@src/__tests__/main/history-manager.test.ts`:
- Around line 1288-1310: Add assertions to the "fs.watch itself throws" test to
verify recovery: after mocking fs.watch to throw and calling
manager.startWatching(...), call manager.stopWatching() and assert it does not
throw (ensuring this.watcher is null/safe), then set mockWatch to a successful
watcher mock and call manager.startWatching(...) again and assert mockWatch was
invoked (or watcher.on('error') was registered) to confirm recovery; reference
manager.startWatching, manager.stopWatching, and mockWatch in the changes.
In `@src/main/history-manager.ts`:
- Around line 496-516: The mkdirSync that creates the historyDir is outside the
guarded start-up block, so a Windows/permission error from fs.mkdirSync can
still crash startup; move the fs.mkdirSync(...) call into the same try block
that creates and assigns this.watcher (the block that calls fs.watch and sets
this.watcher and installs the 'error' listener) so mkdir failures are caught,
logged by the existing catch which sets this.watcher = null, and do not produce
an unhandled exception—adjust the startWatching/init logic around fs.mkdirSync
and this.watcher accordingly.
🪄 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: 335b77a5-77ac-4c9c-aa65-05ccc0ce5a0f
📒 Files selected for processing (9)
src/__tests__/main/history-manager.test.tssrc/__tests__/main/ipc/handlers/git.test.tssrc/main/history-manager.tssrc/main/index.tssrc/main/ipc/handlers/autorun.tssrc/main/ipc/handlers/documentGraph.tssrc/main/ipc/handlers/git.tssrc/main/ipc/handlers/marketplace.tssrc/main/utils/watcher-ignore.ts
| /^(EBUSY|EPERM): [^,]+, lstat /i.test(exceptionValue) && | ||
| /(pagefile\.sys|hiberfil\.sys|swapfile\.sys|DumpStack\.log|System Volume Information)/i.test( | ||
| exceptionValue | ||
| ) |
There was a problem hiding this comment.
Reuse the shared locked-file matcher here.
Line 218 scans the whole exception string with loose substrings, so real EBUSY/EPERM lstat failures for paths like my-pagefile.sys.bak can be dropped from Sentry. Extract the lstat path and apply WINDOWS_LOCKED_SYSTEM_FILES to avoid drift with the watcher ignore rule.
♻️ Proposed fix
import { WakaTimeManager } from './wakatime-manager';
+import { WINDOWS_LOCKED_SYSTEM_FILES } from './utils/watcher-ignore';- if (
- /^(EBUSY|EPERM): [^,]+, lstat /i.test(exceptionValue) &&
- /(pagefile\.sys|hiberfil\.sys|swapfile\.sys|DumpStack\.log|System Volume Information)/i.test(
- exceptionValue
- )
- ) {
+ const lockedSystemFileLstat = exceptionValue.match(
+ /^(?:EBUSY|EPERM): [^,]+, lstat ['"]?(.+?)['"]?$/i
+ );
+ const lstatPath = lockedSystemFileLstat?.[1]?.replace(/[\\/]+$/, '');
+ if (lstatPath && WINDOWS_LOCKED_SYSTEM_FILES.test(lstatPath)) {
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/index.ts` around lines 217 - 220, Replace the loose substring check
against exceptionValue with logic that extracts the lstat path from the existing
regex (the current /^(EBUSY|EPERM): [^,]+, lstat /i.test(exceptionValue) usage),
then test that extracted path against the shared WINDOWS_LOCKED_SYSTEM_FILES
matcher instead of scanning the whole exception string; use the same extraction
and test so false positives like "my-pagefile.sys.bak" are not matched and the
rule stays consistent with the watcher ignore rule.
1 participant
Summary
Triaged the top unresolved main-process Sentry issues and landed fixes for the two that are real bugs on
main.fs.watch()(14 events; "UNKNOWN: unknown error, watch" on Windows).history-manager.tsandmarketplace.tscreated watchers without.on('error'), so transient errors escaped as unhandled rejections.EBUSY: resource busy or locked, lstat 'C:\DumpStack.log.tmp'/hiberfil.sys(1,279 events). Users who point a watcher at or near a drive root hit always-locked Windows system files during chokidar's initial walk.Three other top issues were ruled out (noted in commit body): MAESTRO-87 is from a fork ("semantica" v0.56.1), MAESTRO-B9 is already fixed by
70332d993(MAX_SESSION_FILE_SIZEguard), and MAESTRO-FM lives on thercbranch only.Changes
src/main/utils/watcher-ignore.tsexporting a sharedWINDOWS_LOCKED_SYSTEM_FILESregex (pagefile.sys, hiberfil.sys, swapfile.sys, DumpStack.log[.tmp], System Volume Information).documentGraph,autorun, andgit:watchWorktreeDirectory.history-manager.tsandmarketplace.ts: addedtry/catcharoundfs.watch()creation plus an.on('error', ...)handler that logs vialogger.warninstead of crashing.beforeSendfilter insrc/main/index.tsdrops any residualEBUSY/EPERM lstatevents targeting the locked paths as defense in depth.history-manager.test.tsfor the new error handler and fs.watch-throws fallback; updated the existing FSWatcher mocks to exposeon; relaxed thegit:watchWorktreeDirectoryignored-array assertion.Test plan
npx tsc --noEmit— no new errorsnpx vitest run src/__tests__/main/ipc/handlers/git.test.ts— passesC:\on a Windows box and confirm no EBUSY noise lands in Sentry on the RC channel.Summary by CodeRabbit
Bug Fixes
Improvements