From dae1b20289bd578ef48145ed3aca4d4bbd65f5c4 Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Thu, 16 Apr 2026 14:54:17 +0200 Subject: [PATCH] action_log: Fix race condition when committing changes (#53884) Sometimes the action log would not auto-accept agent edits when commiting. Gpt-5.4 identified this race condition: This fixes a race where `keep_committed_edits` could run after `head_commit` changed but before the new git base text had been applied, leaving committed agent edits marked as unreviewed; `ActionLog` now waits for an explicit `BufferDiffEvent::BaseTextChanged` instead of inferring readiness from generic `DiffChanged` activity, so it only accepts edits after the diff base itself is actually updated. - `ReloadGitState` updates `head_commit` before `ReloadBufferDiffBases` finishes loading and applying the new HEAD text. - In that gap, an unrelated `DiffChanged` can fire from a normal diff recalculation. - The old logic treated that event as the commit signal and ran `keep_committed_edits` too early. - `keep_committed_edits` then read stale diff base text, so it failed to match the committed agent edits. - When the real base-text update arrived later, the HEAD had already been overwritten (`old_head`), and the edits stayed unreviewed. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #ISSUE Release Notes: - Fixed an issue where committing agent written code would sometimes not mark edits as accepted --- Cargo.lock | 1 + crates/action_log/Cargo.toml | 1 + crates/action_log/src/action_log.rs | 135 +++++++++++++++++++++----- crates/buffer_diff/src/buffer_diff.rs | 35 +++++-- 4 files changed, 140 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 87a8eed0c016b47c36a002e5d3744885c5377bd7..14528bfd52b03de5bbf4b0acb19aca525019b800 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -77,6 +77,7 @@ dependencies = [ "ctor", "fs", "futures 0.3.32", + "git", "gpui", "language", "log", diff --git a/crates/action_log/Cargo.toml b/crates/action_log/Cargo.toml index 5227a61651012279e83a3b6e3e68b1484acb0f66..6f103c7b44fc8742a2be7285a6f027eab1531dd2 100644 --- a/crates/action_log/Cargo.toml +++ b/crates/action_log/Cargo.toml @@ -33,6 +33,7 @@ watch.workspace = true [dev-dependencies] buffer_diff = { workspace = true, features = ["test-support"] } +git.workspace = true collections = { workspace = true, features = ["test-support"] } clock = { workspace = true, features = ["test-support"] } ctor.workspace = true diff --git a/crates/action_log/src/action_log.rs b/crates/action_log/src/action_log.rs index cd17392704e1c6c932a3e4d8716b1c6f37489576..0bb4c0fcaa7ceba1952ebff9803ebbf57b852004 100644 --- a/crates/action_log/src/action_log.rs +++ b/crates/action_log/src/action_log.rs @@ -274,7 +274,6 @@ impl ActionLog { mut buffer_updates: mpsc::UnboundedReceiver<(ChangeAuthor, text::BufferSnapshot)>, cx: &mut AsyncApp, ) -> Result<()> { - let git_store = this.read_with(cx, |this, cx| this.project.read(cx).git_store().clone())?; let git_diff = this .update(cx, |this, cx| { this.project.update(cx, |project, cx| { @@ -283,28 +282,18 @@ impl ActionLog { })? .await .ok(); - let buffer_repo = git_store.read_with(cx, |git_store, cx| { - git_store.repository_and_path_for_buffer_id(buffer.read(cx).remote_id(), cx) - }); - let (mut git_diff_updates_tx, mut git_diff_updates_rx) = watch::channel(()); - let _repo_subscription = - if let Some((git_diff, (buffer_repo, _))) = git_diff.as_ref().zip(buffer_repo) { - cx.update(|cx| { - let mut old_head = buffer_repo.read(cx).head_commit.clone(); - Some(cx.subscribe(git_diff, move |_, event, cx| { - if let buffer_diff::BufferDiffEvent::DiffChanged { .. } = event { - let new_head = buffer_repo.read(cx).head_commit.clone(); - if new_head != old_head { - old_head = new_head; - git_diff_updates_tx.send(()).ok(); - } - } - })) - }) - } else { - None - }; + let _diff_subscription = if let Some(git_diff) = git_diff.as_ref() { + cx.update(|cx| { + Some(cx.subscribe(git_diff, move |_, event, _cx| { + if matches!(event, buffer_diff::BufferDiffEvent::BaseTextChanged) { + git_diff_updates_tx.send(()).ok(); + } + })) + }) + } else { + None + }; loop { futures::select_biased! { @@ -2714,6 +2703,108 @@ mod tests { assert_eq!(unreviewed_hunks(&action_log, cx), vec![]); } + /// Regression test: when head_commit updates before the BufferDiff's base + /// text does, an intermediate DiffChanged (e.g. from a buffer-edit diff + /// recalculation) must NOT consume the commit signal. The subscription + /// should only fire once the base text itself has changed. + #[gpui::test] + async fn test_keep_edits_on_commit_with_stale_diff_changed(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "file.txt": "aaa\nbbb\nccc\nddd\neee", + }), + ) + .await; + fs.set_head_for_repo( + path!("/project/.git").as_ref(), + &[("file.txt", "aaa\nbbb\nccc\nddd\neee".into())], + "0000000", + ); + cx.run_until_parked(); + + let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await; + let action_log = cx.new(|_| ActionLog::new(project.clone())); + + let file_path = project + .read_with(cx, |project, cx| { + project.find_project_path(path!("/project/file.txt"), cx) + }) + .unwrap(); + let buffer = project + .update(cx, |project, cx| project.open_buffer(file_path, cx)) + .await + .unwrap(); + + // Agent makes an edit: bbb -> BBB + cx.update(|cx| { + action_log.update(cx, |log, cx| log.buffer_read(buffer.clone(), cx)); + buffer.update(cx, |buffer, cx| { + buffer.edit([(Point::new(1, 0)..Point::new(1, 3), "BBB")], None, cx); + }); + action_log.update(cx, |log, cx| log.buffer_edited(buffer.clone(), cx)); + }); + cx.run_until_parked(); + + // Verify the edit is tracked + let hunks = unreviewed_hunks(&action_log, cx); + assert_eq!(hunks.len(), 1); + let hunk = &hunks[0].1; + assert_eq!(hunk.len(), 1); + assert_eq!(hunk[0].old_text, "bbb\n"); + + // Simulate the race condition: update only the HEAD SHA first, + // without changing the committed file contents. This is analogous + // to compute_snapshot updating head_commit before + // reload_buffer_diff_bases has loaded the new base text. + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.refs.insert("HEAD".into(), "0000001".into()); + }) + .unwrap(); + cx.run_until_parked(); + + // Make a user edit (on a different line) to trigger a buffer diff + // recalculation. This fires DiffChanged while the BufferDiff base + // text is still the OLD text. With the old head_commit-based + // subscription this would "consume" the commit detection. + cx.update(|cx| { + buffer.update(cx, |buffer, cx| { + buffer.edit([(Point::new(3, 0)..Point::new(3, 3), "DDD")], None, cx); + }); + action_log.update(cx, |log, cx| log.buffer_edited(buffer.clone(), cx)); + }); + cx.run_until_parked(); + + // Now update the committed file contents to match the buffer + // (the agent edit was committed). Keep the same SHA so head_commit + // does NOT change again — this is the second half of the race. + { + use git::repository::repo_path; + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state + .head_contents + .insert(repo_path("file.txt"), "aaa\nBBB\nccc\nDDD\neee".into()); + }) + .unwrap(); + } + cx.run_until_parked(); + + // The agent's edit (bbb -> BBB) should be accepted because the + // committed content now matches. Only the user edit (ddd -> DDD) + // should remain, but since the user edit is tracked as coming from + // the user (ChangeAuthor::User) it would have been rebased into + // the diff base already. So no unreviewed hunks should remain. + assert_eq!( + unreviewed_hunks(&action_log, cx), + vec![], + "agent edits should have been accepted after the base text update" + ); + } + #[gpui::test] async fn test_undo_last_reject(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/buffer_diff/src/buffer_diff.rs b/crates/buffer_diff/src/buffer_diff.rs index c168bd2956e0687eca5e5adeb16edbe70e9edd54..56c3fe9f51b1ea5a21f3b636ea9f8ae59e20ed3b 100644 --- a/crates/buffer_diff/src/buffer_diff.rs +++ b/crates/buffer_diff/src/buffer_diff.rs @@ -1515,11 +1515,17 @@ pub struct DiffChanged { #[derive(Clone, Debug)] pub enum BufferDiffEvent { + BaseTextChanged, DiffChanged(DiffChanged), LanguageChanged, HunksStagedOrUnstaged(Option), } +struct SetSnapshotResult { + change: DiffChanged, + base_text_changed: bool, +} + impl EventEmitter for BufferDiff {} impl BufferDiff { @@ -1784,7 +1790,7 @@ impl BufferDiff { secondary_diff_change: Option>, clear_pending_hunks: bool, cx: &mut Context, - ) -> impl Future + use<> { + ) -> impl Future + use<> { log::debug!("set snapshot with secondary {secondary_diff_change:?}"); let old_snapshot = self.snapshot(cx); @@ -1904,10 +1910,13 @@ impl BufferDiff { if let Some(parsing_idle) = parsing_idle { parsing_idle.await; } - DiffChanged { - changed_range, - base_text_changed_range, - extended_range, + SetSnapshotResult { + change: DiffChanged { + changed_range, + base_text_changed_range, + extended_range, + }, + base_text_changed, } } } @@ -1938,12 +1947,15 @@ impl BufferDiff { ); cx.spawn(async move |this, cx| { - let change = fut.await; + let result = fut.await; this.update(cx, |_, cx| { - cx.emit(BufferDiffEvent::DiffChanged(change.clone())); + if result.base_text_changed { + cx.emit(BufferDiffEvent::BaseTextChanged); + } + cx.emit(BufferDiffEvent::DiffChanged(result.change.clone())); }) .ok(); - change.changed_range + result.change.changed_range }) } @@ -2019,8 +2031,11 @@ impl BufferDiff { let fg_executor = cx.foreground_executor().clone(); let snapshot = fg_executor.block_on(fut); let fut = self.set_snapshot_with_secondary_inner(snapshot, buffer, None, false, cx); - let change = fg_executor.block_on(fut); - cx.emit(BufferDiffEvent::DiffChanged(change)); + let result = fg_executor.block_on(fut); + if result.base_text_changed { + cx.emit(BufferDiffEvent::BaseTextChanged); + } + cx.emit(BufferDiffEvent::DiffChanged(result.change)); } pub fn base_text_buffer(&self) -> &Entity {