fix(codex): detect resumed Windows Codex sessions stored under a previous date (#153) - #154
Conversation
…te (graykode#153) Closes graykode#153. The CodexCollector::map_pid_to_jsonl Windows fallback scanned only today's ~/.codex/sessions/YYYY/MM/DD/ directory for rollout-*.jsonl candidates, so a Codex CLI session resumed on a later date — whose rollout file stays under the ORIGINAL date dir — was invisible and its PID left unmapped. Replace the today-only read_dir with a recursive scan of ~/.codex/sessions/ via a new #[cfg(windows)] collect_all_rollouts_with_mtime (walks all YYYY/MM/DD/ subdirectories). The existing strategy is preserved unchanged: candidates sorted by mtime descending, most-recent assigned to the first discovered Codex PID. Linux (/proc/{pid}/fd) and macOS (lsof) arms are untouched — the change is fully #[cfg(target_os="windows")]-gated. Same root cause + fix direction the reporter (@Astiyac) identified and locally validated with a built Windows abtop.exe. Verified: cargo clippy --target x86_64-pc-windows-gnu -- -D warnings clean (also clean on -msvc and aarch64-apple-darwin); cargo test --target x86_64-pc-windows-gnu --no-run links the new #[cfg(windows)] regression test cleanly. CI is ubuntu-only so the windows branch isn't exercised there (the reporter's built-exe validation is the exec confirmation).
graykode
left a comment
There was a problem hiding this comment.
Thanks for addressing #153. The root cause is valid, but I do not think the current implementation is safe to merge yet.
-
The Windows fallback now walks and stats the entire lifetime session archive synchronously. map_pid_to_jsonl is called from collect_sessions on every two-second TUI tick, including before the initial render. The cost therefore grows monotonically with session history and can block the UI on large, synced, or network-backed profiles. Please move discovery behind a cached or asynchronous scanner, refresh it only on meaningful events or a slow interval, validate the expected YYYY/MM/DD directory shape, and enforce a depth bound.
-
The expanded candidate set does not establish ownership. Every historical rollout competes purely by mtime, while the PID list originates from unsorted HashMap iteration. Multiple processes, or a Codex process without a current rollout, can therefore be paired with the wrong transcript. Jump and kill actions then use that assigned PID, and the kill guard only verifies that it is a Codex process rather than the owner of the displayed session. Please preserve a PID-to-rollout mapping once a fresh write is observed and validate initial matches with additional process or session evidence instead of filling unmatched PIDs from arbitrary archive entries.
-
Both the changed production path and the new regression test are gated to Windows, while repository CI runs only on Ubuntu. The green check therefore compiles and tests neither this implementation nor its test. Please add a windows-latest job running at least cargo test and cargo clippy with warnings denied, or extract the selection logic into platform-neutral helpers that CI exercises in addition to Windows compilation.
The local macOS test suite and clippy pass, and I did not find a direct command-injection, privilege-escalation, or data-exfiltration path. The blocking concerns are availability, PID-to-session integrity, and missing coverage of the changed platform-specific code.
Addresses review point 3: the changed Windows code path (collect_all_rollouts_with_mtime, map_pid_to_jsonl Windows branch, #[cfg(windows)] tests) was never compiled or tested by CI since the only job ran on ubuntu-latest and sysinfo is a windows-only dep. Now CI runs clippy+test+build on both ubuntu and windows, with fail-fast:false so the windows result is visible.
|
Thanks for the thorough review — all three points are valid. Here's my plan: 3. CI coverage — addressed in 36aad35: added a windows-latest job to the CI matrix (with fail-fast: false so the Windows result is visible). The workflow is currently action_required since it's a fork PR — needs your approval to run, then the #[cfg(windows)] tests and the Windows code path will actually be compiled/tested on CI. 1. Availability — the synchronous full-archive scan in collect_all_rollouts_with_mtime (codex.rs:364-392) being called every 2s tick from map_pid_to_jsonl (codex.rs:133) is the real issue. I'll move discovery behind a background scanner with a slow rescan interval + depth bound, reusing the existing DesktopRecentRolloutScanner pattern (codex.rs:47-106, already does thread + mpsc + 60s rescan for the Desktop path). The CLI path will consume a cached snapshot instead of re-walking the tree each tick. 2. PID-to-session ownership — splitting into two parts: Ship first (stopgap): make PID ordering deterministic (pids.sort_unstable_by_key in find_codex_pids_from_shared), replace the kill -9 / ps guard (app.rs:724-739, currently unix-only and silently broken on Windows) with taskkill /F + a sysinfo-based check, and make jump explicitly NoOp on Windows (currently the jumpers are all unix terminal multiplexers, jump/mod.rs:61-65). Need your input (root fix): real fd ownership. On Windows, the options I see are:
The current mtime-position pairing (codex.rs:834-838) is exactly the wrong heuristic you flagged — it can pair an unmatched PID with an arbitrary archive rollout. Do you have a preferred direction for real ownership on Windows, or should I prototype sysinfo::Process::open_files first and see what coverage it gives? I'll start on the availability scanner + kill/jump platform fixes next; the real-ownership design will follow your steer. |
…t 1) map_pid_to_jsonl called collect_all_rollouts_with_mtime every 2s TUI tick, synchronously walking the whole session archive — cost grows with history and blocks the UI on large / synced / network-backed profiles. Changes: - Add WindowsRolloutScanner (mirrors DesktopRecentRolloutScanner: mpsc + thread + 60s rescan interval + in_flight guard). collect_sessions consumes its cached snapshot instead of re-walking each tick. - map_pid_to_jsonl takes Option<&[(path, mtime)]>; uses cache when supplied, falls back to synchronous walk when None (tests). - collect_all_rollouts_with_mtime gains depth bound (max 4) and only descends into numeric-named dirs (YYYY/MM/DD shape) to avoid walking unrelated nested trees.
Stopgap for review point 2 (PID-to-session ownership); real fd ownership is the root fix still pending design input. - find_codex_pids_from_shared: sort by PID before returning. process_info is a HashMap so without this the PID list — and thus the Windows mtime-position pairing — varied run to run. - kill_selected: add Windows branch — taskkill /F /PID instead of the unix-only kill -9 (which silently failed), and a sysinfo-based verify_killable_agent_windows guard instead of ps -p (which doesn't exist on Windows). Unix path unchanged. - jump::jumpers: Windows returns an empty registry so resolve returns NoOp up front, rather than shelling out to cmux/tmux/ps which don't exist on Windows. Linux still gets cmux+tmux, macOS all three. Note: the kill guard still only verifies 'PID is a known agent process', not 'PID owns the displayed session' — that ownership check is the root fix (needs real fd lookup, pending direction from maintainer).
|
Update on the three points: 3. CI coverage — done in 36aad35. The matrix now runs clippy+test+build on both 1. Availability — done in 3c2dd47. Added a 2. PID→session ownership — partial, split as discussed:
The stopgap notably does NOT address the "don't fill unmatched PIDs from arbitrary archive entries" part of your point 2 — that's coupled to real fd ownership (the mtime pairing only exists because there's no fd truth), so it lands with the root fix. |
Summary
Closes #153. On Windows,
abtopcould miss an active Codex CLI session that was resumed on a later date than the one it was created on, because the Codex collector's Windows fallback scanned only today's~/.codex/sessions/YYYY/MM/DD/directory.A resumed Codex session keeps writing the rollout file created on the original date — e.g. a session started 2026/07/07 and resumed on 2026/07/08 still appends to
~/.codex/sessions/2026/07/07/rollout-*.jsonl. The Windows fallback scanned~/.codex/sessions/2026/07/08/and never saw it.Root cause
CodexCollector::map_pid_to_jsonlinsrc/collector/codex.rshas a#[cfg(target_os = "windows")]arm (Windows has nolsof//proc/{pid}/fd). It usedSelf::today_session_dir(sessions_dir)to enumerate candidaterollout-*.jsonlfiles — i.e. only~/.codex/sessions/<today>/*.jsonl. A resumed session's rollout file lives under a previous date directory, so it was invisible to the fallback and the PID was left unmapped.This is the same root cause and fix direction the issue reporter (@Astiyac) identified and locally validated; this PR implements it.
Fix
Scan
~/.codex/sessions/recursively forrollout-*.jsonlacross all datedYYYY/MM/DD/subdirectories, instead of only today's. The existing strategy is preserved unchanged:#[cfg(target_os = "linux")](/proc/{pid}/fd) and macOS (lsof) arms are untouchedImplementation: a new
#[cfg(target_os = "windows")] fn collect_all_rollouts_with_mtimerecurses the sessions tree collecting(PathBuf, SystemTime)pairs; the Windows arm ofmap_pid_to_jsonlnow calls it instead oftoday_session_dir+read_dir.Why this is safe
#[cfg(target_os = "windows")]-gated. The non-Windows arms compile to the same code as before.collect_recent_desktop_rollouts), so this mirrors an established approach.Validation
cargo clippy --target x86_64-pc-windows-gnu -- -D warnings— clean.cargo clippy --target x86_64-pc-windows-msvc -- -D warnings— clean.cargo test --target x86_64-pc-windows-gnu --no-run— the test binary compiles and links cleanly (this type-checks the new#[cfg(windows)]function and the#[cfg(windows)]regression test fully; a reference error or signature mismatch would fail to link).windows_map_pid_to_jsonl_finds_resumed_session_under_previous_date_dir(#[cfg(target_os = "windows")]) builds two dated dirs (today + a previous date), writes a rollout in each with the previous-date one more recently modified, and asserts the Windows fallback maps the resumed (previous-date) rollout to the first PID — i.e. the Windows: resumed Codex CLI sessions from previous dates may not be detected #153 scenario now resolves.