Move live thread metadata handling above recorder#21874
Move live thread metadata handling above recorder#21874wiltzius-openai wants to merge 1 commit intowiltzius/codex/thread-metadata-store-apifrom
Conversation
wiltzius-openai
force-pushed
the
wiltzius/codex/live-thread-metadata-handler
branch
2 times, most recently
from
ecbc5b6 to
8f83c53
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-metadata-store-api
branch
from
f8dfb0b to
8a074e0
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/live-thread-metadata-handler
branch
3 times, most recently
from
587e2e4 to
93065e7
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/live-thread-metadata-handler
branch
from
93065e7 to
b41516a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b41516a719
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (initial, prepared) = { | ||
| let mut handler = self.metadata_handler.lock().await; | ||
| let prepared = handler.prepare_items(items); | ||
| let initial = prepared | ||
| .is_some() | ||
| .then(|| handler.take_initial_metadata()) | ||
| .flatten(); | ||
| (initial, prepared) | ||
| }; |
There was a problem hiding this comment.
append_items advances metadata_handler under the mutex, releases it, then performs multiple awaited store calls. Concurrent appends can therefore compute order A then B but enqueue B's rollout item before A's initial/user items or apply B's metadata first, producing out-of-order JSONL/SQLite state for cloned LiveThreads.
Useful? React with 👍 / 👎.
| if let Some(initial) = initial { | ||
| self.append_prepared_metadata(initial).await?; | ||
| } | ||
| let Some(prepared) = prepared else { | ||
| return Ok(()); | ||
| }; | ||
| self.append_prepared_metadata(prepared).await |
There was a problem hiding this comment.
When the initial metadata write fails, this returns before appending the caller's prepared items, but prepare_items has already advanced the handler state. A transient SQLite/apply error during the first append can therefore lose the first user message from the rollout entirely instead of still recording JSONL and retrying metadata later.
Useful? React with 👍 / 👎.
2 participants
Builds on #21873 to remove thread metadata handling from the RolloutRecorder, which is now only responsible for writing jsonl files in the local ThreadStore. Instead LiveThread makes ThreadStore API calls to explicitly store metatdata.
Summary
LiveThreadto ownThreadMetadataHandlerand send canonical rollout items plus metadata updates in orderRolloutRecorderwrite already-canonical JSONL only and remove recorder-owned SQLite metadata syncLocalThreadStorelive writer create/resume paths around canonical recorder params