mirror of
https://github.com/openai/codex.git
synced 2026-11-13 22:47:20 +00:00
Preserve thread names during rollout migration (#39273)
## Why Legacy threads and paginated threads read their displayed names from different metadata. Moving a rollout to paginated history must carry over its legacy-visible name so renamed threads do not lose it. ## What changed - Load legacy indexed names once for an apply run. - When promoting a thread, preserve an existing canonical name or copy its distinct legacy title, falling back to the legacy name index. - Repair missing names when migration encounters an already-paginated rollout. ## Testing Added migration tests covering preservation from both legacy name sources and repair on a rerun. GitOrigin-RevId: 5fc526570b1cf358a159ef174c159ac091ccc40f
This commit is contained in:
1 parent
4a3e829c56
commit
f950a1ba0b
3 files changed
+231
-22
No files matched your search
@@ -61,12 +61,31 @@ WHERE threads.id = ?
|
|||||||
.transpose()
|
.transpose()
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Permanently promote a thread to paginated history without changing metadata or recency.
|
/// Permanently promote a thread, preserving its canonical name or a legacy-visible fallback.
|
||||||
pub async fn mark_thread_paginated(&self, thread_id: ThreadId) -> anyhow::Result<bool> {
|
pub async fn mark_thread_paginated(
|
||||||
let result = sqlx::query("UPDATE threads SET history_mode = 'paginated' WHERE id = ?")
|
&self,
|
||||||
.bind(thread_id.to_string())
|
thread_id: ThreadId,
|
||||||
.execute(self.pool.as_ref())
|
legacy_name: Option<&str>,
|
||||||
.await?;
|
) -> anyhow::Result<bool> {
|
||||||
|
// Legacy threads display `title`, then fall back to the name index. Paginated threads
|
||||||
|
// display `name`; `title` remains derived metadata used for search. Preserve an existing
|
||||||
|
// `name`, otherwise carry over the legacy display name.
|
||||||
|
let result = sqlx::query(
|
||||||
|
r#"
|
||||||
|
UPDATE threads
|
||||||
|
SET
|
||||||
|
history_mode = 'paginated',
|
||||||
|
name = CASE
|
||||||
|
WHEN name IS NULL OR trim(name) = '' THEN ?
|
||||||
|
ELSE name
|
||||||
|
END
|
||||||
|
WHERE id = ?
|
||||||
|
"#,
|
||||||
|
)
|
||||||
|
.bind(legacy_name)
|
||||||
|
.bind(thread_id.to_string())
|
||||||
|
.execute(self.pool.as_ref())
|
||||||
|
.await?;
|
||||||
Ok(result.rows_affected() > 0)
|
Ok(result.rows_affected() > 0)
|
||||||
}
|
}
|
||||||
pub async fn get_thread_memory_mode(&self, id: ThreadId) -> anyhow::Result<Option<String>> {
|
pub async fn get_thread_memory_mode(&self, id: ThreadId) -> anyhow::Result<Option<String>> {
|
||||||
@@ -1594,7 +1613,7 @@ mod tests {
|
|||||||
|
|
||||||
assert!(
|
assert!(
|
||||||
runtime
|
runtime
|
||||||
.mark_thread_paginated(thread_id)
|
.mark_thread_paginated(thread_id, /*legacy_name*/ None)
|
||||||
.await
|
.await
|
||||||
.expect("mark paginated history")
|
.expect("mark paginated history")
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -8,6 +8,7 @@
|
|||||||
//! recoverable paginated rollout. Once the rollout path is replaced, the durable `.pending`
|
//! recoverable paginated rollout. Once the rollout path is replaced, the durable `.pending`
|
||||||
//! journal must be enough for a later migration run to finish SQLite recovery safely.
|
//! journal must be enough for a later migration run to finish SQLite recovery safely.
|
||||||
|
|
||||||
|
use std::collections::HashMap;
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
@@ -32,6 +33,7 @@ use tokio::io::BufWriter;
|
|||||||
use tokio::time::Instant;
|
use tokio::time::Instant;
|
||||||
|
|
||||||
use super::LocalThreadStore;
|
use super::LocalThreadStore;
|
||||||
|
use super::helpers::distinct_thread_metadata_title;
|
||||||
use super::thread_history;
|
use super::thread_history;
|
||||||
use super::thread_history::ProjectedRolloutLine;
|
use super::thread_history::ProjectedRolloutLine;
|
||||||
use super::thread_history::RolloutProjectionStep;
|
use super::thread_history::RolloutProjectionStep;
|
||||||
@@ -297,12 +299,24 @@ impl LocalThreadStore {
|
|||||||
.is_some_and(|thread_id| pending_thread_ids.contains(&thread_id))
|
.is_some_and(|thread_id| pending_thread_ids.contains(&thread_id))
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
// The name index is global, so read it once before either migrating or repairing names.
|
||||||
|
let legacy_names = if options.mode == RolloutMigrationMode::Apply {
|
||||||
|
let thread_ids = paths
|
||||||
|
.iter()
|
||||||
|
.filter_map(|path| thread_id_from_rollout_filename(path))
|
||||||
|
.collect();
|
||||||
|
codex_rollout::find_thread_names_by_ids(&self.config.codex_home, &thread_ids)
|
||||||
|
.await
|
||||||
|
.unwrap_or_default()
|
||||||
|
} else {
|
||||||
|
HashMap::new()
|
||||||
|
};
|
||||||
let total_paths = paths.len();
|
let total_paths = paths.len();
|
||||||
let mut report = RolloutMigrationReport::default();
|
let mut report = RolloutMigrationReport::default();
|
||||||
|
|
||||||
for (index, path) in paths.into_iter().enumerate() {
|
for (index, path) in paths.into_iter().enumerate() {
|
||||||
let outcome = self
|
let outcome = self
|
||||||
.migrate_rollout_path(path, &options, &mut limiter)
|
.migrate_rollout_path(path, &options, &legacy_names, &mut limiter)
|
||||||
.await?;
|
.await?;
|
||||||
let outcome_status = outcome.as_ref().map(|outcome| outcome.status);
|
let outcome_status = outcome.as_ref().map(|outcome| outcome.status);
|
||||||
if let Some(outcome) = outcome {
|
if let Some(outcome) = outcome {
|
||||||
@@ -322,6 +336,7 @@ impl LocalThreadStore {
|
|||||||
&self,
|
&self,
|
||||||
mut path: PathBuf,
|
mut path: PathBuf,
|
||||||
options: &RolloutMigrationOptions,
|
options: &RolloutMigrationOptions,
|
||||||
|
legacy_names: &HashMap<ThreadId, String>,
|
||||||
limiter: &mut RolloutMigrationRateLimiter,
|
limiter: &mut RolloutMigrationRateLimiter,
|
||||||
) -> ThreadStoreResult<Option<RolloutMigrationOutcome>> {
|
) -> ThreadStoreResult<Option<RolloutMigrationOutcome>> {
|
||||||
let metadata = match codex_rollout::read_session_meta_line(&path).await {
|
let metadata = match codex_rollout::read_session_meta_line(&path).await {
|
||||||
@@ -380,7 +395,13 @@ impl LocalThreadStore {
|
|||||||
let result = if pending_published_migration {
|
let result = if pending_published_migration {
|
||||||
let _live_writer_guard = self.live_writer_locks.lock(thread_id).await;
|
let _live_writer_guard = self.live_writer_locks.lock(thread_id).await;
|
||||||
match self
|
match self
|
||||||
.recover_published_migration(thread_id, &path, &journal_path, limiter)
|
.recover_published_migration(
|
||||||
|
thread_id,
|
||||||
|
&path,
|
||||||
|
&journal_path,
|
||||||
|
legacy_names,
|
||||||
|
limiter,
|
||||||
|
)
|
||||||
.await
|
.await
|
||||||
{
|
{
|
||||||
Ok(recovered_path) => {
|
Ok(recovered_path) => {
|
||||||
@@ -399,6 +420,9 @@ impl LocalThreadStore {
|
|||||||
Err(error) => Err(error),
|
Err(error) => Err(error),
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
|
if options.mode == RolloutMigrationMode::Apply {
|
||||||
|
self.promote_legacy_name(thread_id, legacy_names).await?;
|
||||||
|
}
|
||||||
Ok(RolloutMigrationStatus::AlreadyPaginated)
|
Ok(RolloutMigrationStatus::AlreadyPaginated)
|
||||||
};
|
};
|
||||||
let bytes_processed = limiter.bytes_processed.saturating_sub(bytes_before);
|
let bytes_processed = limiter.bytes_processed.saturating_sub(bytes_before);
|
||||||
@@ -438,7 +462,7 @@ impl LocalThreadStore {
|
|||||||
};
|
};
|
||||||
let bytes_before = limiter.bytes_processed;
|
let bytes_before = limiter.bytes_processed;
|
||||||
let result = match self
|
let result = match self
|
||||||
.migrate_one_rollout(thread_id, &path, &journal_path, kind, limiter)
|
.migrate_one_rollout(thread_id, &path, &journal_path, kind, legacy_names, limiter)
|
||||||
.await
|
.await
|
||||||
{
|
{
|
||||||
Ok(()) => Ok(RolloutMigrationStatus::Migrated),
|
Ok(()) => Ok(RolloutMigrationStatus::Migrated),
|
||||||
@@ -470,6 +494,7 @@ impl LocalThreadStore {
|
|||||||
rollout_path: &Path,
|
rollout_path: &Path,
|
||||||
journal_path: &Path,
|
journal_path: &Path,
|
||||||
kind: RolloutMigrationKind,
|
kind: RolloutMigrationKind,
|
||||||
|
legacy_names: &HashMap<ThreadId, String>,
|
||||||
limiter: &mut RolloutMigrationRateLimiter,
|
limiter: &mut RolloutMigrationRateLimiter,
|
||||||
) -> ThreadStoreResult<()> {
|
) -> ThreadStoreResult<()> {
|
||||||
if let Some(state_db) = &self.state_db
|
if let Some(state_db) = &self.state_db
|
||||||
@@ -627,7 +652,7 @@ impl LocalThreadStore {
|
|||||||
.map_err(migration_error)?;
|
.map_err(migration_error)?;
|
||||||
}
|
}
|
||||||
sync_parent_directory(rollout_path).await?;
|
sync_parent_directory(rollout_path).await?;
|
||||||
self.finish_published_migration(thread_id, journal_path)
|
self.finish_published_migration(thread_id, journal_path, legacy_names)
|
||||||
.await
|
.await
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -798,6 +823,7 @@ impl LocalThreadStore {
|
|||||||
thread_id: ThreadId,
|
thread_id: ThreadId,
|
||||||
rollout_path: &Path,
|
rollout_path: &Path,
|
||||||
journal_path: &Path,
|
journal_path: &Path,
|
||||||
|
legacy_names: &HashMap<ThreadId, String>,
|
||||||
limiter: &mut RolloutMigrationRateLimiter,
|
limiter: &mut RolloutMigrationRateLimiter,
|
||||||
) -> ThreadStoreResult<PathBuf> {
|
) -> ThreadStoreResult<PathBuf> {
|
||||||
let _writer_guard = self.writer_lock_coordinator.acquire(thread_id)?;
|
let _writer_guard = self.writer_lock_coordinator.acquire(thread_id)?;
|
||||||
@@ -838,7 +864,7 @@ impl LocalThreadStore {
|
|||||||
if let Some(decompressed_path) = decompressed_path.as_ref() {
|
if let Some(decompressed_path) = decompressed_path.as_ref() {
|
||||||
remove_file_if_present(decompressed_path).await?;
|
remove_file_if_present(decompressed_path).await?;
|
||||||
}
|
}
|
||||||
self.finish_published_migration(thread_id, journal_path)
|
self.finish_published_migration(thread_id, journal_path, legacy_names)
|
||||||
.await?;
|
.await?;
|
||||||
Ok(rollout_path.to_path_buf())
|
Ok(rollout_path.to_path_buf())
|
||||||
}
|
}
|
||||||
@@ -870,23 +896,52 @@ impl LocalThreadStore {
|
|||||||
&self,
|
&self,
|
||||||
thread_id: ThreadId,
|
thread_id: ThreadId,
|
||||||
journal_path: &Path,
|
journal_path: &Path,
|
||||||
|
legacy_names: &HashMap<ThreadId, String>,
|
||||||
) -> ThreadStoreResult<()> {
|
) -> ThreadStoreResult<()> {
|
||||||
if let Some(state_db) = &self.state_db
|
self.promote_legacy_name(thread_id, legacy_names).await?;
|
||||||
&& !state_db
|
|
||||||
.mark_thread_paginated(thread_id)
|
|
||||||
.await
|
|
||||||
.map_err(migration_error)?
|
|
||||||
{
|
|
||||||
return Err(migration_error(format!(
|
|
||||||
"thread {thread_id} is missing its SQLite metadata"
|
|
||||||
)));
|
|
||||||
}
|
|
||||||
tokio::fs::remove_file(journal_path)
|
tokio::fs::remove_file(journal_path)
|
||||||
.await
|
.await
|
||||||
.map_err(migration_error)?;
|
.map_err(migration_error)?;
|
||||||
sync_parent_directory(journal_path).await
|
sync_parent_directory(journal_path).await
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async fn promote_legacy_name(
|
||||||
|
&self,
|
||||||
|
thread_id: ThreadId,
|
||||||
|
legacy_names: &HashMap<ThreadId, String>,
|
||||||
|
) -> ThreadStoreResult<()> {
|
||||||
|
if let Some(state_db) = &self.state_db {
|
||||||
|
let metadata = state_db
|
||||||
|
.get_thread(thread_id)
|
||||||
|
.await
|
||||||
|
.map_err(migration_error)?
|
||||||
|
.ok_or_else(|| {
|
||||||
|
migration_error(format!("thread {thread_id} is missing its SQLite metadata"))
|
||||||
|
})?;
|
||||||
|
if metadata.history_mode == ThreadHistoryMode::Paginated
|
||||||
|
&& metadata
|
||||||
|
.name
|
||||||
|
.as_deref()
|
||||||
|
.is_some_and(|name| !name.trim().is_empty())
|
||||||
|
{
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
let legacy_name = distinct_thread_metadata_title(&metadata)
|
||||||
|
.or_else(|| legacy_names.get(&thread_id).cloned())
|
||||||
|
.filter(|name| !name.trim().is_empty());
|
||||||
|
if !state_db
|
||||||
|
.mark_thread_paginated(thread_id, legacy_name.as_deref())
|
||||||
|
.await
|
||||||
|
.map_err(migration_error)?
|
||||||
|
{
|
||||||
|
return Err(migration_error(format!(
|
||||||
|
"thread {thread_id} is missing its SQLite metadata"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
async fn project_rollout_in_batches(
|
async fn project_rollout_in_batches(
|
||||||
&self,
|
&self,
|
||||||
thread_id: ThreadId,
|
thread_id: ThreadId,
|
||||||
|
|||||||
@@ -56,13 +56,17 @@ use super::thread_history;
|
|||||||
use super::write_migration_journal;
|
use super::write_migration_journal;
|
||||||
use crate::ItemSortKey;
|
use crate::ItemSortKey;
|
||||||
use crate::ListItemsParams;
|
use crate::ListItemsParams;
|
||||||
|
use crate::ListThreadsParams;
|
||||||
use crate::ListTurnsParams;
|
use crate::ListTurnsParams;
|
||||||
use crate::LoadThreadHistoryParams;
|
use crate::LoadThreadHistoryParams;
|
||||||
use crate::SortDirection;
|
use crate::SortDirection;
|
||||||
use crate::StoredTurnItemsView;
|
use crate::StoredTurnItemsView;
|
||||||
|
use crate::ThreadMetadataPatch;
|
||||||
|
use crate::ThreadSortKey;
|
||||||
use crate::ThreadStore;
|
use crate::ThreadStore;
|
||||||
use crate::ThreadStoreError;
|
use crate::ThreadStoreError;
|
||||||
use crate::TurnPage;
|
use crate::TurnPage;
|
||||||
|
use crate::UpdateThreadMetadataParams;
|
||||||
use crate::local::test_support::test_config;
|
use crate::local::test_support::test_config;
|
||||||
|
|
||||||
const TIMESTAMP: &str = "2025-01-03T12:00:00Z";
|
const TIMESTAMP: &str = "2025-01-03T12:00:00Z";
|
||||||
@@ -1766,6 +1770,137 @@ async fn migration_migrates_archived_rollouts_without_unarchiving_them() {
|
|||||||
assert_eq!(turns.turns[0].items.len(), 2);
|
assert_eq!(turns.turns[0].items.len(), 2);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn migration_preserves_legacy_displayed_thread_names() {
|
||||||
|
let home = TempDir::new().expect("create Codex home");
|
||||||
|
let title_thread_id = ThreadId::new();
|
||||||
|
write_rollout(
|
||||||
|
home.path(),
|
||||||
|
title_thread_id,
|
||||||
|
SessionSource::Cli,
|
||||||
|
vec![user_message("title question")],
|
||||||
|
);
|
||||||
|
let index_thread_id = ThreadId::new();
|
||||||
|
write_rollout(
|
||||||
|
home.path(),
|
||||||
|
index_thread_id,
|
||||||
|
SessionSource::Cli,
|
||||||
|
vec![user_message("index question")],
|
||||||
|
);
|
||||||
|
let store = indexed_store(home.path()).await;
|
||||||
|
store
|
||||||
|
.update_thread_metadata(UpdateThreadMetadataParams {
|
||||||
|
thread_id: title_thread_id,
|
||||||
|
patch: ThreadMetadataPatch {
|
||||||
|
name: Some(Some("renamed title".to_string())),
|
||||||
|
..Default::default()
|
||||||
|
},
|
||||||
|
include_archived: false,
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.expect("rename legacy thread");
|
||||||
|
codex_rollout::append_thread_name(home.path(), title_thread_id, "stale index title")
|
||||||
|
.await
|
||||||
|
.expect("write stale legacy index name");
|
||||||
|
codex_rollout::append_thread_name(home.path(), index_thread_id, "indexed title")
|
||||||
|
.await
|
||||||
|
.expect("write legacy index name");
|
||||||
|
|
||||||
|
store
|
||||||
|
.migrate_rollouts(apply_options())
|
||||||
|
.await
|
||||||
|
.expect("migrate named rollouts");
|
||||||
|
|
||||||
|
let page = store
|
||||||
|
.list_threads(ListThreadsParams {
|
||||||
|
page_size: 10,
|
||||||
|
cursor: None,
|
||||||
|
sort_key: ThreadSortKey::CreatedAt,
|
||||||
|
sort_direction: SortDirection::Desc,
|
||||||
|
allowed_sources: Vec::new(),
|
||||||
|
model_providers: None,
|
||||||
|
cwd_filters: None,
|
||||||
|
section: None,
|
||||||
|
project_id: None,
|
||||||
|
archived: false,
|
||||||
|
search_term: None,
|
||||||
|
relation_filter: None,
|
||||||
|
use_state_db_only: true,
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.expect("list migrated threads");
|
||||||
|
let title_thread = page
|
||||||
|
.items
|
||||||
|
.iter()
|
||||||
|
.find(|thread| thread.thread_id == title_thread_id)
|
||||||
|
.expect("renamed title thread");
|
||||||
|
let index_thread = page
|
||||||
|
.items
|
||||||
|
.iter()
|
||||||
|
.find(|thread| thread.thread_id == index_thread_id)
|
||||||
|
.expect("indexed title thread");
|
||||||
|
|
||||||
|
assert_eq!(title_thread.name.as_deref(), Some("renamed title"));
|
||||||
|
assert_eq!(index_thread.name.as_deref(), Some("indexed title"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn migration_repairs_a_missing_paginated_name_when_rerun() {
|
||||||
|
let home = TempDir::new().expect("create Codex home");
|
||||||
|
let thread_id = ThreadId::new();
|
||||||
|
write_rollout(
|
||||||
|
home.path(),
|
||||||
|
thread_id,
|
||||||
|
SessionSource::Cli,
|
||||||
|
vec![user_message("question")],
|
||||||
|
);
|
||||||
|
let store = indexed_store(home.path()).await;
|
||||||
|
store
|
||||||
|
.update_thread_metadata(UpdateThreadMetadataParams {
|
||||||
|
thread_id,
|
||||||
|
patch: ThreadMetadataPatch {
|
||||||
|
name: Some(Some("renamed title".to_string())),
|
||||||
|
..Default::default()
|
||||||
|
},
|
||||||
|
include_archived: false,
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.expect("rename legacy thread");
|
||||||
|
store
|
||||||
|
.migrate_rollouts(apply_options())
|
||||||
|
.await
|
||||||
|
.expect("migrate named rollout");
|
||||||
|
let state_db = store.state_db().await.expect("state runtime");
|
||||||
|
state_db
|
||||||
|
.update_thread_title(thread_id, "question")
|
||||||
|
.await
|
||||||
|
.expect("restore derived title");
|
||||||
|
state_db
|
||||||
|
.update_thread_name(thread_id, /*name*/ None)
|
||||||
|
.await
|
||||||
|
.expect("clear migrated name");
|
||||||
|
|
||||||
|
let report = store
|
||||||
|
.migrate_rollouts(apply_options())
|
||||||
|
.await
|
||||||
|
.expect("repair migrated name");
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
report.outcomes[0].status,
|
||||||
|
RolloutMigrationStatus::AlreadyPaginated
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
state_db
|
||||||
|
.get_thread(thread_id)
|
||||||
|
.await
|
||||||
|
.expect("read repaired metadata")
|
||||||
|
.expect("repaired thread")
|
||||||
|
.name
|
||||||
|
.as_deref(),
|
||||||
|
Some("renamed title")
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(unix)]
|
#[cfg(unix)]
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn decompression_temporaries_are_owner_only() {
|
async fn decompression_temporaries_are_owner_only() {
|
||||||
|
|||||||
Reference in new issue
Block a user