Repository navigation
feat(OMN-15255): composite runner readiness — one typed fleet view (6 signals, ALL-must-pass) + quarantine gate that stops the healthy-but-idle bounce storm - #2500
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 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 (18)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical findings | No |
blocked |
CRITICAL findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524)
…ibase_infra#2500 (#5147) * evidence(OMN-15255): author OCC companion for OmniNode-ai/omnibase_infra#2500 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 5adf2dd6866d25abc4dea34164b5fad40e1a1878. * evidence(OMN-15255): self-bind OCC#5147 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
PAIR_INCOMPATIBLE with #2502 (OMN-15234) — flagged, half fixed hereBoth PRs edit 1.
|
Red-check triage — neither red is caused by this diff
|
|
OMN-15263 fix is up: #2504 — your The 12 #2504 supplies the var in the three hermetic fixtures (compose itself untouched — the fail-fast stays) and generalizes the OMN-5240 coverage gate to all registered render fixtures, fail-closed on unregistered ones. Action for this PR: once #2504 lands on |
…quarantine gate that stops the healthy-but-idle bounce storm Friction F-04/Recommended-4: "online means registered, not ready to execute the governed workload." On 2026-07-27T16:40Z the GitHub registry read 64/64 online while 53 of 64 containers read docker-unhealthy, and nothing in the system adjudicated — a human diffed three surfaces by hand. node_runner_fleet_health_compute now emits composite per-runner readiness: a CONJUNCTION over six independently-probed signals (github_registration, docker_health, diag_heartbeat, listener_topology, container_stability, disk_capacity), every one evaluated every tick. The pre-existing state field is a first-match-wins precedence pick that never evaluates container health or listener topology at all; the two legitimately disagree. Quarantine/bounce split closes the false-positive restart storm: readiness fails CLOSED (UNKNOWN is not READY), the bounce gate fails SAFE (no mutation on indeterminate sources, never a busy runner, never for a cause a recreate cannot fix — a full host disk and a GitHub status lag with healthy local evidence quarantine without ever recommending a restart, OMN-14057). Net-negative: the four state-keyed RESTART_RUNNER branches (CRASH_LOOPING 0.9 / LISTENER_ZOMBIE 0.85 / OFFLINE_IDLE 0.6 / WEDGED 0.5) are DELETED — bounce-eligibility is now the single producer of a restart recommendation. node_runner_health_snapshot_effect gathers the three facts no prior probe collected (container health status, Runner.Listener process/orphan topology, host disk used-percent), all read-only. Unknowns stay unknown: a failed probe never defaults to a passing value, so this is inert before rollout. No host mutation in this change. Contracts bumped 1.1.0 on both nodes.
Removes a mechanical collision with the concurrent OMN-15234 lane (PR #2502), which deletes this file s check-env-reads allowlist entry and replaces it with per-NAME grandfathering. RUNNER_READINESS_MAX_DISK_USED_PERCENT is a NEW name in this file, so it would not be grandfathered — this PR would have gone RED on a required gate the moment #2502 landed. Rule 10: fix the underlying issue rather than widen an allowlist. The ceiling stays a documented constant until the runner-health thresholds get a typed config surface. Tunability is not lost silently; it was never granted.
4bc8caf to
32db718
Compare
What / Why
Friction report §Recommended-4 / F-04 (
docs/tracking/friction/2026-07-27-weekend-friction-report.md): "'Online' currently means registered, not ready to execute the governed workload."Live measurement, 2026-07-27T16:40Z (OMN-15233 canary comment): the GitHub org registry read
{total: 64, online: 64, busy: 0}while 53 of 64 containers read docker-unhealthy. Two surfaces, opposite verdicts, and no third surface adjudicating — the adjudication was a human diffinggh api .../actions/runners,docker ps, and per-container_diagmtimes by hand.This PR makes
node_runner_fleet_health_computethat third surface.Composite readiness — a conjunction, not a precedence chain
readinessis READY only when all six independently-probed signals PASS. Every signal is evaluated every tick, including the passing ones:github_registrationonlinedocker_healthrunning+ healthhealthy(ornone— image declares no healthcheck)diag_heartbeat_diagage ≤RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS(4500s, brackets the measured ~50-53 min token-refresh cycle — OMN-15233)listener_topologyRunner.Listener, zero at PPID 1container_stabilityRestartCount≤CRASHLOOP_RESTART_THRESHOLD(5)disk_capacityRUNNER_READINESS_MAX_DISK_USED_PERCENT(90)The pre-existing
statefield is unchanged and still a first-match-wins precedence pick. It does not evaluate container health or listener topology at all — so the two legitimately disagree, and that disagreement is the F-04 finding. A GitHub-online runner with a fresh heartbeat, an unhealthy container and two listeners isstate=HEALTHYandreadiness=NOT_READY. Asserted directly intest_online_plus_unhealthy_container_plus_duplicate_listener.Fleet rollup on the verdict:
ready_count(the real capacity number, notonline_count),not_ready_count,readiness_unknown_count,fleet_ready,quarantined_runners,bounce_eligible_runners, and per-signalreadiness_signal_rollups(which surface disagrees, about how many runners).Quarantine ≠ restart — the false-positive storm class
Two fail directions, deliberately opposite:
UNKNOWNis notREADY; an unprobeable runner is not counted as capacity.NOT_READYand sources were determinate and the runner is notgithub_busyand at least one failing signal is something a force-recreate can actually fix.Consequences, each a test:
Net-negative surface
Deleted: the four state-keyed
RESTART_RUNNERbranches in_recommend_for_assessment(CRASH_LOOPING0.9 /LISTENER_ZOMBIE0.85 /OFFLINE_IDLE0.6 /WEDGED0.5). Bounce-eligibility is now the single producer of a restart recommendation. Four independently-tunable confidence heuristics over the same facts is how one misread threshold (the retired 900s heartbeat window) became a fleet-wide restart storm with no second condition anywhere able to veto it.Retired, rollout-gated (stated, not claimed): the manual three-surface comparison procedure in
docs/runbooks/runner-fleet-listener-liveness.mdis superseded by the fleet view, but remains the operator procedure until the extended probe is deployed. The runbook says so explicitly rather than implying the view is live.Follow-up filed, not taken here: deleting
collector_runner_health._classify_runner+EnumRunnerHealthState(the legacy CLI collector's duplicate classification) once the node is live-proven. Not taken in this PR because that surface is the one currently running.Seams (OMN-14208)
Producer
node_runner_health_snapshot_effect→ consumernode_runner_fleet_health_compute, matched field-by-field:ModelRunnerFleetRunnerFact.docker_healthstr(""= unknown)handler_runner_fleet_snapshot_docker_health_signal.listener_process_countint | None_listener_topology_signal.orphaned_listener_countint | NoneModelRunnerFleetSnapshot.host_disk_used_percentfloat | None_fetch_host_disk_used_percent_disk_capacity_signalDriven end-to-end by the real handler pair —
test_pre_rollout_probe_output_degrades_to_unknown(producer side) andtest_unprobed_readiness_facts_read_unknown_not_pass(consumer side) are the cross-boundary pair proving the unknown path, not two independent unit suites.Rollout is inert by construction
The new facts only exist once the effect's extended SSH probe runs against the fleet. Until then all three signals report
UNKNOWN→ readinessUNKNOWN→ nothing quarantined, nothing bounced, no recommendation emitted. Shipping this ahead of rollout cannot change fleet behavior.test_unprobed_readiness_facts_read_unknown_not_passasserts exactly that.No host mutation in this PR. No container recreated, no host file written, no cron changed,
omninode-pcuntouched — the OMN-15233 fan-out lane owns the runner-fleet host files concurrently.runner-monitor.shis deliberately not edited: it is rsynced to the host bydeploy-runners.sh, so a change ondevwould land on the host as a side effect of that lane's sync. Wiring the bash monitor to consume this verdict is the rollout follow-up.test_no_mutating_docker_verb_reaches_the_runner_hostasserts against the command string actually dispatched over SSH (not the source text) that nodocker restart|stop|rm|kill|start|compose|tag|commitor--force-recreatecan reach the host.RED → GREEN
Same 38 tests, held constant:
origin/devsrc (tests applied, source reverted)Falsifiability inside the change, not just against the old tree:
test_the_retired_900s_threshold_reproduces_the_stormpins the retired threshold via monkeypatch and the identical healthy-idle fixture flips to 8/8 bounce-eligible with 8 restart recommendations, whiletest_idle_fleet_past_the_retired_900s_window_is_ready_and_unbounced(5 idle ages, 1000–4400s) reads 8/8 READY and zero restarts at the corrected default.test_a_genuinely_dead_listener_still_bounces(9000s) proves the recalibration did not disarm the check.Per-signal RED discipline:
test_single_failing_signal_makes_the_runner_not_readyis parametrized over six cases, each failing exactly one signal with the other five asserted PASS — the only shape that proves a signal is load-bearing rather than decorative.Gates — run on
.200(ssh stickybeatz-studio), rule 11aPatch-transfer discipline: patch
sha256 2bc913cd…46e1a4verified identical on both hosts;git diff --cached --statidentical (18 files, 1704 insertions, 47 deletions). Commit + push executed on.200.ruff check src/ tests/— All checks passedruff format --check— 4542 files already formattedmypy src/omnibase_infra/— Success, 2622 source filespre-commit run --files <all changed>— all Passed/Skipped, no file rewritten by a hookscripts/check-env-reads.sh --base origin/dev— exit 0 (the disk-threshold read lands in the already-allowlistedhandler_runner_fleet_health_evaluate.py; no new allowlist entry added — OMN-15234 still owns narrowing that matcher){"selected_paths":["tests/"],"is_full_suite":true,"full_suite_reason":"shared_module"}→ full suite running on.200; result posted to the ticket.Contracts bumped to
1.1.0on both touched nodes (new output fields = minor).Not claimed
proof_class: receipt-bound, NOTdeployed. No live fleet readback. The six signals have never been evaluated against real probe output — only against fixtures.bounce_eligible_runners.Refs: OMN-15255 (child/related OMN-13915), OMN-15233, OMN-15177, OMN-14057, OMN-15234.
No merges, no
--auto, no drafts. No OCC file modified by this branch.Evidence-Ticket: OMN-15255
Evidence-Source: OCC#5159