Skip to content

fix(OMN-15233): recalibrate runner healthcheck (900->4500s), fail on PPID-1 orphans + rate-based crash loops, reap before respawn - #2492

Merged
jonahgabriel merged 9 commits into
devfrom
jonah/omn-15233-runner-healthcheck-recalibration
Jul 27, 2026
Merged

jonahgabriel merged 9 commits into
devfrom
jonah/omn-15233-runner-healthcheck-recalibration

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Jul 27, 2026 •

Copy link
Copy Markdown
Collaborator

OMN-15233 — runner healthcheck recalibration + orphan/crash-loop detection

Evidence-Source: OCC#5117
Evidence-Ticket: OMN-15233

Ticket: OMN-15233 (dod_evidence on the ticket; OCC companion PR opened in onex_change_control — see "Companion" below).

The OMN-13915 healthcheck landed by #2194 was miscalibrated on the healthy path and inverted on the failure mode it exists to catch. Both defects live in the same check.

(a) False positive by arithmetic

RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS=900 sat far below the ~50-minute IDLE _diag write cadence — when a runner has no job, only the OAuth/AAD token refresh writes _diag; the minutes-scale cadence holds only while jobs run. An idle runner therefore read unhealthy for ~35 of every 50 minutes with nothing degraded.

Observed 2026-07-27: the Docker-unhealthy count went 13 → 37 → 59 while the GitHub registry reported 64/64 online throughout; 59 → 4 resolved with only 8 restarts and the untouched control group self-healed. That was measurement phase, not fleet degradation.

Default is now 4500s (75 min), justified inline against the idle cadence.

(b) Inversion — the real zombie scored HEALTHY

An orphaned Runner.Listener reparented to PPID 1 keeps holding the GitHub broker session. The watchdog spawns a replacement, which crash-loops every ~5 min on TaskAgentSessionConflictException — and every crash mints a fresh Runner_*.log, keeping the _diag mtime fresh, so the check read HEALTHY forever. Runners 1, 43, 55, 57 sat in that state with 88–234 log files (vs 3–7 normal) and were found only by process scan.

Changes

docker/runners/healthcheck.sh

  • Threshold 900 → 4500, with the idle-cadence justification inline (an unexplained magic number invites the next well-meaning tightening back to 900).
  • New layer 2 — process topology. Fails on duplicate Runner.Listener processes, and on any listener with PPID 1. A healthy listener's chain is entrypoint.sh(PID 1) → run.sh → run-helper.sh → Runner.Listener, so PPID 1 is unambiguously an orphan.
  • New layer 4 — rate-based crash-loop. Counts Runner_*.log files touched inside RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES (60), failing above RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR (6) normalized to that window (ceil(per_hour × window_minutes / 60), integer-safe — see "Verifier remediation" for why the un-normalized form was a bug). A ~5-min crash cadence is ~12/hour. Deliberately NOT cumulative — a cumulative count grows monotonically with container uptime, so any long-lived healthy container eventually red-lines forever, and a permanently-red check is a disabled check. Both tunables fail closed on unusable input.

docker/runners/entrypoint.sh

  • Reap before respawn. _reap_orphaned_listeners (TERM, then KILL after LISTENER_REAP_TIMEOUT_SECONDS=30) runs before every run.sh spawn, not just the watchdog-recycle path, and confirms the listener is gone. Spawn-without-reap is what manufactures the session conflict. If a listener survives SIGKILL the entrypoint exits so the restart policy replaces the whole PID namespace instead of looping silently.
  • The OMN-14564 comment block is corrected: OMN-15233 flips the ordering of the alert (4500s) and kill (3600s) thresholds. That is intentional — the watchdog is the narrower signal (requires LISTENER_HEARTBEAT_MISSES consecutive ticks, never fires while a Runner.Worker runs), so it can act on staleness the unconditional healthcheck must not flag.

docs/runbooks/runner-fleet-listener-liveness.md

  • Interim operating rule recorded: cross-check the GitHub runner registry before ANY restart sweep — if runners are online, the flag is the bug. Includes the probe command. Detection-layer table updated; new "Orphan / session-conflict mode" section.

Test proof — RED on the old logic, GREEN on the new

tests/ci/test_runner_listener_liveness.py, following the existing #2194 harness pattern (real bash healthcheck.sh / bash entrypoint.sh subprocesses against a synthetic RUNNER_HOME, not a surrogate reimplementation).

