Repository navigation
fix(OMN-15749): deploy-agent docker fixture absorbs daemon/registry timeouts - #2826
Conversation
…ry timeouts Every docker call in the OMN-15181 real_container fixture already passed check=False, so a non-zero exit was the fixture's business to interpret. But subprocess.run raises TimeoutExpired regardless of check=, so the one failure mode the fixture could not absorb was the one that actually fires on the shared self-hosted fleet: docker daemon and registry contention. Route all four docker calls through a _docker() helper that collapses a timeout to None, putting it back under the fixture's control: - docker pull is a best-effort warm-up; a timeout no longer errors both tests (the image is usually already cached and docker create is the real gate). - docker create timing out, or failing because busybox never became available locally, skips -- that is an environment condition, not a statement about docker inspect semantics. Any OTHER non-zero exit still raises, so a genuine docker semantic is not swallowed. - docker rm -f timing out at teardown no longer errors a passing test; the container name is uuid-unique so a leaked container cannot collide. Create/remove budgets raised 30s -> 90s/60s per the ticket's option 1, same rationale as OMN-8535 and OMN-7871. Same reasoning as the merged OMN-16317 fix (#2819), which applied it to the sibling bare-sha256 negative test in this directory; this closes the remaining live-network docker calls in the module. Refs: OMN-15749
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Limit details: You’ve used the included review currently available. Your 117 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
OCC autobind did not mint a companion for this PR: no changed-file candidate could be proven RED against the merge base, and emitting a PR-existence probe instead would be non-falsifiable evidence (OMN-15247). Hand-authored evidence is required. |
✅ Hostile Reviewer — PASSEDBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
#6819) * evidence(OMN-15749): OCC companion for OmniNode-ai/omnibase_infra#2826 Hand-authored because OCC autobind formally DECLINED this PR. onexbot-occ-writer commented on omnibase_infra#2826 at 2026-08-21T04:48:24Z with marker `occ-autobind-no-red-derivable:2826`: "no changed-file candidate could be proven RED against the merge base, and emitting a PR-existence probe instead would be non-falsifiable evidence (OMN-15247). Hand-authored evidence is required." That satisfies the pr-landing-ceremony §1.3 precondition for hand-authoring. The probe is content-bound, not a PR-existence check, so it clears the evidence-admissibility gate on all three axes: - EXECUTED — `gh api` is on the allowlist; bare `gh pr view` is not. - FALSIFIABLE — verified GREEN at head cc972ae1a and RED at merge-base 2444b99f1, where the fixture still calls subprocess.run directly and _PULL_TIMEOUT_SECONDS does not exist. Both directions run before the receipt was written; the receipt records the real exit status. - OUTSIDE ITS OWN DIFF — it reads omnibase_infra, which this OCC PR does not author. Uses the OMN-15411 Rule E pipe-safe shape (capture into "$body", then pipe to grep) with the ?ref= URL single-quoted, and `grep -qF --` because the needle would otherwise be parsed as an option. Refs: OMN-15749, omnibase_infra#2826, OMN-15247 (admissibility), OMN-13888 (per-entry hash), OMN-15411 (pipe-safe probe shape) * evidence(OMN-15749): add the OCC self-bind entry for PR #6819 occ-preflight failed post-recovery with reason=pr_ticket_mismatch: no PASS receipt for one or more tickets binds to PR #6819 or one of its commit SHAs: OMN-15749 The product-PR receipt was found and its hashes verified clean (missing_or_nonpass_receipts and stale_receipt_bindings were both empty) -- the gap was purely binding. That receipt carries pr_number 2826, the omnibase_infra PR, so nothing bound OMN-15749 to this OCC PR itself, which is what the eligibility checker resolves against. Every other OCC companion carries such an entry (occ-self-bind-pr-6805, -6809, -6810, -6811); I omitted it because the worked example I copied from, OCC#6814, appends to a contract whose self-bind another PR had already landed. A companion that CREATES its contract needs its own. Adds occ-self-bind-pr-6819 following the established convention, with the probe run live and the real stdout recorded. Also updates the product-PR receipt's contract_sha256, which necessarily moved when the file gained an entry -- both contract_entry_sha256 values are unchanged, per OMN-13888. Refs: OMN-15749, omnibase_infra#2826, OMN-13888
1 participant
Ticket
OMN-15749 — CI: OMN-15181 deploy-agent digest-verification fixture tests time out on real
docker create/docker rm -funder runner contention.Problem
scripts/deploy-agent/tests/unit/test_executor_digest_verification_image_id_omn15181.pybuilds a real, disposable busybox container to pin actualdocker inspectsemantics. All four of its docker calls already passedcheck=False— the author's intent was that a non-zero exit is the fixture's business to interpret, not an automatic test failure. Butsubprocess.runraisesTimeoutExpiredregardless ofcheck=, so the one failure mode the fixture could not absorb is the one that actually fires on the shared self-hosted fleet: docker daemon and registry contention.Three separate PRs have been red on this, none of them related in content:
96427221459(2026-08-20T12:54:17Z) —subprocess.TimeoutExpired: Command ['docker', 'pull', 'busybox:latest'] timed out after 90 seconds, erroring both tests in the class at fixture setup. This is the live signature that motivated the fix.93142435731—docker rm -f ...timed out after 30 seconds at teardown, erroring an otherwise-passing test.93142579498—docker create --name ... busybox:latesttimed out after 30 seconds, erroring both tests at setup.Deploy Agent Tests (OMN-15378) / deploy-agent-testsis a required merge gate, so each occurrence blocks a PR on a condition that says nothing about the code under test.Fix
Route all four docker calls through a
_docker()helper that collapses a timeout toNone, putting the timeout back under the fixture's control:docker pullis a best-effort warm-up. A timeout no longer errors both tests — the image is usually already cached on the runner, anddocker createis the real gate.docker createtiming out, or failing because busybox never became available locally, skips. That is an environment condition, not a statement aboutdocker inspectsemantics, and the class is already@requires_docker-gated on exactly that kind of condition. Any other non-zero exit still raisesAssertionError, so a genuine docker semantic is not swallowed.docker rm -ftiming out at teardown no longer errors a passing test. The container name is uuid-unique, so a leaked container cannot collide with a later run.Create/remove budgets raised 30s → 90s/60s per the ticket's option 1, same rationale as OMN-8535 (deploy-agent build timeout 300s → 900s) and OMN-7871.
This is the same reasoning as the already-merged OMN-16317 fix (infra#2819, commit
0289995d7), which applied it to the siblingtest_bare_sha256_is_an_invalid_pull_referencenegative test in this same module. That fix covered one call site; this one closes the remaining live-network docker calls in the file.dod_evidence
Branch behaviour proven directly. All five fixture paths were driven with a stubbed
subprocess.run(throwaway probe, not committed):docker pulltimes out,createOKdocker createtimes outSkipped, not an errordocker createfails "No such image"Skipped, not an errordocker createfails "conflict: name already in use"AssertionError— real errors still surfacepull/90s,create/90s,rm/60sThe fourth row is the important one: the gate is not weakened, only the environment-conditional paths are.
Local verification:
uv run --project scripts/deploy-agent --extra dev pytest scripts/deploy-agent/tests -q→198 passed, 8 skipped(the 8 skips are the@requires_dockerclasses; this Mac has no docker daemon).mypyon the changed file: 11 errors before, 11 after — no new error introduced. All 11 pre-date this change and are outside CI's mypy scope (ci.ymlrunsmypy src/omnibase_infra).uv run ruff format/ruff checkonscripts/deploy-agent/: clean.pre-commit run --all-files: 92 hooks, 0 failures.CI acceptance signal: on the self-hosted runners docker is reachable, so
Deploy Agent Testsmust report 203 passed — the same count as a healthy pre-change run. A drop to201 passed, 2 skippedwould mean the fixture is wrongly skipping on a healthy runner and this PR should not merge.Context
Found while diagnosing two "recurring CI failure clusters" across open
omnibase_infraPRs. The other cluster (Kafka Schema Handshake) is entirely environmental — runner-fleetgit fetchfailures downstream of the OMN-16284 burst incident — and needs no code change; that analysis is recorded on OMN-16284 and OMN-16030. This fixture defect was the only genuine mechanical root fix in either cluster.Evidence-Source: OCC#6819
Evidence-Ticket: OMN-15749