Snapshots: more logging, fix memory leak #1641
Conversation
andrewbranch
requested review from
Copilot and
gabritto
and removed request for
Copilot
| // during `npm install` or git operations. | ||
| // !!! This can probably be replaced by ScheduleDiagnosticsRefresh() | ||
| snapshotUpdateCancel context.CancelFunc | ||
| snapshotUpdateMu sync.Mutex |
There was a problem hiding this comment.
Removing this and the related code was a bit of a drive-by cleanup. There’s really no reason to trigger a snapshot change internally; the only time we were using this we also scheduled a diagnostics refresh, and the subsequent diagnostics request would have updated the snapshot as needed. It's simpler and cleaner to let snapshot updates be purely reactive to a client request/notification.
There was a problem hiding this comment.
Pull Request Overview
This PR improves logging infrastructure and fixes a memory leak in the disk file cache. The snapshot cloning process gains more detailed logging for debugging, while cached disk files are now properly cleaned up when projects are closed.
- Added detailed reason logging for snapshot updates to improve debugging visibility
- Fixed memory leak by implementing disk file cache cleanup when projects are no longer active
- Enhanced cache statistics logging and removed redundant snapshot update scheduling
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/project/snapshot_test.go | Adds test to verify disk file cache cleanup functionality |
| internal/project/snapshot.go | Implements disk file cleanup logic and enhanced logging for snapshot cloning |
| internal/project/session.go | Adds update reason tracking, removes redundant snapshot scheduling, and improves cache logging |
| internal/project/projectcollectionbuilder.go | Preserves seenFiles when cloning programs to maintain file tracking |
| internal/project/overlayfs.go | Removes unused IncludesWatchChangesOnly tracking |
| internal/project/filechange.go | Removes IncludesWatchChangesOnly field from FileChangeSummary |
| internal/project/compilerhost.go | Adds seenFiles tracking to compiler host for file access monitoring |
| internal/project/ata/ata_test.go | Updates test to trigger diagnostics refresh for proper ATA testing |
| internal/lsp/server.go | Adds support for LSP trace notifications to control logging verbosity |
I forgot to clean up the diskFile cache, so it was growing infinitely. The new logging exposed the problem.