Prove six-worker harness stall diagnosis - #3942
Conversation
📝 WalkthroughWalkthroughThe change adds regression coverage for six pytest-xdist workers in uninterruptible Changesxdist stall follow-up
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
![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/fixtures/reindex_incident_coverage/campaign_graph.json`:
- Line 4: Update the source_snapshot_sha256 value in campaign_graph.json to
4952994decfd523ce773249e90c4e6e9f54b1597f462552e0292facc732190b1, preserving the
surrounding fixture content.
🪄 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: 90c6b0d2-9b49-45df-a73e-bf8450b34ef4
📒 Files selected for processing (3)
.beads/issues.jsonltests/fixtures/reindex_incident_coverage/campaign_graph.jsontests/unit/devtools/test_verify.py
| "schema_version": 1, | ||
| "source_commit": "12618c007a987b227e73cfc9e4f462dcaa669914", | ||
| "source_snapshot_sha256": "767d60ce405c65d6fa19a8302c6a6920f054ca8be6adbfaca1d0c88444b9b424", | ||
| "source_snapshot_sha256": "2ff286443fd3f2b4b08e16c22d39162b68ca601752bab3ce57290d4bc37abf99", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
graph='tests/fixtures/reindex_incident_coverage/campaign_graph.json'
commit="$(jq -r '.source_commit' "$graph")"
source_path="$(jq -r '.source_path' "$graph")"
expected="$(jq -r '.source_snapshot_sha256' "$graph")"
actual="$(git show "${commit}:${source_path}" | sha256sum | cut -d' ' -f1)"
printf 'expected=%s\nactual=%s\n' "$expected" "$actual"
test "$actual" = "$expected"Repository: Sinity/polylogue
Length of output: 300
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
graph='tests/fixtures/reindex_incident_coverage/campaign_graph.json'
commit="$(jq -r '.source_commit' "$graph")"
source_path="$(jq -r '.source_path' "$graph")"
expected="$(jq -r '.source_snapshot_sha256' "$graph")"
printf 'commit=%s\nsource_path=%s\nexpected=%s\n' "$commit" "$source_path" "$expected"
git cat-file -e "${commit}^{commit}"
git cat-file -e "${commit}:${source_path}"
git show "${commit}:${source_path}" > /tmp/source_snapshot.bin
actual="$(sha256sum /tmp/source_snapshot.bin | cut -d' ' -f1)"
printf 'actual=%s\n' "$actual"
printf '\nCurrent fixture fields:\n'
sed -n '1,8p' "$graph"
printf '\nSource object metadata:\n'
git cat-file -s "${commit}:${source_path}"
file /tmp/source_snapshot.bin
test "$actual" = "$expected"Repository: Sinity/polylogue
Length of output: 779
Update source_snapshot_sha256.
The exact bytes of .beads/issues.jsonl at source_commit hash to 4952994decfd523ce773249e90c4e6e9f54b1597f462552e0292facc732190b1, not the recorded value. Set line 4 to this hash.
🤖 Prompt for 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.
In `@tests/fixtures/reindex_incident_coverage/campaign_graph.json` at line 4,
Update the source_snapshot_sha256 value in campaign_graph.json to
4952994decfd523ce773249e90c4e6e9f54b1597f462552e0292facc732190b1, preserving the
surrounding fixture content.
![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: ee93e8d248
ℹ️ 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".
| assert sample["xdist_worker_count"] == 6 | ||
| assert sample["xdist_uninterruptible_count"] == 6 | ||
| reason = xdist_uninterruptible_stall_reason(sample, started_at=10.0, now=40.1, timeout_s=30.0) |
There was a problem hiding this comment.
When one of the six session_started receipts is missing or unreadable, ResourceSampler observes five workers, reports five uninterruptible workers, and sets all_xdist_workers_uninterruptible=True; the stall helper can then terminate the run even if the unobserved sixth worker is still progressing. This fixture creates all six receipts, while the existing partial test manually supplies a false flag, so neither exercises that partial-observation failure and the commit closes AC4 prematurely. Pass the configured xdist worker count into the decision and add a five-of-six sampler/termination regression before closing the bead.
AGENTS.md reference: AGENTS.md:L480-L482
Useful? React with 👍 / 👎.
|
|
||
| assert sample["xdist_worker_count"] == 6 | ||
| assert sample["xdist_uninterruptible_count"] == 6 | ||
| reason = xdist_uninterruptible_stall_reason(sample, started_at=10.0, now=40.1, timeout_s=30.0) |
There was a problem hiding this comment.
If _run stops sampling D-state workers or fails to pass this reason to _request_supervisor_termination, this test still passes because it invokes the pure classifier directly and never exercises the monitor loop, subprocess containment, or terminal receipt. Removing the production D-state branch would therefore leave this claimed proof green, so it does not establish the typed owned-process-group termination named in the bead and its close reason. Add an _run-level controlled regression that asserts the return code, typed termination reason, containment signals, and terminated receipt, or leave that acceptance scope open.
AGENTS.md reference: AGENTS.md:L477-L482
Useful? React with 👍 / 👎.
| env={ | ||
| "POLYLOGUE_PYTEST_BASETEMP_ROOT": str(tmp_path), | ||
| "POLYLOGUE_PYTEST_EVENTS_DIR": str(events), |
There was a problem hiding this comment.
In a real _run(..., run=VerifyRun(...)), env_for_pytest_step directs the plugin to the step-specific artifacts.events_dir, but _read_latest_pytest_event() monitors only the global current-events directory/path, which are copied or merged after the subprocess finishes. This test passes that per-run directory directly to ResourceSampler, so it misses that the live supervisor never observes worker test-progress events: with controller output flowing, seen_any_progress_event remains false and the progress-stall detector cannot fire, while the D-state timer cannot be checked against actual progress age. Thread the active artifacts directory into the progress reader and cover the real route before closing the acceptance scope.
AGENTS.md reference: AGENTS.md:L480-L482
Useful? React with 👍 / 👎.
1 participant
Summary
Problem
The resource harness had in-process worker identity support and typed all-worker stall logic, but lacked a controlled six-worker regression proving that post-exec worker identities are sufficient for the terminal diagnosis. The parent Bead was therefore kept open after exact-head review rather than being closed on the earlier implementation evidence.
Solution
Add a controlled six-worker sampler fixture with six
session_startedworker receipts and D-state process observations. The fixture proves complete worker observation, typed stall diagnosis only after the configured interval, and compatibility with the existing partial-observation fail-closed behavior. The Beads export closes the parent and its three named residual successors; this is harness implementation/verification scope only.Verification
devtools test tests/unit/devtools/test_verify.py -k 'six_worker_d_state_fixture or resource_sampler_resolves_worker_identity'— 2 passeddevtools verify --quick— 25/25 steps passedSummary by CodeRabbit
Bug Fixes
Tests