Fix filtered thread-list resume regression in TUI#19591
Conversation
etraut-openai
changed the title
etraut-openai
force-pushed
the
etraut/resume-perf-regression
branch
from
1536c25 to
319d4e1
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: 1536c25e63
ℹ️ 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".
etraut-openai
force-pushed
the
etraut/resume-perf-regression
branch
from
319d4e1 to
905e1d0
Compare
etraut-openai
changed the title
etraut-openai
force-pushed
the
etraut/resume-perf-regression
branch
from
905e1d0 to
6fecc78
Compare
etraut-openai
force-pushed
the
etraut/resume-perf-regression
branch
from
6fecc78 to
4075c53
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4075c5390a
ℹ️ 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".
| if search_term.is_some() { | ||
| state_db::reconcile_rollout( | ||
| state_db_ctx.as_deref(), | ||
| item.path.as_path(), |
There was a problem hiding this comment.
When search_term is None, filtered listings (source/provider/cwd) now call read_repair_rollout_path for filesystem hits. That helper does not refresh full metadata fields like model_provider/source from JSONL. If SQLite has stale filter fields for a thread present in fs_page, the later DB loop never fixes it (it only reconciles DB-only hits), so subsequent state-db-only filtered listings can keep missing valid threads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes, this is a small but legitimate hole, but it existed prior to #18502. The only reason that #18502 "fixed" this hole is because it forced us to go through an expensive remediation path every time.
If we decide that this is a hole we want to fix (I'd look to @jif-oai for advice here), then I think we should do it as a follow-up PR.
etraut-openai
changed the title
|
Glad that a fix is coming, after switching to 0.124.0, discovered that thread-list was extremely slow. |
| if search_term.is_some() { | ||
| state_db::reconcile_rollout( | ||
| state_db_ctx.as_deref(), | ||
| item.path.as_path(), |
3 participants
Why
codex resumeregressed after #18502 changed the defaultthread/listscan-and-repair path for metadata-filtered listings. The TUI resume picker usesthread/listwith source/provider/cwd filters anduseStateDbOnly: false, which is the intended correctness-preserving mode: it should still consult the filesystem so healthy, missing, or stale SQLite state can be repaired.The regression was that #18502 made that filtered, filesystem-backed path call
reconcile_rolloutfor every filesystem hit, and then call it again for each SQLite hit. Whenreconcile_rolloutdoes not already have extracted rollout items, it falls back to loading the full JSONL rollout. That changed the resume picker’s first page from a cheap rollout-head scan plus SQLite read-repair into full-file reads for large sessions, so a few long threads could dominate TUI startup/resume latency.This change addresses the regression by keeping
useStateDbOnly: falseon the correctness-preserving path while avoiding unnecessary full JSONL reads for rows the filesystem scan has already validated. Source/provider/cwd filters can be decided from rollout-head metadata, so non-search resume listings only need the lightweight read-repair path for filesystem hits. Full reconciliation is still used for DB-only filtered rows because those can be stale false positives, and for search listings because search can depend on title metadata that may require scanning the full rollout.This fixes #19483.
What changed
read_repair_rollout_pathpath instead of fullreconcile_rollout.Verification
cargo test -p codex-rollout list_threadscargo test -p codex-app-server thread_list