[app-server] improve thread list and resume rpc paths#28801
[app-server] improve thread list and resume rpc paths#28801anaiskillian wants to merge 3 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 135772bf29
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4969665c0
ℹ️ 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".
a496966 to
ce3329b
Compare
| if items.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| if !canonical_items.is_empty() { |
There was a problem hiding this comment.
This reorders history repair after explicit metadata writes... thread/resume -> thread/name/set -> first turn now overwrites SQLite’s chosen title with the history-derived first-message title
| } | ||
| } | ||
|
|
||
| let metadata_by_id = match ctx.get_threads(&thread_ids).await { |
There was a problem hiding this comment.
This makes read-repair use a page-wide snapshot of full rows. It means a live metadata update after get_threads for example can be overwritten when a later path/cwd/... repair full-upserts its stale clone
| params.parent_thread_id, | ||
| params.archived, | ||
| params.search_term.as_deref(), | ||
| /*validate_rollout_paths*/ false, |
There was a problem hiding this comment.
Previously we skipped jsonl metadata repair but still called existing_rollout_path and removed rows whose rollout had been deleted. As a best-effort process
This new direct path disables that validation. so thread/list can return sessions that immediately fail thread/read or resume or so
|
IMO those comments are more symptoms than isolated bugs. Please do not hack-fix |
|
replaced by three reviewable PRs:
this split addresses the review feedback at the ownership boundaries instead of patching each symptom independently. |
thread/list, using list-specific row types so the app-server only reads the fields needed for list responses instead of materializing the full thread. It preserves the existing filters, parent filtering, ordering, anchors, and repair validation while reducing the amount of DB work on the hot list path.thread/listandthread/resume. The release-binary validation below separately compares this PR's release build with the installed Codex CLI and the backend bundled with the stable Codex app.Release-binary comparison
The PR was faster in every comparable scenario against both the installed Codex CLI
0.141and the stable Codex app's bundled backend0.140.0-alpha.2. Each comparison used three deterministic fixture seeds and seven paired timing repetitions per seed, alternated baseline and candidate ordering between seeds, and required the comparable RPC responses to pass the same correctness checks.Each PR timing below comes from the same paired run as the adjacent installed baseline, so the two PR columns should not be compared with each other.
thread/list, 100 threadsthread/list, 6000 threadsthread/list, filtered 6000thread/list, one parentthread/list, scan and repairthread/resume, coldthread/resume, hot-equivalentthread/resume, exclude turnsWhat each row measures
thread/list, 100 threads: This case lists 50 matching CLI or VS Code threads from a state database containing 100 seeded threads, with a working-directory filter anduseStateDbOnly=true, and validates the exact returned IDs and order.thread/list, 6000 threads: This case lists the first 50 matching CLI or VS Code threads from a 6000-thread state database without a working-directory filter, usinguseStateDbOnly=trueand validating the exact IDs and descending update order.thread/list, filtered 6000: This case lists from the same 6000-thread database with model-provider, source-kind, and working-directory filters applied, then validates that the exact filtered IDs are returned in the expected order.thread/list, one parent: This case requests onlysubAgentThreadSpawnthreads belonging to one selected parent from the 6000-thread database, sorts them by creation time, and validates the exact child set and order.thread/list, scan and repair: This case removes eight expected rows from the 6000-thread state database, callsthread/listwith scan-and-repair enabled, and validates both the returned list and restoration of all eight rows from rollout JSONL metadata.thread/resume, cold: This case starts a fresh app-server over a freshly copied Codex home, resumes a 2400-line rollout with turns included, and validates that the complete reconstructed turn history is returned.thread/resume, hot-equivalent: This case immediately repeats the same full-history resume on the already running app-server after the cold request and validates the same complete turn history.thread/resume, exclude turns: This case starts a fresh app-server over a freshly copied Codex home, resumes a 10000-line rollout withexcludeTurns=true, and validates that the thread is restored without returning any turns.Validation
I ran
just fmt,CARGO_INCREMENTAL=0 just fix -p codex-app-server -p codex-core -p codex-rollout -p codex-state -p codex-thread-store,CARGO_INCREMENTAL=0 just test -p codex-app-server --test all thread_list,CARGO_INCREMENTAL=0 just test -p codex-app-server --test all thread_resume,CARGO_INCREMENTAL=0 just test -p codex-state,CARGO_INCREMENTAL=0 just test -p codex-rollout -p codex-thread-store, andCARGO_INCREMENTAL=0 just test -p codex-core --liblocally.Footnotes
The stable app's older
0.140.0-alpha.2backend does not support the current experimentalparentThreadIdfiltering behavior, so this row cannot be compared correctly against that binary. ↩