Repository navigation
fix(OMN-14645): harden coverage-sweep-gate against cancel storms + fail closed - #1777
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
69094ca to
c3c77c8
Compare
…1777 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head c3c77c88e75e7c95bd30c5f04b67a3461d0a46b1.
…imarket#1777 (#4228) * evidence(OMN-14645): author OCC companion for OmniNode-ai/omnimarket#1777 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head c3c77c88e75e7c95bd30c5f04b67a3461d0a46b1. * evidence(OMN-14645): self-bind OCC#4228 + rebind contract_sha256 * evidence(OMN-14645): canonicalize OCC companion checks --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
…il closed Single-flight per-ref concurrency (queue, never cancel), explicit 40m job timeout, whole-process-group reaping on generation timeout, and a strict success-only CI-Summary check so a cancelled/skipped coverage sweep can never be masked as a measured-clean census. Ref: OMN-14645 (epic OMN-14643).
c3c77c8 to
f2b3e8c
Compare
…imarket#1777 (#4233) * evidence(OMN-14645): author OCC companion for OmniNode-ai/omnimarket#1777 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head f2b3e8cf41009c2cefa35506094b755b35f5a4ba. * evidence(OMN-14645): self-bind OCC#4233 + rebind contract_sha256 * evidence(OMN-14645): append final-head OCC proof * evidence(OMN-14645): scope final-head checks to product PR --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
✅ Architectural Review — PASSEDErrors: 0 What this checks
No architectural violations found. Static architectural lint — no model inference (OMN-14176). |
✅ Hostile Reviewer — PASSEDBlocking findings (critical/error): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
OMN-14645 — Harden the coverage-sweep-gate against cancel storms (merge-flow WS2)
Closes OMN-14645 (epic OMN-14643).
Deconfliction with #1776 (OMN-14641) — read first
This PR and omnimarket #1776 (OMN-14641, Codex's PR; OCC companion #4226 already merged) both touch
scripts/ci/run_coverage_sweep_gate.py. #1776's single job is to stream thepytest --covsubprocess output to the CI job log instead of capturing it. This revision of #1777 inherits that streaming behavior directly and reconciles the file into ONE coherent hunk: streaming output + whole-process-group reaping on timeout, together.Why this matters — the earlier PIPE-capture cut of this PR would have REGRESSED the storm. The prior revision converted
subprocess.run→subprocess.Popenbut re-addedstdout=PIPE/stderr=PIPE(viacommunicate(timeout=...)), which buffers the entire ~8-minute coverage run until the child exits. That freezes the CI run'supdatedAtfor the whole longest-poleCoveragejob — and a frozenupdatedAtis exactly what trips the Codex merge controller's stale-run heuristic into cancel+rerun (the storm OMN-14645 exists to fix; the controller self-diagnosed this at the merge-controller ledger ~17:39Z). Streaming the output live is the durable root-cause fix for the stale-run heuristic: the run keeps heartbeating, so the controller never sees it as stale. Do not re-introduce PIPE/capture_outputon this subprocess.If #1776 lands first, its streaming change is already present on
devand this PR is a clean superset (streaming + reaping in one function); if this PR lands first, #1776 becomes a no-op on the same lines.Problem (verified, not assumed)
Live omnimarket PRs #1754 / #1758 / #1774 were wedged
mergeStateStatus=BLOCKED. The sole required context ondevisCI Summary. Verified from the per-attempt jobs API (notgh pr checkstext):29421305846(PR 1758) attempt 24:Coverage Sweep Gate= cancelled (started 17:29:50Z, cancelled 17:30:20Z = 30s), and the same attempt'sCI Summary= failure.grep -n concurrency .github/workflows/ci.ymlreturned NONE → not a within-workflow concurrency cancel. Every setup step succeeds; only the long final step is cancelled.Root cause (verified): the
coverage-sweep-gatejob is the longest pole (3 fresh sibling clones +uv sync --all-extras+ fullpytest --cov). When its output is captured/buffered, the run'supdatedAtfreezes for the whole job window; the Codex merge controller reads that frozen timestamp as a stale run and issues a same-headgh run rerun(cancel+rerun). Because coverage is the longest pole, a run-wide cancel almost always catches it mid-flight →cancelled. Theci-summaryverdict loop treated a dependency as passing only ifsuccessORskipped, so acancelledcoverage sweep →FAILED=true→CI Summaryred → BLOCKED. The loop (freeze updatedAt → controller reruns → cancel long job → CI Summary fail → repeat) is self-perpetuating.What this PR does (defense-in-depth, shadow-safe)
scripts/ci/run_coverage_sweep_gate.py). Thepytest --covchild inherits this process's stdout/stderr and streams live to the CI job log, keeping the run heartbeating so the Codex stale-run heuristic never fires. This is the durable storm fix and deconflicts with fix(OMN-14641): stream coverage sweep subprocess output #1776 (OMN-14641). The subprocess is bounded withproc.wait(timeout=...)(streaming) rather thancommunicate(timeout=...)(which requires PIPE capture).start_new_session=True; a timeout SIGTERM→SIGKILLs the entire process group (pytest-xdist workers + spawned subprocesses) via_reap_process_group, then fails loudly — it never leaves a stale artifact to be swept as clean. The streamed pytest output above the failure already shows the hang site, so no silent capture-and-hold is needed for diagnostics.ci.yml:concurrency.cancel-in-progress: false, keyed by${{ github.workflow }}-${{ github.ref }}). A newer same-ref run waits for the in-flight one to reach a terminal result instead of cancelling it. Different PRs / main / merge_group never block each other.timeout-minutes: 40oncoverage-sweep-gate(was the 360-min GitHub default). Bounds the longest pole; a wedged child can no longer hold a runner slot for hours.success||skippedloop into a strict success-only check (matching the OMN-14350 / OMN-14590 pattern). Closes a real fail-OPEN hole (askipped/cancelledcoverage sweep previously PASSED CI Summary). Acancelledsweep still fails, loudly.Honest scope / what this PR does NOT do
pytest --cov, with fail-closed stale/wrong-head validation. Substantial redesign touching thetestjob's coverage upload and the sweep's authoritative path; scoped as a dedicated follow-up under OMN-14645/epic OMN-14643.concurrencyglobal group is NOT safe as the slot: under >2 concurrent PRs GHA cancels the pending job, reintroducing the exactcancelled → CI Summary redfailure this PR fixes. A real single-slot needs an external coordinator/lock (or the artifact-reuse redesign). Scoped as follow-up.dod_evidence
subprocess.Popenwithstdout=PIPE/stderr=PIPE+communicate(timeout=...)(captures/buffers the whole run → frozenupdatedAt→ Codex stale-run cancel storm);coverage-sweep-gatehad notimeout-minutes(360-min default); no workflowconcurrency; CI Summary countedskipped/cancelledas pass for the coverage gate. Verified cancel→failure: run 29421305846 attempt 24 (above).subprocess.Popen(start_new_session=True)with nostdout/stderr/capture_outputkwargs (streams live) +proc.wait(timeout=...)+_reap_process_groupon timeout; workflowconcurrencypresent (cancel-in-progress: false);coverage-sweep-gate.timeout-minutes = 40; CI Summary has a strict success-only coverage check.uv run pytest tests/ci/test_run_coverage_sweep_gate.py -q→ 9 passed, including:test_generate_helper_invokes_pytest_covnow asserts nocapture_output/stdout/stderrkwargs are passed toPopen(streaming, not captured), andtest_generation_timeout_reaps_and_fails_loudlyproves a timed-out generation reaps the process group and fails loudly (no stale-artifact pass).ruff format/ruff checkclean;mypy scripts/ci/run_coverage_sweep_gate.py→ no issues;pre-commit run --files ...all Passed (thetopic-naming-linthook requiresOMNIBASE_INFRA_PATHto the sibling repo — an environmental prerequisite, passes once set).concurrencyblock, onetimeout-minutes, and a strict CI-Summary check — no behavior removed from the authoritative coverage requirement.$OMNI_HOME/omni_worktrees/OMN-14645/omnimarket, rebased on freshorigin/dev(f6249696). Will be cleaned up after merge.Merge policy
Codex owns the merge queue. This PR is opened/updated only — not merged, not auto-armed. omnimarket
devis squash-only, no queue, auto-merge disabled.Evidence-Ticket: OMN-14645
Evidence-Source: OCC#4233
Evidence-Commit: 112d401f0aad0c33352f2c8216c5747c2b8cdc11
Evidence-Head: f2b3e8c