fix(status): wait for inference after gateway recovery - #8572
Open
jyaunches wants to merge 11 commits into
Open
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSandbox status checks record successful managed-gateway recovery and retry the inference route up to three times with two-second delays. Tests cover recovery outcomes, ordinary status lookup, test timeouts, and the updated approval documentation fixture. ChangesInference recovery probing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxStatusSnapshot
participant ManagedGateway
participant InferenceRoute
participant ProbeDelay
ManagedGateway->>SandboxStatusSnapshot: complete successful recovery
SandboxStatusSnapshot->>InferenceRoute: perform inference probe
InferenceRoute-->>SandboxStatusSnapshot: return probe result
SandboxStatusSnapshot->>ProbeDelay: wait two seconds after failure
ProbeDelay-->>SandboxStatusSnapshot: allow next probe
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
![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. 3 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: 1 optional E2E recommendation
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>
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
1 participant
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