Fix crash in apply text buffer operations found when opening a channel notes (#51978)

Max Brunsfeld and Ben Kunkle created 3 weeks ago

To do

* [x] turn the operations from collab into a failing test
* [x] fix the crash
* [ ] turn the huge set of operations into a succinct test that can be
checked in on main

Release Notes:

- N/A

---------

Co-authored-by: Ben Kunkle <[e-mail]>

Change summary

crates/rope/src/rope.rs  | 12 ++++++++++++
crates/text/src/tests.rs | 42 ++++++++++++++++++++++++++++++++++++++++++
crates/text/src/text.rs  | 11 +++++++----
3 files changed, 61 insertions(+), 4 deletions(-)

Detailed changes

crates/rope/src/rope.rs

@@ -699,6 +699,10 @@ impl<'a> Cursor<'a> {
             self.offset,
             end_offset
         );
+        assert!(
+            end_offset <= self.rope.len(),
+            "cannot summarize past end of rope"
+        );
 
         self.chunks.seek_forward(&end_offset, Bias::Right);
         self.offset = end_offset;
@@ -711,6 +715,10 @@ impl<'a> Cursor<'a> {
             self.offset,
             end_offset
         );
+        assert!(
+            end_offset <= self.rope.len(),
+            "cannot summarize past end of rope"
+        );
 
         let mut slice = Rope::new();
         if let Some(start_chunk) = self.chunks.item() {
@@ -741,6 +749,10 @@ impl<'a> Cursor<'a> {
             self.offset,
             end_offset
         );
+        assert!(
+            end_offset <= self.rope.len(),
+            "cannot summarize past end of rope"
+        );
 
         let mut summary = D::zero(());
         if let Some(start_chunk) = self.chunks.item() {

crates/text/src/tests.rs

@@ -749,6 +749,48 @@ fn test_concurrent_edits() {
     assert_eq!(buffer3.text(), "a12c34e56");
 }
 
+// Regression test: applying a remote edit whose FullOffset range partially
+// overlaps a fragment that was already deleted (observed but not visible)
+// used to leave the fragment unsplit, causing the rope builder to read past
+// the end of the rope.
+#[test]
+fn test_edit_partially_intersecting_a_deleted_fragment() {
+    let mut buffer = Buffer::new(ReplicaId::new(1), BufferId::new(1).unwrap(), "abcdefgh");
+
+    // Delete "cde", creating a single deleted fragment at FullOffset 2..5.
+    // After this the fragment layout is:
+    //   "ab"(vis, FullOffset 0..2)  "cde"(del, 2..5)  "fgh"(vis, 5..8)
+    buffer.edit([(2..5, "")]);
+    assert_eq!(buffer.text(), "abfgh");
+
+    // Construct a synthetic remote edit whose version includes the deletion (so
+    // the "cde" fragment is observed + deleted → !was_visible) but whose
+    // FullOffset range only partially overlaps it. This state arises in
+    // production when concurrent edits cause different fragment splits on
+    // different replicas.
+    let synthetic_timestamp = clock::Lamport {
+        replica_id: ReplicaId::new(2),
+        value: 10,
+    };
+    let synthetic_edit = Operation::Edit(EditOperation {
+        timestamp: synthetic_timestamp,
+        version: buffer.version(),
+        // Range 1..4 partially overlaps the deleted "cde" (FullOffset 2..5):
+        // it covers "b" (1..2) and only "cd" (2..4), leaving "e" (4..5) out.
+        ranges: vec![FullOffset(1)..FullOffset(4)],
+        new_text: vec!["".into()],
+    });
+
+    // Without the fix this panics with "cannot summarize past end of rope"
+    // because the full 3-byte "cde" fragment is consumed from the deleted
+    // rope instead of only the 2-byte intersection.
+    buffer.apply_ops([synthetic_edit]);
+    assert_eq!(buffer.text(), "afgh");
+
+    buffer.undo_operations([(synthetic_timestamp, u32::MAX)].into_iter().collect());
+    assert_eq!(buffer.text(), "abfgh");
+}
+
 #[gpui::test(iterations = 100)]
 fn test_random_concurrent_edits(mut rng: StdRng) {
     let peers = env::var("PEERS")

crates/text/src/text.rs

@@ -1234,15 +1234,18 @@ impl Buffer {
                 let fragment_end = old_fragments.end().0.full_offset();
                 let mut intersection = fragment.clone();
                 let intersection_end = cmp::min(range.end, fragment_end);
-                if fragment.was_visible(version, &self.undo_map) {
+                if version.observed(fragment.timestamp) {
                     intersection.len = (intersection_end.0 - fragment_start.0) as u32;
                     intersection.insertion_offset +=
                         (fragment_start - old_fragments.start().0.full_offset()) as u32;
                     intersection.id =
                         Locator::between(&new_fragments.summary().max_id, &intersection.id);
-                    intersection.deletions.push(timestamp);
-                    intersection.visible = false;
-                    insertion_slices.push(InsertionSlice::from_fragment(timestamp, &intersection));
+                    if fragment.was_visible(version, &self.undo_map) {
+                        intersection.deletions.push(timestamp);
+                        intersection.visible = false;
+                        insertion_slices
+                            .push(InsertionSlice::from_fragment(timestamp, &intersection));
+                    }
                 }
                 if intersection.len > 0 {
                     if fragment.visible && !intersection.visible {