fix(codex-activity): one-batch start+clear on a Pending turn records the completion (kata pkvz) - #750
Merged
Merged
Conversation
…id cargo commands, whole-crate load check, no escape clause
…ck, reconcile 120s budget from PR #744
…eplaces invalid load check, fix stale base metadata
…the completion (kata pkvz) reconcile_rollout's promotion guard computed effective_clear from the same-batch observed_clear, so a LIVE (Pending) turn whose rollout was drained in one batch (task_started+task_complete together, the load-induced hub drain ordering) had its promotion shadowed by its own clear: accepted_start_at stayed None, the Pending clear branch re-armed via has_queued_submit's unwrap_or(true), and terminal.turn.complete never fired (kata pkvz). Bypass the same-batch clear for a Pending turn whose start belongs to the pending submit (started_at >= pending_submit_at, matching the TS reference codex-activity-tracker.ts:446), so the one-batch live turn promotes-then-clears and records exactly one completion -- matching the already-green separate-batch path. Historical rollouts whose start predates the pending submit keep the same-batch clear in the guard (no false completion), and Idle rollouts keep it unconditionally, preserving reconcile_ignores_an_already_resolved_rollout.
…kata pkvz) Forces the load-induced one-batch path deterministically: writes session_meta + task_started + task_complete ALL AT ONCE before the locator resolves, so CodexAttach's initial drain reads all three lines in one reconcile_rollout call. Uses far-future timestamps so task_started > queued_submit_at (matching the original flake's timeline where task_started is appended after both Enters). Fails before the Task 1 fix (same-batch clear shadows the promotion, 120s timeout); passes after (5s). No load dependence — the root-cause end-to-end proof.
…delta review r1) Adding a second test to the binary invalidated the single-sequential-test assumption: both tests mutate CODEX_HOME/CODEX_ARGV_CAPTURE_PATH and share the PID-only fake-script path. Add ENV_LOCK (the repo's convention, e.g. codex_fork_rebind.rs) to serialize them, preventing env races and ETXTBSY under concurrent libtest execution. Also trims the production comment to a short note per the plan's constraint.
…ta review r2) Replace the blind 3s sleep with a wait_for_frame for codex.activity.updated phase=pending (proves note_possible_submit completed the re-snapshot AND note_input set Pending), then a 2.2s sleep for the 2s window resolution. This eliminates the load-sensitive race where the server hadn't processed the first Enter before the rollout was written (which would permanently exclude it from the re-snapshot).
…tep + R3 evidence (delta review r3) - rustfmt: format the production conditional, the historical-start fixture, and the pending-frame predicate. - Test: increase the post-pending sleep to 3s (matching the existing test's proven 850ms margin over the 2.15s worst-case window resolution) and document why it's sufficient in isolation. - Plan: the RED step now uses git stash to temporarily revert the fix (verified during execution: 125s timeout without the fix). - Plan: R3 clarifies that the deterministic one-batch test is a STRONGER proof than a probabilistic loaded run.
The integration test can't deterministically reproduce the one-batch suppression (the queued-submit race is not deterministically controllable from the WS client). Reframe it as a non-regression guard: writes session_meta + task_started + task_complete within the first 2s window (after the pending frame proves the re-snapshot completed), and the 150ms sweep finds it. The deterministic RED/GREEN evidence is in the three unit tests (which set queued_submit_at directly). Removes the second Enter, the 3s sleep, and the unreachable RED step from the plan. 7.5s vs 10.5s.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Kata pkvz:
fresh_pane_locator_identity_reaches_activity_and_turn_completeflaked under whole-workspacecargo testload. The codex activity tracker'sreconcile_rolloutpromotion guard computedeffective_clear = max_ts(observed_clear, last_cleared_at). When the rollout'stask_started+task_completewere read in one batch (the load-induced hub drain ordering), the same-batch clear shadowed the start promotion —accepted_start_atstayedNone, the Pending clear branch re-armed viahas_queued_submit'sunwrap_or(true), andterminal.turn.completenever fired.Fix
For a LIVE (Pending) turn whose start belongs to the pending submit (
started_at >= pending_submit_at, matching the TS referencecodex-activity-tracker.ts:446), bypass the same-batch clear in the promotion guard — the start-then-clear is the pending submit's complete turn cycle, not a stale echo. Historical rollouts (start before the pending submit) and Idle rollouts keep the same-batch clear, preservingreconcile_ignores_an_already_resolved_rollout.crates/freshell-activity/src/codex.rs:389Tests
freshell-activity/src/codex.rs(deterministic RED→GREEN):reconcile_one_batch_start_and_clear_on_a_pending_turn_with_newer_start_completes— the suppression (RED without fix, GREEN with fix)reconcile_one_batch_start_and_clear_on_a_pending_turn_with_older_start_rearms— re-arm parity (no over-ring)reconcile_one_batch_historical_start_before_pending_submit_records_nothing— regression guard for historical rolloutsfreshell-ws/tests/codex_locator_activity.rs(non-regression guard):fresh_pane_locator_one_batch_drain_records_turn_complete— proves the one-batch drain path works end-to-end through the real server, socket, PTY, inotify watcher, and activity hubcargo fmt --checkclean,cargo clippycleanNotes
wait_for_framebudget 30s→120s (symptom suppression); this PR fixes the root cause.server/coding-cli/codex-activity-tracker.ts) has the same suppression — recorded as an out-of-scope finding for a separate PR.