mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Use state DB first for resume --last (#26462)
## Summary `codex resume --last` currently lists sessions by updated time using scan-and-repair. Updated-time filesystem listing must stat every rollout before applying the cwd, provider, and source filters, so startup scales with the entire local session history... This change queries the state DB first for the latest matching session. For local workspaces, we only accept the indexed result when its rollout path still exists; otherwise we retry with scan-and-repair. The same lookup path is shared by `fork --last`. I benchmarked the same `thread/list` request used by `resume --last` in my local `ruff` checkout against a Codex home with 2,599 active rollouts totaling 3.7 GiB, including 90 Ruff threads. Across five fresh release app-server processes with warm filesystem caches, the state-DB-only lookup had median latency of 0.37-0.44 ms, while scan-and-repair had median latency of 139-162 ms. First-request latency was 0.7-1.7 ms versus 142-185 ms. So this **removes roughly 140-160 ms from the `resume --last` lookup** on this machine, and makes that lookup over 300x faster. The tradeoff is that this does leave two correctness gaps: - If a newer matching rollout is missing from SQLite but an older matching row exists, the fast path resumes the older thread and never falls back to the filesystem scan. - If an existing row has stale filter or ordering metadata, the fast path can select a different thread from scan-and-repair. The rollout tests already demonstrate this for stale cwd metadata: state-DB-only returns the stale match, while scan-and-repair removes and repairs it. So you could end up seeing the "wrong" result in cases like... 1. A crash or SQLite error occurs between Codex writing the conversation file and updating SQLite, leaving the newer file unindexed. 2. An older Codex version, restore, or manual copy adds a conversation file after SQLite’s one-time backfill completed. These seem pretty rare though (and sessions can always be recovered via other mechanisms -- `--last` is just a convenience feature), and I think the tradeoffs are good here?
This commit is contained in:
committed by
GitHub
Unverified
parent
76c0a5379c
commit
345cf6e8d0
+185
-95
@@ -744,18 +744,37 @@ async fn lookup_latest_session_target_with_app_server(
|
||||
cwd_filter: Option<&Path>,
|
||||
include_non_interactive: bool,
|
||||
) -> color_eyre::Result<Option<resume_picker::SessionTarget>> {
|
||||
let response = app_server
|
||||
.thread_list(latest_session_lookup_params(
|
||||
app_server.uses_remote_workspace(),
|
||||
config,
|
||||
cwd_filter,
|
||||
include_non_interactive,
|
||||
))
|
||||
.await?;
|
||||
Ok(response
|
||||
.data
|
||||
.into_iter()
|
||||
.find_map(session_target_from_app_server_thread))
|
||||
let uses_remote_workspace = app_server.uses_remote_workspace();
|
||||
for lookup_mode in [
|
||||
LatestSessionLookupMode::StateDbOnly,
|
||||
LatestSessionLookupMode::ScanAndRepair,
|
||||
] {
|
||||
let response = app_server
|
||||
.thread_list(latest_session_lookup_params(
|
||||
uses_remote_workspace,
|
||||
config,
|
||||
cwd_filter,
|
||||
include_non_interactive,
|
||||
lookup_mode,
|
||||
))
|
||||
.await?;
|
||||
let target = response
|
||||
.data
|
||||
.into_iter()
|
||||
.find_map(session_target_from_app_server_thread);
|
||||
if target.as_ref().is_some_and(|target| {
|
||||
uses_remote_workspace || target.path.as_deref().is_some_and(std::path::Path::exists)
|
||||
}) {
|
||||
return Ok(target);
|
||||
}
|
||||
}
|
||||
Ok(None)
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
enum LatestSessionLookupMode {
|
||||
StateDbOnly,
|
||||
ScanAndRepair,
|
||||
}
|
||||
|
||||
fn latest_session_lookup_params(
|
||||
@@ -763,6 +782,7 @@ fn latest_session_lookup_params(
|
||||
config: &Config,
|
||||
cwd_filter: Option<&Path>,
|
||||
include_non_interactive: bool,
|
||||
lookup_mode: LatestSessionLookupMode,
|
||||
) -> ThreadListParams {
|
||||
ThreadListParams {
|
||||
cursor: None,
|
||||
@@ -777,7 +797,10 @@ fn latest_session_lookup_params(
|
||||
source_kinds: Some(resume_source_kinds(include_non_interactive)),
|
||||
archived: Some(false),
|
||||
cwd: cwd_filter.map(|cwd| ThreadListCwdFilter::One(cwd.to_string_lossy().to_string())),
|
||||
use_state_db_only: false,
|
||||
use_state_db_only: match lookup_mode {
|
||||
LatestSessionLookupMode::StateDbOnly => true,
|
||||
LatestSessionLookupMode::ScanAndRepair => false,
|
||||
},
|
||||
search_term: None,
|
||||
}
|
||||
}
|
||||
@@ -2040,6 +2063,85 @@ mod tests {
|
||||
.await
|
||||
}
|
||||
|
||||
fn write_session_rollout(
|
||||
codex_home: &Path,
|
||||
filename_ts: &str,
|
||||
meta_rfc3339: &str,
|
||||
preview: &str,
|
||||
model_provider: &str,
|
||||
cwd: &Path,
|
||||
) -> color_eyre::Result<ThreadId> {
|
||||
let uuid = Uuid::new_v4();
|
||||
let uuid_str = uuid.to_string();
|
||||
let thread_id = ThreadId::from_string(&uuid_str)?;
|
||||
let year = &filename_ts[0..4];
|
||||
let month = &filename_ts[5..7];
|
||||
let day = &filename_ts[8..10];
|
||||
let rollout_path = codex_home
|
||||
.join("sessions")
|
||||
.join(year)
|
||||
.join(month)
|
||||
.join(day)
|
||||
.join(format!("rollout-{filename_ts}-{uuid_str}.jsonl"));
|
||||
let parent = rollout_path
|
||||
.parent()
|
||||
.ok_or_else(|| color_eyre::eyre::eyre!("rollout path is missing a parent directory"))?;
|
||||
std::fs::create_dir_all(parent)?;
|
||||
|
||||
let session_meta = codex_protocol::protocol::SessionMeta {
|
||||
id: thread_id,
|
||||
timestamp: meta_rfc3339.to_string(),
|
||||
cwd: cwd.to_path_buf(),
|
||||
originator: "codex".to_string(),
|
||||
cli_version: "0.0.0".to_string(),
|
||||
source: codex_protocol::protocol::SessionSource::Cli,
|
||||
model_provider: Some(model_provider.to_string()),
|
||||
..Default::default()
|
||||
};
|
||||
let session_meta = serde_json::to_value(codex_protocol::protocol::SessionMetaLine {
|
||||
meta: session_meta,
|
||||
git: None,
|
||||
})?;
|
||||
let lines = [
|
||||
serde_json::json!({
|
||||
"timestamp": meta_rfc3339,
|
||||
"type": "session_meta",
|
||||
"payload": session_meta,
|
||||
})
|
||||
.to_string(),
|
||||
serde_json::json!({
|
||||
"timestamp": meta_rfc3339,
|
||||
"type": "response_item",
|
||||
"payload": {
|
||||
"type": "message",
|
||||
"role": "user",
|
||||
"content": [{"type": "input_text", "text": preview}],
|
||||
},
|
||||
})
|
||||
.to_string(),
|
||||
serde_json::json!({
|
||||
"timestamp": meta_rfc3339,
|
||||
"type": "event_msg",
|
||||
"payload": {
|
||||
"type": "user_message",
|
||||
"message": preview,
|
||||
"kind": "plain",
|
||||
},
|
||||
})
|
||||
.to_string(),
|
||||
];
|
||||
std::fs::write(&rollout_path, lines.join("\n") + "\n")?;
|
||||
let updated_at =
|
||||
chrono::DateTime::parse_from_rfc3339(meta_rfc3339)?.with_timezone(&chrono::Utc);
|
||||
let times = std::fs::FileTimes::new().set_modified(updated_at.into());
|
||||
std::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(rollout_path)?
|
||||
.set_times(times)?;
|
||||
|
||||
Ok(thread_id)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn startup_removes_legacy_tui_log_file() -> std::io::Result<()> {
|
||||
let temp_dir = TempDir::new()?;
|
||||
@@ -2328,13 +2430,27 @@ mod tests {
|
||||
&config,
|
||||
Some(cwd.as_path()),
|
||||
/*include_non_interactive*/ false,
|
||||
LatestSessionLookupMode::StateDbOnly,
|
||||
);
|
||||
|
||||
assert_eq!(params.model_providers, Some(vec![config.model_provider_id]));
|
||||
assert_eq!(
|
||||
params.model_providers,
|
||||
Some(vec![config.model_provider_id.clone()])
|
||||
);
|
||||
assert_eq!(
|
||||
params.cwd,
|
||||
Some(ThreadListCwdFilter::One(cwd.to_string_lossy().to_string()))
|
||||
);
|
||||
assert!(params.use_state_db_only);
|
||||
|
||||
let scan_params = latest_session_lookup_params(
|
||||
/*uses_remote_workspace*/ false,
|
||||
&config,
|
||||
Some(cwd.as_path()),
|
||||
/*include_non_interactive*/ false,
|
||||
LatestSessionLookupMode::ScanAndRepair,
|
||||
);
|
||||
assert!(!scan_params.use_state_db_only);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -2355,6 +2471,7 @@ mod tests {
|
||||
&config,
|
||||
Some(cwd.as_path()),
|
||||
/*include_non_interactive*/ false,
|
||||
LatestSessionLookupMode::StateDbOnly,
|
||||
);
|
||||
|
||||
assert_eq!(params.model_providers, Some(vec![config.model_provider_id]));
|
||||
@@ -2372,8 +2489,11 @@ mod tests {
|
||||
let config = build_config(&temp_dir).await?;
|
||||
|
||||
let params = latest_session_lookup_params(
|
||||
/*uses_remote_workspace*/ true, &config, /*cwd_filter*/ None,
|
||||
/*uses_remote_workspace*/ true,
|
||||
&config,
|
||||
/*cwd_filter*/ None,
|
||||
/*include_non_interactive*/ false,
|
||||
LatestSessionLookupMode::StateDbOnly,
|
||||
);
|
||||
|
||||
assert_eq!(params.model_providers, None);
|
||||
@@ -2388,8 +2508,11 @@ mod tests {
|
||||
let config = build_config(&temp_dir).await?;
|
||||
|
||||
let params = latest_session_lookup_params(
|
||||
/*uses_remote_workspace*/ true, &config, /*cwd_filter*/ None,
|
||||
/*uses_remote_workspace*/ true,
|
||||
&config,
|
||||
/*cwd_filter*/ None,
|
||||
/*include_non_interactive*/ true,
|
||||
LatestSessionLookupMode::StateDbOnly,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
@@ -2416,6 +2539,7 @@ mod tests {
|
||||
&config,
|
||||
Some(cwd),
|
||||
/*include_non_interactive*/ false,
|
||||
LatestSessionLookupMode::StateDbOnly,
|
||||
);
|
||||
|
||||
assert_eq!(params.model_providers, None);
|
||||
@@ -2455,85 +2579,6 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn fork_last_filters_latest_session_by_cwd_unless_show_all() -> color_eyre::Result<()> {
|
||||
fn write_session_rollout(
|
||||
codex_home: &Path,
|
||||
filename_ts: &str,
|
||||
meta_rfc3339: &str,
|
||||
preview: &str,
|
||||
model_provider: &str,
|
||||
cwd: &Path,
|
||||
) -> color_eyre::Result<ThreadId> {
|
||||
let uuid = Uuid::new_v4();
|
||||
let uuid_str = uuid.to_string();
|
||||
let thread_id = ThreadId::from_string(&uuid_str)?;
|
||||
let year = &filename_ts[0..4];
|
||||
let month = &filename_ts[5..7];
|
||||
let day = &filename_ts[8..10];
|
||||
let rollout_path = codex_home
|
||||
.join("sessions")
|
||||
.join(year)
|
||||
.join(month)
|
||||
.join(day)
|
||||
.join(format!("rollout-{filename_ts}-{uuid_str}.jsonl"));
|
||||
let parent = rollout_path.parent().ok_or_else(|| {
|
||||
color_eyre::eyre::eyre!("rollout path is missing a parent directory")
|
||||
})?;
|
||||
std::fs::create_dir_all(parent)?;
|
||||
|
||||
let session_meta = codex_protocol::protocol::SessionMeta {
|
||||
id: thread_id,
|
||||
timestamp: meta_rfc3339.to_string(),
|
||||
cwd: cwd.to_path_buf(),
|
||||
originator: "codex".to_string(),
|
||||
cli_version: "0.0.0".to_string(),
|
||||
source: codex_protocol::protocol::SessionSource::Cli,
|
||||
model_provider: Some(model_provider.to_string()),
|
||||
..Default::default()
|
||||
};
|
||||
let session_meta = serde_json::to_value(codex_protocol::protocol::SessionMetaLine {
|
||||
meta: session_meta,
|
||||
git: None,
|
||||
})?;
|
||||
let lines = [
|
||||
serde_json::json!({
|
||||
"timestamp": meta_rfc3339,
|
||||
"type": "session_meta",
|
||||
"payload": session_meta,
|
||||
})
|
||||
.to_string(),
|
||||
serde_json::json!({
|
||||
"timestamp": meta_rfc3339,
|
||||
"type": "response_item",
|
||||
"payload": {
|
||||
"type": "message",
|
||||
"role": "user",
|
||||
"content": [{"type": "input_text", "text": preview}],
|
||||
},
|
||||
})
|
||||
.to_string(),
|
||||
serde_json::json!({
|
||||
"timestamp": meta_rfc3339,
|
||||
"type": "event_msg",
|
||||
"payload": {
|
||||
"type": "user_message",
|
||||
"message": preview,
|
||||
"kind": "plain",
|
||||
},
|
||||
})
|
||||
.to_string(),
|
||||
];
|
||||
std::fs::write(&rollout_path, lines.join("\n") + "\n")?;
|
||||
let updated_at =
|
||||
chrono::DateTime::parse_from_rfc3339(meta_rfc3339)?.with_timezone(&chrono::Utc);
|
||||
let times = std::fs::FileTimes::new().set_modified(updated_at.into());
|
||||
std::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(rollout_path)?
|
||||
.set_times(times)?;
|
||||
|
||||
Ok(thread_id)
|
||||
}
|
||||
|
||||
let temp_dir = TempDir::new()?;
|
||||
let project_cwd = temp_dir.path().join("project");
|
||||
let other_cwd = temp_dir.path().join("other-project");
|
||||
@@ -2603,6 +2648,51 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn latest_session_lookup_falls_back_for_rollout_missing_from_state_db()
|
||||
-> color_eyre::Result<()> {
|
||||
let temp_dir = TempDir::new()?;
|
||||
let project_cwd = temp_dir.path().join("project");
|
||||
std::fs::create_dir_all(&project_cwd)?;
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(temp_dir.path().to_path_buf())
|
||||
.harness_overrides(ConfigOverrides {
|
||||
cwd: Some(project_cwd.clone()),
|
||||
..Default::default()
|
||||
})
|
||||
.build()
|
||||
.await?;
|
||||
let mut app_server = AppServerSession::new(
|
||||
codex_app_server_client::AppServerClient::InProcess(
|
||||
start_test_embedded_app_server(config.clone()).await?,
|
||||
),
|
||||
ThreadParamsMode::Embedded,
|
||||
);
|
||||
|
||||
// Simulate a legacy writer creating a rollout after the state DB backfill completed.
|
||||
let thread_id = write_session_rollout(
|
||||
temp_dir.path(),
|
||||
"2025-01-02T10-00-00",
|
||||
"2025-01-02T10:00:00Z",
|
||||
"legacy writer session",
|
||||
config.model_provider_id.as_str(),
|
||||
&project_cwd,
|
||||
)?;
|
||||
|
||||
let target = lookup_latest_session_target_with_app_server(
|
||||
&mut app_server,
|
||||
&config,
|
||||
Some(project_cwd.as_path()),
|
||||
/*include_non_interactive*/ false,
|
||||
)
|
||||
.await?
|
||||
.expect("expected scan-and-repair fallback to find the rollout");
|
||||
app_server.shutdown().await?;
|
||||
|
||||
assert_eq!(target.thread_id, thread_id);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn config_cwd_for_app_server_target_omits_cwd_for_remote_sessions() -> std::io::Result<()>
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user