fix(inference): share the Ollama auth-proxy token across gateway ports - #8736
Open
laitingsheng wants to merge 1 commit into
Open
fix(inference): share the Ollama auth-proxy token across gateway ports#8736laitingsheng wants to merge 1 commit into
laitingsheng wants to merge 1 commit into
Conversation
The auth proxy binds one host port for every gateway, but its token, PID, backend URL, and status files lived under the gateway-scoped state root. Onboarding a second gateway port found no token there, minted one, and rebound the shared port, so the first gateway's sandboxes got HTTP 401. Read and write that state from a shared host root, adopt a token an earlier gateway-scoped run left behind, stop relocating the token and PID during a legacy port migration, and read the same root for the status auth-proxy probe. Treat the proxy files as shared host state during uninstall so a scoped run neither reports a phantom default gateway nor stops a proxy a sibling gateway still uses. Fixes #8704 Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe Ollama auth proxy now stores its token and backend state in a host-shared location across gateway ports. Legacy credentials migrate into shared state. Migration, uninstall behavior, tests, and documentation reflect the shared proxy model. ChangesShared Ollama proxy state
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GatewayOnboarding
participant OllamaAuthProxy
participant SharedLocalAdapterState
GatewayOnboarding->>OllamaAuthProxy: start proxy for gateway port
OllamaAuthProxy->>SharedLocalAdapterState: load shared token and backend URL
OllamaAuthProxy->>SharedLocalAdapterState: adopt legacy gateway token when needed
OllamaAuthProxy-->>GatewayOnboarding: persist and use shared token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
provider: ollama
Contributor
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8736.docs.buildwithfern.com/nemoclaw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ollama-proxy-recovery.test.ts (1)
843-851: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover gateway-scoped backend URL adoption.
Lines 126-129 in
src/lib/inference/ollama/proxy.tsalso migrateollama-backend. This helper cannot seed or assert that value. A regression that resets the backend URL to the default would keep this token-only test passing.Seed a custom gateway-scoped backend URL. Capture
OLLAMA_BACKEND_URLfrom the spawned proxy. Assert that shared state stores and reuses that URL.As per path instructions, review tests for behavioral confidence rather than implementation lock-in.
🤖 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 `@test/ollama-proxy-recovery.test.ts` around lines 843 - 851, Extend the test around runSecondGatewayProxyStart to seed a custom gateway-scoped backend URL, capture OLLAMA_BACKEND_URL from the spawned proxy, and assert that shared state stores and reuses that custom URL alongside the scoped token. Keep the assertions focused on adoption behavior rather than implementation details.Source: Path instructions
🤖 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 `@src/lib/inference/ollama/proxy.ts`:
- Around line 119-130: Add host-wide cross-process locking around the shared
Ollama proxy startup and recovery flows, covering token selection, token
persistence, and proxy restart in startOllamaAuthProxy and
ensureOllamaAuthProxy. Ensure concurrent gateway onboarding and recovery cannot
leave the shared token, PID, and running proxy inconsistent, and add a test that
exercises concurrent processes.
---
Nitpick comments:
In `@test/ollama-proxy-recovery.test.ts`:
- Around line 843-851: Extend the test around runSecondGatewayProxyStart to seed
a custom gateway-scoped backend URL, capture OLLAMA_BACKEND_URL from the spawned
proxy, and assert that shared state stores and reuses that custom URL alongside
the scoped token. Keep the assertions focused on adoption behavior rather than
implementation details.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3cda04d5-5910-4114-a1e0-8c4701f18c01
📒 Files selected for processing (12)
ci/source-architecture-budget.jsondocs/inference/set-up-ollama.mdxdocs/reference/architecture.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxsrc/lib/actions/uninstall/run-plan.tssrc/lib/inference/local-adapter-lifecycle.tssrc/lib/inference/local.test.tssrc/lib/inference/local.tssrc/lib/inference/ollama/proxy.tssrc/lib/state/legacy-port-migration.tstest/ollama-proxy-recovery.test.ts
Comment on lines
+119
to
+130
| return readLocalAdapterTextFile(PROXY_TOKEN_PATH) ?? adoptGatewayScopedProxyToken(); | ||
| } | ||
|
|
||
| function adoptGatewayScopedProxyToken(): string | null { | ||
| if (GATEWAY_SCOPED_PROXY_TOKEN_PATH === PROXY_TOKEN_PATH) return null; | ||
| const token = readLocalAdapterTextFile(GATEWAY_SCOPED_PROXY_TOKEN_PATH); | ||
| if (!token) return null; | ||
| const backendUrl = | ||
| readLocalAdapterTextFile(GATEWAY_SCOPED_PROXY_BACKEND_PATH) ?? | ||
| readLocalAdapterTextFile(PROXY_BACKEND_PATH); | ||
| persistProxyToken(token, backendUrl || undefined); | ||
| return token; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/lib/inference/ollama/proxy.ts --items all
rg -n -C 8 'ensureOllamaAuthProxy|startOllamaAuthProxy|loadPersistedProxyToken|persistProxyToken|randomBytes|lock|flock|acquire' \
src/lib/inference/ollama/proxy.ts
rg -n -C 6 'concurrent|second gateway|gateway port|ollama auth proxy' \
test src/lib/inference --glob '*test.ts'Repository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- proxy imports and lifecycle ---'
sed -n '1,55p;370,505p' src/lib/inference/ollama/proxy.ts
printf '%s\n' '--- lock implementations and proxy callers ---'
rg -n -C 5 'withAdapterLock|tryAcquireAdapterLock|LOCK_PATH|adapter lock|acquire.*lock|lock.*acquire' src test \
-g '*.ts' -g '*.tsx' -g '*.js' | head -n 240
printf '%s\n' '--- proxy test concurrency coverage ---'
sed -n '738,890p' test/ollama-proxy-recovery.test.ts
rg -n -C 8 'ensureOllamaAuthProxy\(|startOllamaAuthProxy\(' src test -g '*.ts' | head -n 240Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lock-related source files ---'
rg -l 'withAdapterLock|tryAcquireAdapterLock|LOCK_PATH|flock|mutation lock|lifecycle lock' src/lib \
-g '*.ts' | sort
printf '%s\n' '--- lock imports and definitions ---'
rg -n -C 8 'withAdapterLock|tryAcquireAdapterLock|LOCK_PATH|flock|mutation lock|lifecycle lock' \
src/lib/inference/https-pin-runtime-adapter.ts \
src/lib/inference/ollama \
src/lib/actions/sandbox/connect.ts \
src/lib/onboard/setup-nim-ollama.ts \
src/lib -g '*.ts' | head -n 320
printf '%s\n' '--- onboarding call path around proxy start ---'
sed -n '100,180p' src/lib/onboard/setup-nim-ollama.ts
rg -n -C 8 'setupNimOllama|startProxyOrAnnounceDirect|startOllamaAuthProxy|with.*Lock|acquire.*Lock' \
src/lib/onboard src/lib/actions src/lib/inference -g '*.ts' | head -n 320Repository: NVIDIA/NemoClaw
Length of output: 50371
Serialize shared Ollama proxy startup and recovery.
startOllamaAuthProxy reads the shared token and can generate one without cross-process synchronization. ensureOllamaAuthProxy can also restart the shared proxy without synchronization. Concurrent gateway onboarding or recovery can leave the token, PID, and running proxy out of sync. Add a host-wide lock around token selection, persistence, and proxy restart, with a concurrent-process test.
🤖 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/inference/ollama/proxy.ts` around lines 119 - 130, Add host-wide
cross-process locking around the shared Ollama proxy startup and recovery flows,
covering token selection, token persistence, and proxy restart in
startOllamaAuthProxy and ensureOllamaAuthProxy. Ensure concurrent gateway
onboarding and recovery cannot leave the shared token, PID, and running proxy
inconsistent, and add a test that exercises concurrent processes.
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
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: 3 optional E2E recommendations
Blockers
|
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
The Ollama auth proxy binds one host port for every gateway, but its token lived under the gateway-scoped state root. Onboarding a second gateway port found no token in its own root, minted a replacement, killed the running proxy, and rebound the shared port with the new token, so sandboxes on the first gateway got HTTP 401 on every inference request while still reporting
phase: Ready. The proxy's token, backend URL, PID, and status files now live in a root shared by every gateway on the host, so a second gateway reuses the token the running proxy already holds.Related Issue
Fixes #8704
Changes
resolveSharedLocalAdapterStateRoot()andSHARED_LOCAL_ADAPTER_STATE_DIRinsrc/lib/inference/local-adapter-lifecycle.ts, and read the Ollama proxy token, backend URL, PID, and status files from it insrc/lib/inference/ollama/proxy.ts. The helper pinsDEFAULT_GATEWAY_PORTrather than taking the selected port, becausenemoclawStateRootmaps that port to the top-level~/.nemoclawlocation; keying on the running listener instead of the calling gateway is the whole point of the fix.NEMOCLAW_OLLAMA_PROXY_PORTis host-wide by design, so one host has one proxy and must have one token.adoptGatewayScopedProxyToken). Current consumer: a host that already onboarded a non-default gateway port with Local Ollama. Without it, the first run after this change would find an empty shared root, mint a token, and break the sandbox that works today — reintroducing this same defect during the upgrade. It copies the token to the shared root on first read so repeated runs converge instead of re-reading the old path. Protected byadopts a token an earlier gateway-scoped run left behind (#8704).ollama-proxy-tokenandollama-auth-proxy.pidduring a legacy port migration insrc/lib/state/legacy-port-migration.ts. Those names stay inLEGACY_BUNDLE_ENTRIESso a migration intent recorded by an earlier release still validates and resumes; a newMIGRATABLE_BUNDLE_ENTRIESexcludes them from the entries a new migration moves and from the ambiguous-state warning.defaultLoadOllamaProxyTokeninsrc/lib/inference/local.ts. This reader was missed in the first pass: on a host onboarded fresh under a non-default gateway port, no gateway-scoped token is written any more, soprobeOllamaAuthProxyHealthreturnednulland theInference (auth proxy)line silently disappeared fromstatusanddoctor. This inverts the existinglocal.test.tsexpectation, which pinned the gateway-scoped read; that test now asserts the shared read.src/lib/actions/uninstall/run-plan.ts, alongside the existingHTTPS_PIN_RUNTIME_ADAPTER_STATE_ENTRIESprecedent. Two consequences of the state move needed this: a host that only ever onboarded a non-default gateway would otherwise have shared-root entries thatinspectOtherGatewayEnvironmentsreads as proof of a default-port environment, makingNEMOCLAW_GATEWAY_PORT=<port> nemoclaw uninstallreport a gateway8080that was never created; and a gateway-scoped uninstall would stop the proxy its sibling gateways still use, because the shared PID file is now always current. A scoped run now preserves the proxy and says so.src/lib/state/state-root.tsfan-in budget inci/source-architecture-budget.jsonfrom 21 to 20.local.tsno longer importsnemoclawStateRootdirectly, sonpm run checks:repositoryrequired the stale limit be lowered to the measured value.docs/inference/set-up-ollama.mdx,docs/reference/architecture.mdx,docs/reference/host-files-and-state.mdx, anddocs/reference/commands.mdxfor the shared-proxy model, including the restart and failure behaviour of onboarding a second gateway port and the wider blast radius of deleting the token file.Type of Change
Quality Gates
0600auth token from one gateway's state root to a root shared by every gateway on the host. That is the correct scope for a listener every gateway already reaches on one port, but it is a deliberate widening and a maintainer should confirm it. Uninstall behaviour for the same files also changed, and the proxy token file is now longer-lived across gateway lifecycles.Documentation Writer Review
docs-updatedVerification
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 unavailablenpx vitest run src/lib/actions/uninstall/ test/ollama-proxy-recovery.test.ts src/lib/inference/local.test.ts— 17 files, 270 tests passed;npx vitest run src/lib/inference src/lib/onboard/inference-providers src/lib/state— 141 files, 2387 tests passednpm 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: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit