mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Read compressed rollouts and materialize before append (#25087)
## Why Local rollout compression needs a cold `.jsonl.zst` representation without letting compressed physical paths leak into append-mode writers. The unsafe case is resume or metadata update code successfully reading a compressed rollout and then appending raw JSONL bytes to the zstd file. This PR folds the former #25088 materialization slice into the read-support PR so the reader changes and append-safety invariant land together. ## What Changed - Teach rollout readers, discovery, listing, search, and ID lookup to understand compressed `.jsonl.zst` rollouts. - Keep `.jsonl` as the logical/stored rollout path while allowing read paths to open either plain or compressed storage. - Materialize compressed rollouts back to plain `.jsonl` before append-mode writes, including resume and direct metadata append paths. - Preserve compressed-file permissions when materializing back to plain JSONL. - Refresh thread-store resolved rollout paths after compatibility metadata writes so reconciliation follows the materialized file. - Avoid treating transient compression temp files as real rollout lookup results. ## Remaining Stack #25089 remains the separate worker PR. It is based directly on this PR and stays behind the disabled `local_thread_store_compression` feature flag. The worker still has a broader coordination question: a resume or metadata update can race with background compression while a plain file is being replaced by `.jsonl.zst`. This PR handles the read and materialize-before-append primitives; it does not make the worker production-ready. ## Validation - `just test -p codex-rollout` - `just test -p codex-thread-store` - `just fix -p codex-rollout` - `just fix -p codex-thread-store` - `just bazel-lock-check`
This commit is contained in:
committed by
GitHub
Unverified
parent
f27bbbd49c
commit
a8a6071279
@@ -74,10 +74,11 @@ pub(super) fn matching_rollout_file_name(
|
||||
),
|
||||
});
|
||||
};
|
||||
let required_suffix = format!("{thread_id}.jsonl");
|
||||
if file_name
|
||||
.to_string_lossy()
|
||||
.ends_with(required_suffix.as_str())
|
||||
let required_plain_suffix = format!("{thread_id}.jsonl");
|
||||
let required_compressed_suffix = format!("{required_plain_suffix}.zst");
|
||||
let file_name_str = file_name.to_string_lossy();
|
||||
if file_name_str.ends_with(required_plain_suffix.as_str())
|
||||
|| file_name_str.ends_with(required_compressed_suffix.as_str())
|
||||
{
|
||||
Ok(file_name)
|
||||
} else {
|
||||
@@ -117,10 +118,11 @@ pub(super) fn stored_thread_from_rollout_item(
|
||||
.clone()
|
||||
.or_else(|| item.first_user_message.clone())
|
||||
.unwrap_or_default();
|
||||
let rollout_path = codex_rollout::plain_rollout_path(item.path.as_path());
|
||||
|
||||
Some(StoredThread {
|
||||
thread_id,
|
||||
rollout_path: Some(item.path),
|
||||
rollout_path: Some(rollout_path),
|
||||
forked_from_id: None,
|
||||
parent_thread_id: item.parent_thread_id,
|
||||
preview,
|
||||
@@ -224,6 +226,7 @@ pub(super) fn git_info_from_parts(
|
||||
|
||||
fn thread_id_from_rollout_path(path: &Path) -> Option<ThreadId> {
|
||||
let file_name = path.file_name()?.to_str()?;
|
||||
let file_name = file_name.strip_suffix(".zst").unwrap_or(file_name);
|
||||
let stem = file_name.strip_suffix(".jsonl")?;
|
||||
if stem.len() < 37 {
|
||||
return None;
|
||||
@@ -234,3 +237,36 @@ fn thread_id_from_rollout_path(path: &Path) -> Option<ThreadId> {
|
||||
}
|
||||
ThreadId::from_string(&stem[uuid_start..]).ok()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use codex_rollout::ThreadItem;
|
||||
use pretty_assertions::assert_eq;
|
||||
use uuid::Uuid;
|
||||
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn stored_thread_from_rollout_item_returns_logical_rollout_path() {
|
||||
let uuid = Uuid::from_u128(1);
|
||||
let compressed_path = PathBuf::from(format!(
|
||||
"/tmp/sessions/2025/01/03/rollout-2025-01-03T12-00-00-{uuid}.jsonl.zst"
|
||||
));
|
||||
let thread = stored_thread_from_rollout_item(
|
||||
ThreadItem {
|
||||
path: compressed_path.clone(),
|
||||
..Default::default()
|
||||
},
|
||||
/*archived*/ false,
|
||||
"test-provider",
|
||||
)
|
||||
.expect("stored thread");
|
||||
|
||||
assert_eq!(
|
||||
thread.rollout_path,
|
||||
Some(
|
||||
compressed_path.with_file_name(format!("rollout-2025-01-03T12-00-00-{uuid}.jsonl"))
|
||||
)
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -156,9 +156,9 @@ async fn sync_materialized_rollout_path(
|
||||
thread_id: ThreadId,
|
||||
) -> ThreadStoreResult<()> {
|
||||
let rollout_path = rollout_path(store, thread_id).await?;
|
||||
if !tokio::fs::try_exists(rollout_path.as_path())
|
||||
if codex_rollout::existing_rollout_path(rollout_path.as_path())
|
||||
.await
|
||||
.unwrap_or(false)
|
||||
.is_none()
|
||||
{
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -90,7 +90,7 @@ async fn sqlite_rollout_path_can_load_history_for_thread(
|
||||
path: &std::path::Path,
|
||||
thread_id: codex_protocol::ThreadId,
|
||||
) -> bool {
|
||||
if !tokio::fs::try_exists(path).await.unwrap_or(false) {
|
||||
if codex_rollout::existing_rollout_path(path).await.is_none() {
|
||||
return false;
|
||||
}
|
||||
// SQLite metadata can outlive a moved/recreated rollout path. When history is
|
||||
@@ -107,7 +107,7 @@ pub(super) async fn read_thread_by_rollout_path(
|
||||
include_archived: bool,
|
||||
include_history: bool,
|
||||
) -> ThreadStoreResult<StoredThread> {
|
||||
let path = resolve_requested_rollout_path(store, rollout_path)?;
|
||||
let path = resolve_requested_rollout_path(store, rollout_path).await?;
|
||||
let mut thread = read_thread_from_rollout_path(store, path).await?;
|
||||
if !include_archived && thread.archived_at.is_some() {
|
||||
return Err(ThreadStoreError::InvalidRequest {
|
||||
@@ -134,7 +134,7 @@ pub(super) async fn read_thread_by_rollout_path(
|
||||
Ok(thread)
|
||||
}
|
||||
|
||||
fn resolve_requested_rollout_path(
|
||||
async fn resolve_requested_rollout_path(
|
||||
store: &LocalThreadStore,
|
||||
rollout_path: std::path::PathBuf,
|
||||
) -> ThreadStoreResult<std::path::PathBuf> {
|
||||
@@ -143,7 +143,15 @@ fn resolve_requested_rollout_path(
|
||||
} else {
|
||||
rollout_path
|
||||
};
|
||||
std::fs::canonicalize(&path).map_err(|err| ThreadStoreError::InvalidRequest {
|
||||
let Some(path) = codex_rollout::existing_rollout_path(path.as_path()).await else {
|
||||
return Err(ThreadStoreError::InvalidRequest {
|
||||
message: format!(
|
||||
"failed to resolve rollout path `{}`: file does not exist",
|
||||
path.display()
|
||||
),
|
||||
});
|
||||
};
|
||||
std::fs::canonicalize(path.as_path()).map_err(|err| ThreadStoreError::InvalidRequest {
|
||||
message: format!("failed to resolve rollout path `{}`: {err}", path.display()),
|
||||
})
|
||||
}
|
||||
@@ -172,11 +180,9 @@ async fn resolve_rollout_path(
|
||||
include_archived: bool,
|
||||
) -> ThreadStoreResult<Option<std::path::PathBuf>> {
|
||||
if let Ok(path) = live_writer::rollout_path(store, thread_id).await
|
||||
&& tokio::fs::try_exists(path.as_path()).await.map_err(|err| {
|
||||
ThreadStoreError::InvalidRequest {
|
||||
message: format!("failed to check rollout path for thread id {thread_id}: {err}"),
|
||||
}
|
||||
})?
|
||||
&& codex_rollout::existing_rollout_path(path.as_path())
|
||||
.await
|
||||
.is_some()
|
||||
&& (include_archived || !rollout_path_is_archived(store.config.codex_home.as_path(), &path))
|
||||
{
|
||||
return Ok(Some(path));
|
||||
@@ -233,6 +239,7 @@ async fn read_thread_from_rollout_path(
|
||||
.ok_or_else(|| ThreadStoreError::Internal {
|
||||
message: format!("failed to read thread id from {}", path.display()),
|
||||
})?;
|
||||
thread.rollout_path = Some(codex_rollout::plain_rollout_path(path.as_path()));
|
||||
if let Ok(meta_line) = read_session_meta_line(path.as_path()).await {
|
||||
thread.forked_from_id = meta_line.meta.forked_from_id;
|
||||
thread.parent_thread_id = meta_line.meta.parent_thread_id;
|
||||
@@ -287,6 +294,7 @@ async fn stored_thread_from_sqlite_metadata(
|
||||
.await
|
||||
.ok()
|
||||
.map(|meta_line| meta_line.meta);
|
||||
let rollout_path = codex_rollout::plain_rollout_path(metadata.rollout_path.as_path());
|
||||
let forked_from_id = session_meta.as_ref().and_then(|meta| meta.forked_from_id);
|
||||
let parent_thread_id = session_meta.as_ref().and_then(|meta| meta.parent_thread_id);
|
||||
let preview = metadata
|
||||
@@ -298,7 +306,7 @@ async fn stored_thread_from_sqlite_metadata(
|
||||
permission_profile_from_metadata_value(&metadata.sandbox_policy, metadata.cwd.as_path());
|
||||
StoredThread {
|
||||
thread_id: metadata.id,
|
||||
rollout_path: Some(metadata.rollout_path),
|
||||
rollout_path: Some(rollout_path),
|
||||
forked_from_id,
|
||||
parent_thread_id,
|
||||
preview,
|
||||
@@ -360,9 +368,10 @@ fn stored_thread_from_meta_line(
|
||||
.and_then(|meta| meta.modified().ok())
|
||||
.map(DateTime::<Utc>::from)
|
||||
.unwrap_or(created_at);
|
||||
let rollout_path = codex_rollout::plain_rollout_path(path.as_path());
|
||||
StoredThread {
|
||||
thread_id: meta_line.meta.id,
|
||||
rollout_path: Some(path),
|
||||
rollout_path: Some(rollout_path),
|
||||
forked_from_id: meta_line.meta.forked_from_id,
|
||||
parent_thread_id: meta_line.meta.parent_thread_id,
|
||||
preview: String::new(),
|
||||
|
||||
@@ -69,13 +69,14 @@ pub(super) async fn update_thread_metadata(
|
||||
if live_writer::rollout_path(store, thread_id).await.is_ok() {
|
||||
live_writer::persist_thread(store, thread_id).await?;
|
||||
}
|
||||
let resolved_rollout_path =
|
||||
let mut resolved_rollout_path =
|
||||
resolve_rollout_path(store, thread_id, params.include_archived).await?;
|
||||
let name = patch.name;
|
||||
let git_info = patch.git_info;
|
||||
if let Some(memory_mode) = patch.memory_mode {
|
||||
apply_thread_memory_mode(resolved_rollout_path.path.as_path(), thread_id, memory_mode)
|
||||
.await?;
|
||||
refresh_resolved_rollout_path(&mut resolved_rollout_path).await;
|
||||
}
|
||||
|
||||
let state_db_ctx = store.state_db().await;
|
||||
@@ -143,6 +144,7 @@ pub(super) async fn update_thread_metadata(
|
||||
memory_mode.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
refresh_resolved_rollout_path(&mut resolved_rollout_path).await;
|
||||
apply_thread_git_info(store, thread_id, sha, branch, origin_url).await?;
|
||||
}
|
||||
|
||||
@@ -173,6 +175,12 @@ pub(super) async fn update_thread_metadata(
|
||||
Ok(thread)
|
||||
}
|
||||
|
||||
async fn refresh_resolved_rollout_path(resolved: &mut ResolvedRolloutPath) {
|
||||
if let Some(path) = codex_rollout::existing_rollout_path(resolved.path.as_path()).await {
|
||||
resolved.path = path;
|
||||
}
|
||||
}
|
||||
|
||||
async fn apply_metadata_update(
|
||||
store: &LocalThreadStore,
|
||||
thread_id: ThreadId,
|
||||
|
||||
Reference in New Issue
Block a user