Repository navigation
fix(lsp): fence document cleanup to the owning server generation - #1078
Merged
Merged
Conversation
`discardServer` guarded the SERVER REGISTRY deletion with object identity — `if (this.servers.get(server.key) === server)` — and then deleted `docs` and `serverDocuments` by `server.key`. A server's key is derived from the language spec and the workspace root, so a replacement spawned after a crash REUSES it. The registry comparison already knew that; the two cleanup loops did not. So a late callback from the DEAD server — its `exit` event, a destroyed-stream error, a rejected `initialized` — deleted the REPLACEMENT server's documents. What that looks like from the editor: it is attached to a healthy new server, its documents vanish from both maps with diagnostics cleared, and the server still holds them open. Nothing re-opens them until the editor remounts, so completions and diagnostics simply stop for a file that is on screen and fine. Each `ServerRecord` now carries a `generation`, stamped on every document record it owns, and the two loops match on that. The dead server's OWN resources are still disposed unconditionally — its connection and its process are its own, and leaving them is a leak whoever holds the key now — and a server dying with its own documents open still clears them and tells the editor, or the file would keep stale diagnostics from a process that is gone. WHY `nextServerGeneration` IS A NAMED EXPORT rather than an inline `randomUUID()`: the whole fence collapses if a replacement can share its predecessor's value, and `createServer` cannot be driven without spawning a real language server. Without a named seam, the only thing standing between a working fence and a useless one would be that nobody replaced the call with a constant — and the suite could not tell. Mutating it to a constant now fails. Mutation-checked, 16 tests: docs fenced on the key again (the bug) 2 failed serverDocuments fenced on the key 2 failed the generation is shared by every server 1 failed no cleanup of the server's own docs 2 failed the dead server is not disposed 1 failed restored 16 passed Fixes #921 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Juliusolsson05
added a commit
that referenced
this pull request
Sep 20, 2026
#1073 and #1076 merged; #928 deliberately reopened because only its atomic/no-clobber half shipped. #1074, #1077 and #1078 are in review. Records the two reviews that caught near-catastrophes — #1077's retry guard keyed on a shape with no prompt field, which would have blocked fan-out forever with no way to clear it, and #1074's dispose destroying the registry before draining — and the two build traps (lockfile resync after a source-only submodule bump, and the neutral control-sdk type-check). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on #1078. The behavioural fix was right; the argument for it was not. FINDING 1 (high): THE PR'S CENTRAL CLAIM WAS FALSE. It said exporting `nextServerGeneration` meant "mutating it to a constant now fails a test". Only mutating the FUNCTION BODY failed. Mutating the CALL passed everything — and `generation: key`, using `createServer`'s own reusable `<root>::<spec>` composite, degenerates the fence to the pre-PR key comparison and RESTORES #921 with all 1243 main-process unit tests green. The export bought a guarantee it did not deliver. `lspServerCreation.system.test.ts` drives the REAL `createServer`, with `spawn` faked as a pair of pipes and a real vscode-jsonrpc peer on the far end answering `initialize`. It asserts the generation is not the key and that two servers for the same key differ — which is the crash-and-respawn sequence, and the thing the fence actually depends on. `generation: key` and `generation: 'fixed'` both fail now. FINDING 2 (high): NOTHING DROVE THE STAMPING SITES. Both test files poked `discardServer` with hand-built records, so writing `server.key` into `serverGeneration` at all three sites left 16/16 green — and the reason is worth recording: the live doc then carried the key while the dead record carried 'gen-1', so "the replacement's documents survive" still held. The tests agreed with themselves. The fix used the seam `lspManager.test.ts` already uses — stub `getOrCreateServer`, drive the real `openDocument` — which the reviewer demonstrated before I did. The PR's excuse ("createServer cannot be driven without spawning a real language server") was true of `createServer` and false of what needed covering, and is now false of both. FINDING 3 (medium): branch B of `openDocument` creates a record after TWO awaits with no `server.closed` re-check, unlike branch A. The fence deliberately removes the only mechanism that ever reclaimed a cross-generation record, so a record stamped with an already-discarded generation would be adopted into every later generation without re-stamping and never collected. Neither the reviewer nor I could construct a production sequence that reaches it — `connection.dispose` rejects pending responses and that path is guarded — but an invariant this load-bearing should not rest on that reasoning being exhaustive. Guarded, commented, and pinned. FINDING 4 (medium): `initialized.then(() => this.servers.get(key))` sends the notification to whoever holds the key NOW, while `record` is in scope one line below. Fixed and pinned. FINDING 5 (low): `lspManager.test.ts`'s hand-built shapes lacked `serverGeneration` entirely, so two `undefined` generations would compare equal and the fence would silently match everything if one of those tests ever reached `discardServer`. Brought along. FINDING 6 (low): the "late callback" enumeration overstated two of its three examples. A FIRST discard is never late — it is what lets a replacement exist. The comment now says which callback actually arrives that way, and why. Mutation-checked, 22 tests: generation derived from the key (the bug) 2 failed generation is a constant 1 failed stamping writes the key (all sites) 1 failed stamping writes the key (shared record) 1 failed branch B skips the closed check 1 failed initialized goes to whoever holds the key 1 failed docs fenced on the key again 2 failed restored 22 passed Refs #921 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Juliusolsson05
added a commit
that referenced
this pull request
Sep 25, 2026
…20 ms The #1078 review test slept 20 ms and asserted both servers had been sent 'initialized'. The notification follows each fake server's initialize reply asynchronously, and on a loaded CI runner under coverage 20 ms was not enough (this PR's quality gate received []). It now waits for the two notifications and still asserts which server each one reached, so the bug it guards (a notification landing on whoever holds the key now) still fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #921.
The bug
discardServerguarded the server registry deletion with object identity:…and then deleted
docsandserverDocumentsbyserver.key.A server's key is derived from the language spec and the workspace root, so a replacement spawned after a crash reuses it. The registry comparison already knew that; the two cleanup loops did not. So a late callback from the dead server — its
exitevent, a destroyed-stream error, a rejectedinitialized— deleted the replacement server's documents.From the editor: it is attached to a healthy new server, its documents vanish from both maps with diagnostics cleared, and the server still holds them open. Nothing re-opens them until the editor remounts, so completions and diagnostics simply stop for a file that is on screen and fine.
The fix
Each
ServerRecordcarries ageneration, stamped on every document record it owns, and the two loops match on that instead of the reusable key.What deliberately did not change:
Why
nextServerGenerationis a named exportThe whole fence collapses if a replacement can share its predecessor's value, and
createServercannot be driven without spawning a real language server. Without a named seam, the only thing standing between a working fence and a useless one would be that nobody replacedrandomUUID()with a constant — and the suite could not tell. Mutating it to a constant now fails a test.Tests
Driven against the real
discardServerwith two real record shapes, because the bug is precisely about telling two records with the same key apart.serverDocumentsfenced on the keynpx tsc -bclean. Widersrc/main/run: 1648 passed, with one failure —src/main/workflows/control.system.test.ts— that fails identically on cleanorigin/main.Scope
This is the first of four LSP issues filed together from the same planning pass. #922 (ordered document admission), #923 (post-synchronization revision tickets) and #924 (releasing stale read waits) are untouched here; they are ordering problems rather than identity ones, and they belong with each other rather than with this.
🤖 Generated with Claude Code