fix(testmon): stop seed admission after harness failures - #3961
Conversation
📝 WalkthroughWalkthroughSeed testmon execution now continues after ordinary ChangesSeed shard failure handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SeedShardExecution
participant FailureClassifier
participant ShardStatusAndReceipt
SeedShardExecution->>FailureClassifier: classify shard exit code and diagnosis
FailureClassifier-->>SeedShardExecution: continue after pytest_failed or stop
SeedShardExecution->>ShardStatusAndReceipt: finalize shard and receipt states
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![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
Lines 3450 to 3454 in 1c4959e
Return the infrastructure failure after an earlier red shard
When an ordinary failing-test shard runs first and a later shard times out or is resource-refused, this branch stops admission but leaves exit_code at the earlier value 1 because it only records the fatal shard's code when the accumulator is zero. The command and top-level verification receipt therefore report an ordinary test failure instead of the infrastructure-fatal 124/125 result that actually terminated the seed, preventing callers from distinguishing a retryable harness failure from a red baseline. A stop-requiring shard should replace the accumulated exit code even if an earlier shard was red.
ℹ️ 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 exit_code == 0: | ||
| return False | ||
| return not (exit_code == 1 and step.get("diagnosis") == "pytest_failed") |
There was a problem hiding this comment.
When xdist aborts a shard after exhausting crashed-worker restarts, it can return exit 1, but _run() obtains its diagnosis from classify_pytest_result(), which labels every positive non-terminated result pytest_failed; it never produces the xdist_worker_crash diagnosis used elsewhere in the seed code. Consequently this condition mistakes that infrastructure failure for an ordinary red test and starts the next expensive shard, contrary to the new admission policy. The decision needs evidence that the shard completed normally, rather than relying solely on exit 1 plus this generic diagnosis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6aa95be. The continue decision now requires the checkpointed shard ledger to be complete in addition to exit 1 and pytest_failed, so an xdist crash with missing/worker_crash outcomes stops admission. The incomplete-generic-exit-1 regression is included in the 5 focused cases.
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devtools/verify.py`:
- Around line 3458-3464: The shard aggregation logic around
_seed_shard_failure_requires_stop must retain the terminal shard diagnosis
whenever a later infrastructure failure replaces exit_code, so the
pytest_failed-then-pytest_timeout scenario reports pytest_timeout in JSON
history and VerifyRun. Update devtools/verify.py lines 3458-3464 accordingly,
and add or adjust the assertion in tests/unit/devtools/test_verify.py lines
3178-3285 to verify the JSON diagnosis is pytest_timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94a691ae-e198-4ce8-b5ae-eda080e90d4d
📒 Files selected for processing (2)
devtools/verify.pytests/unit/devtools/test_verify.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11742db04
ℹ️ 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".
| exit_code = step.get("exit") | ||
| if exit_code == 0: | ||
| return False | ||
| return not (exit_code == 1 and step.get("diagnosis") == "pytest_failed") |
There was a problem hiding this comment.
When a shard subprocess exits 0 but its selection or progress artifacts are missing/incomplete, _checkpoint_testmon_seed_shard() records the shard as incomplete, yet this early return treats it as healthy and the loop admits the next expensive shard. This can occur if the pytest progress plugin fails to produce a complete selection.json or terminal events even though pytest itself exits successfully; base the admission decision on the checkpointed shard status as well as the process exit.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Stop resumable testmon seed admission after an infrastructure-fatal shard while preserving continued dependency capture after ordinary red tests.
Problem
The failed seed run at
/realm/worktrees/polylogue-testmon-final/.cache/verify/runs/20260812T114200Z-seed-testmon-55347-bc4afc5ftimed out shard 1 after 2,700.84 seconds, then immediately attempted shard 2. At timeout, the shard had completed 3,860/4,096 nodes, peaked at 2,568,784 KiB process-tree RSS and 2,221,538 KiB PSS, used 571,952 KiB swap PSS, and issued 27,789,180,928 bytes of writes while the basetemp peaked at only 97,570 KiB. Admitting more work after that bounded resource stop produced no trustworthy release baseline and obscured the original terminal condition.Ordinary assertion failures are different: their testmon rows remain useful, and later shards should continue to populate the resumable graph.
PR #3960 owns bounded response-file argv and managed worker execution. This change is intentionally disjoint: it controls admission after a shard has already returned.
Solution
exit 1,pytest_failed);Verification
.venv/bin/python -m devtools test tests/unit/devtools/test_verify.py -k 'stops_only_after_infrastructure_failed_shard or seed_resource_timeout_has_a_distinct_typed_terminal_outcome'— 5 passed, 149 deselected in 1.81s..venv/bin/python -m devtools test tests/unit/devtools/test_verify.py— 154 passed in 19.93s.PATH="$PWD/.venv/bin:$PATH" .venv/bin/python -m devtools verify --quick— all 25 steps passed in 196.05s at exact head6aa95bea4; strict mypy passed over 2,735 files.devtools testover the oversized annotation-batch API node and lexical archive-query node — 1 passed, 1 pre-existing failure in 7.94s. The query call took 6.2385s versus 28.4543s in the serial testmon shard. The existing API failure isReadOnlyArchiveError: read-only archive evidence cannot mutate user.db; it is retained unchanged.devtools verify— refused before tests because this linked worktree has no completed testmon seed. No replacement multi-hour seed was started while PR fix(testmon): run resumable shards with bounded argv #3960 owns that repair.Residual baseline
The supplied seed artifact is not green: it includes existing API/reindex/convergence failures and terminated before completing the 20,965-node corpus. This PR neither excludes those nodes nor grants release authority.
Bead disposition
85e0b3512,1c4959e76,d11742db0,6aa95bea4; focused and module verification above