Every assertion about the OMN-15233 behavior is behavioral — the real healthcheck.sh / entrypoint.sh driven as bash subprocesses against a synthetic RUNNER_HOME, with each tunable left UNSET where the default is what is under test. There are no source-text greps left on this ticket's logic (see "Verifier remediation" below).

RED proof, tests held constant:

Baseline Result
docker/runners/{healthcheck,entrypoint}.sh at origin/dev 16 failed / 31 passed
healthcheck.sh at pre-remediation head 1729c7c3 (unnormalized layer 4) 6 failed / 41 passed
Full change applied 47 passed
Test Proves
test_default_threshold_brackets_the_idle_cadence (4 params) Drives the script with RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS unset: 16 min (just past the old 900s default), 50 min (the observed idle OAuth/AAD refresh ceiling) and 70 min read healthy; 80 min reads unhealthy. Brackets the shipped default from both sides, so neither "tighten it back to 900" nor "clear the idle cadence by disabling the layer" survives
test_idle_runner_past_old_900s_window_reads_healthy 45 min idle silence → healthy on the script default
test_recalibration_did_not_disable_the_check 2.5 h silence still unhealthy
test_duplicate_listeners_read_unhealthy two listeners + fresh _diag → unhealthy
test_orphaned_ppid1_listener_reads_unhealthy double-forked PPID-1 orphan + fresh _diag → unhealthy (skips, with reason, if the host has a subreaper and will not reparent to PID 1)
test_threshold_is_normalized_to_the_rate_window (4 params) The per-hour budget is scaled to the measured window: 3 starts in 30m healthy / 4 unhealthy; 12 starts in 120m healthy / 13 unhealthy. Two of the four are RED in opposite directions against the unnormalized code — (30m, 4 starts) read healthy unnormalized (4 > 6 false) and (120m, 12 starts) read unhealthy unnormalized (12 > 6 true)
test_unusable_rate_tunables_fail_closed (4 params) Window 0/abc and threshold -1/six all exit 1 with a fail closed reason. Unnormalized, abc read healthy ("0 start(s) in abcm") and six crashed on set -u with empty stdout
test_identical_logs_flip_the_verdict_purely_by_window_membership Rate, not cumulative, on a constant file set: 12 logs aged outside a 30m window → healthy; the same 12 touched into the window → unhealthy. A cumulative implementation cannot produce the first verdict
test_crash_loop_rate_flags_but_history_does_not 234 historical logs + 1 active → healthy; 12 fresh logs → unhealthy
test_entrypoint_reaps_orphan_and_replacement_sees_no_session_conflict Functional end-to-end, and now the only proof of reap-before-spawn ordering: stub run.sh emits TaskAgentSessionConflictException if a listener already holds the session, so the only way past it is a real reap. Asserts against LOG_FILE (the tee'd run.sh output), not entrypoint stdout — the entrypoint's own REAP banner names the exception, so asserting on stdout would be vacuous

Verifier remediation (2026-07-27, commit dbfc0c98)

Three findings from adversarial verification, all closed in code:

1. Layer 4 was not normalized — a real bug, not a test gap. recent_starts was counted over RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES but compared directly against RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR. The two are in different units, so the comparison was correct only at the 60m default: a 30m window enforced 6-per-30m (= 12/hour, double the intended budget) and a 120m window enforced 6-per-120m (= 3/hour, half of it). Retuning the window silently retuned the threshold.

Fixed by scaling the per-hour budget to the measured window before comparing:

max_starts_in_window=$(( (RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR * window_minutes + 59) / 60 ))

Integer arithmetic only (there is no bc in the runner image) and the +59 numerator rounds up, so a fractional budget (6/hour over a 5m window = 0.5) never floors to 0 — a zero allowance would fail on the first legitimate listener start. Rationale is stated inline at the comparison. Both tunables now fail closed on non-integer / non-positive input instead of feeding garbage into the arithmetic; previously RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES=abc produced a healthy verdict. The unhealthy message and the healthy message both report recent/allowed, so an operator reading a container log sees the effective budget, not just the per-hour knob.

2. Three surrogate (source-text grep) tests replaced. Each would have passed on a comment-only change:

Removed Replacement
test_default_threshold_clears_idle_cadence_and_is_justified — regex for 4500 + "idle" in the file test_default_threshold_brackets_the_idle_cadence — behavioral, threshold unset, 4 params
test_crash_loop_signal_is_documented_as_rate_not_cumulative — grep for the literal string NOT CUMULATIVE test_identical_logs_flip_the_verdict_purely_by_window_membership — behavioral, constant file set, mtime is the only variable
test_reap_precedes_the_run_sh_spawn — content.index(reap) < content.index(spawn) Deleted, not replaced. Ordering was already proven behaviorally by test_entrypoint_reaps_orphan_and_replacement_sees_no_session_conflict, whose stub run.sh fails with TaskAgentSessionConflictException whenever a listener is alive at spawn time — spawning before reaping is therefore observable in LOG_FILE. A source-position grep proves strictly less, so it was removed rather than kept

The one thing lost is the "the magic number carries an inline justification" assertion. That is a documentation property, not a behavior, and is not behaviorally testable; per the no-surrogates rule it was dropped rather than kept as a grep. The justification comment itself is still in healthcheck.sh.

3. Stale-red gate / CodeRabbit Thread Check. It read FAILURE at the previous head from stale ordering while both threads were isResolved: true. The dbfc0c98 push re-triggered it and it is SUCCESS.

CI readback — terminal state at 024ca2a8

109 SUCCESS / 7 skipped / 0 failure, mergeable: MERGEABLE, mergeStateStatus: CLEAN. The single required context on omnibase_infra dev (CI Summary, verified live via branches/dev/protection/required_status_checks) is SUCCESS.

Getting there took two further commits. Neither is cosmetic and neither was caused by the remediation — both were reds already present at 1729c7c3 before any of this work, and the identical failure set (CI Summary, Contract Sync Gate, deploy-gate / deploy-gate, Integration Silent-Skip Guard) is visible on that commit's check-runs.

509233b8 — Contract Sync Gate (Wave C) [OMN-8915]. 1729c7c3 changed handlers/handler_runner_fleet_health_evaluate.py without touching the node's contract.yaml, which is exactly what this gate exists to catch. Fixed as a real contract update rather than a token touch: the handler's RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS default moved 900 → 4500, which changes the verdict this classifier emits for an idle runner, so contract_version/node_version went 1.0.1 → 1.0.2 and the threshold semantics (including the cross-surface equality invariant) are recorded in the description. Verified by running the gate script itself against the real gh pr diff 2492 --name-only set. This change escalated the governed selector to the full suite (shared_module), so uv run pytest tests/ -n auto was run on .200: 26363 passed / 28 failed / 43 errors, every failure environmental on that host (no live postgres, LLM endpoints or runtime containers) or xdist env-pollution — the same subset run serially passes, and stashing the change reproduces an identical failure set.

024ca2a8 — Tests (Split 1/15) → CI Tests Gate → Test-Failure Ratchet Gate → CI Summary. The full-suite escalation surfaced tests/integration/ci/test_workflow_uses_refs_resolve_live.py, which resolves every cross-repo uses: pin against the GitHub contents API and fails closed on an unverifiable pin. Neither pytest step passed a token, so all 15 pins returned HTTP 403: API rate limit exceeded for 20.169.98.150 — unauthenticated calls sharing the runner egress IP's budget. Fixed by threading GH_TOKEN: ${{ github.token }} into both pytest steps, which is the fix the test's own failure message prescribes. Not skip-tokened, and not written off as a flake: the 403 is a named, reproducible cause, and the test goes RED unauthenticated / GREEN authenticated on demand.

That test's docstring still claims it is "DELIBERATELY RED in CI on this repo" pending the omniclaude OMN-14941 PR. That condition has expired — both formerly-404 pins resolve on dev today (call-occ-companion-effect-reusable.yml → c540d981, call-occ-autobind-reusable.yml → 5f8f64e4), so rate limiting was the sole remaining cause. Blast radius was checked before touching a shared workflow: no test in this suite skips on token presence, so the change only authenticates requests that were already being made.

Gates (rule 11a — run on .200, ssh stickybeatz-studio)

Patch-transfer discipline: per-file sha256 verified identical on both hosts after applying the patch on .200 (not just the patch file's own hash), git diff --cached --stat matched, and the commit + push were executed on .200. Re-verified for the dbfc0c98 remediation.

Re-run in full at dbfc0c98:

  • ruff check src/ tests/ — clean
  • ruff format --check — 4525 files already formatted
  • mypy src/omnibase_infra/ — clean, 2615 files
  • pre-commit run --files <all changed> — clean, no file rewritten by a hook
  • Governed selector (scripts/ci/detect_test_paths.py) selected tests/ci/ for this changeset → uv run pytest tests/ci/ = 972 passed, 1 skipped
  • tests/integration/test_runner_listener_liveness_integration.py — 1 passed
  • bash -n + shellcheck -S warning clean on both scripts

Deviation to state: the RED baselines (origin/dev and head 1729c7c3) were reproduced on this Mac, not .200 — they require repeatedly checking out and restoring docker/runners/*.sh in the worktree, which is edit-locality-hostile over ssh. The GREEN run those numbers are contrasted against is the authoritative .200 run above, and the local GREEN (47 passed on the same file) matched it.

Node-surface alignment (landed in 1729c7c3) + follow-up OMN-15234

src/omnibase_infra/nodes/node_runner_fleet_health_compute/handlers/handler_runner_fleet_health_evaluate.py reads the same RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS and shipped the same 900s default, mapping LISTENER_ZOMBIE → RESTART_RUNNER at confidence 0.85 — so leaving it would have kept the node-based fleet-health verdict recommending restarts of idle-but-healthy runners even after the bash surface was fixed. 1729c7c3 aligns that default to 4500.

Honest note on how it got past the gate: scripts/check-env-reads.sh fires on any added line containing os.environ, so editing the default of a pre-existing, already-grandfathered read is indistinguishable to the matcher from introducing a new env-resolution surface — there is no formulation of "change 900 to 4500" that avoids re-adding that line. 1729c7c3 added the file to APPROVED_INFIX_PATTERNS with that reason inline. That is an allowlist entry, i.e. the weaker of the two available fixes; the correct fix is to narrow the matcher so a same-name read already present in the file's base version does not trip it. That is a repo-wide required CI gate with blast radius across every open PR, so it stays out of a runner-healthcheck PR. OMN-15234 carries the matcher narrowing and its regression test matrix, and should also remove this allowlist entry once the matcher lands.

Safety

No live-fleet mutation, no lane mutation, no container recreate, no .201/omninode-pc container touched. Note that healthcheck definitions are frozen at container creation and entrypoint.sh changes require a force-recreate — rollout is a separate, operator-gated step per the runbook's safe-bounce recipe.

Companion

OCC companion: OmniNode-ai/onex_change_control#5117 — evidence(OMN-15233): add deploy-scope runner evaluator receipt, merged to OCC dev with central deploy-scope evidence after the initial autobind companion #5101 proved insufficient for Deploy Gate. Evidence Source OCC#5117 and Evidence Ticket OMN-15233 at the top of this body pin the merged companion that deploy-gate, preflight, and receipt gate should resolve.

No merge was performed by an agent on either repo.

…espawn

The OMN-13915/#2194 container healthcheck was miscalibrated on the healthy
path and inverted on the failure mode it exists to catch. Both defects were
in the same check.

(a) FALSE POSITIVE BY ARITHMETIC. RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS=900 sat
far below the ~50-minute IDLE _diag write cadence -- when a runner has no
job, only the OAuth/AAD token refresh writes _diag. An idle runner therefore
read unhealthy for ~35 of every 50 minutes with nothing degraded. That is what
produced the 2026-07-27 13 -> 37 -> 59 "unhealthy growth" while the GitHub
registry reported 64/64 online throughout; 59 -> 4 resolved with only 8
restarts and the untouched control group self-healed. Default is now 4500s
(75 min), justified inline against the idle cadence.

(b) INVERSION. An orphaned Runner.Listener reparented to PPID 1 keeps holding
the GitHub broker session; the watchdog spawns a replacement that crash-loops
every ~5 min on TaskAgentSessionConflictException; every crash mints a fresh
Runner_*.log, which keeps the _diag mtime fresh -- so the check read HEALTHY
forever. Runners 1/43/55/57 sat in that state with 88-234 log files (vs 3-7
normal) and were found only by process scan.

healthcheck.sh now fails on duplicate Runner.Listener processes and on any
listener with PPID 1 (a healthy listener chains entrypoint.sh(PID 1) ->
run.sh -> run-helper.sh -> Runner.Listener, so PPID 1 is unambiguously an
orphan), and adds a RATE-based crash-loop layer: Runner_*.log files touched
inside RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES (60), failing above
RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR (6). Deliberately NOT cumulative -- a
cumulative count grows with container uptime and would red-line every
long-lived healthy container forever, and a permanently-red check is a
disabled check.

entrypoint.sh reaps any surviving listener (TERM, then KILL after
LISTENER_REAP_TIMEOUT_SECONDS) and confirms it is gone BEFORE spawning a
replacement; spawn-without-reap is what manufactures the session conflict.
If a listener survives SIGKILL the entrypoint exits so the restart policy
replaces the whole PID namespace rather than looping silently.

Runbook records the interim operating rule: cross-check the GitHub runner
registry before ANY restart sweep -- if runners are online, the flag is the
bug.

Tests go RED on the pre-change scripts and GREEN after (9 failed / 3 passed
against origin/dev scripts; 12/12 pass after), including a functional
end-to-end reap proof whose stub run.sh emits
TaskAgentSessionConflictException when a listener already holds the session.

No live-fleet mutation. Follow-up OMN-15234 covers the second surface
(node_runner_fleet_health_compute still defaults to 900s) and the over-broad
check-env-reads matcher that blocks fixing it here.
@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Runner listener liveness

Layer / File(s) Summary
Healthcheck detection and thresholds
docker/runners/healthcheck.sh, tests/ci/test_runner_listener_liveness.py
The healthcheck recalibrates heartbeat diagnostics, detects missing, duplicate, or PPID-1 listeners, counts recent listener starts, and adds coverage for these conditions.
Orphan reap before respawn
docker/runners/entrypoint.sh, tests/ci/test_runner_listener_liveness.py
The entrypoint reaps matching listeners with TERM-to-KILL escalation before run.sh, exits on unreapable listeners, and tests the ordering and functional replacement flow.
Runbook and operational validation
docs/runbooks/runner-fleet-listener-liveness.md, tests/ci/test_runner_listener_liveness.py
The runbook documents registry cross-checks, updated thresholds, detection layers, orphan/session-conflict handling, and related CI assertions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Entrypoint as entrypoint.sh
  participant Listener as Runner.Listener
  participant RunScript as run.sh
  participant RestartPolicy as container restart policy
  Entrypoint->>Listener: Find matching listener processes
  Entrypoint->>Listener: Send TERM and wait for reap timeout
  Entrypoint->>Listener: Send KILL if the listener remains
  Entrypoint->>RunScript: Start replacement when no listener remains
  Entrypoint->>RestartPolicy: Exit 1 when a listener persists
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: healthcheck recalibration, orphan/crash-loop detection, and orphan reaping before respawn.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jonah/omn-15233-runner-healthcheck-recalibration

Comment @coderabbitai help to get the list of available commands.

jonahgabriel pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Jul 27, 2026
…fra#2492

OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 934465c9b86b1f8f279ae6059b2d845e57d6519b.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
tests/ci/test_runner_listener_liveness.py (2)

195-203: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Add a timeout= to the ps call for consistency.

Every other subprocess.run/wait in this module is bounded; _ppid_of is called in polling loops (Line 323, Line 756) and would hang the suite indefinitely if ps wedged.

     result = subprocess.run(
         ["ps", "-o", "ppid=", "-p", str(pid)],
         check=False,
         capture_output=True,
         text=True,
+        timeout=10,
     )
🤖 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/ci/test_runner_listener_liveness.py` around lines 195 - 203, Bound the
subprocess invocation in _ppid_of by adding the module’s established timeout
value to subprocess.run. Preserve the existing output parsing and None behavior
while ensuring polling callers cannot wait indefinitely for ps.

743-751: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Fixed sleep(2) + unconditional read_text on LOG_FILE is a flake.

Everywhere else in this test you poll to a deadline. Here, if run.sh hasn't created listener.log yet on a loaded CI host, this raises FileNotFoundError instead of a meaningful assertion. Poll for the file the same way.

♻️ Poll instead of sleeping
-            time.sleep(2)
-            replacement_log = (tmp_path / "listener.log").read_text(encoding="utf-8")
+            listener_log = tmp_path / "listener.log"
+            deadline = time.time() + 20
+            replacement_log = ""
+            while time.time() < deadline:
+                if listener_log.exists():
+                    replacement_log = listener_log.read_text(encoding="utf-8")
+                    if replacement_log.strip():
+                        break
+                time.sleep(0.5)
             assert "TaskAgentSessionConflictException" not in replacement_log, (
                 f"replacement spawned into a contested session: {replacement_log}"
             )
🤖 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/ci/test_runner_listener_liveness.py` around lines 743 - 751, Replace
the fixed time.sleep(2) and unconditional listener.log read in the
replacement-session assertion with the test’s existing deadline-based polling
approach. Wait until listener.log exists, then read it and retain the
TaskAgentSessionConflictException assertion; if the deadline expires, fail with
a meaningful assertion rather than allowing FileNotFoundError.
docker/runners/healthcheck.sh (1)

132-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

..._PER_HOUR is compared against a raw count over ..._WINDOW_MINUTES.

The two tunables are independent, but the comparison assumes the window is exactly 60 minutes. Setting RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES=15 silently makes the effective trip point 24/hour, and the failure message still claims > 6/hour. Either scale the threshold to the window or document that the window is fixed at 60.

♻️ Scale the allowance to the configured window
 window_minutes="${RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES}"
+# Allowance scales with the window so the two tunables stay independent.
+max_starts_in_window=$(( (RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR * window_minutes + 59) / 60 ))
 recent_starts=$(find "${diag_dir}" -maxdepth 1 -type f -name 'Runner_*.log' -mmin "-${window_minutes}" -print 2>/dev/null | wc -l | tr -d '[:space:]')
-if [[ "${recent_starts}" -gt "${RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR}" ]]; then
-  echo "unhealthy: ${recent_starts} listener starts in the last ${window_minutes}m (> ${RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR}/hour) — listener crash-looping (OMN-15233 crash-loop rate)"
+if [[ "${recent_starts}" -gt "${max_starts_in_window}" ]]; then
+  echo "unhealthy: ${recent_starts} listener starts in the last ${window_minutes}m (> ${max_starts_in_window} allowed at ${RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR}/hour) — listener crash-looping (OMN-15233 crash-loop rate)"
   exit 1
 fi
🤖 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 `@docker/runners/healthcheck.sh` around lines 132 - 137, Update the rate check
using recent_starts and RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR so the allowed
count is scaled proportionally to RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES, while
preserving the existing crash-loop failure behavior. Ensure the unhealthy
message reports the effective threshold for the configured window rather than
claiming the hourly value applies directly.
🤖 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 `@docker/runners/healthcheck.sh`:
- Around line 40-54: Update the default RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS value
in handler_runner_fleet_health_evaluate.py from 900 to 4500 so the node
evaluator matches the healthcheck threshold. Preserve the existing
environment-variable override behavior and leave the other health settings
unchanged.

In `@docs/runbooks/runner-fleet-listener-liveness.md`:
- Around line 8-12: Fix the markdownlint MD028 violation in the quoted section
by adding a blockquote marker to the blank separating line between the two
blockquote paragraphs, preserving the existing text and formatting.

---

Nitpick comments:
In `@docker/runners/healthcheck.sh`:
- Around line 132-137: Update the rate check using recent_starts and
RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR so the allowed count is scaled
proportionally to RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES, while preserving the
existing crash-loop failure behavior. Ensure the unhealthy message reports the
effective threshold for the configured window rather than claiming the hourly
value applies directly.

In `@tests/ci/test_runner_listener_liveness.py`:
- Around line 195-203: Bound the subprocess invocation in _ppid_of by adding the
module’s established timeout value to subprocess.run. Preserve the existing
output parsing and None behavior while ensuring polling callers cannot wait
indefinitely for ps.
- Around line 743-751: Replace the fixed time.sleep(2) and unconditional
listener.log read in the replacement-session assertion with the test’s existing
deadline-based polling approach. Wait until listener.log exists, then read it
and retain the TaskAgentSessionConflictException assertion; if the deadline
expires, fail with a meaningful assertion rather than allowing
FileNotFoundError.
🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1dbccc18-1514-4030-b079-fbd0bde23d08

📥 Commits

Reviewing files that changed from the base of the PR and between f7fb7cd and ff8d373.

📒 Files selected for processing (4)
  • docker/runners/entrypoint.sh
  • docker/runners/healthcheck.sh
  • docs/runbooks/runner-fleet-listener-liveness.md
  • tests/ci/test_runner_listener_liveness.py

Comment thread docker/runners/healthcheck.sh
Comment thread docs/runbooks/runner-fleet-listener-liveness.md
jonahgabriel added a commit to OmniNode-ai/onex_change_control that referenced this pull request Jul 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Hostile Reviewer — DEGRADED (informational)

Blocking findings (critical): 0
Total findings: 0
Models succeeded: none

Note: All reviewer models failed or were unavailable. Degraded results are informational during the pilot phase (OMN-8468/OMN-8524) and do not block merge. Error: all review endpoints [192.168.86.201:8000 192.168.86.201:8001 ] unreachable — preflight short-circuit (no models available)


Gate semantics (pilot phase)

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)

jonahgabriel and others added 6 commits July 27, 2026 09:19
…place 3 surrogate grep tests with behavioral ones

Verifier remediation on #2492.

1. LAYER-4 NOT NORMALIZED (real bug). healthcheck.sh counted Runner_*.log
   starts over RUNNER_HEALTH_LOG_RATE_WINDOW_MINUTES but compared that count
   directly against RUNNER_HEALTH_MAX_LOG_STARTS_PER_HOUR, which is only
   correct at the 60m default: a 30m window enforced 6-per-30m (12/hour,
   double the intended budget) and a 120m window enforced 6-per-120m (3/hour,
   half of it). The per-hour budget is now scaled to the measured window with
   integer-safe ceiling arithmetic (no bc in the runner image), documented
   inline, and both tunables fail closed on non-integer / non-positive input
   rather than producing a zero-or-garbage allowance.

2. SURROGATE TESTS replaced with behavioral assertions on the real-bash
   subprocess harness:
   - test_default_threshold_clears_idle_cadence_and_is_justified (regex on the
     4500 literal) -> test_default_threshold_brackets_the_idle_cadence: drives
     the script with the threshold UNSET at 16/50/70/80 idle minutes.
   - test_crash_loop_signal_is_documented_as_rate_not_cumulative (grep for the
     string "NOT CUMULATIVE") ->
     test_identical_logs_flip_the_verdict_purely_by_window_membership: the same
     12 log files flip healthy->unhealthy on mtime alone.
   - test_reap_precedes_the_run_sh_spawn (source-index ordering) -> DELETED.
     Ordering is already proven behaviorally by
     test_entrypoint_reaps_orphan_and_replacement_sees_no_session_conflict,
     whose stub run.sh fails with TaskAgentSessionConflictException whenever a
     listener is alive at spawn time.
   New: test_threshold_is_normalized_to_the_rate_window (4 params) and
   test_unusable_rate_tunables_fail_closed (4 params).

RED proof: against #2492 head 1729c7c the 6 new normalization/fail-closed
cases FAIL (including both directions -- 30m/4-starts reads healthy
unnormalized, 120m/12-starts reads unhealthy unnormalized); against origin/dev
16 cases FAIL. All 47 pass with the fix.

Gates on .200 (rule 11a, patch-transfer; sha256 verified identical on both
hosts): ruff check clean, ruff format 4525 already formatted, mypy clean 2615
files, pre-commit --files clean, shellcheck -S warning + bash -n clean on both
scripts, governed selector -> tests/ci/ = 972 passed / 1 skipped.
…he 4500s threshold change

Clears the red `Contract Sync Gate (Wave C) [OMN-8915]`, which has been failing
on this PR since 1729c7c changed
handlers/handler_runner_fleet_health_evaluate.py without touching the node
contract.

This is a real contract update, not a token touch to satisfy the gate: the
handler default for RUNNER_HEALTH_MAX_DIAG_AGE_SECONDS moved 900 -> 4500, which
changes what verdict this classifier emits for an idle runner (it previously
emitted LISTENER_ZOMBIE -> RESTART_RUNNER at confidence 0.85 for idle-but-healthy
runners). Contract/node version bumped 1.0.1 -> 1.0.2 and the threshold
semantics recorded in the description, including the invariant that the default
is held identical across healthcheck.sh, runner-monitor.sh and this node.

Gates on .200 (rule 11a, patch-transfer; per-file sha256 matched on both hosts;
yamlfmt-normalized copy pulled back so the two hosts stay byte-identical):
- pre-commit --files <contract.yaml> clean on rerun
- `scripts/validate-pr-contract-sync.sh --from-env` against the real
  `gh pr diff 2492 --name-only` file set: "OK: contract-sync gate passed"
- governed selector escalated to the full suite (shared_module), so
  `uv run pytest tests/ -n auto` was run: 26363 passed / 28 failed / 43 errors.
  Every failure is environmental on this host (no live postgres, LLM endpoints
  or runtime containers) or xdist env-pollution -- the same subset run serially
  passes, and stashing this change reproduces the identical failure set.
- Tests covering the change directly (tests/unit/nodes/node_runner_fleet_maintain
  + tests/ci): 1004 passed, 1 skipped.
…s-pin gate stops failing on rate limits

Clears the red `Tests (Split 1/15)` -> `CI Tests Gate` ->
`Test-Failure Ratchet Gate` -> `CI Summary` chain, which is the only remaining
failure on this PR.

Root cause: tests/integration/ci/test_workflow_uses_refs_resolve_live.py
resolves every cross-repo `uses:` pin against the GitHub contents API and FAILS
CLOSED on an unverifiable pin. Neither pytest step passed a token, so the calls
were unauthenticated and shared the runner egress IP 60/hr budget. All 15 pins
came back HTTP 403 "API rate limit exceeded for 20.169.98.150" -- a red with
nothing to do with the pins themselves. The test names this fix in its own
failure message ("thread GH_TOKEN into the test step or fix runner egress") and
reads GH_TOKEN or GITHUB_TOKEN.

Not a flake and not skip-tokened: this is the missing wiring the gate asks for.
The test module docstring still says it is DELIBERATELY RED while the omniclaude
OMN-14941 PR is unmerged, but that condition has expired -- both previously-404
pins resolve on `dev` today
(call-occ-companion-effect-reusable.yml c540d981,
call-occ-autobind-reusable.yml 5f8f64e4), so 403 was the sole cause.

Blast-radius check before touching a shared workflow: no test in this suite
skips on token presence (`grep -rn "GH_TOKEN|GITHUB_TOKEN" tests/ | grep -i skip`
is empty), so this only authenticates requests that were already being made --
it activates no new tests.

Gates on .200 (rule 11a, patch-transfer; per-file sha256 matched on both hosts):
- ci.yml parses under yaml.safe_load; env block confirmed present on BOTH the
  smart-selection and full-suite pytest steps
- pre-commit --files .github/workflows/ci.yml clean
- tests/ci/test_ci_workflow_resilience.py + test_workflow_uses_refs_resolve.py:
  37 passed
- RED->GREEN on the actual failing test: `CI=1 GH_TOKEN=... uv run pytest
  tests/integration/ci/test_workflow_uses_refs_resolve_live.py` = 1 passed
  (unauthenticated it is the 403 failure CI hit)
- governed selector -> tests/ci/ -> 972 passed, 1 skipped
@jonahgabriel
jonahgabriel merged commit 5dce582 into dev Jul 27, 2026
126 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-15233-runner-healthcheck-recalibration branch July 27, 2026 14:57
jonahgabriel added a commit that referenced this pull request Jul 27, 2026
…tgres image pull, not on a downstream exit 127 (#2501)

* fix(OMN-15249): make the Integration Silent-Skip Guard die AT the Postgres image pull, not on a downstream exit 127

The `integration-guard` job provisioned Postgres via a GitHub-managed
`services:` block. On #2492 that image pull timed out against
registry-1.docker.io inside GitHub's own "Initialize containers" step; every
normal step was skipped, but the verdict step carried a bare `if: always()`,
ran with no toolchain installed, and terminated the job on
`uv: command not found` / exit 127 — several steps removed from the registry
timeout that actually caused it.

- Own the pull: explicit first step with bounded retry (3 attempts, 120s
  per-attempt `timeout`) that fails closed with an `::error::` naming the
  image, the registry, and the timeout.
- Own the container: explicit `docker run` with the same health probe and an
  ephemeral published port, fail-closed on an unhealthy container, plus an
  unconditional `docker rm --force` teardown.
- Gate the verdict on `steps.run_curated_proofs.conclusion != 'skipped'` instead
  of bare `always()`, so the guard cannot report on a run whose Postgres never
  materialized, while still firing on genuine test failures.

Proof: tests/ci/test_integration_guard_pull_fatality.py — 8/8 RED at
origin/dev, 8/8 GREEN here. Executes the workflow's real pull `run:` body as
a bash subprocess against a stubbed docker, and replays the step graph through
a GitHub-`if`-semantics simulator that fails closed on unmodelled conditions.
Live end-to-end on real docker (.201, ephemeral standalone container, no lane
touched): happy path pull/start/port/teardown green; blackholed registry
exhausted 3 bounded attempts in 15s and exited 1 with the named annotation.

* chore(OMN-15249): re-trigger occ-preflight after Evidence-Source: OCC#5149 bind

---------

Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant