Decouple snapshot ownership from project.Session so api.Session only uses one in LSP mode - #64163
Conversation
…uses one in LSP mode
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
🟡 Changes recommended
Async connection shutdown can wait indefinitely for handlers whose context remains live after EOF.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Decouples snapshot ownership from project.Session, enabling standalone API sessions to manage snapshots directly while LSP sessions retain canonical state.
Changes:
- Introduces
SnapshotStorefor snapshot creation, caches, IDs, and disposal. - Splits API construction into standalone and LSP-backed sessions.
- Tracks asynchronous IPC handlers during connection shutdown.
File summaries
| File | Description |
|---|---|
tsc/internal/project/snapshotstore.go |
Adds shared snapshot infrastructure. |
tsc/internal/project/snapshot.go |
Moves snapshot dependencies to SnapshotStore. |
tsc/internal/project/snapshot_test.go |
Updates snapshot adoption test. |
tsc/internal/project/session.go |
Delegates snapshot ownership to the store. |
tsc/internal/project/refcountcache_test.go |
Updates cache ownership references. |
tsc/internal/project/extendedconfigcache_test.go |
Updates extended-config cache references. |
tsc/internal/project/api.go |
Exposes background snapshot adoption. |
tsc/internal/lsp/server.go |
Creates LSP-backed API sessions. |
tsc/internal/ipc/conn_async.go |
Waits for asynchronous handlers on shutdown. |
tsc/internal/ipc/conn_async_test.go |
Tests handler waiting behavior. |
tsc/internal/api/session.go |
Supports standalone and LSP snapshot stores. |
tsc/internal/api/session_temporary_test.go |
Uses the LSP session constructor. |
tsc/internal/api/session_createprogram_test.go |
Updates and expands program tests. |
tsc/internal/api/session_completion_test.go |
Uses the LSP session constructor. |
tsc/internal/api/session_apistate_test.go |
Adds standalone-session coverage. |
tsc/internal/api/server.go |
Removes standalone project.Session creation. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // compatibilitySnapshot is the standalone API session's canonical snapshot. | ||
| // It preserves the legacy linear updateSnapshot behavior. | ||
| compatibilitySnapshot *project.Snapshot | ||
| compatibilityMu sync.Mutex |
There was a problem hiding this comment.
This is now the standalone API’s “latest snapshot” instead of using s.projectSession.Snapshot(), and will be removed in a subsequent PR.
| defer func() { c.closePendingCalls(err) }() | ||
| defer func() { | ||
| c.closePendingCalls(err) | ||
| c.handlers.Wait() |
There was a problem hiding this comment.
Drive-by fix
There was a problem hiding this comment.
Yeah, I think this is similar to #64142
There was a problem hiding this comment.
🟡 Changes recommended
project.Session.Close currently recurses indefinitely, and one changed line also fails gofmt.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Direct host cloning drops the LSP client and can reuse content-mapped diagnostics under the wrong locale.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Snapshot ownership, reference counting, and asynchronous shutdown behavior span several tightly coupled subsystems and warrant final human review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| return nil | ||
| } | ||
|
|
||
| type queuedProtocol struct { |
There was a problem hiding this comment.
This looks somewhat like #64142, was this the same race?
| defer func() { c.closePendingCalls(err) }() | ||
| defer func() { | ||
| c.closePendingCalls(err) | ||
| c.handlers.Wait() |
There was a problem hiding this comment.
Yeah, I think this is similar to #64142
|
Oh, I didn't see #64142. It looks similar but not quite the same? This fix is for handlers that are still in-flight while closing. Closing the API session disposes its snapshots, so there's a possibility that a handler that's still running could panic if it tries to fork the snapshot it's running on, like what happens when a request discovers it needs to re-run with auto-imports. |
This is a refactor that will make #64154 cleaner. A
project.Sessionowns a canonical/latest snapshot, which makes sense as the state backing an LSP server. But that will make it awkward to use in the standalone API when we want to remove the concept of a singular latest snapshot.¹ This moves the infrastructure necessary to clone a snapshot (mostly caches) into aproject.SnapshotHost(open to naming suggestions) soapi.Sessioncan deal with that for most of its operations, while in LSP mode, the methods that can actually affect LSP state still go throughproject.Session, so they're very easy to identify.¹ This doesn't yet remove the single-latest-snapshot concept from the API, as I think that will necessarily change how things look all the way down to the client, and I wanted to take the opportunity to extract this as a scoped refactor.