Repository navigation
fix(client): keep session moves made during an in-flight read - #57184
WilliamAGH wants to merge 1 commit into
Conversation
Agent-Session: a435ec61-6096-4d48-82ea-d70df6783e62
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes #57139. On the base, the bot's reproduction fails all 3 cases (a read already in flight undoes the move for a single session and for a family read, and workspaceID survives a move). With this change all 3 pass. The packages/client tests (238 pass) and typecheck are clean.
Public API: unchanged. The changes are internal to the createData store, and no exported signatures or events change.
The approach looks sound: moves are collected only while a read is running, even for sessions that aren't cached yet, and applied over the response in the same synchronous batch. A second failure still rejects, and reconcile correctly drops old location keys.
A few things worth addressing:
- The retry also fires on moves of unrelated sessions during a family read (see the inline comment).
- The new tests add about 210 lines for 9 cases. Most of that is copied setup, plus fetch stubs that branch on
children,moved,cachedand request counts, and the concurrent-snapshot test needs a stub that throws on a third request. That makes it hard to see what each case checks. Please cut them down to the core cases: a single read and a family read held open across a move, theworkspaceIDandsubpathcleanup, and one retry case, using a small shared helper for the data/bus/held-fetch setup. - #56886 changes the same
session.syncfunction to stop stale reads restoring deleted sessions. The two will conflict, and the 'events that arrive during a pending read' tracking may be better shared than added twice for different events.
Related: #56736
| return | ||
| case "session.moved": { | ||
| for (const read of sessionReads) { | ||
| if (read.sessionID !== event.data.sessionID && !read.children) continue |
There was a problem hiding this comment.
For a family read, read.revision goes up for a session.moved on any session, related or not, so a real failure gets retried whenever some unrelated session moves during the read. Collecting every move makes sense, since children aren't known until the list returns. But the retry check could look only at moves for read.sessionID, or at moves for sessions this read actually asked about, rather than any movement at all.
Issue for this PR
Closes #57139
Type of change
What does this PR do?
session.syncwrote whatever the server returned, even if asession.movedevent came in while the request was still out. So a stale read could undo a move, and the TUI would go back to the old (sometimes deleted) directory and show the location prompt again.Now each running sync collects the moves that arrive while it's waiting: for its own session, or for any session in a family read, since the children aren't known until the list comes back. When the read finishes, those moves are applied on top of the response before it goes into the store. If the read fails and a move arrived while it was running, it retries once, because the failure was probably about the old directory. A second failure throws like before, so it can't loop.
I also changed the
session.movedhandler toreconcilethe location instead of merging it, so aworkspaceIDfrom the old location doesn't survive the move.How did you verify your code works?
I added tests to
packages/client/test/solid-data.test.tsthat hold the read open, emitsession.moved, then let the read finish:workspaceIDorsubpathAll 9 fail on
devand pass with this change.bun test test/solid-data.test.ts(39 pass) andbun typecheckare clean inpackages/client. I also ran thepackages/tuisuite ondevand on this branch. The failures were timeouts on both, from a heavily loaded machine, and the one move-related TUI test failed the same way on both.Screenshots / recordings
No UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #56736