Skip to content

domain: route fs callback throws to uncaughtException, unblocking 4 tests (domain 88%→96%) - #34661

Open
cirospaciari wants to merge 99 commits into
claude/port-node-domain-testsfrom
claude/domain-fs-callback-throw
Open

domain: route fs callback throws to uncaughtException, unblocking 4 tests (domain 88%→96%)#34661
cirospaciari wants to merge 99 commits into
claude/port-node-domain-testsfrom
claude/domain-fs-callback-throw

test,fs: skip abort entries in promises.watch filter, parent-owned te…

9298497
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Aug 8, 2026 in 35m 42s

Code review found 3 potential issues

Found 3 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 3
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/js/node/fs.promises.ts:100-110 fs.promises.watch native callback missed by guardCallback sweep: throwing ignore fn hangs
🟡 Nit src/js/thirdparty/ws.js:428-430 ws.js once() override is now a pure passthrough (dead code from #armAndOn refactor)
🟡 Nit src/js/internal/fs/watch.ts:188-192 fs.watch abort branch's bare catch{} swallows a throwing 'error' listener

Annotations

Check warning on line 110 in src/js/node/fs.promises.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

fs.promises.watch native callback missed by guardCallback sweep: throwing ignore fn hangs

The 4b143eb72d `guardCallback` sweep wrapped `internal/fs/watch.ts:162` and `internal/fs/watchfile.ts:26`, but this third native `fs.watch` listener consumer passes an unwrapped callback that invokes the user-supplied `ignoreMatcher?.(filename)` at L106. If `options.ignore` is a function that throws, the throw reaches native `emit_js` → `report_active_exception_as_unhandled` → keep-alive `uncaught_exception()`, and with this PR's removal of `unhandled_error_counter` from `is_event_loop_alive_exc

Check warning on line 430 in src/js/thirdparty/ws.js

See this annotation in the file changed.

@claude claude / Claude Code Review

ws.js once() override is now a pure passthrough (dead code from #armAndOn refactor)

The `once()` override is now a pure passthrough after 8eb8f030a9 collapsed `#onOrOnce` into `#armAndOn`: `super.once()` (events.ts:343) does `this.on(type, _onceWrap(...))`, which dispatches to BunWebSocket's overridden `on()` → `#armAndOn()`, so inheriting `EventEmitter.prototype.once` directly is byte-identical. Per REVIEW.md § *Delete dead code in the same PR that makes it dead*, drop the three-line override (the `onceObject` constant is still used at L312/L548 and stays).

Check warning on line 192 in src/js/internal/fs/watch.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

fs.watch abort branch's bare catch{} swallows a throwing 'error' listener

The bare `try { this.emit('error', ...) } catch {}` in the new `'abort'` branch is broader than its stated purpose (suppressing ERR_UNHANDLED_ERROR when no listener exists): it also swallows a throw from the user's own `'error'` listener, whereas the sibling `'error'` branch at L199 emits unwrapped so a throwing listener there reaches `guardCallback` → `reportUncaughtException` → fatal. Gate on `if (this.listenerCount('error') > 0) this.emit('error', filenameOrError)` instead — that keeps `test-