[Juliusolsson05]

Closes #922, #923, #924 — three ordering findings from the #918 planning audit. None had been reproduced when they were filed. All three reproduce against the real path, and one of them is not a race at all.

#923 — a completion request threw away the result of its own synchronization

Two Monaco surfaces on one file are two client URIs with two drafts, and the language server sees one document. So a request from the inactive alias has to push that alias's text first — and changeSharedDocument advances version on every alias of the shared URI, including the requesting one.

getCompletions captured requestedVersion before sendDocRequest ran that restore, then compared it against the post-restore version at the end and returned an empty list.

This is not a timing window. It is deterministic whenever the other surface was last active with different text — which is the normal state of a split view. Every completion from the inactive half came back empty.

The fix mints a ticket inside the request, immediately after the restore, carrying the client revision and the server version separately — they answer different questions (what the caller asked about vs. what the server was told). The guard that mattered is unchanged and has its own test: an edit the user makes while the server is answering still invalidates the list.

#924 — a newer intent could not get past an obsolete request

sendDocRequest held both the per-client and the per-server-document queue until the server replied or the 15 s timeout fired. Bumping the intent epoch marked the in-flight response invalid but released nothing — and cancellation is advisory in LSP, so a server is entitled to ignore it. The only thing still waiting on that dead request was the user's next keystroke.

Intent bumps now notify waiters; a request races its response against that signal; an abandoned one cancels, allows 250 ms for a late answer, and lets go. The grace is deliberate — the reply usually lands within a few turns and taking it saves the next request a re-sync. It is a grace period, not a deadline, which is why it is a second number rather than a smaller version of the 15 s one.

Abandoned-but-unanswered requests are counted per server, because each one holds a response handler on the connection. Past 32 the server is retired through the same path a crash already takes, so the next open re-spawns. That turns an unbounded leak into a bounded one plus a deliberate restart.

#922 — text typed during authorization was acknowledged and dropped

lsp:open-document registers renderer ownership synchronously, before its queued entry awaits authorization and a server spawn. A change arriving in that window passed the ownership check while LspManager still had no document for the URI, hit if (!doc) return, resolved successfully, and the open then installed the original text.

The renderer was told its edit landed. The server never saw it.

Changes now go through the same IPC queue the open holds, so the ordering is the obvious one — open with the text the open carried, then didChange to what the user has since typed. And changeDocument answers whether it applied, so a change with no document rejects instead of acknowledging a write that never happened. The cost is that this IPC call now resolves only once the open ahead of it finishes, which during a cold spawn is seconds; that is the right trade against a silent loss.

Mutation battery

Eleven, each applied individually against a verified-clean tree with the file restored between runs (a batched harness reported a live mutation as dead earlier in this series). All killed:

Mutation	Result
ticket captured pre-sync	1/18 fail
abandonment race removed	2/18
intent bump notifies nobody	2/18
subscription without its initial check	1/18
change routed around the IPC queue	1/18
no server retirement	1/18
change reports success with no document	1/18
grace widened to 60 s	3/18
waiter left in the set after a request settles	1/18

The initial-check and waiter-removal mutations survived the first round and are the reason two more tests exist: the epoch can advance between the last check and the subscription existing (changeDocument bumps synchronously, before it queues), and a subscription that outlives its request would add a dead closure per language request for the life of the app.

Two fakes had to grow

ServerRecord gained a field and sendDocRequest gained a callback, so the existing stubs had to honour both. A stub that ignored the callback made the #923 fix look broken, and a fake server without the counter read NaN and retired itself on the first abandonment. Both are recorded in the test file — a stand-in that silently disagrees with the real record is how a fix gets judged wrong.

Verification

npx tsc -b clean on Node 24. src/main + src/mcp, unit + system: 1866 passed, 1 failed — workflows/control.system.test.ts, which this branch does not touch (byte-identical to origin/main) and which the #1101 reviewer independently reproduced on main.

That run also caught lspServerCreation.system.test.ts waiting setTimeout(resolve, 20) for two spawns and failing under load. Fixed in the second commit: the sleep became vi.waitFor, the assertion unchanged. Both halves of that finding are written up in #1107.

🤖 Generated with Claude Code

Review (2 Codex + 1 Pi; two rounds)

