mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix stale thread-name resume lookups (#16646)
Addresses #15943 Problem: Name-based resume could stop on a newer session_index entry whose rollout was never persisted, shadowing an older saved thread with the same name. Solution: Materialize rollouts before indexing thread names and make name lookup skip unresolved entries until it finds a persisted rollout.
This commit is contained in:
committed by
GitHub
Unverified
parent
4dca906e19
commit
36586eafed
@@ -2217,14 +2217,19 @@ impl Session {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn ensure_rollout_materialized(&self) {
|
||||
pub(crate) async fn try_ensure_rollout_materialized(&self) -> std::io::Result<()> {
|
||||
let recorder = {
|
||||
let guard = self.services.rollout.lock().await;
|
||||
guard.clone()
|
||||
};
|
||||
if let Some(rec) = recorder
|
||||
&& let Err(e) = rec.persist().await
|
||||
{
|
||||
if let Some(rec) = recorder {
|
||||
rec.persist().await?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn ensure_rollout_materialized(&self) {
|
||||
if let Err(e) = self.try_ensure_rollout_materialized().await {
|
||||
warn!("failed to materialize rollout recorder: {e}");
|
||||
}
|
||||
}
|
||||
@@ -5541,6 +5546,18 @@ mod handlers {
|
||||
return;
|
||||
};
|
||||
|
||||
if let Err(e) = sess.try_ensure_rollout_materialized().await {
|
||||
let event = Event {
|
||||
id: sub_id,
|
||||
msg: EventMsg::Error(ErrorEvent {
|
||||
message: format!("Failed to set thread name: {e}"),
|
||||
codex_error_info: Some(CodexErrorInfo::Other),
|
||||
}),
|
||||
};
|
||||
sess.send_event_raw(event).await;
|
||||
return;
|
||||
}
|
||||
|
||||
let codex_home = sess.codex_home().await;
|
||||
if let Err(e) =
|
||||
session_index::append_thread_name(&codex_home, sess.conversation_id, &name).await
|
||||
|
||||
@@ -2355,7 +2355,7 @@ async fn attach_rollout_recorder(session: &Arc<Session>) -> PathBuf {
|
||||
let recorder = RolloutRecorder::new(
|
||||
config.as_ref(),
|
||||
RolloutRecorderParams::new(
|
||||
ThreadId::default(),
|
||||
session.conversation_id,
|
||||
/*forked_from_id*/ None,
|
||||
SessionSource::Exec,
|
||||
BaseInstructions::default(),
|
||||
|
||||
@@ -94,7 +94,7 @@ pub enum RolloutRecorderParams {
|
||||
enum RolloutCmd {
|
||||
AddItems(Vec<RolloutItem>),
|
||||
Persist {
|
||||
ack: oneshot::Sender<()>,
|
||||
ack: oneshot::Sender<std::io::Result<()>>,
|
||||
},
|
||||
/// Ensure all prior writes are processed; respond when flushed.
|
||||
Flush {
|
||||
@@ -514,7 +514,7 @@ impl RolloutRecorder {
|
||||
.await
|
||||
.map_err(|e| IoError::other(format!("failed to queue rollout persist: {e}")))?;
|
||||
rx.await
|
||||
.map_err(|e| IoError::other(format!("failed waiting for rollout persist: {e}")))
|
||||
.map_err(|e| IoError::other(format!("failed waiting for rollout persist: {e}")))?
|
||||
}
|
||||
|
||||
/// Flush all queued writes and wait until they are committed by the writer task.
|
||||
@@ -810,11 +810,13 @@ async fn rollout_writer(
|
||||
.await;
|
||||
|
||||
if let Err(err) = result {
|
||||
let _ = ack.send(());
|
||||
return Err(err);
|
||||
let kind = err.kind();
|
||||
let message = err.to_string();
|
||||
let _ = ack.send(Err(IoError::new(kind, message.clone())));
|
||||
return Err(IoError::new(kind, message));
|
||||
}
|
||||
}
|
||||
let _ = ack.send(());
|
||||
let _ = ack.send(Ok(()));
|
||||
}
|
||||
RolloutCmd::Flush { ack } => {
|
||||
// Deferred fresh threads may not have an initialized file yet.
|
||||
|
||||
@@ -111,11 +111,12 @@ pub async fn find_thread_names_by_ids(
|
||||
Ok(names)
|
||||
}
|
||||
|
||||
/// Find the most recently updated thread id for a thread name, if any.
|
||||
pub async fn find_thread_id_by_name(
|
||||
/// Locate a recorded thread rollout file by thread name using newest-first ordering.
|
||||
/// Returns `Ok(Some(path))` if found, `Ok(None)` if not present.
|
||||
pub async fn find_thread_path_by_name_str(
|
||||
codex_home: &Path,
|
||||
name: &str,
|
||||
) -> std::io::Result<Option<ThreadId>> {
|
||||
) -> std::io::Result<Option<PathBuf>> {
|
||||
if name.trim().is_empty() {
|
||||
return Ok(None);
|
||||
}
|
||||
@@ -123,23 +124,28 @@ pub async fn find_thread_id_by_name(
|
||||
if !path.exists() {
|
||||
return Ok(None);
|
||||
}
|
||||
let (tx, mut rx) = tokio::sync::mpsc::channel(1);
|
||||
let name = name.to_string();
|
||||
let entry = tokio::task::spawn_blocking(move || scan_index_from_end_by_name(&path, &name))
|
||||
.await
|
||||
.map_err(std::io::Error::other)??;
|
||||
Ok(entry.map(|entry| entry.id))
|
||||
}
|
||||
// Stream matching ids newest-first instead of stopping at the first name hit: the newest entry
|
||||
// may point at a thread whose rollout was never materialized.
|
||||
let scan =
|
||||
tokio::task::spawn_blocking(move || stream_thread_ids_from_end_by_name(&path, &name, tx));
|
||||
|
||||
/// Locate a recorded thread rollout file by thread name using newest-first ordering.
|
||||
/// Returns `Ok(Some(path))` if found, `Ok(None)` if not present.
|
||||
pub async fn find_thread_path_by_name_str(
|
||||
codex_home: &Path,
|
||||
name: &str,
|
||||
) -> std::io::Result<Option<PathBuf>> {
|
||||
let Some(thread_id) = find_thread_id_by_name(codex_home, name).await? else {
|
||||
return Ok(None);
|
||||
};
|
||||
super::list::find_thread_path_by_id_str(codex_home, &thread_id.to_string()).await
|
||||
while let Some(thread_id) = rx.recv().await {
|
||||
// Keep walking until a matching id resolves to a loadable rollout so an unsaved or partial
|
||||
// rename cannot shadow an older persisted session with the same name.
|
||||
if let Some(path) =
|
||||
super::list::find_thread_path_by_id_str(codex_home, &thread_id.to_string()).await?
|
||||
&& super::list::read_session_meta_line(&path).await.is_ok()
|
||||
{
|
||||
drop(rx);
|
||||
scan.await.map_err(std::io::Error::other)??;
|
||||
return Ok(Some(path));
|
||||
}
|
||||
}
|
||||
scan.await.map_err(std::io::Error::other)??;
|
||||
|
||||
Ok(None)
|
||||
}
|
||||
|
||||
fn session_index_path(codex_home: &Path) -> PathBuf {
|
||||
@@ -153,11 +159,22 @@ fn scan_index_from_end_by_id(
|
||||
scan_index_from_end(path, |entry| entry.id == *thread_id)
|
||||
}
|
||||
|
||||
fn scan_index_from_end_by_name(
|
||||
fn stream_thread_ids_from_end_by_name(
|
||||
path: &Path,
|
||||
name: &str,
|
||||
) -> std::io::Result<Option<SessionIndexEntry>> {
|
||||
scan_index_from_end(path, |entry| entry.thread_name == name)
|
||||
tx: tokio::sync::mpsc::Sender<ThreadId>,
|
||||
) -> std::io::Result<()> {
|
||||
let mut seen = HashSet::new();
|
||||
scan_index_from_end_for_each(path, |entry| {
|
||||
// The first row seen for an id is its latest name. Ignore older rows for that id so a
|
||||
// historical name cannot be treated as the current one after the thread is renamed.
|
||||
if seen.insert(entry.id) && entry.thread_name == name && tx.blocking_send(entry.id).is_err()
|
||||
{
|
||||
return Ok(Some(entry.clone()));
|
||||
}
|
||||
Ok(None)
|
||||
})?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn scan_index_from_end<F>(
|
||||
@@ -166,6 +183,21 @@ fn scan_index_from_end<F>(
|
||||
) -> std::io::Result<Option<SessionIndexEntry>>
|
||||
where
|
||||
F: FnMut(&SessionIndexEntry) -> bool,
|
||||
{
|
||||
scan_index_from_end_for_each(path, |entry| {
|
||||
if predicate(entry) {
|
||||
return Ok(Some(entry.clone()));
|
||||
}
|
||||
Ok(None)
|
||||
})
|
||||
}
|
||||
|
||||
fn scan_index_from_end_for_each<F>(
|
||||
path: &Path,
|
||||
mut visit_entry: F,
|
||||
) -> std::io::Result<Option<SessionIndexEntry>>
|
||||
where
|
||||
F: FnMut(&SessionIndexEntry) -> std::io::Result<Option<SessionIndexEntry>>,
|
||||
{
|
||||
let mut file = File::open(path)?;
|
||||
let mut remaining = file.metadata()?.len();
|
||||
@@ -181,7 +213,7 @@ where
|
||||
|
||||
for &byte in buf[..read_size].iter().rev() {
|
||||
if byte == b'\n' {
|
||||
if let Some(entry) = parse_line_from_rev(&mut line_rev, &mut predicate)? {
|
||||
if let Some(entry) = parse_line_from_rev(&mut line_rev, &mut visit_entry)? {
|
||||
return Ok(Some(entry));
|
||||
}
|
||||
continue;
|
||||
@@ -190,7 +222,7 @@ where
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(entry) = parse_line_from_rev(&mut line_rev, &mut predicate)? {
|
||||
if let Some(entry) = parse_line_from_rev(&mut line_rev, &mut visit_entry)? {
|
||||
return Ok(Some(entry));
|
||||
}
|
||||
|
||||
@@ -199,10 +231,10 @@ where
|
||||
|
||||
fn parse_line_from_rev<F>(
|
||||
line_rev: &mut Vec<u8>,
|
||||
predicate: &mut F,
|
||||
visit_entry: &mut F,
|
||||
) -> std::io::Result<Option<SessionIndexEntry>>
|
||||
where
|
||||
F: FnMut(&SessionIndexEntry) -> bool,
|
||||
F: FnMut(&SessionIndexEntry) -> std::io::Result<Option<SessionIndexEntry>>,
|
||||
{
|
||||
if line_rev.is_empty() {
|
||||
return Ok(None);
|
||||
@@ -222,10 +254,7 @@ where
|
||||
let Ok(entry) = serde_json::from_str::<SessionIndexEntry>(trimmed) else {
|
||||
return Ok(None);
|
||||
};
|
||||
if predicate(&entry) {
|
||||
return Ok(Some(entry));
|
||||
}
|
||||
Ok(None)
|
||||
visit_entry(&entry)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -1,6 +1,11 @@
|
||||
#![allow(warnings, clippy::all)]
|
||||
|
||||
use super::*;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::RolloutLine;
|
||||
use codex_protocol::protocol::SessionMeta;
|
||||
use codex_protocol::protocol::SessionMetaLine;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
@@ -14,6 +19,34 @@ fn write_index(path: &Path, lines: &[SessionIndexEntry]) -> std::io::Result<()>
|
||||
std::fs::write(path, out)
|
||||
}
|
||||
|
||||
fn write_rollout_with_metadata(path: &Path, thread_id: ThreadId) -> std::io::Result<()> {
|
||||
let timestamp = "2024-01-01T00-00-00Z".to_string();
|
||||
let line = RolloutLine {
|
||||
timestamp: timestamp.clone(),
|
||||
item: RolloutItem::SessionMeta(SessionMetaLine {
|
||||
meta: SessionMeta {
|
||||
id: thread_id,
|
||||
forked_from_id: None,
|
||||
timestamp,
|
||||
cwd: ".".into(),
|
||||
originator: "test_originator".into(),
|
||||
cli_version: "test_version".into(),
|
||||
source: SessionSource::Cli,
|
||||
agent_path: None,
|
||||
agent_nickname: None,
|
||||
agent_role: None,
|
||||
model_provider: Some("test-provider".into()),
|
||||
base_instructions: None,
|
||||
dynamic_tools: None,
|
||||
memory_mode: None,
|
||||
},
|
||||
git: None,
|
||||
}),
|
||||
};
|
||||
let body = serde_json::to_string(&line).map_err(std::io::Error::other)?;
|
||||
std::fs::write(path, format!("{body}\n"))
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_thread_id_by_name_prefers_latest_entry() -> std::io::Result<()> {
|
||||
let temp = TempDir::new()?;
|
||||
@@ -34,11 +67,116 @@ fn find_thread_id_by_name_prefers_latest_entry() -> std::io::Result<()> {
|
||||
];
|
||||
write_index(&path, &lines)?;
|
||||
|
||||
let found = scan_index_from_end_by_name(&path, "same")?;
|
||||
let found = scan_index_from_end(&path, |entry| entry.thread_name == "same")?;
|
||||
assert_eq!(found.map(|entry| entry.id), Some(id2));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn find_thread_path_by_name_str_skips_newest_entry_without_rollout() -> std::io::Result<()> {
|
||||
// A newer unsaved name entry should not shadow an older persisted rollout with the same name.
|
||||
let temp = TempDir::new()?;
|
||||
let path = session_index_path(temp.path());
|
||||
let saved_id = ThreadId::new();
|
||||
let unsaved_id = ThreadId::new();
|
||||
let saved_rollout_path = temp
|
||||
.path()
|
||||
.join("sessions/2024/01/01")
|
||||
.join(format!("rollout-2024-01-01T00-00-00-{saved_id}.jsonl"));
|
||||
std::fs::create_dir_all(saved_rollout_path.parent().expect("rollout parent"))?;
|
||||
write_rollout_with_metadata(&saved_rollout_path, saved_id)?;
|
||||
let lines = vec![
|
||||
SessionIndexEntry {
|
||||
id: saved_id,
|
||||
thread_name: "same".to_string(),
|
||||
updated_at: "2024-01-01T00:00:00Z".to_string(),
|
||||
},
|
||||
SessionIndexEntry {
|
||||
id: unsaved_id,
|
||||
thread_name: "same".to_string(),
|
||||
updated_at: "2024-01-02T00:00:00Z".to_string(),
|
||||
},
|
||||
];
|
||||
write_index(&path, &lines)?;
|
||||
|
||||
let found = find_thread_path_by_name_str(temp.path(), "same").await?;
|
||||
|
||||
assert_eq!(found, Some(saved_rollout_path));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn find_thread_path_by_name_str_skips_partial_rollout() -> std::io::Result<()> {
|
||||
let temp = TempDir::new()?;
|
||||
let path = session_index_path(temp.path());
|
||||
let saved_id = ThreadId::new();
|
||||
let partial_id = ThreadId::new();
|
||||
let rollout_dir = temp.path().join("sessions/2024/01/01");
|
||||
let saved_rollout_path =
|
||||
rollout_dir.join(format!("rollout-2024-01-01T00-00-00-{saved_id}.jsonl"));
|
||||
let partial_rollout_path =
|
||||
rollout_dir.join(format!("rollout-2024-01-01T00-00-01-{partial_id}.jsonl"));
|
||||
std::fs::create_dir_all(&rollout_dir)?;
|
||||
write_rollout_with_metadata(&saved_rollout_path, saved_id)?;
|
||||
std::fs::write(&partial_rollout_path, "")?;
|
||||
let lines = vec![
|
||||
SessionIndexEntry {
|
||||
id: saved_id,
|
||||
thread_name: "same".to_string(),
|
||||
updated_at: "2024-01-01T00:00:00Z".to_string(),
|
||||
},
|
||||
SessionIndexEntry {
|
||||
id: partial_id,
|
||||
thread_name: "same".to_string(),
|
||||
updated_at: "2024-01-02T00:00:00Z".to_string(),
|
||||
},
|
||||
];
|
||||
write_index(&path, &lines)?;
|
||||
|
||||
let found = find_thread_path_by_name_str(temp.path(), "same").await?;
|
||||
|
||||
assert_eq!(found, Some(saved_rollout_path));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn find_thread_path_by_name_str_ignores_historical_name_after_rename() -> std::io::Result<()>
|
||||
{
|
||||
let temp = TempDir::new()?;
|
||||
let path = session_index_path(temp.path());
|
||||
let renamed_id = ThreadId::new();
|
||||
let current_id = ThreadId::new();
|
||||
let current_rollout_path = temp
|
||||
.path()
|
||||
.join("sessions/2024/01/01")
|
||||
.join(format!("rollout-2024-01-01T00-00-00-{current_id}.jsonl"));
|
||||
std::fs::create_dir_all(current_rollout_path.parent().expect("rollout parent"))?;
|
||||
write_rollout_with_metadata(¤t_rollout_path, current_id)?;
|
||||
let lines = vec![
|
||||
SessionIndexEntry {
|
||||
id: renamed_id,
|
||||
thread_name: "same".to_string(),
|
||||
updated_at: "2024-01-01T00:00:00Z".to_string(),
|
||||
},
|
||||
SessionIndexEntry {
|
||||
id: current_id,
|
||||
thread_name: "same".to_string(),
|
||||
updated_at: "2024-01-02T00:00:00Z".to_string(),
|
||||
},
|
||||
SessionIndexEntry {
|
||||
id: renamed_id,
|
||||
thread_name: "different".to_string(),
|
||||
updated_at: "2024-01-03T00:00:00Z".to_string(),
|
||||
},
|
||||
];
|
||||
write_index(&path, &lines)?;
|
||||
|
||||
let found = find_thread_path_by_name_str(temp.path(), "same").await?;
|
||||
|
||||
assert_eq!(found, Some(current_rollout_path));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_thread_name_by_id_prefers_latest_entry() -> std::io::Result<()> {
|
||||
let temp = TempDir::new()?;
|
||||
@@ -78,7 +216,7 @@ fn scan_index_returns_none_when_entry_missing() -> std::io::Result<()> {
|
||||
}];
|
||||
write_index(&path, &lines)?;
|
||||
|
||||
let missing_name = scan_index_from_end_by_name(&path, "missing")?;
|
||||
let missing_name = scan_index_from_end(&path, |entry| entry.thread_name == "missing")?;
|
||||
assert_eq!(missing_name, None);
|
||||
|
||||
let missing_id = scan_index_from_end_by_id(&path, &ThreadId::new())?;
|
||||
@@ -157,7 +295,7 @@ fn scan_index_finds_latest_match_among_mixed_entries() -> std::io::Result<()> {
|
||||
];
|
||||
write_index(&path, &lines)?;
|
||||
|
||||
let found_by_name = scan_index_from_end_by_name(&path, "target")?;
|
||||
let found_by_name = scan_index_from_end(&path, |entry| entry.thread_name == "target")?;
|
||||
assert_eq!(found_by_name, Some(expected.clone()));
|
||||
|
||||
let found_by_id = scan_index_from_end_by_id(&path, &id_target)?;
|
||||
|
||||
@@ -2090,6 +2090,10 @@ impl ChatWidget {
|
||||
|
||||
fn on_thread_name_updated(&mut self, event: codex_protocol::protocol::ThreadNameUpdatedEvent) {
|
||||
if self.thread_id == Some(event.thread_id) {
|
||||
if let Some(name) = event.thread_name.as_deref() {
|
||||
let cell = Self::rename_confirmation_cell(name, self.thread_id);
|
||||
self.add_boxed_history(Box::new(cell));
|
||||
}
|
||||
self.thread_name = event.thread_name;
|
||||
self.refresh_terminal_title();
|
||||
self.request_redraw();
|
||||
@@ -5399,9 +5403,6 @@ impl ChatWidget {
|
||||
self.add_error_message("Thread name cannot be empty.".to_string());
|
||||
return;
|
||||
};
|
||||
let cell = Self::rename_confirmation_cell(&name, self.thread_id);
|
||||
self.add_boxed_history(Box::new(cell));
|
||||
self.request_redraw();
|
||||
self.app_event_tx.set_thread_name(name);
|
||||
self.bottom_pane.drain_pending_submission_state();
|
||||
}
|
||||
@@ -5479,7 +5480,6 @@ impl ChatWidget {
|
||||
} else {
|
||||
"Name thread"
|
||||
};
|
||||
let thread_id = self.thread_id;
|
||||
let view = CustomPromptView::new(
|
||||
title.to_string(),
|
||||
"Type a name and press Enter".to_string(),
|
||||
@@ -5491,8 +5491,6 @@ impl ChatWidget {
|
||||
)));
|
||||
return;
|
||||
};
|
||||
let cell = Self::rename_confirmation_cell(&name, thread_id);
|
||||
tx.send(AppEvent::InsertHistoryCell(Box::new(cell)));
|
||||
tx.set_thread_name(name);
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -590,6 +590,30 @@ async fn live_app_server_invalid_thread_name_update_is_ignored() {
|
||||
assert_eq!(chat.thread_name, Some("original name".to_string()));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn live_app_server_thread_name_update_shows_resume_hint() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let thread_id = ThreadId::new();
|
||||
chat.thread_id = Some(thread_id);
|
||||
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::ThreadNameUpdated(
|
||||
codex_app_server_protocol::ThreadNameUpdatedNotification {
|
||||
thread_id: thread_id.to_string(),
|
||||
thread_name: Some("review-fix".to_string()),
|
||||
},
|
||||
),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
|
||||
assert_eq!(chat.thread_name, Some("review-fix".to_string()));
|
||||
let cells = drain_insert_history(&mut rx);
|
||||
assert_eq!(cells.len(), 1);
|
||||
let rendered = lines_to_single_string(&cells[0]);
|
||||
assert!(rendered.contains("Thread renamed to review-fix"));
|
||||
assert!(rendered.contains("codex resume review-fix"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn live_app_server_thread_closed_requests_immediate_exit() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
Reference in New Issue
Block a user