mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
Distinguish turn-start thread persistence (#37926)
## What changed - Add `PersistContext` to the thread-store persistence contract so stores can identify persistence requested immediately before model sampling. A `TurnStart` request may be enqueued in the background when later flush and shutdown operations fence it and report any failure; all other persistence remains synchronous. - Use the turn-start context only for the initial input of a regular turn, while admission acknowledgments, steered input, and other persistence paths keep the standard durability behavior. - Flush pending metadata before turn-start persistence and ensure shutdown is still attempted if that metadata update fails. ## Testing - Verify that developer instructions and user input are persisted before the model request completes for a default paginated-history thread. GitOrigin-RevId: 162f0ec796a61aebe66ca0b909fbcccec3047b85
This commit is contained in:
@@ -26,6 +26,7 @@ use crate::DeleteThreadParams;
|
||||
use crate::ListThreadsParams;
|
||||
use crate::LoadThreadHistoryParams;
|
||||
use crate::MoveThreadToSectionParams;
|
||||
use crate::PersistContext;
|
||||
use crate::ReadThreadByRolloutPathParams;
|
||||
use crate::ReadThreadParams;
|
||||
use crate::ResumeThreadParams;
|
||||
@@ -816,7 +817,11 @@ impl ThreadStore for InMemoryThreadStore {
|
||||
Box::pin(InMemoryThreadStore::append_items(self, params))
|
||||
}
|
||||
|
||||
fn persist_thread(&self, _thread_id: ThreadId) -> ThreadStoreFuture<'_, ()> {
|
||||
fn persist_thread(
|
||||
&self,
|
||||
_thread_id: ThreadId,
|
||||
_context: PersistContext,
|
||||
) -> ThreadStoreFuture<'_, ()> {
|
||||
Box::pin(async move {
|
||||
self.state.lock().await.calls.persist_thread += 1;
|
||||
Ok(())
|
||||
|
||||
@@ -32,6 +32,7 @@ pub use local::RolloutMigrationReport;
|
||||
pub use local::RolloutMigrationStatus;
|
||||
pub use queue_store::LocalQueueStore;
|
||||
pub use queue_store::QueueStore;
|
||||
pub use store::PersistContext;
|
||||
pub use store::ThreadStore;
|
||||
pub use store::ThreadStoreFuture;
|
||||
pub use thread_sections::CreateThreadSectionParams;
|
||||
|
||||
@@ -15,6 +15,7 @@ use crate::AppendThreadItemsParams;
|
||||
use crate::CreateThreadParams;
|
||||
use crate::LoadThreadHistoryParams;
|
||||
use crate::LocalThreadStore;
|
||||
use crate::PersistContext;
|
||||
use crate::ReadThreadParams;
|
||||
use crate::ResumeThreadParams;
|
||||
use crate::StoredThread;
|
||||
@@ -253,8 +254,14 @@ impl LiveThread {
|
||||
Ok(items)
|
||||
}
|
||||
|
||||
pub async fn persist(&self) -> ThreadStoreResult<()> {
|
||||
self.thread_store.persist_thread(self.thread_id).await?;
|
||||
pub async fn persist(&self, context: PersistContext) -> ThreadStoreResult<()> {
|
||||
if context == PersistContext::TurnStart {
|
||||
self.flush_pending_metadata_update_for_existing_history()
|
||||
.await?;
|
||||
}
|
||||
self.thread_store
|
||||
.persist_thread(self.thread_id, context)
|
||||
.await?;
|
||||
self.flush_pending_metadata_update().await
|
||||
}
|
||||
|
||||
@@ -265,9 +272,19 @@ impl LiveThread {
|
||||
}
|
||||
|
||||
pub async fn shutdown(&self) -> ThreadStoreResult<()> {
|
||||
self.flush_pending_metadata_update_for_existing_history()
|
||||
.await?;
|
||||
self.thread_store.shutdown_thread(self.thread_id).await
|
||||
let metadata_result = self
|
||||
.flush_pending_metadata_update_for_existing_history()
|
||||
.await;
|
||||
let shutdown_result = self.thread_store.shutdown_thread(self.thread_id).await;
|
||||
match (metadata_result, shutdown_result) {
|
||||
(Err(metadata_error), Err(shutdown_error)) => Err(ThreadStoreError::Internal {
|
||||
message: format!(
|
||||
"thread metadata update failed: {metadata_error}; thread shutdown failed: {shutdown_error}"
|
||||
),
|
||||
}),
|
||||
(Err(metadata_error), Ok(())) => Err(metadata_error),
|
||||
(Ok(()), result) => result,
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn discard(&self) -> ThreadStoreResult<()> {
|
||||
|
||||
@@ -54,6 +54,7 @@ use crate::ListThreadsParams;
|
||||
use crate::ListTurnsParams;
|
||||
use crate::LoadThreadHistoryParams;
|
||||
use crate::MoveThreadToSectionParams;
|
||||
use crate::PersistContext;
|
||||
use crate::PrepareForkParams;
|
||||
use crate::PreparedFork;
|
||||
use crate::ReadThreadByRolloutPathParams;
|
||||
@@ -400,7 +401,11 @@ impl ThreadStore for LocalThreadStore {
|
||||
Box::pin(async move { live_writer::append_items(self, params).await })
|
||||
}
|
||||
|
||||
fn persist_thread(&self, thread_id: ThreadId) -> ThreadStoreFuture<'_, ()> {
|
||||
fn persist_thread(
|
||||
&self,
|
||||
thread_id: ThreadId,
|
||||
_context: PersistContext,
|
||||
) -> ThreadStoreFuture<'_, ()> {
|
||||
Box::pin(async move { live_writer::persist_thread(self, thread_id).await })
|
||||
}
|
||||
|
||||
@@ -615,7 +620,7 @@ mod tests {
|
||||
.await
|
||||
.expect("append live item");
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist live thread");
|
||||
store
|
||||
@@ -843,7 +848,10 @@ mod tests {
|
||||
)
|
||||
.await
|
||||
.expect("create live thread with inherited context");
|
||||
live_thread.persist().await.expect("persist thread");
|
||||
live_thread
|
||||
.persist(PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist thread");
|
||||
let inherited_metadata = runtime
|
||||
.get_thread(thread_id)
|
||||
.await
|
||||
@@ -1277,7 +1285,7 @@ mod tests {
|
||||
.await
|
||||
.expect("append initial item");
|
||||
first_store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist initial thread");
|
||||
first_store
|
||||
@@ -1337,7 +1345,7 @@ mod tests {
|
||||
.await
|
||||
.expect("create live thread");
|
||||
primary
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist thread for resume");
|
||||
let rollout_path = primary
|
||||
|
||||
@@ -44,6 +44,7 @@ use crate::DeleteThreadParams;
|
||||
use crate::ForkBoundary;
|
||||
use crate::ListThreadsParams;
|
||||
use crate::ListTurnsParams;
|
||||
use crate::PersistContext;
|
||||
use crate::PrepareForkParams;
|
||||
use crate::PreparedFork;
|
||||
use crate::ResumeThreadParams;
|
||||
@@ -72,7 +73,7 @@ async fn paginated_history_without_state_db_does_not_initialize_sqlite() {
|
||||
.await
|
||||
.expect("append paginated rollout");
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist paginated rollout");
|
||||
store
|
||||
@@ -288,7 +289,7 @@ async fn paginated_live_append_materializes_turn_items_and_state() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -459,7 +460,7 @@ async fn referenced_paginated_rollout_projects_inherited_ordinal_range() {
|
||||
)
|
||||
.await;
|
||||
store
|
||||
.persist_thread(child_id)
|
||||
.persist_thread(child_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist child metadata");
|
||||
assert_eq!(
|
||||
@@ -563,7 +564,7 @@ async fn named_fork_boundaries_reject_invisible_and_noncanonical_turns() {
|
||||
)
|
||||
.await;
|
||||
store
|
||||
.persist_thread(child_id)
|
||||
.persist_thread(child_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist child metadata");
|
||||
store
|
||||
@@ -730,7 +731,7 @@ async fn paginated_fork_materializes_compressed_source_and_ancestor() {
|
||||
let ancestor_thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, ancestor_thread_id).await;
|
||||
store
|
||||
.persist_thread(ancestor_thread_id)
|
||||
.persist_thread(ancestor_thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist ancestor meta");
|
||||
store
|
||||
@@ -766,7 +767,7 @@ async fn paginated_fork_materializes_compressed_source_and_ancestor() {
|
||||
)
|
||||
.await;
|
||||
store
|
||||
.persist_thread(source_thread_id)
|
||||
.persist_thread(source_thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist source meta");
|
||||
store
|
||||
@@ -863,7 +864,7 @@ async fn cancelled_fork_keeps_source_reserved_until_lineage_materialization_fini
|
||||
)
|
||||
.await;
|
||||
store
|
||||
.persist_thread(source_thread_id)
|
||||
.persist_thread(source_thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist source metadata");
|
||||
let source_path = store
|
||||
@@ -924,7 +925,7 @@ async fn prepared_fork_reserves_source_until_child_reference_is_durable() {
|
||||
let source_thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, source_thread_id).await;
|
||||
store
|
||||
.persist_thread(source_thread_id)
|
||||
.persist_thread(source_thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist source metadata");
|
||||
store
|
||||
@@ -982,7 +983,7 @@ async fn prepared_fork_reserves_source_until_child_reference_is_durable() {
|
||||
)
|
||||
.await;
|
||||
store
|
||||
.persist_thread(child_thread_id)
|
||||
.persist_thread(child_thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist child history reference");
|
||||
drop(prepared);
|
||||
@@ -1010,7 +1011,7 @@ async fn subagent_prefix_advances_projection_without_materializing_history() {
|
||||
)
|
||||
.await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -1269,7 +1270,7 @@ async fn summary_items_use_final_answers_and_ignore_commentary() {
|
||||
let store = LocalThreadStore::new(config, Some(runtime));
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -1371,7 +1372,7 @@ async fn paginated_projection_accepts_float_rate_limits_and_later_final_answers(
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
store
|
||||
@@ -1512,7 +1513,7 @@ async fn next_write_catches_up_unprojected_durable_suffix() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -1639,7 +1640,7 @@ async fn catch_up_preserves_trailing_partial_line_boundaries() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -1725,7 +1726,7 @@ async fn catch_up_rejects_invalid_complete_suffixes_without_advancing_state() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -1796,7 +1797,7 @@ async fn catch_up_rejects_missing_rollout_after_projection() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
let rollout_path = store
|
||||
@@ -1854,7 +1855,7 @@ async fn blank_and_rejected_rollout_lines_do_not_poison_projection() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -1942,7 +1943,7 @@ async fn unprojectable_rollout_lines_wait_for_later_ordinals() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -2007,7 +2008,7 @@ async fn event_timestamps_allow_invalid_rollout_timestamps() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -2066,7 +2067,7 @@ async fn malformed_rollout_lines_skip_inferred_ordinal_gaps() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
|
||||
@@ -2153,7 +2154,7 @@ async fn delete_waits_for_in_flight_projection_before_removing_rows() {
|
||||
let thread_id = ThreadId::default();
|
||||
create_paginated_thread(&store, thread_id).await;
|
||||
store
|
||||
.persist_thread(thread_id)
|
||||
.persist_thread(thread_id, PersistContext::Standard)
|
||||
.await
|
||||
.expect("persist session metadata");
|
||||
let write_permit = store.live_writer_locks.lock(thread_id).await;
|
||||
|
||||
@@ -43,6 +43,15 @@ use crate::UpdateThreadMetadataParams;
|
||||
/// Future returned by [`ThreadStore`] operations.
|
||||
pub type ThreadStoreFuture<'a, T> = Pin<Box<dyn Future<Output = ThreadStoreResult<T>> + Send + 'a>>;
|
||||
|
||||
/// Why thread persistence is being requested.
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
pub enum PersistContext {
|
||||
/// Standard persistence makes the thread and all queued items durable and readable.
|
||||
Standard,
|
||||
/// A turn is about to begin sampling after its input has been recorded.
|
||||
TurnStart,
|
||||
}
|
||||
|
||||
/// Storage-neutral thread persistence boundary.
|
||||
pub trait ThreadStore: Any + Send + Sync {
|
||||
/// Return this store as [`Any`] for implementation-owned escape hatches.
|
||||
@@ -69,7 +78,15 @@ pub trait ThreadStore: Any + Send + Sync {
|
||||
fn append_items(&self, params: AppendThreadItemsParams) -> ThreadStoreFuture<'_, ()>;
|
||||
|
||||
/// Materializes the thread if persistence is lazy, then persists all queued items.
|
||||
fn persist_thread(&self, thread_id: ThreadId) -> ThreadStoreFuture<'_, ()>;
|
||||
///
|
||||
/// Standard persistence must complete before returning. Turn-start persistence may complete
|
||||
/// in the background when the implementation enqueues it before returning, fences it with
|
||||
/// subsequent flush or shutdown operations, and surfaces failures through those operations.
|
||||
fn persist_thread(
|
||||
&self,
|
||||
thread_id: ThreadId,
|
||||
context: PersistContext,
|
||||
) -> ThreadStoreFuture<'_, ()>;
|
||||
|
||||
/// Flushes all queued items and returns once they are durable/readable.
|
||||
fn flush_thread(&self, thread_id: ThreadId) -> ThreadStoreFuture<'_, ()>;
|
||||
|
||||
Reference in New Issue
Block a user