lsp: a didClose reordered behind a didOpen deletes the reopened document, leaving the buffer absent
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Found while reviewing #1983 and confirmed to predate it. Filed separately because the fix is a different mechanism from that PR's version guard, and because it needs a wire-level test rather than a store-level one.
Observed behavior
documentStore.close deletes unconditionally (pkg/flowstate/v1/flowfile/lsp/store.go), and the connection wraps handlers in jsonrpc2.AsyncHandler, which starts a goroutine per message. didClose is not on NewHandler's ordered announcement path, so a client that closes a document and immediately reopens it can have the two handlers run in the opposite order: open first, close second.
The close then deletes the document the reopen just established. The client believes the buffer is open; the server holds nothing. A subsequent range edit finds no document, and a subsequent full-text change starts from empty.
Evidence
Driving the store directly, with store.go from origin/main and from #1983's branch:
store.go |
reopen@1 over incumbent@1 | after the late close |
|---|---|---|
origin/main |
stores the new text | absent |
| #1983 | keeps the previous text | absent |
The end state is the same either way, which is what establishes this as pre-existing rather than introduced by that PR's guard. The guard changes only the window between the reopen and the late close — a reader in it sees the previous text instead of the new text — and both paths end with the document gone.
Desired outcome
A close and a reopen of one URI take effect in the order the client sent them, so a reopened buffer is present afterwards. The store's contents are a function of the notifications the client sent rather than of goroutine scheduling — the same property #1965 established for open against change, extended to close.
Acceptance criteria
- A test that sends
didClosethendidOpenfor one URI and forces the close's handler to run last leaves the store holding the reopened document, and fails against the currentclose. - A close with no reopen still removes the document and still wakes waiters, so nothing is left holding a channel that never closes.
- The ordering fix does not reintroduce the reversion #1965 fixed: an
openovertaken by achangemust still not move the document backwards.
Constraints
closecarries no version — the LSPdidClosenotification has only aTextDocumentIdentifier— so it cannot be ordered by the same version comparisonopenandchangeuse. That is why this needs its own mechanism: a per-URI generation or tombstone, or serializing the two notifications for one URI on the read loop.handler.go's doc comment already places arrival-order application on the read loop outside that package, so a per-URI serialization may belong there rather than in the store.- Whatever is chosen has to keep
closewaking waiters, since it is the one mutation that can take a document away.
Open question
Is a tombstone (remember that a close was seen for this URI at this point, and let a later open supersede it) simpler than serializing, given close has no version to order by? A tombstone needs its own eviction rule, which is a second thing to get right.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with pkg/flowstate/v1/flowfile/lsp/store.go and handler.go, then trace how didOpen, didChange, and didClose are dispatched through jsonrpc2.AsyncHandler. Build the wire-level ordering test described in the acceptance criteria, including a close without reopen, and verify that a reopened document remains present while the existing open-versus-change ordering guarantee still holds.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- devtools
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100