Repository navigation
Fix watch coalescing for FSEvents - #4379
Conversation
Coalesce dense directory watch sets into recursive ancestor watches on fast-recursive backends such as FSEvents and Windows. This avoids opening one stream per dependency directory while still surfacing startup failures.
|
This is doing this in the layer above fswatch, but, going to try and shove it down... |
Consolidate dense directory subscriptions inside fswatch for fast recursive backends. Parent streams now serve later child subscriptions with per-subscriber filtering, so all fswatch callers benefit without watch-manager-specific planning.
Add a tsc watch integration baseline covering many dependency files under a Bun-style node_modules/.bun layout. The lower-level fswatch tests assert stream coalescing; this keeps the command-line watch shape covered end-to-end.
# Conflicts: # internal/fswatch/watcher.go
There was a problem hiding this comment.
Pull request overview
This PR addresses tsgo --watch startup failures on large directory sets by consolidating many directory watches into fewer recursive watches when the underlying backend supports fast recursion (notably macOS FSEvents and Windows), reducing the likelihood of hitting backend stream/handle limits.
Changes:
- Add fast-recursive-aware directory watch consolidation in
internal/fswatch, including per-subscriber filtering when multiple logical watches share one recursive underlying watch. - Extend the in-process LSP watcher backend interface to expose “fast recursive backend” capability and use that when installing some watches.
- Add regression coverage: a new
tscWatchscenario modeled after Bun’s.bun/pkg*layout plus focusedfswatchunit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/baselines/reference/tscWatch/commandLineWatch/watch-handles-many-bun-dependency-files.js | Adds expected baseline output for a new watch scenario with many Bun-style dependency directories. |
| internal/lsp/lspwatcher/lspwatcher.go | Exposes backend fast-recursive capability and conditionally requests recursive watching in reconcile logic. |
| internal/lsp/lspwatcher/lspwatcher_test.go | Updates the fake backend to model fast-recursive capability for tests. |
| internal/fswatch/watcher.go | Implements consolidation of dense directory watch sets into recursive ancestor watches for fast-recursive backends and refines callback filtering. |
| internal/fswatch/watcher_test.go | Adds tests validating consolidation behavior and filtering correctness. |
| internal/execute/tsctests/tscwatch_test.go | Adds a new tscWatch scenario that creates many Bun-style dependency files and runs in watch mode. |
|
I'm not 100% certain about this; the general idea is to coalesce watches of many subdirs into one watch. That staves off the problem a bit at the cost of more watch events that need to be ignored. Maybe that's fine, but surely we can hit this limit another way? Maybe we need to have this fall back to kqueue or something on error, if we can even detect when the error is us using too many resources. And we know kqueue is already slow. But, it is what Node uses (not fsevents). Not sure. |
|
The other option is to move this into app logic, like the LSP watcher does unconditionally today and conditionally in https://github.com/andrewbranch/typescript-go/tree/lsp-builtin-watcher-granular. |
|
That's what I had in the PR originally, but I feel like it's still better to have it pushed down, since theoretically we could make fswatch transparently switch over to kqueue, whereas that would be kind of annoying above. I need to see if I can make the kqueue backend any faster... |
|
In my experience, kqueue is untenable for the scale of watches we often try to register. I profiled it and it was all syscall time. Nothing to optimize. |
|
I guess, registering them in parallel would probably help, but I still don't think that's a good approach. |
|
Ultimately, I guess my question is why Strada did not have major issues here, given it could only use kqueue as Node via libuv only did not do fsevents. |
|
I think this PR is fine, but, I think #4447 is the wider fix; might be able to un-coalesce, even. |
Fixes #4378
Coalesce dense directory watch sets into recursive ancestor watches on fast-recursive backends such as FSEvents and Windows. This avoids opening one stream per dependency directory while still surfacing startup failures.