Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# External shell inline replay performance guard

## Lifecycle matrix

| Lifecycle / path | Behavior | Verdict |
| --- | --- | --- |
| Small completed external output | Build one bounded in-memory preview and attach a complete replay state to the event. | pass |
| Output above preview threshold | Preserve the existing `.slog` writer and SQLite replay metadata path. | pass |
| Running / replay-owned event | Preserve the existing early return; no new work occurs. | pass |
| Persistence failure on large output | Preserve the existing bounded incomplete-preview fallback and error. | pass |
| Repeated imported events | Each small event performs O(output bytes) copying bounded by `SHELL_REPLAY_PREVIEW_BYTES`; no background task or retained global state is added. | pass |

## Resource findings

| Area | Finding | Verdict | Reason / mitigation |
| --- | --- | --- | --- |
| CPU | Small output is concatenated once instead of framed and serialized through replay storage. | keep | Copy cost is bounded by the existing preview threshold. |
| Memory | The complete small output is retained in `terminal_preview`. | keep | The same bytes were already retained as a preview; size cannot exceed `SHELL_REPLAY_PREVIEW_BYTES`. |
| Filesystem / SQLite | Small output skips `.slog` creation and replay-table writes; large output is unchanged. | keep | The branch threshold exactly matches the maximum complete inline preview. |
| Compatibility | The event still exposes a complete `ShellReplayState`, with zero readable range bytes indicating no backing artifact. | keep | Range consumers use `terminal_preview` for the complete bounded output; the large-output paging contract is unchanged. |
| Cleanup | No new files, rows, timers, workers, or global caches are created. | keep | Nothing new survives beyond the event state. |

## Verdict

**Pass for boundedness and lifecycle.** Unit coverage proves the small path creates
neither a replay row nor a backing replay artifact. No real-device I/O benchmark
was run, so the PR claims removal of redundant writes for the bounded path, not a
measured end-to-end speedup.
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ pub fn persist_external_shell_replays(events: &mut [SessionEvent]) {
total.saturating_add(part.text.len() as u64)
});
let call_id = event.call_id.clone().unwrap_or_else(|| event.id.clone());
if expected_bytes <= SHELL_REPLAY_PREVIEW_BYTES as u64 {
// The bounded preview can hold the complete output, so creating a
// .slog file and three SQLite transactions would only turn a
// small, rebuildable history row into synchronous storage work.
// A Codex turn can contain hundreds of these calls; keeping them
// inline avoids one fsync chain per call while preserving every
// output byte needed by the UI.
event.shell_replay = Some(inline_complete_state(event, &call_id, &parts));
continue;
}
let replay_root = resolve_replay_root();
match persist_one(event, &call_id, &replay_root, &parts, expected_bytes) {
Ok(state) => event.shell_replay = Some(state),
Expand Down Expand Up @@ -192,6 +202,33 @@ fn output_parts(event: &SessionEvent) -> Vec<OutputPart<'_>> {
Vec::new()
}

fn inline_complete_state(
event: &SessionEvent,
call_id: &str,
parts: &[OutputPart<'_>],
) -> ShellReplayState {
let mut output = String::new();
for part in parts {
output.push_str(part.text);
}
debug_assert!(output.len() <= SHELL_REPLAY_PREVIEW_BYTES);
ShellReplayState {
replay_ref: ShellReplayRef {
session_id: event.session_id.clone(),
call_id: call_id.to_string(),
format_version: SHELL_REPLAY_FORMAT_VERSION,
},
// Zero readable bytes intentionally tells range consumers that the
// complete output already lives in terminal_preview; there is no
// backing artifact to page.
bookmark: ShellReplayBookmark::default(),
terminal_preview: output,
status: ShellReplayStatus::Complete,
error: None,
completed_at: Some(event.created_at.clone()),
}
}

fn first_string_at_paths<'a>(value: &'a serde_json::Value, paths: &[&[&str]]) -> Option<&'a str> {
paths.iter().find_map(|path| string_at_path(value, path))
}
Expand Down Expand Up @@ -293,4 +330,32 @@ mod tests {
assert!(state.terminal_preview.ends_with("TAIL"));
assert!(state.terminal_preview.len() <= SHELL_REPLAY_PREVIEW_BYTES);
}

#[test]
#[serial_test::serial]
fn small_external_shell_stays_complete_without_replay_storage() {
let _sandbox = test_helpers::test_env::sandbox();
let conn = database::db::get_connection().unwrap();
database::init_shell_replay_tables(&conn).unwrap();
let output = "small complete output".to_string();
let mut event = external_shell_event(output.clone());

persist_external_shell_replays(std::slice::from_mut(&mut event));

let state = event.shell_replay.as_ref().expect("inline replay state");
assert_eq!(state.status, ShellReplayStatus::Complete);
assert_eq!(state.bookmark.visible_bytes, 0);
assert_eq!(state.terminal_preview, output);
assert_eq!(state.error, None);
assert!(
load_replay_state(&event.session_id, "external-shell-call")
.unwrap()
.is_none(),
"a complete bounded preview must not create a replay artifact"
);
let stored_replays: i64 = conn
.query_row("SELECT COUNT(*) FROM shell_replays", [], |row| row.get(0))
.unwrap();
assert_eq!(stored_replays, 0);
}
}
Loading