fix(testmon): run resumable shards with bounded argv - #3960
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughSeed-shard execution now writes node IDs to per-shard response files. The generated pytest command uses capped managed xdist workers, work stealing, and Testmon no-selection mode. Unit tests validate the updated command. ChangesSeed-shard execution
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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 `@tests/unit/devtools/test_verify.py`:
- Around line 341-349: Update the test around _seed_shard_command to mock
adaptive_pytest_worker_count with a value greater than ten, then assert the
generated -n argument is exactly "10" rather than merely not "0". Preserve the
existing command and nodeid assertions.
🪄 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: ff3c1cca-f902-499c-8c35-5c5674c114ca
📒 Files selected for processing (2)
devtools/verify.pytests/unit/devtools/test_verify.py
|
The initial Circle failure was the missing structured PR-scope carrier, before pytest ran. I have added the v2 carrier to the PR body; the code head remains unchanged at 6042be0. |
![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: 6042be0d7f
ℹ️ 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".
| # has an xdist-aware controller database; retaining the managed worker pool | ||
| # here avoids turning a 20k-node seed into hours of serial fixture setup. | ||
| command.extend( | ||
| ["--dist=worksteal", *_pytest_worker_args(maximum=10), "--testmon", "--testmon-noselect", f"@{nodeids_file}"] |
There was a problem hiding this comment.
When devtools verify --seed-testmon uses the normal adaptive worker count, this forces every shard through xdist even though the seed's base_marker still includes load_sensitive and tui tests, such as tests/unit/cli/test_interactive_cli.py:23 and tests/unit/ui/test_tui.py:25. The full-suite path in this same builder explicitly runs those markers serially because worker contention flakes their timing, socket, and TUI behavior (devtools/verify.py:2013-2040), so applying parallelism here makes seed completion unreliable on precisely the hosts where the adaptive count exceeds one. Exclude these markers from parallel shards and execute them in a separate serial shard.
Useful? React with 👍 / 👎.
| # has an xdist-aware controller database; retaining the managed worker pool | ||
| # here avoids turning a 20k-node seed into hours of serial fixture setup. | ||
| command.extend( | ||
| ["--dist=worksteal", *_pytest_worker_args(maximum=10), "--testmon", "--testmon-noselect", f"@{nodeids_file}"] |
There was a problem hiding this comment.
When adaptive worker selection fails after the serial collection—for example because available memory has fallen below the managed minimum or POLYLOGUE_PYTEST_WORKERS is invalid—_pytest_worker_args() raises PytestResourceError here before _run() is entered. The existing handler around build_verify_steps() cannot catch this late call, and _run()'s managed refusal path is never reached, so the seed exits with a traceback and bypasses _finalize_testmon_seed_attempt, leaving its attempt ledger marked running instead of recording the intended exit-125 diagnosis. Build the shard command inside a matching handler or move worker selection into _run()'s protected preflight.
AGENTS.md reference: AGENTS.md:L521-L524
Useful? React with 👍 / 👎.
| # has an xdist-aware controller database; retaining the managed worker pool | ||
| # here avoids turning a 20k-node seed into hours of serial fixture setup. | ||
| command.extend( | ||
| ["--dist=worksteal", *_pytest_worker_args(maximum=10), "--testmon", "--testmon-noselect", f"@{nodeids_file}"] |
There was a problem hiding this comment.
When a seed shard contains tests marked with xdist_group, --dist=worksteal does not co-locate those groups; that behavior requires xdist's loadgroup scheduler. This affects the unfiltered seed corpus directly: tests/unit/daemon/test_web_reader.py:16-28 and tests/unit/archive/query/test_continuation_surface_parity.py:16-46 deliberately share the web-reader group because their real HTTP servers must not be split across workers. The new scheduler can therefore reintroduce the port/event-loop interference those markers prevent even after the separately noted load_sensitive tests are isolated. Use a group-aware distribution mode or execute grouped nodes in an isolated shard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50f102e738
ℹ️ 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".
| nodeids_file.parent.mkdir(parents=True, exist_ok=True) | ||
| nodeids_file.write_text("\n".join(nodeids) + "\n", encoding="utf-8") |
There was a problem hiding this comment.
When the checkout filesystem fills or becomes unwritable between collection and shard execution, the newly added write_text raises OSError before _run() performs its managed resource preflight. The caller catches only PytestResourceError, so this path exits with a traceback and leaves the durable seed-attempt ledger marked running rather than checkpointing/finalizing it with the intended resource diagnosis; handle response-file I/O failures through the same exit-125 path.
AGENTS.md reference: AGENTS.md:L521-L524
Useful? React with 👍 / 👎.
| else: | ||
| command.extend( | ||
| [ | ||
| "--dist=loadgroup", |
There was a problem hiding this comment.
When a shard contains any existing xdist_group test, --dist=loadgroup changes its collected ID: pytest-xdist's hook explicitly adds the group name as a node-ID suffix (v3.8.0 source). The serial collection stored unsuffixed expected IDs, but the progress plugin's try-last collection hook and test reports record IDs such as ...@web-reader; _checkpoint_testmon_seed_shard therefore fails selected == nodeids and cannot match terminal events, leaving the shard incomplete even though its tests passed. Preserve or normalize canonical node IDs before validating the shard.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| observed.update(nodeids) | ||
| if observed != set(expected): |
There was a problem hiding this comment.
When an interrupted or malformed attempt repeats a node in two otherwise valid shard records, updating a set hides the overlap, so this validator accepts a ledger that violates its stated disjoint-shard contract. Resume/finalization can then process the node twice, and _seed_shard_outcomes silently uses the later result; a passing duplicate can replace an earlier failure. Reject a node already present in observed, or additionally require the total observed count to equal the expected count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4297fc1dc
ℹ️ 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".
| else: | ||
| command.extend( | ||
| [ | ||
| "--dist=loadgroup", |
There was a problem hiding this comment.
When a parallel shard contains an xdist_group test, --dist=loadgroup appends @<group> to that item's node ID during collection, as confirmed by pytest-xdist 3.8's worker collection hook. The progress plugin consequently records the suffixed ID in selection.json, but _checkpoint_testmon_seed_shard still requires selected == nodeids, so every such shard is marked incomplete even when all tests pass. Fresh evidence in this revision is that _canonical_seed_nodeid normalizes only event records; the selection ledger remains unnormalized. Normalize the selected IDs before the exact comparison as well.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92ca98f24e
ℹ️ 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".
| else: | ||
| command.extend( | ||
| [ | ||
| "--dist=loadgroup", |
There was a problem hiding this comment.
With the default adaptive worker count above zero, --dist=loadgroup changes grouped item IDs to names such as ...@web-reader, and pytest-testmon records those IDs as test_execution.test_name; however, _testmon_database_state() passes the original unsuffixed shard IDs to inspect_testmon_database(), which requires exact names and therefore reports the grouped tests as missing. Since the seed corpus includes grouped tests in tests/unit/daemon/test_web_reader.py and tests/unit/archive/query/test_continuation_surface_parity.py, an otherwise passing seed cannot produce a complete reusable graph; canonicalize the stored execution names as well as the selection/event artifacts.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1da9c07b2d
ℹ️ 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".
| prepared_seed_attempt = _checkpoint_testmon_seed_shard( | ||
| prepared=prepared_seed_attempt, | ||
| shard_index=shard_index, | ||
| step=resource_failure_result, | ||
| ) |
There was a problem hiding this comment.
When response-file creation raises OSError because the checkout filesystem is full, this recovery path immediately calls _checkpoint_testmon_seed_shard, whose _atomic_write_json performs another unguarded temporary-file write on the same filesystem. That write will also fail under the motivating ENOSPC scenario, so the seed still exits with a traceback and leaves its attempt ledger marked running instead of producing the intended managed exit 125. Fresh evidence after the earlier comment is that the new catch handles the first write but not this recovery write; guard the checkpoint/finalization path or arrange the durable state transition before allocating the response file.
AGENTS.md reference: AGENTS.md:L521-L524
Useful? React with 👍 / 👎.
## 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-bc4afc5f` timed 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 - classify shard results at the seed-loop boundary; - continue only for a structured ordinary pytest red result (`exit 1`, `pytest_failed`); - checkpoint and stop before another shard after timeouts, resource refusals, worker/internal errors, usage errors, or unclassified failures; - cover both mutations: removing the stop re-admits work after timeout, while an unconditional stop loses useful dependency evidence after red tests. ## 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 head `6aa95bea4`; strict mypy passed over 2,735 files. - Real-route diagnostic: `devtools test` over 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 is `ReadOnlyArchiveError: read-only archive evidence cannot mutate user.db`; it is retained unchanged. - Default `devtools verify` — refused before tests because this linked worktree has no completed testmon seed. No replacement multi-hour seed was started while PR #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 | Scope | Disposition | Evidence | | --- | --- | --- | | Self-contained harness fix | Satisfied | Commits `85e0b3512`, `1c4959e76`, `d11742db0`, `6aa95bea4`; focused and module verification above | | Beads | Unchanged | No assigned or mutated Beads | <!-- polylogue-pr-scope:v2 { "assigned_beads": [], "dispositions": [], "mutated_beads": [], "scope_digest": "79a7984a9ec80a157c96dc8d28561159c642c15de3793837eff751fa7eac34a1", "scope_kind": "self_contained", "version": 2 } -->
1 participant
Summary
Make resumable testmon seed shards executable at the current 4,096-node shard size and retain managed parallel execution.
Problem
The post-merge seed reached shard 1 and spent 2,700 seconds executing 3,860 nodes before the supervisor timeout. Shard 2 then failed before pytest started with
OSError: [Errno 7] Argument list too longbecause every node ID was appended to thesystemd-runargv. Serial shard execution also turns the approximately 20,000-node seed into hours of fixture work.Solution
Verification
devtools test tests/unit/devtools/test_verify.py— 150 passed;ruff format --check devtools/verify.py tests/unit/devtools/test_verify.py— clean;ruff check devtools/verify.py tests/unit/devtools/test_verify.py— clean;The existing red nodes are retained as baseline evidence; this change does not silence or reclassify them.