fs.watch: fully initialize ConcurrentTask in FSWatchTask.enqueue - #29950
Closed
robobun wants to merge 7 commits into
Closed
fs.watch: fully initialize ConcurrentTask in FSWatchTask.enqueue#29950robobun wants to merge 7 commits into
robobun wants to merge 7 commits into
Claude / Claude Code Review
completed
May 4, 2026 in 18m 0s
Code review found 1 potential issue
Found 1 candidates, confirmed 1. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/node/watch/fs.watch.concurrent-task.test.ts:62-64 |
Inaccurate comment: close() does not exercise the enqueueTaskConcurrent assertion |
Annotations
Check warning on line 64 in test/js/node/watch/fs.watch.concurrent-task.test.ts
claude / Claude Code Review
Inaccurate comment: close() does not exercise the enqueueTaskConcurrent assertion
Nit: this comment (and the one at line 44, "close tasks (also routed via enqueue)") is inaccurate — on POSIX, `FSWatcher.close()` runs on the JS thread and emits `'close'` synchronously via `emitJS()`; it never goes through `FSWatchTask.enqueue()` / `enqueueTaskConcurrent()`. So if this 10s fallback fires with `received === 0`, the new assertion was *not* exercised. Suggest rewording to e.g. "the regression signal is the assertion / no heap corruption, not delivery count, so still pass" and drop
Loading