Finding	Verdict	Disposition
A1 major: a sibling's intent landing before a request subscribes (queued behind another alias, or during draft restore) still stalled edits 15 s	valid	fixed in b7a67006: a per-server-document intent count, recorded before the shared queue; both reproductions are tests
B1–B3 major: the fix-pass auto-reopen could reuse a destroyed owner's authorization, skip symlink containment, and drop mount refcounts	valid	withdrawn; ipc/lsp.ts is back to its reviewed version; recovery is #1208
A2 / Pi 1: late-answer decrement untested	valid	test in b7a67006
A3: cancel on abandonment untested	valid	test in b7a67006
B4 / Pi 2: cooldown untested	moot	the feature was withdrawn
Round 2: shared intent entries left behind by a failed first open	valid, minor	fixed in 4982cfa6 (only counted while the document exists; dispose clears)
Pi 5: no test that a tab close releases a stuck request	valid	test in 7b158eb9
Pi 3: the 250 ms grace value is unpinned	valid, minor	accepted: it changes only when queues release, not a result the user sees
Pi 4: the #922 wording overstated harm (the file editor's post-open reconcile already re-pushed the text)	valid	noted here: the main-process ack was false, but the file editor healed within the open; the fix still matters for the contract and for any caller without that reconcile

Also closes #923 and #924 (by hand after merge; the trailer names only #922).

[Juliusolsson05]

Review round one — all nine findings addressed in b65b74aa. Three were real defects I would have shipped, and finding 1 is the one that matters most: I fixed #924 for the alias that typed and left the case the PR spends its body on.

1 — the wake was keyed by the wrong thing. Correct and severe. The queue that blocks is the server-document queue; the wake was per client URI. Monaco gives two surfaces on one file two model URIs on purpose, both mapping to one server document, so a request from alias A held that queue while the user typed in alias B — B's didChange waiting the full 15 s, in exactly the split-view scenario #923 is about. My test opened two aliases and then bumped the requesting alias's epoch, so it never touched the path.

documentIntentWaiters is now keyed by serverDocumentKey, and openDocumentNow announces on the key it is about to queue on — bumpDocumentIntent cannot fan out for an opening URI because it has no record yet. Your argument for why nothing is lost is the one I put in the comment: when B's change lands, changeSharedDocument advances version on every alias, so A's answer dies at the ticket check regardless. The 15 s bought nobody anything.

2 — the abandonment branch could throw a cancellation error at Monaco. Correct, and the reasoning about provideDocumentSemanticTokens having no catch is what makes it serious rather than cosmetic. Every branch now races one settled wrapper, so a rejection arrives as a non-answer. Your diagnosis of why the suite could not see it is exact: no fake honoured the cancellation token. One now does, and rejects with -32800 on cancel.

3 — the throw is too broad for the contract it lands on. Correct. "No document" is also the ordinary state for an uninstalled server, which the registry documents as fail open. openDocument now answers whether a document exists and the IPC layer keeps a lspBackedDocuments set, so only a URI that was actually backed can lose one. I replaced the old test (which asserted the too-broad behaviour) with the two cases that matter: a document that was open and is now gone rejects; a language with no server resolves.

4 — the ticket comparison was untested. Confirmed: your mutation survives, because the "typed past" case is rejected earlier by the epoch guard inside sendDocRequest. The ticket is the only thing that rejects a list a sibling invalidated, and it now has a test that advances every alias's version mid-request.

5 — the grace timer. Correct and real. The timer is now cleared in a local finally inside the branch, so the 14.9 s case cannot strand the closure or skip the count it most deserved.

6 — serverVersion was dead. Deleted. You are right that handlePublishDiagnostics already compares against shared.version and has no ticket in scope, so the field's stated purpose was already served. A field nobody reads is a claim nobody checks.

7 — stacked JSDoc. The #923 rationale now lives on DocRequestTicket, where it is visible on hover instead of being dropped by TypeScript in favour of the block below it.

8 — the intent comment overclaimed. Corrected: only open/change/close bump. Two hovers with no edit between them still serialize, and should — nothing about the second makes the first's answer wrong.

9 — cancel could delete a set it no longer owns. Guarded, as suggested.

On the regression run, you corrected me and I had it wrong. I said control.system.test.ts "reproduces on main". It reproduces on main's source, in a worktree, and passes in the main checkout — because packages/workflow-mcp/dist/ exists in the root and not in a fresh worktree. I verified that directly. My framing gave it more standing than the evidence supported, and the real question is why a system test's outcome depends on whether a submodule happens to be built. That, plus the second failure you found (sessionManager.terminalReplay.test.ts, 5 s budget under parallel load), is now recorded on #1107.

Process note, since it bears on trusting the numbers below. My first re-measurement of these mutations was invalid: the backup directory did not exist, cp failed silently, and the mutations accumulated — the rising failure counts were the tell. I restored each one individually, verified the tree green at 22/22, and re-ran with a backup whose existence is checked and whose restore is diffed. Seven mutations, each against a verified-clean tree, all killed:

Mutation	Result
wake keyed by client URI	4/23 fail
raw promise back in the race	1/23
ticket check dropped	1/23
fail-open always throws	1/23
backed-set never populated	1/23
open does not announce its key	1/23
(the original six from round one)	re-confirmed

npx tsc -b clean on Node 24; LSP suites 27/27.