Add batching to remote editor sync#10772
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR batches remote editor buffer edits behind a debounce timer and flushes pending edits before remote saves.
Concerns
- Pending edits are removed from the batch and sent with a best-effort notification before save; if the notification cannot be enqueued, the following SaveBuffer request can still persist stale server content.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // Flush any pending edit batch so the server has the latest | ||
| // content before persisting to disk. | ||
| if let Some(batch) = pending_batch.take() { | ||
| batch.flush(&client, &path); |
There was a problem hiding this comment.
send_buffer_edit is best-effort (try_send), so this can drop the only pending batch and still issue SaveBuffer, letting save succeed with stale server content; make the flush fallible/awaited before taking the batch or sending the save request.
There was a problem hiding this comment.
i think this is reasonable
| if let Some(timer) = &self.debounce_timer { | ||
| timer.abort(); | ||
| } | ||
| log::debug!( |
There was a problem hiding this comment.
nit nit: should this log::debug be inside the if so it only logs if we actually aborted
| // Flush any pending edit batch so the server has the latest | ||
| // content before persisting to disk. | ||
| if let Some(batch) = pending_batch.take() { | ||
| batch.flush(&client, &path); |
There was a problem hiding this comment.
i think this is reasonable
2 participants
Batch Client Buffer Edits with Debouncing
Problem
Every user keystroke in a remote file editor immediately sends a
BufferEditproto message to the remote server daemon. Rapid typing floods the SSH transport with many small messages, each with fixed overhead (proto encoding, length prefix, SSH framing).Solution
Add a
PendingEditBatchtoBufferSource::Remotethat accumulates edits during a 200ms debounce window, then sends them as a singleBufferEditmessage. Key design points:sync_clock.client_versionis bumped immediately on each keystroke (not deferred), so conflict detection works correctly even before the batch is flushedSaveBufferso the server has the latest contentChanges
PendingEditBatchstruct withflush(self, client, path)anddiscard(self)methodspush_edit_to_pending_batchonGlobalBufferModelhandles accumulation + CV bump + timer cancellationTimer::after+abort_handlepattern (same asLanguageServerShutdownManager)handle_buffer_conflict_detected,handle_buffer_updated_pushconflict branch,apply_open_buffer_response)GlobalBufferModel::savefor remote buffers)global_buffer_model_tests.rscovering discard-on-push, discard-on-conflict, clean-push, and immediate CV bumpVersion Vector Correctness
The batched
BufferEditcarries a singleS_expected(captured when the first edit in the batch arrives) and the finalC_new. The server only cares about the latest accepted C, so skipping intermediate values is correct. Server-sideapply_client_editalready acceptsrepeated TextEdit, so no server changes needed.Oz conversation | Plan