acp: Fix panic with edit file tool (#36732)

Cole Miller and Conrad created 2 months ago

We had a frequent panic when the agent was using our edit file tool. The
root cause was that we were constructing a `BufferDiff` with
`BufferDiff::new`, then calling `set_base_text`, but not waiting for
that asynchronous operation to finish. This means there was a window of
time where the diff's base text was set to the initial value of
`""`--that's not a problem in itself, but it was possible for us to call
`PendingDiff::update` during that window, which calls
`BufferDiff::update_diff`, which calls
`BufferDiffSnapshot::new_with_base_buffer`, which takes two arguments
`base_text` and `base_text_snapshot` that are supposed to represent the
same text. We were getting the first of those arguments from the
`base_text` field of `PendingDiff`, which is set immediately to the
target base text without waiting for `BufferDiff::set_base_text` to run
to completion; and the second from the `BufferDiff` itself, which still
has the empty base text during that window.

As a result of that mismatch, we could end up adding `DeletedHunk` diff
transforms to the multibuffer for the diff card even though the
multibuffer's base text was empty, ultimately leading to a panic very
far away in rendering code.

I've fixed this by adding a new `BufferDiff` constructor for the case
where the buffer contents and the base text are (initially) the same,
like for the diff cards, and so we don't need an async diff calculation.
I also added a debug assertion to catch the basic issue here earlier,
when `BufferDiffSnapshot::new_with_base_buffer` is called with two base
texts that don't match.

Release Notes:

- N/A

---------

Co-authored-by: Conrad <[e-mail]>

Change summary

crates/acp_thread/src/diff.rs         | 40 ++++++++++++++++++----------
crates/buffer_diff/src/buffer_diff.rs | 33 +++++++++++++++++++++++
2 files changed, 57 insertions(+), 16 deletions(-)

Detailed changes

crates/acp_thread/src/diff.rs

@@ -85,27 +85,19 @@ impl Diff {
     }
 
     pub fn new(buffer: Entity<Buffer>, cx: &mut Context<Self>) -> Self {
-        let buffer_snapshot = buffer.read(cx).snapshot();
-        let base_text = buffer_snapshot.text();
-        let language_registry = buffer.read(cx).language_registry();
-        let text_snapshot = buffer.read(cx).text_snapshot();
+        let buffer_text_snapshot = buffer.read(cx).text_snapshot();
+        let base_text_snapshot = buffer.read(cx).snapshot();
+        let base_text = base_text_snapshot.text();
+        debug_assert_eq!(buffer_text_snapshot.text(), base_text);
         let buffer_diff = cx.new(|cx| {
-            let mut diff = BufferDiff::new(&text_snapshot, cx);
-            let _ = diff.set_base_text(
-                buffer_snapshot.clone(),
-                language_registry,
-                text_snapshot,
-                cx,
-            );
+            let mut diff = BufferDiff::new_unchanged(&buffer_text_snapshot, base_text_snapshot);
             let snapshot = diff.snapshot(cx);
-
             let secondary_diff = cx.new(|cx| {
-                let mut diff = BufferDiff::new(&buffer_snapshot, cx);
-                diff.set_snapshot(snapshot, &buffer_snapshot, cx);
+                let mut diff = BufferDiff::new(&buffer_text_snapshot, cx);
+                diff.set_snapshot(snapshot, &buffer_text_snapshot, cx);
                 diff
             });
             diff.set_secondary_diff(secondary_diff);
-
             diff
         });
 
@@ -412,3 +404,21 @@ async fn build_buffer_diff(
         diff
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use gpui::{AppContext as _, TestAppContext};
+    use language::Buffer;
+
+    use crate::Diff;
+
+    #[gpui::test]
+    async fn test_pending_diff(cx: &mut TestAppContext) {
+        let buffer = cx.new(|cx| Buffer::local("hello!", cx));
+        let _diff = cx.new(|cx| Diff::new(buffer.clone(), cx));
+        buffer.update(cx, |buffer, cx| {
+            buffer.set_text("HELLO!", cx);
+        });
+        cx.run_until_parked();
+    }
+}

crates/buffer_diff/src/buffer_diff.rs

@@ -162,6 +162,22 @@ impl BufferDiffSnapshot {
         }
     }
 
+    fn unchanged(
+        buffer: &text::BufferSnapshot,
+        base_text: language::BufferSnapshot,
+    ) -> BufferDiffSnapshot {
+        debug_assert_eq!(buffer.text(), base_text.text());
+        BufferDiffSnapshot {
+            inner: BufferDiffInner {
+                base_text,
+                hunks: SumTree::new(buffer),
+                pending_hunks: SumTree::new(buffer),
+                base_text_exists: false,
+            },
+            secondary_diff: None,
+        }
+    }
+
     fn new_with_base_text(
         buffer: text::BufferSnapshot,
         base_text: Option<Arc<String>>,
@@ -213,7 +229,10 @@ impl BufferDiffSnapshot {
         cx: &App,
     ) -> impl Future<Output = Self> + use<> {
         let base_text_exists = base_text.is_some();
-        let base_text_pair = base_text.map(|text| (text, base_text_snapshot.as_rope().clone()));
+        let base_text_pair = base_text.map(|text| {
+            debug_assert_eq!(&*text, &base_text_snapshot.text());
+            (text, base_text_snapshot.as_rope().clone())
+        });
         cx.background_executor()
             .spawn_labeled(*CALCULATE_DIFF_TASK, async move {
                 Self {
@@ -873,6 +892,18 @@ impl BufferDiff {
         }
     }
 
+    pub fn new_unchanged(
+        buffer: &text::BufferSnapshot,
+        base_text: language::BufferSnapshot,
+    ) -> Self {
+        debug_assert_eq!(buffer.text(), base_text.text());
+        BufferDiff {
+            buffer_id: buffer.remote_id(),
+            inner: BufferDiffSnapshot::unchanged(buffer, base_text).inner,
+            secondary_diff: None,
+        }
+    }
+
     #[cfg(any(test, feature = "test-support"))]
     pub fn new_with_base_text(
         base_text: &str,