picatz / picatz/flowstate

lsp: a didClose reordered behind a didOpen deletes the reopened document, leaving the buffer absent

Open
#1,986 0 comments 0 reactions 0 assignees View on GitHub

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 didClose then didOpen for one URI and forces the close's handler to run last leaves the store holding the reopened document, and fails against the current close.
  • 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 open overtaken by a change must still not move the document backwards.

Constraints

  • close carries no version — the LSP didClose notification has only a TextDocumentIdentifier — so it cannot be ordered by the same version comparison open and change use. 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 close waking 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.