Repository navigation
fix(OMN-15642): remediation — unwire attach-readiness fold from the gated /health status - #2619
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 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 (4)
Comment |
#5912) * evidence(OMN-15642): add OCC companion for infra 2619 * evidence(OMN-15642): self-bind OCC 5912 --------- Co-authored-by: Jonah Gray <jonah@omninode.ai>
|
| 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)
…gated /health status An adversarial verifier found the first version of this fix (merged as 363120f / PR #2618) is a live deploy-wedge risk, not a safe visibility fix: folding boot attach-readiness DEGRADED into /health's gated status converts OMN-13237's deliberately-non-fatal per-contract NOT_READY skip (ModelRuntimeAttachReadiness's own docstring: attach status is not a source of truth for contract lifecycle) into a hard boot/deploy failure for four real automated consumers with no DEGRADED tolerance -- reusable-runtime-boot.yml, deploy_agent/executor.py, verify_stability_refresh.py, verify_dev_refresh.py. A single unprovisioned topic (documented live on onex-dev, OMN-15330) would wedge every future boot. Revert the fold from _handle_health entirely (both DEGRADED and the currently-unreachable-in-prod FAILED/core_gap case, for consistency); keep it on _handle_health_detailed, which is unread by all four consumers (verified) and by deploy-onex-staging (also verified -- that workflow never reads either /health endpoint, so neither version of this fix ever closed the gap the original PR body claimed to close there). Removes the now-dead elif status=="degraded" http_status branch on /health/detailed (the only pre-fold paths reaching it already set http_status=200). RED-before/GREEN-after (mutation-proven on .200): 2 of the rewritten TestAttachReadinessJoin assertions fail against the merged 363120f source (exists-but-wrong: degraded/unhealthy where healthy is now required), pass after this diff. 53/53 in the 3 touched test files, 167/167 across the full focused set. ruff+mypy+pre-commit clean. Honest scope: this PR does NOT close AC2/AC3 for the correlation_id regression itself (delegation projection / system-event rows absent by correlation_id on onex-dev) -- see the PR body for the live-access attempt and the additional bisection evidence gathered without it. OMN-15642
7e721eb to
1d6ab43
Compare
#5916) * evidence(OMN-15642): bind rebased infra 2619 head * evidence(OMN-15642): self-bind OCC 5916 --------- Co-authored-by: Jonah Gray <jonah@omninode.ai>
OMN-15642 remediation round
This is a follow-up to #2618 (already merged as
363120f4c), fixing 6 verifier-found defects in that PR on the SAME branch. #2618's own head branch (jonah/omn-15642-build-lane) was auto-deleted by GitHub after merge, so this reopens as a new PR from the recreated branch (see "Branch history" below) rather than reusing PR #2618, which stays MERGED.What was wrong with #2618
An independent adversarial verifier found 6 defects. This PR fixes the safety-critical ones (3-6) directly; 1-2 are addressed honestly, not falsely closed (see "Honest scope").
/health's top-levelstatusis a hard, no-DEGRADED-tolerance gate for four real automated consumers —.github/workflows/reusable-runtime-boot.yml(jq -e '.status=="healthy" ...' || exit 1, 6 call sites),scripts/deploy-agent/deploy_agent/executor.py's_runtime_health_passed, andscripts/runtime_build/verify_stability_refresh.py/verify_dev_refresh.py. None were audited before fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618 merged.ModelRuntimeAttachReadiness's own docstring says attach-readiness "is not a source of truth for contract lifecycle," and OMN-13237 deliberately made a NOT_READY/DEGRADED contract non-fatal (recorded, skipped, never restart/redeploy-triggering). fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618 folded that DEGRADED state into/health's gated status anyway — the first ordinary NOT_READY skip (e.g. one unprovisioned topic, a documented live condition on onex-dev per OMN-15330) would fail every one of the four consumers above on every future boot/deploy.deploy-onex-staging.ymlnever reads/healthor/health/detailedat all (verified:grep -nEi 'health|readyz|/ready'shows only k8s/ready, omnidash's/api/health/data-sources, and consumer-group health) — so fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618's stated purpose (closing deploy-onex-staging's blind spot) was never true for either endpoint, and its actual effect was entirely in the four consumers in (3)/(4), unaudited.elif status == "degraded": http_status = 200on/health/detailedcould never change anything (every pre-fold path reaching "degraded" already sethttp_status=200, and the fold never returns "degraded" from a pre-fold "unhealthy").The fix
_handle_health(/health, the gated liveness endpoint):fold_attach_readiness_into_statuscall removed entirely — for both DEGRADED and the currently-production-unreachable FAILED/core_gapcase, so there's no partial/inconsistent behavior depending on severity./health'sstatusis pinned back to its pre-OMN-15642 behavior: attach readiness never changes it. The aggregate stays fully visible via the pre-existing (OMN-15512)details.components.runtime_wiringblock — just not gated._handle_health_detailed: keeps the fold (both DEGRADED and FAILED/core_gap) — verified unread by any of the four hard-gate consumers above, and bydeploy-onex-staging.yml, so it's safe and still closes the original human/dashboard visibility gap this ticket's incident motivated.elif status == "degraded": http_status = 200branch on/health/detailed, with a comment explaining why it was always a no-op.fold_attach_readiness_into_status) to state the caller-scoping decision explicitly, so a future caller doesn't re-wire this into/healthwithout re-reading the wedge-risk analysis.Seam definition
fold_attach_readiness_into_status(payload_status, readiness_state) -> status(unchanged signature/behavior) — now called byServiceHealth._handle_health_detailedonly./healthJSONstatusfield: no longer readsself._attach_readinessat all (reverted to pre-OMN-15642 shape).details.components.runtime_wiring(unchanged, OMN-15512) remains the only place/healthsurfaces the aggregate./health/detailedJSONstatus/http_status: unchanged from fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618 (DEGRADED→"degraded"/200, FAILED→"unhealthy"/503), minus the dead branch.Acceptance-criteria mapping (this round)
tests/unit/runtime/test_service_health_attach_readiness.py::TestAttachReadinessJoin::test_degraded_attach_readiness_does_not_degrade_gated_health_status,::test_core_gap_does_not_flip_gated_health_status_either/health's gatedstatusstays"healthy"under both DEGRADED and FAILED attach-readiness states — the four consumers'status=="healthy"assertion can no longer be broken by this mechanismgrepoutput above (no test possible — proving a negative about a different repo's workflow)test_detailed_endpoint_flips_http_status_on_core_gap/test_detailed_endpoint_also_folds_attach_readinessstill cover the two real (non-dead) http_status transitions on/health/detailedRED-before/GREEN-after (mutation-proven on
.200): stashed only the two source files (kept the rewritten tests), ran against the merged363120f4csource — 2 of 53 tests in the touched files fail (assert 'degraded' == 'healthy',assert 'unhealthy' == 'healthy'— exists-but-wrong, not merely absent), restored, reran — 53/53 pass. 167/167 across the full focused set (health/attach-readiness/wiring/serialization). ruff format+check clean, mypy clean,pre-commit run --files <changed>100% Passed/Skipped, governed pre-push selector escalated to the full suite (shared module) — 23036 passed, 40 skipped, 0 failed in 454s.Honest scope (defects 1-2, NOT closed here)
This PR does not close AC2 ("bisect the correlation seam on the live plane") or AC3 ("a test that fails at 72d27ef and passes at the fix") for the correlation_id regression itself. Delegation-projection and system-event-stream rows are still absent by
correlation_idon onex-dev as of this PR.Live access attempt (repeated from #2618, same result):
aws sts get-caller-identity— live (account 272493677981).kubectl --kubeconfig ~/.kube/omninode-mvp1 get pods -n omninode-k3s-dev-system(corrected namespace, peromni_home/docs/runbooks/DEPLOY_ENVIRONMENTS.md— not theonex-devnamespace I first guessed) — empty, andkubectl config get-contextsshows only a placeholderdefault/default/defaultcontext with no live cluster (cluster-infohangs to timeout). Per memoryreference_k3s_cloud_deploy_surface("SSM-not-kubectl"), direct kubectl access needs an SSM tunnel this lane did not have time to stand up. Same infeasibility the previous lane hit, confirmed independently.Additional bisection done without live access (static, code-level, clearly INFERENCE not proof —
omnibase_infraonly, the repo in scope for this ticket):node_projection_delegation(the delegation projection) and the unified system-event-stream writer are not implemented in this repo (grep -rln "DelegationProjection\|node_projection_delegation" src/→ zero hits outside the vendored migration SQL from OMN-15503). They're owned byomnimarket. This repo's role is limited to (a) the runtime engine that wiresomnimarket's contracts' Kafka subscriptions at boot, and (b) whatever ingress code setscorrelation_idon outbound envelopes.cc5ec9253(OMN-15546, the ticket's prime suspect) independently: it modifiesvalidate_runtime_local_ingress_payloadinruntime_local_ingress.py, which is the local-ingress path (used by e.g.onex delegateCLI dispatch), not the gateway-drivenPOST /v1/workflowspath the business-proof gate actually drives. Concur with fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618's ruling-out, on independent re-reading.subscribe_wired_contract_topics(handler_wiring.py), a contract with zero registered dispatchers (if not result.dispatchers_registered: ... continue, an OMN-15474-adjacent path) is filtered out ofeligiblebefore_interleave_contractruns — such contracts never appear inattach_results/attach_results_outand therefore never appear inModelRuntimeAttachReadiness.resultsat all. They are invisible to the DEGRADED signal, not merely counted as a NOT_READY/FAILED entry within it. If the delegation-projection or system-event-stream consumer's contract lost its dispatcher registration entirely in this window (as opposed to attaching-then-failing), the symptom would show with no visible DEGRADED signal anywhere, including incomponents.runtime_wiring— which would also mean fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618's fix (even before this revert) could never have caught it. I could not confirm this against a live boot log or manifest within this lane's time; flagging as a genuinely different, deeper hypothesis than fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618's._require_registered_contract_dispatcher_scope(the function fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618's body attributed the DEGRADED mechanism to) is called outside any try/except in_interleave_contract— if it ever raised there, the exception would propagate through the unguardedasyncio.gatherinsubscribe_wired_contract_topics, not degrade gracefully. In practice this never fires because the zero-dispatcher pre-filter above removes every contract that would trigger its empty-scope check before it's reached. Noted for whoever picks up AC2/AC3 next — this function's failure mode is NOT what fix(OMN-15642): fold boot attach-readiness into /health status (close false-green consumer-drop gap) #2618 described.Recommend as a genuine follow-up (not absorbed here, and not by this repo alone): live cluster access (via SSM, not kubectl directly) to read the actual boot-time wiring report / dispatcher registration for the delegation-projection and system-event-stream contracts on the corrected tree, cross-repo with
omnimarketif the owning contract's dispatcher registration is what changed.Branch history
jonah/omn-15642-build-lane's original head (5de9e9a79) was squash-merged as363120f4c(PR #2618, 2026-08-02T00:01:31Z) and GitHub auto-deleted the branch. This PR's branch was recreated fromorigin/dev(which already contains363120f4c) plus this remediation commit on top — confirmed viagit diff 5de9e9a792 origin/dev -- <the 4 touched files>returning empty before making any new edits, i.e. the new commit's parent tree for these files is byte-identical to what #2618 already landed.Gates
Run on
stickybeatz-studio(.200) via patch-transfer + sha256 verification (all 4 changed files matched local before any gate ran, and again after the RED/GREEN stash round-trip):ruff format --check/ruff check— cleanmypyon both changed source files — cleanpre-commit run --files <changed>(viazsh -lcfor the interactive PATH /uv) — 100% Passed/Skippedgit pushpre-push hook chain: mypy, architecture validation, governed impacted-test selector escalated to the full suite (shared module —health_checker.pyis broadly imported) — 23036 passed, 40 skipped, 0 failed in 454s, deploy-scope DoD parity NOTICE (OCC companion not yet merged, expected pre-merge)OMN-15642
Evidence-Source: 09c5dd565e7ded968640151d0c968366f2485abf
Evidence-Ticket: OMN-15642