fix(status): wait for inference after gateway recovery - #8572
Merged
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSandbox status checks now retry inference-route probes after successful managed-gateway recovery. Tests cover recovery and non-recovery paths. Three unrelated integration tests receive longer timeouts. ChangesInference recovery probing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxStatusSnapshot
participant ManagedGateway
participant InferenceRouteProbe
participant RecoveryProbeDelay
ManagedGateway->>SandboxStatusSnapshot: report successful recovery
SandboxStatusSnapshot->>InferenceRouteProbe: run authoritative probe
InferenceRouteProbe-->>SandboxStatusSnapshot: return probe result
SandboxStatusSnapshot->>RecoveryProbeDelay: wait two seconds after failure
RecoveryProbeDelay-->>SandboxStatusSnapshot: permit next probe
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 13ee280 in the TypeScript / code-coverage/cliThe overall coverage in commit 13ee280 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/status-snapshot.ts (1)
571-578: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the recovery probe loop into a private helper.
collectSandboxStatusSnapshotalready owns several lifecycle paths. Move this retry loop to a focused helper that acceptssandboxName,recoveredManagedGateway,probe, anddelay. Keep the action as the workflow owner.As per coding guidelines, “Keep function complexity low.”
🤖 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 `@src/lib/actions/sandbox/status-snapshot.ts` around lines 571 - 578, Extract the retry loop from collectSandboxStatusSnapshot into a private helper that accepts sandboxName, recoveredManagedGateway, probe, and delay, and returns the resulting gatewayChain. Replace the inline loop with a call to this helper while keeping collectSandboxStatusSnapshot responsible for the surrounding workflow and preserving the existing retry counts and delay behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/lib/actions/sandbox/status-snapshot.ts`:
- Around line 571-578: Extract the retry loop from collectSandboxStatusSnapshot
into a private helper that accepts sandboxName, recoveredManagedGateway, probe,
and delay, and returns the resulting gatewayChain. Replace the inline loop with
a call to this helper while keeping collectSandboxStatusSnapshot responsible for
the surrounding workflow and preserving the existing retry counts and delay
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 63a3c6b7-59b9-4e76-b72a-70a8e5470db9
📒 Files selected for processing (2)
src/lib/actions/sandbox/status-snapshot-inference-health.test.tssrc/lib/actions/sandbox/status-snapshot.ts
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
5 tasks
prekshivyas
added a commit
that referenced
this pull request
Aug 8, 2026
) ## Summary First incremental step on #6014. The Ollama auth proxy now independently verifies that the Ollama backend is listening only on loopback before declaring itself ready, with a structured exit signal the host CLI renders as a specific actionable remediation. Leaves the existing root-level systemd loopback override (#5996, #5716) in place for this PR. ## Why an independent probe in the proxy The proxy currently trusts the systemd drop-in to keep Ollama bound to `127.0.0.1`. If a user manually edits `OLLAMA_HOST` to `0.0.0.0`, the proxy still forwards to `127.0.0.1:11434` successfully (Ollama listens there too) but Ollama is ALSO publicly reachable on `0.0.0.0:11434`, bypassing the proxy's bearer-token check entirely. The new probe runs before `server.listen` and refuses to start with exit code 2 if it sees any non-loopback listener on the backend port. This moves bind-policy enforcement off the root-coupled systemd path and onto the proxy itself; the systemd drop-in becomes pure defense-in-depth that subsequent PRs can retire. ## Changes - `scripts/ollama-auth-proxy.mts`: - Independently enumerates backend listeners through `/proc/net/tcp{,6}` with an `lsof` fallback and refuses any non-loopback listener before `server.listen`. - Recognizes the full `127.0.0.0/8`, `::1`, and IPv4-mapped IPv6 loopback shapes both in listener classification and in deciding whether a local backend URL requires the probe. - Writes structured startup failure status for host-side remediation and retains the explicit audited operator override. - Is fully checked by `tsconfig.cli.json`; no `@ts-nocheck` suppression remains. - Keeps side effects inside `main()`, gated by `import.meta.main`, while exporting typed helpers for focused tests. - `src/lib/inference/ollama/proxy.ts`: - Persist a sentinel path (`~/.nemoclaw/ollama-auth-proxy.status`) and pass it to the spawned proxy via env - On proxy spawn, unlink any stale status file so a later read sees the new proxy's reason - When the readiness loop observes the proxy gone, read the status file via `readProxyExitStatus` and render specific remediation via `printProxyStartupReason` for the `backend-not-loopback` reason; fall back to existing port-conflict or generic message when no status file is present - `test/ollama-auth-proxy-bind-probe.test.ts`: 40 Vitest cases cover listener parsing, all supported loopback encodings, explicit rejects, local-versus-remote backend trigger selection, the exit-code contract, and Linux `/proc` integration. ## What this does NOT do (follow-up PRs per #6014) - Does not delete `ensureOllamaLoopbackSystemdOverride`. The systemd drop-in still runs on Linux and stays the authority for Ollama's bind on a fresh install. The probe is independent enforcement on top, not a replacement. - Does not relocate `OLLAMA_CONTEXT_LENGTH` or the Spark `OLLAMA_LLM_LIBRARY=cuda_v13` overrides off the systemd drop-in. Those are load-bearing for non-security reasons and belong in a follow-up that moves them to a config-only path before the drop-in writer can be deleted. - Does not add periodic re-probing during proxy lifetime; the current PR only checks at startup. A follow-up could probe periodically to catch mid-run bind changes. - Does not cover Docker-Desktop topologies (WSL + Windows-host Ollama, WSL + WSL-local Ollama). Those bypass the proxy entirely via `containerCanReachHostLoopback()` and are out of scope per #6014. ## Verification - `npx vitest run test/ollama-auth-proxy-bind-probe.test.ts` — 38 passed, 2 platform skips on macOS - Eight focused Ollama proxy suites — 110 passed, 2 platform skips - `npm run typecheck:cli` — passed with the proxy script fully type-checked - `npm run checks:repository` — repository architecture and source-shape checks passed - `npm run docs` — 0 errors, 2 existing warnings - `src/lib/shields/policy-transition.test.ts` carries the exact one-line setup-hook stabilization from upstream PR #8572 (commit `78f681e72`) after current-main CI reproduced the 10-second hook timeout three times on this PR. ## Related - Issue #6014 (architectural follow-up) - PR #5996 (the symptom fix that opened #6014) - Issue #5716 (the user-visible bug) ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Documentation Writer Review <!-- Required for code and documentation changes after the changes and applicable validation are complete. Keep one review checkbox and one instance of each visible or hidden field. For Evidence, list changed documentation paths. For documentation-only changes, also state that the writing rules and documentation style were reviewed. For other results, explain why no documentation change is needed or why the review is blocked. For Agent, use a consistent product and surface name, such as Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all review changes, put `git rev-parse --short HEAD` and `git rev-parse --short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review and refresh that metadata after any new commit. This receipt is advisory during the data-collection pilot. --> - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `SECURITY.md`. Independent Codex Desktop review passed for exact head `74431d39a`. The threat model accurately documents the Ollama auth proxy loopback bind probe, its full `127.0.0.0/8`, `::1`, and IPv4-mapped IPv6 loopback coverage, non-loopback refusal, operator override, unavailable-probe fallback, startup-only enforcement, regression coverage, and scope limits. Removing `@ts-nocheck` preserves behavior, the broadened trigger aligns all recognized loopback backend hostnames with that documented guarantee, and the Vitest setup-hook timeout change requires no additional documentation. - Agent: Codex Desktop <!-- docs-review-head-sha: 74431d3 --> <!-- docs-review-agents-blob-sha: 12ad395 --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added structured proxy startup failure “status file” and clearer readiness-loop diagnostics. * Enhanced the Ollama auth proxy with Bearer-token authentication and loopback-only backend enforcement. * **Bug Fixes** * Improved startup failure reporting by surfacing a specific “backend-not-loopback” reason and remediation guidance when misconfigured. * Improved proxy forwarding error responses with consistent HTTP status handling. * **Tests** * Added Vitest coverage for loopback bind/probe detection, address classification (proc/net and lsof), and contract constant assertions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
cv
approved these changes
Aug 9, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings. The bounded retry runs only after successful managed gateway recovery, preserves single-probe ordinary status behavior, and fails closed after exhaustion.
23 tasks
23 tasks
wscurran
added
bug-fix
22 tasks
ericksoa
added a commit
that referenced
this pull request
Sep 4, 2026
<!-- markdownlint-disable MD041 --> ## Outcome `nemoclaw <sandbox> status` no longer exits nonzero for a Ready sandbox solely because its first in-sandbox inference request receives a transient gateway or availability response. The command retries HTTP `429`, `502`, `503`, and `504` up to three total attempts, with a two-second delay. It reports success when a later request succeeds and preserves the existing unhealthy result when every attempt fails. Before each retry, it reports the failed probe, next attempt, and delay on stderr without contaminating JSON stdout. ## Reason `collectSandboxStatusSnapshot` already used a bounded retry primitive, but an ordinary status run supplied no retry delays. Only a status run that had just recovered a stopped managed gateway made multiple attempts. A Ready sandbox with a live gateway therefore treated one transient provider response as authoritative even though route reachability, upstream health, and sandbox phase were all healthy. Onboarding already treats HTTP `429`, `502`, `503`, and `504` as transient. This PR gives status probes the same classification without adding another retry layer to normal agent traffic. ### Related issues Fixes #10709 Related to #10631. That issue reproduces a direct NVIDIA cloud endpoint `503` with a correctly entitled key. This PR mitigates the false-negative status result caused by one such response; it does not claim to repair provider capacity or add a provider-owned `Retry-After` header, so it does not close #10631. ## Changes - Define the four transient inference-probe statuses once in `src/lib/inference/probe/transient-http-policy.ts` and reuse that definition in onboarding validation and sandbox status. - Retry the route and invocation probe pair up to three total attempts only after an invocation returns HTTP `429`, `502`, `503`, or `504` during an ordinary status run. - Keep HTTP `401`, `403`, `404`, and `500`, malformed 2xx responses, statusless failures, and failing route probes final on the first attempt during an ordinary run. - Preserve the recovered-managed-gateway behavior from #8572, which retries every failed route or invocation probe while the restarted chain settles. - Report every scheduled retry on stderr with the failed HTTP status or probe boundary, next attempt, total attempts, and delay. - Preserve request timeout, final error classification, bounded response capture, JSON stdout, and redacted diagnostics. - Document the retry trigger, token cost, timeout envelope, permanent-auth behavior, and final unhealthy result across the owning status pages. ## Retry ownership The OpenShell/NemoClaw gateway remains a pass-through and does not replay agent POST or streaming requests. Supported agents already own bounded runtime retries: - OpenClaw uses the OpenAI client retry policy for HTTP `503` and propagates request cancellation and timeout signals. - Hermes classifies HTTP `503` as overloaded, retries within its bounded backoff policy, and checks interruption before and during backoff. - NemoClaw onboarding validation already retries the same transient status set with its bounded validation schedule. Adding another gateway retry would multiply attempts across layers and could replay partially observed POST or streaming work. Permanent authorization responses remain outside this PR's transient set. ## Verification - `npx vitest run --project cli src/lib/actions/sandbox/status-snapshot-inference-health.test.ts src/lib/actions/sandbox/inference-route-health.test.ts src/lib/inference/openai-validation-session-fallback.test.ts` — 3 files and 88 tests passed before the diagnostic follow-up. - `npx vitest run --project cli src/lib/actions/sandbox/status-snapshot-inference-health.test.ts` — 34 tests passed after the diagnostic follow-up, including stderr progress and terminal no-retry assertions. - `npm run typecheck:cli` — passed. - `npm run docs` — passed; Fern reported 0 errors and 5 existing warnings after rendering the OpenClaw, Hermes, and Deep Agents variants. - `npx oxfmt --check` and focused `npx oxlint` on the changed TypeScript files — passed. - `npx tsx scripts/check-docs-published-routes.mts` — passed for 69 guarded pages. - `npm run checks:repository` — passed, including the live E2E assertion census after merging the mainline correction from #11001. - `npm run validate:pr` — passed on the exact head, covering pre-commit, commit-message, and pre-push checks. - `npm run test:changed` — the growth guard passed 45 tests and six of seven affected files passed. One unrelated Oclif adapter test exceeded its 5-second timeout while several other worktrees saturated the local machine; an isolated replay also timed out under the same load. The exact-head hosted CLI shards all pass, so the test timeout policy is unchanged. Exact-head hosted validation passes all 12 CLI shards, aggregate CLI tests, build/typecheck, plugin tests, installer integration, repository checks, docs validation and parity, CodeQL, and ShellCheck. All nine PR Review Advisor specialists report no defect. The preceding exact-code head passed the full self-hosted PR qualification matrix, including gateway isolation. Current required checks are blocked only by a repository-wide npm audit endpoint outage: the standalone audit and both image architectures timed out after their three built-in attempts and one targeted rerun, while four concurrent full PR runs show the same failure. Further retries are held until a newer GitHub-run audit demonstrates recovery. ## Review notes The retry is bounded at three attempts and has a narrow response signature. It does not retry HTTP `500`, permanent authorization failures, invalid successful responses, requests without an HTTP status, or a failed route probe on an ordinary run. A persistent `503` still reports `unhealthy` and exits nonzero with the final diagnostic. All CodeRabbit threads are resolved. CodeRabbit confirmed that `151b69a11` fixes its final exact-head documentation finding by describing one request per attempt and naming HTTP status as optional retry evidence. The earlier PR Review Advisor operability finding is corrected in `3571b140a`: scheduled retries are visible on stderr and the documented timeout envelope reflects all three possible probe pairs. --- Signed-off-by: Hai Nguyen <haingu@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved inference status checks by retrying transient failures (HTTP 429, 502, 503, and 504) while avoiding retries for authorization failures and other final errors. * Managed gateway recovery now retries route and inference checks appropriately, with persistent failures correctly reported as unhealthy. * **Documentation** * Clarified inference status outcomes, retry behavior, and route verification. * Updated troubleshooting and Hermes guidance for managed forwarding, recovery, runtime-specific checks, and credential handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Hai Nguyen <haingu@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sandbox status now waits for the authoritative inference route to converge after it automatically recovers a stopped agent gateway. Ordinary status checks remain single-probe, and persistent recovery failures still fail closed.
Changes
inference.localup to three times, with two-second delays, only after that recovery.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/reference/commands.mdxalready states that status restores the gateway and host forwards before probing inference.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests