Bound external agent session detection work#26291
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: defa48a5e1
ℹ️ 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".
| file_candidates.push((Reverse(modified_at_nanos), path)); | ||
| if file_candidates.len() > SESSION_IMPORT_MAX_COUNT { | ||
| file_candidates.pop(); | ||
| } |
There was a problem hiding this comment.
Backfill candidates after filtering unimportable sessions
When a project has more than 50 recent .jsonl files but the newest files are not importable (for example their cwd has been deleted or summarize_session returns None), this caps the heap before those files are parsed and validated. The later filters then drop them without considering older valid sessions, so detection can return an empty or partial list even though importable recent sessions exist; the previous implementation truncated only after validation.
Useful? React with 👍 / 👎.
| if ledger_changed { | ||
| save_import_ledger(codex_home, &ledger)?; |
There was a problem hiding this comment.
Do not fail detection on opportunistic ledger writes
If codex_home or the ledger file is not writable (for example read-only permissions or a full disk), this opportunistic refresh turns an otherwise successful detection into an Err after migrations has already been built. The write only caches source mtimes for future skip decisions, so a save failure should be ignored or logged; otherwise users can lose the migration prompt even though the session files were readable and importable.
Useful? React with 👍 / 👎.
| let now = now_unix_seconds(); | ||
| let ledger = load_import_ledger(codex_home)?; | ||
| let mut candidates = Vec::new(); | ||
| let mut ledger = load_import_ledger(codex_home)?; |
There was a problem hiding this comment.
Avoid overwriting concurrent ledger imports
When a detection request loads this snapshot and later refreshes an already-imported session, any import that completes in the meantime writes a newer ledger record, but the subsequent save_import_ledger from detection writes this stale snapshot back out and drops that import record. In the app server this can happen because session imports run in a spawned background task while new detect requests are still accepted, so the imported session can be offered/imported again.
Useful? React with 👍 / 👎.
Why
External agent migration detection parsed and hashed every JSONL session file. For users with many large conversations, launching migration could consume substantial CPU and disk resources.
Detection only needs the most recent sessions for the migration UI, so full-content work should be bounded.
What
Validation
RUST_MIN_STACK=8388608 cargo nextest run --no-fail-fast -p codex-external-agent-sessionsThe benchmark used warm filesystem caches and measured the reduction in parsing, hashing, and CPU work.