Repository navigation
Reload cached files for Created watch events - #64909
John Favret (johnfav03) wants to merge 3 commits into
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
🟡 Changes recommended
Directory deletions can leave symlink caches stale, and ordinary atomic saves unnecessarily traverse every cached alias.
2 open issues
What changed in this PR
Fixes #64195 by refreshing language-server file caches when atomic saves produce Created watch events.
Changes:
- Reloads cached files for created paths, directories, and symlink aliases.
- Compares open-file overlays with disk contents after save and watch events.
- Adds regression coverage for atomic saves and bulk notifications.
| File | Description |
|---|---|
| tsc/internal/project/snapshotfs.go | Expands created events and reloads cached contents. |
| tsc/internal/project/snapshotfs_test.go | Tests created events and symlink cache handling. |
| tsc/internal/project/snapshot.go | Expands events before bulk invalidation. |
| tsc/internal/project/session_test.go | Tests atomic saves and overlay consistency. |
| tsc/internal/project/overlayfs.go | Updates overlay-to-disk comparisons. |
| tsc/internal/project/overlayfs_test.go | Tests save/watch ordering. |
| tsc/internal/project/bulkcache_test.go | Tests created-event batches and directory notifications. |
Review effort: Balanced
Jake Bailey (jakebailey)
left a comment
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
I had copilot look at this locally and it noticed a few things
I think the fix itself is right: treating a Created event on a cached file as a possible content change fixes the atomic-save case, and the tests cover it well. My main concern is the disk checks added before the large-batch check (see the comments on snapshot.go and the two DirectoryExists calls), and I think they can be removed entirely.
| fileChanges = s.fs.expandRealpathAliases(fileChanges, fs.fs) | ||
| fileChanges = fs.expandCreatedCacheEntries(fileChanges) |
There was a problem hiding this comment.
These two expansions now run before the HasExcessiveWatchEvents() check, and both call DirectoryExists on every created URI. A large npm install or branch switch can send tens of thousands of Created events, so we'd make tens of thousands of disk calls before we decide to take the cheap path for large batches. That could be noticeable on Windows.
I don't think the disk check is needed (see the comments on the two DirectoryExists calls). Without it, this stays a set of in-memory lookups, as it was before.
| previousOpenFiles := overlayFileHandles(previousOverlays) | ||
| openFiles := overlayFileHandles(overlays) | ||
| fileChanges = s.fs.expandRealpathAliases(fileChanges, fs.fs) | ||
| fileChanges = fs.expandCreatedCacheEntries(fileChanges) |
There was a problem hiding this comment.
Since the expansion happens before the threshold check, one Created event for a directory that has many cached files under it can push the batch over excessiveChangeThreshold. If that directory is outside node_modules, the whole file cache gets cleared. Is that intended? If it is, a test showing it would help.
| _, contentMapperWatchedFiles := s.contentMapperWatchState() | ||
| fileChanges = fs.expandAndFilterWatchEvents(fileChanges, contentMapperExtensions, contentMapperWatchedFiles, previousOpenFiles, openFiles) | ||
| fileChanges = s.fs.expandRealpathAliases(fileChanges) | ||
| fileChanges = s.fs.expandRealpathAliases(fileChanges, fs.fs) |
There was a problem hiding this comment.
In the non-excessive branch, expandRealpathAliases now runs twice. I see the second call is still needed for directory deletes expanded by expandAndFilterWatchEvents. Could the first call move into the excessive branch, or could the second call only expand Deleted?
| } | ||
| createdDirectories := collections.NewSetWithSizeHint[tspath.PathKey](change.Created.Len()) | ||
| for uri := range change.Created.Keys() { | ||
| if s.fs.DirectoryExists(tspath.RootedDirectoryPathFromPath(uri.FileName().AsPath())) { |
There was a problem hiding this comment.
collectFilesRecursive only walks cacheDirectories, so could this use s.cacheDirectories.Get(path) instead of s.fs.DirectoryExists(...)? That's how directory Deleted events are handled in expandAndFilterWatchEvents, and it avoids touching the disk.
| if created { | ||
| if s.fs.Overlays()[path] != nil { |
There was a problem hiding this comment.
This now filters Created events: it drops open files and cached files whose content hasn't changed. That goes against the comment in expandAndFilterWatchEvents ("We can't filter created events…"). It also means the config file registry and the auto-import registry never see those events.
I couldn't find a case that breaks, but could you add a comment explaining why this is safe? Also, why are open files dropped here when the Changed path keeps them?
| if change.Created.Len() > 0 { | ||
| createdDirectories := collections.NewSetWithSizeHint[tspath.PathKey](change.Created.Len()) | ||
| for uri := range change.Created.Keys() { | ||
| if currentFS.DirectoryExists(tspath.RootedDirectoryPathFromPath(uri.FileName().AsPath())) { |
There was a problem hiding this comment.
The DirectoryExists check doesn't seem necessary here either. A created path can only be a parent of a cached realpath if it's a directory. So every created path can go into createdDirectories and the parent walk will still only match real directories. That would also let you drop the new currentFS parameter.
|
|
||
| if events.saved { | ||
| if o != nil { | ||
| if o != nil && (events.watchObserved || events.saved) { |
There was a problem hiding this comment.
Nit: with this change, every didSave reads the file from disk while holding fs.mu. Before, an open file was simply assumed to match the disk after a save. Comparing against the disk is more accurate, so this is probably fine, but I wanted to point out the extra read on every save.
Fixes #64195
Atomic saves replace an existing file by renaming a temporary file over it; file watchers can report that replacement as
Createdrather thanChanged. We previously only invalidated the cache onChangedevents; when a file that already exists in the program received aCreatedevent, its cached contents remained stale, causing diagnostics to reflect outdated info.