Add app-server thread/delete API#25018
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df653fcc2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d86f1d76f5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b169881ec
ℹ️ 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".
thread/delete API
jif-oai
left a comment
There was a problem hiding this comment.
I’m worried this is modeling hard delete as a best-effort orchestration across several persistence systems, without a durable deleted/tombstone state
I think the API shape is fine, but the commit point needs to move to a durable store-level delete/tombstone contract before this can safely promise permanent full-tree deletion (which is the whole point and pretty important if we consider the data safety question)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d49dccc8ea
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60c0f05bf0
ℹ️ 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".
|
Thinking more about the feedback... We need to decide the intended use of If we treat this as a guaranteed deletion of the data, then we need to fail the operation if we're unable to delete one or more of the rollout files, and that leaves users stuck with visible threads that they no longer want to see — and with no recourse to remove them other than to attempt to find and delete the files manually and then somehow repair or reconstruct their db. This is probably a reasonable tradeoff. I've changed the implementation to fail if one or more rollout files cannot be deleted. Database updates are now best-effort. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ddb4d9f9c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e19cfeb55
ℹ️ 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".
jif-oai
left a comment
There was a problem hiding this comment.
I would reinforce the sqlite reliability
| let thread_id = ThreadId::from_string(¶ms.thread_id) | ||
| .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; | ||
|
|
||
| let mut thread_ids = self.state_db_spawn_subtree_thread_ids(thread_id).await?; |
There was a problem hiding this comment.
this is a bit racy to snapshot at that time. I'm not sure what's possible to do though
| warn!("failed to delete logs for thread {thread_id}: {err}"); | ||
| } | ||
|
|
||
| if let Err(err) = self.memories.delete_thread_memory(thread_id).await { |
There was a problem hiding this comment.
This still make it best-effort. Should we have a retry policy (with some exponential back-off)?
Or even dumping what needs to be done somewhere (like in a file) and then having an async worker that retry later?
There was a problem hiding this comment.
I don't think we should add this level of complexity.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0d3423e6a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc4bad38ef
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bb48c783f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3a9560c18
ℹ️ 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".
|
I'm not convinced that we need to so far as to guarantee deletion of metadata, but I understand your perspective. I've changed the design to return failure if any part of the deletion fails — either file system or db. Since the operation isn't transactional, we have no practical way to roll back if we get half way through a deletion. That means we'll end up with partially-deleted threads. The new approach is designed to do these deletions in an order that can be retried. For example, we don't delete the subagent hierarchy metadata until the end because doing it earlier would cause the subagent associations to be lost. You suggested that we should maybe implement a retry policy with backoff and persisted work queues, etc. I think that's overkill in terms of complexity. |
de99a95 to
e04ba87
Compare
| self.validate_root_thread_delete(thread_id, thread_ids.len() > 1) | ||
| .await?; | ||
| for thread_id_to_delete in thread_ids.iter().copied() { | ||
| self.prepare_thread_for_delete(thread_id_to_delete).await; |
There was a problem hiding this comment.
I think we will need to think a bit about sub-agents and their state in cleanups but we can do this later
| return Err(ThreadStoreError::ThreadNotFound { thread_id }); | ||
| } | ||
|
|
||
| store.live_recorders.lock().await.remove(&thread_id); |
There was a problem hiding this comment.
thread/name/set also persists the id and name in session_index.jsonl, but this path returns success without removing those records. I think we should just happen something in the file to unassign the name (and btw I would like to nuke this file in the future)
There was a problem hiding this comment.
Yes, agreed. I added code to delete the name from this file.
Add a v2 thread/delete method that hard-deletes a requested thread, discovers spawned descendants before the root delete, emits thread/deleted notifications for committed deletes, and cleans up associated state best effort after the root state row is deleted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b30942237
ℹ️ 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".
1b30942 to
126b310
Compare
Why
Clients can archive and unarchive threads today, but there is no app-server API for permanently removing a thread. Deletion also needs to cover the full session tree: deleting a main thread should remove spawned subagent threads and the related local metadata instead of leaving orphaned rollout files, goals, or subagent state behind.
What
thread/deleterequest andthread/deletednotification, with the response shape kept consistent withthread/archive.