core: cache turn diff rendering (#27489)

## Summary

Turn diff updates repeatedly rendered and serialized the entire
accumulated diff after every `apply_patch`. The event path also rendered
once before updating the tracker solely to test whether a diff existed.
In production feedback CODEX-20PW, 2,589 patches across 72 paths
produced 401 notifications totaling 441 MB, with the hottest paths
patched 518 and 495 times.

This change:

- replaces the pre-update render with a cheap cached-state check
- caches each rendered file diff by path and content revision, so an
update only invokes Myers for affected paths
- caches the deterministic aggregate diff so event emission and turn
completion reuse it without recomputation
- preserves invalidation and net-zero clear notifications
- applies a 100 ms per-file `similar` timeout; ordinary files complete
far below this threshold, while pathological rewrites fall back to a
coarse unified hunk that still represents the exact final contents

The 100 ms deadline bounds synchronous tool-completion latency while
leaving substantial headroom for normal diffs. The regression test
applies the fallback diff through the repository's patch parser and
verifies byte-for-byte final contents.

## Validation

- `cargo test -p codex-core turn_diff_tracker::tests` (14 passed)
- `cargo test -p codex-core tools::events::tests` (4 passed)
- `just fix -p codex-core`
- `just fmt`

Focused coverage verifies that 42 updates across two files perform 42
file renders rather than repeatedly rendering the accumulated set,
unchanged paths are not re-diffed, clear events remain correct, and a
48,000-line near-total rewrite returns promptly and applies to the exact
expected result. The full `codex-core` suite was not used as the final
gate because an unrelated existing multi-agent test hit a stack overflow
when run during investigation.

## Bug context

- Sentry feedback: CODEX-20PW
- Correlation IDs: `019eb2a9-13d2-74e0-b690-27ee224ffb6d`,
`019e9ad7-09c3-7cb2-b728-ee3acba103ab`
This commit is contained in:
Jeremy Rose
2026-06-10 17:17:44 -07:00
committed by GitHub
Unverified
parent ac9c534c21
commit b389b950e1
3 changed files with 330 additions and 51 deletions
+96 -2
View File
@@ -588,7 +588,7 @@ async fn emit_patch_end(
if let Some(tracker) = ctx.turn_diff_tracker {
let (should_emit_turn_diff, unified_diff) = {
let mut guard = tracker.lock().await;
let previous_diff = guard.get_unified_diff();
let had_unified_diff = guard.has_unified_diff();
let tracker_changed = match tracker_update {
TurnDiffTrackerUpdate::Track {
environment_id,
@@ -605,7 +605,7 @@ async fn emit_patch_end(
};
let unified_diff = guard.get_unified_diff();
(
tracker_changed && (previous_diff.is_some() || unified_diff.is_some()),
tracker_changed && (had_unified_diff || unified_diff.is_some()),
unified_diff.unwrap_or_default(),
)
};
@@ -718,4 +718,98 @@ mod tests {
)
.await;
}
#[tokio::test]
async fn net_zero_patch_emits_empty_turn_diff() {
let (session, turn, rx_event) =
make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await;
let tracker = Arc::new(Mutex::new(TurnDiffTracker::new()));
let dir = tempdir().expect("tempdir");
let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).expect("absolute cwd");
for patch in [
"*** Begin Patch\n*** Add File: a.txt\n+one\n*** End Patch",
"*** Begin Patch\n*** Delete File: a.txt\n*** End Patch",
] {
let mut stdout = Vec::new();
let mut stderr = Vec::new();
let delta = codex_apply_patch::apply_patch(
patch,
&cwd,
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await
.expect("apply patch");
emit_patch_end(
ToolEventCtx::new(session.as_ref(), turn.as_ref(), "call-id", Some(&tracker)),
HashMap::new(),
String::new(),
String::new(),
PatchApplyStatus::Completed,
TurnDiffTrackerUpdate::Track {
environment_id: None,
delta: &delta,
},
)
.await;
rx_event.recv().await.expect("item completed event");
let unified_diff = loop {
let event = rx_event.recv().await.expect("turn diff event");
if let EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) = event.msg {
break unified_diff;
}
};
if patch.contains("Delete File") {
assert_eq!(unified_diff, "");
} else {
assert!(unified_diff.contains("+one"));
}
}
}
#[tokio::test]
async fn invalidation_emits_empty_turn_diff() {
let (session, turn, rx_event) =
make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await;
let tracker = Arc::new(Mutex::new(TurnDiffTracker::new()));
let dir = tempdir().expect("tempdir");
let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).expect("absolute cwd");
let mut stdout = Vec::new();
let mut stderr = Vec::new();
let delta = codex_apply_patch::apply_patch(
"*** Begin Patch\n*** Add File: a.txt\n+one\n*** End Patch",
&cwd,
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await
.expect("apply patch");
tracker.lock().await.track_delta("", &delta);
emit_patch_end(
ToolEventCtx::new(session.as_ref(), turn.as_ref(), "call-id", Some(&tracker)),
HashMap::new(),
String::new(),
String::new(),
PatchApplyStatus::Completed,
TurnDiffTrackerUpdate::Invalidate,
)
.await;
rx_event.recv().await.expect("item completed event");
loop {
let event = rx_event.recv().await.expect("turn diff event");
if let EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) = event.msg {
assert_eq!(unified_diff, "");
break;
}
}
}
}
+110 -49
View File
@@ -2,6 +2,7 @@ use std::collections::HashMap;
use std::collections::HashSet;
use std::path::Path;
use std::path::PathBuf;
use std::time::Duration;
use sha1::digest::Output;
@@ -12,6 +13,14 @@ use codex_apply_patch::AppliedPatchFileChange;
const ZERO_OID: &str = "0000000000000000000000000000000000000000";
const DEV_NULL: &str = "/dev/null";
const REGULAR_FILE_MODE: &str = "100644";
// Normal edits finish well within 100 ms; pathological inputs fall back to a coarse,
// content-exact diff without stalling tool completion.
const DIFF_TIMEOUT: Duration = Duration::from_millis(100);
struct TrackedContent {
content: String,
revision: u64,
}
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
struct TrackedPath {
@@ -28,14 +37,27 @@ impl TrackedPath {
}
}
#[derive(Eq, Hash, PartialEq)]
struct DiffCacheKey {
left_path: TrackedPath,
left_revision: Option<u64>,
right_path: TrackedPath,
right_revision: Option<u64>,
}
/// Tracks the net text diff for the current turn from committed apply_patch
/// mutations, without rereading the workspace filesystem.
pub struct TurnDiffTracker {
valid: bool,
display_roots_by_environment: HashMap<String, PathBuf>,
baseline_by_path: HashMap<TrackedPath, String>,
current_by_path: HashMap<TrackedPath, String>,
baseline_by_path: HashMap<TrackedPath, TrackedContent>,
current_by_path: HashMap<TrackedPath, TrackedContent>,
origin_by_current_path: HashMap<TrackedPath, TrackedPath>,
next_revision: u64,
rendered_diffs: HashMap<DiffCacheKey, Option<String>>,
unified_diff: Option<String>,
#[cfg(test)]
rendered_diff_count: std::cell::Cell<usize>,
}
impl Default for TurnDiffTracker {
@@ -46,6 +68,11 @@ impl Default for TurnDiffTracker {
baseline_by_path: HashMap::new(),
current_by_path: HashMap::new(),
origin_by_current_path: HashMap::new(),
next_revision: 0,
rendered_diffs: HashMap::new(),
unified_diff: None,
#[cfg(test)]
rendered_diff_count: std::cell::Cell::new(0),
}
}
}
@@ -64,6 +91,10 @@ impl TurnDiffTracker {
}
pub fn track_delta(&mut self, environment_id: &str, delta: &AppliedPatchDelta) {
if !self.valid {
return;
}
if !delta.is_exact() {
self.invalidate();
return;
@@ -72,17 +103,24 @@ impl TurnDiffTracker {
for change in delta.changes() {
self.apply_change(environment_id, change);
}
self.refresh_unified_diff();
}
pub fn invalidate(&mut self) {
self.valid = false;
self.rendered_diffs.clear();
self.unified_diff = None;
}
pub fn get_unified_diff(&self) -> Option<String> {
if !self.valid {
return None;
}
self.unified_diff.clone()
}
pub(crate) fn has_unified_diff(&self) -> bool {
self.unified_diff.is_some()
}
fn refresh_unified_diff(&mut self) {
let rename_pairs = self.rename_pairs();
let paired_destinations = rename_pairs.values().cloned().collect::<HashSet<_>>();
let mut handled = HashSet::new();
@@ -95,6 +133,8 @@ impl TurnDiffTracker {
paths.sort_by_key(|path| self.display_path(path));
paths.dedup();
let mut previous_diffs = std::mem::take(&mut self.rendered_diffs);
let mut rendered_diffs = HashMap::new();
let mut aggregated = String::new();
for path in paths {
if !handled.insert(path.clone()) {
@@ -105,22 +145,41 @@ impl TurnDiffTracker {
continue;
}
let diff = if let Some(dest) = rename_pairs.get(&path) {
let (left_path, right_path) = if let Some(dest) = rename_pairs.get(&path) {
handled.insert(dest.clone());
self.render_rename_diff(&path, dest)
(&path, dest)
} else {
self.render_path_diff(&path)
(&path, &path)
};
if let Some(diff) = diff {
aggregated.push_str(&diff);
let left_content = self.baseline_by_path.get(left_path);
let right_content = self.current_by_path.get(right_path);
let key = DiffCacheKey {
left_path: left_path.clone(),
left_revision: left_content.map(|content| content.revision),
right_path: right_path.clone(),
right_revision: right_content.map(|content| content.revision),
};
let rendered = previous_diffs.remove(&key).unwrap_or_else(|| {
self.render_diff(
left_path,
left_content.map(|content| content.content.as_str()),
right_path,
right_content.map(|content| content.content.as_str()),
)
});
if let Some(diff) = rendered.as_deref() {
aggregated.push_str(diff);
if !aggregated.ends_with('\n') {
aggregated.push('\n');
}
}
rendered_diffs.insert(key, rendered);
}
(!aggregated.is_empty()).then_some(aggregated)
self.rendered_diffs = rendered_diffs;
self.unified_diff = (!aggregated.is_empty()).then_some(aggregated);
}
fn apply_change(&mut self, environment_id: &str, change: &AppliedPatchChange) {
@@ -157,18 +216,20 @@ impl TurnDiffTracker {
&& !self.baseline_by_path.contains_key(&path)
&& let Some(overwritten_content) = overwritten_content
{
let overwritten_content = self.tracked_content(overwritten_content);
self.baseline_by_path
.insert(path.clone(), overwritten_content.to_string());
.insert(path.clone(), overwritten_content);
}
self.current_by_path.insert(path, content.to_string());
let content = self.tracked_content(content);
self.current_by_path.insert(path, content);
}
fn apply_delete(&mut self, path: TrackedPath, content: &str) {
if self.current_by_path.remove(&path).is_none()
&& !self.baseline_by_path.contains_key(&path)
{
self.baseline_by_path
.insert(path.clone(), content.to_string());
let content = self.tracked_content(content);
self.baseline_by_path.insert(path.clone(), content);
}
self.origin_by_current_path.remove(&path);
}
@@ -184,8 +245,9 @@ impl TurnDiffTracker {
if !self.current_by_path.contains_key(&source_path)
&& !self.baseline_by_path.contains_key(&source_path)
{
let old_content = self.tracked_content(old_content);
self.baseline_by_path
.insert(source_path.clone(), old_content.to_string());
.insert(source_path.clone(), old_content);
}
match move_path {
@@ -194,28 +256,38 @@ impl TurnDiffTracker {
&& !self.baseline_by_path.contains_key(&dest_path)
&& let Some(overwritten_move_content) = overwritten_move_content
{
let overwritten_move_content = self.tracked_content(overwritten_move_content);
self.baseline_by_path
.insert(dest_path.clone(), overwritten_move_content.to_string());
.insert(dest_path.clone(), overwritten_move_content);
}
let origin = self
.origin_by_current_path
.remove(&source_path)
.unwrap_or_else(|| source_path.clone());
self.current_by_path.remove(&source_path);
self.current_by_path
.insert(dest_path.clone(), new_content.to_string());
let new_content = self.tracked_content(new_content);
self.current_by_path.insert(dest_path.clone(), new_content);
self.origin_by_current_path.remove(&dest_path);
if dest_path != origin {
self.origin_by_current_path.insert(dest_path, origin);
}
}
None => {
self.current_by_path
.insert(source_path, new_content.to_string());
let new_content = self.tracked_content(new_content);
self.current_by_path.insert(source_path, new_content);
}
}
}
fn tracked_content(&mut self, content: &str) -> TrackedContent {
let revision = self.next_revision;
self.next_revision += 1;
TrackedContent {
content: content.to_string(),
revision,
}
}
fn rename_pairs(&self) -> HashMap<TrackedPath, TrackedPath> {
self.origin_by_current_path
.iter()
@@ -234,28 +306,6 @@ impl TurnDiffTracker {
.collect()
}
fn render_path_diff(&self, path: &TrackedPath) -> Option<String> {
self.render_diff(
path,
self.baseline_by_path.get(path).map(String::as_str),
path,
self.current_by_path.get(path).map(String::as_str),
)
}
fn render_rename_diff(
&self,
source_path: &TrackedPath,
dest_path: &TrackedPath,
) -> Option<String> {
self.render_diff(
source_path,
self.baseline_by_path.get(source_path).map(String::as_str),
dest_path,
self.current_by_path.get(dest_path).map(String::as_str),
)
}
fn render_diff(
&self,
left_path: &TrackedPath,
@@ -267,6 +317,10 @@ impl TurnDiffTracker {
return None;
}
#[cfg(test)]
self.rendered_diff_count
.set(self.rendered_diff_count.get() + 1);
let left_display = self.display_path(left_path);
let right_display = self.display_path(right_path);
let left_oid = left_content.map_or_else(
@@ -299,16 +353,23 @@ impl TurnDiffTracker {
DEV_NULL.to_string()
};
let unified =
similar::TextDiff::from_lines(left_content.unwrap_or(""), right_content.unwrap_or(""))
.unified_diff()
.context_radius(3)
.header(&old_header, &new_header)
.to_string();
let mut config = similar::TextDiff::configure();
config.timeout(DIFF_TIMEOUT);
let unified = config
.diff_lines(left_content.unwrap_or(""), right_content.unwrap_or(""))
.unified_diff()
.context_radius(3)
.header(&old_header, &new_header)
.to_string();
diff.push_str(&unified);
Some(diff)
}
#[cfg(test)]
fn rendered_diff_count(&self) -> usize {
self.rendered_diff_count.get()
}
fn display_path(&self, path: &TrackedPath) -> String {
let display = self
.display_roots_by_environment
@@ -2,10 +2,15 @@ use super::*;
use codex_apply_patch::AppliedPatchDelta;
use codex_apply_patch::MaybeApplyPatchVerified;
use codex_exec_server::LOCAL_FS;
use codex_git_utils::ApplyGitRequest;
use codex_git_utils::apply_git_patch;
use codex_utils_absolute_path::AbsolutePathBuf;
use pretty_assertions::assert_eq;
use std::fs;
use std::path::Path;
use std::process::Command;
use std::time::Duration;
use std::time::Instant;
use tempfile::tempdir;
fn git_blob_sha1_hex(data: &str) -> String {
@@ -369,3 +374,122 @@ index {left_oid_b}..{right_oid_b}
);
assert_eq!(tracker.get_unified_diff(), Some(expected));
}
#[tokio::test]
async fn reuses_rendered_diffs_for_unchanged_paths() {
let dir = tempdir().expect("tempdir");
let mut tracker = tracker_with_root(dir.path());
let add_a = apply_verified_patch(
dir.path(),
"*** Begin Patch\n*** Add File: a.txt\n+one\n*** End Patch",
)
.await;
tracker.track_delta("", &add_a);
assert_eq!(tracker.rendered_diff_count(), 1);
let add_b = apply_verified_patch(
dir.path(),
"*** Begin Patch\n*** Add File: b.txt\n+two\n*** End Patch",
)
.await;
tracker.track_delta("", &add_b);
assert_eq!(tracker.rendered_diff_count(), 2);
assert_eq!(
tracker.get_unified_diff(),
tracker.get_unified_diff(),
"reading the cached aggregate must not render file diffs",
);
assert_eq!(tracker.rendered_diff_count(), 2);
}
#[tokio::test]
async fn repeated_updates_only_rerender_the_touched_path() {
let dir = tempdir().expect("tempdir");
let mut tracker = tracker_with_root(dir.path());
for patch in [
"*** Begin Patch\n*** Add File: stable.txt\n+stable\n*** End Patch".to_string(),
"*** Begin Patch\n*** Add File: hot.txt\n+value 0\n*** End Patch".to_string(),
] {
tracker.track_delta("", &apply_verified_patch(dir.path(), &patch).await);
}
for value in 1..=40 {
let patch = format!(
"*** Begin Patch\n*** Update File: hot.txt\n@@\n-value {}\n+value {value}\n*** End Patch",
value - 1,
);
tracker.track_delta("", &apply_verified_patch(dir.path(), &patch).await);
}
assert_eq!(tracker.rendered_diff_count(), 42);
}
#[test]
fn large_rewrite_returns_promptly_and_preserves_exact_content() {
let dir = tempdir().expect("tempdir");
assert!(
Command::new("git")
.args(["init", "--quiet"])
.current_dir(dir.path())
.status()
.expect("run git init")
.success()
);
assert!(
Command::new("git")
.args(["config", "core.autocrlf", "false"])
.current_dir(dir.path())
.status()
.expect("disable line ending conversion")
.success()
);
let old_content = (0..48_000)
.map(|line| format!("old line {line:05}\n"))
.collect::<String>();
let new_content = (0..48_000)
.map(|line| format!("new line {line:05}\n"))
.collect::<String>();
let path = dir.path().join("large.txt");
fs::write(&path, &old_content).expect("seed large file");
assert!(
Command::new("git")
.args(["add", "large.txt"])
.current_dir(dir.path())
.status()
.expect("run git add")
.success()
);
let tracker = tracker_with_root(dir.path());
let tracked_path = TrackedPath::new("", &path);
let started = Instant::now();
let diff = tracker
.render_diff(
&tracked_path,
Some(&old_content),
&tracked_path,
Some(&new_content),
)
.expect("complete rewrite should produce a diff");
assert!(
started.elapsed() < Duration::from_secs(2),
"large rewrite took {:?}",
started.elapsed(),
);
let result = apply_git_patch(&ApplyGitRequest {
cwd: dir.path().to_path_buf(),
diff,
revert: false,
preflight: false,
})
.expect("apply generated diff");
assert_eq!(result.exit_code, 0, "{}", result.stderr);
assert_eq!(
fs::read_to_string(path).expect("read large file"),
new_content
);
}