-
Notifications
You must be signed in to change notification settings - Fork 718
Invalidate caches on batches of 1000+ watch changes #1869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements cache invalidation for scenarios where a large number (1000+) of file system watch changes are processed in a single batch, which can overwhelm the system's ability to track individual changes accurately.
Key changes:
- Adds logic to detect excessive watch changes (>1000) and invalidate file system and configuration caches
- Distinguishes between changes in
node_modules
vs. other directories to apply appropriate invalidation scope - Removes unused
loadKind
parameter from configuration discovery methods
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
internal/project/filechange.go |
Adds constants and methods to detect excessive watch changes and distinguish watch event types |
internal/project/overlayfs.go |
Tracks whether watch changes occur outside node_modules directories |
internal/project/snapshot.go |
Implements cache invalidation logic based on change volume and location |
internal/project/snapshotfs.go |
Adds methods to invalidate file system caches selectively or completely |
internal/project/configfileregistrybuilder.go |
Adds bulk configuration invalidation and removes unused loadKind parameter |
internal/project/configfileregistry.go |
Updates config file entry to store file names for cache validation |
internal/project/projectcollectionbuilder.go |
Updates method calls to remove unused loadKind parameter |
internal/project/dirty/map.go |
Adds Clear() method to reset dirty map state |
internal/project/bulkcache_test.go |
Comprehensive tests for bulk cache invalidation behavior |
One worry I have with this approach is that now that we are watching basically all TS/JS within the workspace, that some other build system like vite or angular or something writes a bunch of dist files and we detect those and then effectively restart everything. Is that something that can happen or do we filter this somehow? I assume we don't given the filtering is the expensive thing we are trying to avoid... |
Filtering is not expensive, but how would you propose we filter for that? |
I suppose the happy case is that none of the changes we pick up are in our cache at all. That's easy to check for. |
Although, that doesn’t help much with regard to new files being created on disk—if you add 1000 new files, is that an external build tool or a |
Refactored with a bit more of a sophisticated strategy. In order for the 1000+ changes to trigger a cache invalidation, we have to find something in the events that were collected that impacts the cache in question. If something is found, only then do we assume other events that affect the same cache could have been missed. |
1000 was picked arbitrarily, but I’m not sure what else we can do.
Fixes #1646