fix: warm Ollama model after daemon restart#6076
Conversation
|
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:
📝 WalkthroughWalkthroughAdds Ollama restart recovery for OpenClaw passthrough: it probes model status, warms an unloaded ChangesOllama daemon restart recovery
Sequence Diagram(s)sequenceDiagram
participant OpenClawAgent
participant runAgentPassthrough
participant maybeWarmOllamaAfterDaemonRestart
participant OllamaDaemon
OpenClawAgent->>runAgentPassthrough: dispatch passthrough command
runAgentPassthrough->>runAgentPassthrough: read registry provider/model
alt provider is ollama-local
runAgentPassthrough->>maybeWarmOllamaAfterDaemonRestart: route(provider, model)
maybeWarmOllamaAfterDaemonRestart->>OllamaDaemon: probe model status
OllamaDaemon-->>maybeWarmOllamaAfterDaemonRestart: reachable / loaded state
opt reachable and not loaded
maybeWarmOllamaAfterDaemonRestart->>OllamaDaemon: warm-up request
OllamaDaemon-->>maybeWarmOllamaAfterDaemonRestart: warm-up result
end
maybeWarmOllamaAfterDaemonRestart-->>runAgentPassthrough: skipped / warmed
end
runAgentPassthrough->>OllamaDaemon: execJson / exec command
OllamaDaemon-->>runAgentPassthrough: inference response
runAgentPassthrough-->>OpenClawAgent: dispatch result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/actions/sandbox/agent/ollama-restart-recovery.test.ts (1)
7-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEvery test injects both
probeRuntimeModelStatusandrunCaptureExImpl, so the real default wiring is never exercised.All six tests fully replace
deps.probeRuntimeModelStatus, which is exactly the path with the call-signature bug flagged inollama-restart-recovery.ts(lines 70-76). None of these tests would catch that the defaultprobeOllamaRuntimeModelStatusis called with mismatched arguments. Consider adding at least one test that omitsprobeRuntimeModelStatus/runCaptureExImplfrom deps and asserts on the real integration (or spies on the actual imported functions), so the suite can catch wiring breaks like this one. Also missing: a test for themissing-modelskip reason whenroute.modelis empty.As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/agent/ollama-restart-recovery.test.ts` around lines 7 - 143, The ollama recovery tests are over-mocking the dependencies, so they never exercise the real default wiring in maybeWarmOllamaAfterDaemonRestart and can miss the probeOllamaRuntimeModelStatus call-signature bug. Add at least one test in ollama-restart-recovery.test.ts that omits the injected probeRuntimeModelStatus and runCaptureExImpl deps (or spies on the imported implementations) and asserts the actual integration path, and also add coverage for the missing-model skip branch when route.model is empty.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/actions/sandbox/agent/ollama-restart-recovery.ts`:
- Around line 5-9: The Ollama recovery path is calling the wrong probe helper
with a host getter in the capture slot. Update the logic in
ollama-restart-recovery to use the host-aware probe from ollama-runtime-context
instead of probeOllamaRuntimeModelStatus from local, or remove the extra host
argument so the function is only given runCaptureImpl. Keep the existing symbols
getOllamaProbeCommand, getResolvedOllamaHost, and the recovery flow aligned so
the probe warms correctly rather than hitting unreachable.
In `@src/lib/actions/sandbox/agent/passthrough.ts`:
- Around line 446-451: The Ollama recovery path in
maybeWarmOllamaAfterDaemonRestart/recoverOllama is doing a synchronous warm-up
with no user feedback and discards the result. Add a short status message to
proc.stderr before calling recoverOllama(route), and check the returned
ok/timedOut result so failures or timeouts are logged with enough context before
the agent dispatch continues.
---
Nitpick comments:
In `@src/lib/actions/sandbox/agent/ollama-restart-recovery.test.ts`:
- Around line 7-143: The ollama recovery tests are over-mocking the
dependencies, so they never exercise the real default wiring in
maybeWarmOllamaAfterDaemonRestart and can miss the probeOllamaRuntimeModelStatus
call-signature bug. Add at least one test in ollama-restart-recovery.test.ts
that omits the injected probeRuntimeModelStatus and runCaptureExImpl deps (or
spies on the imported implementations) and asserts the actual integration path,
and also add coverage for the missing-model skip branch when route.model is
empty.
🪄 Autofix (Beta)
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: 515fce65-4437-4386-8bc0-ed132a5edde9
📒 Files selected for processing (6)
docs/inference/use-local-inference.mdxdocs/reference/troubleshooting.mdxsrc/lib/actions/sandbox/agent/ollama-restart-recovery.test.tssrc/lib/actions/sandbox/agent/ollama-restart-recovery.tssrc/lib/actions/sandbox/agent/passthrough.test.tssrc/lib/actions/sandbox/agent/passthrough.ts
cf7c2bf to
821423a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/agent/passthrough.test.ts (1)
124-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name overstates the mocked outcome.
The test title says "warms a registered Ollama model" but
maybeWarmOllamaAfterDaemonRestartis mocked to return{kind: "skipped", reason: "already-loaded"}— no warming actually occurs. The test really verifies that the recovery check is invoked with the correct route and beforeexecJson, not that warming happens. Consider renaming (e.g., "checks Ollama model readiness before OpenClaw JSON dispatch") to avoid confusing future readers about what's exercised.🤖 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/agent/passthrough.test.ts` around lines 124 - 156, The test name in passthrough.test.ts overstates what the mocked flow actually does: `maybeWarmOllamaAfterDaemonRestart` returns a skipped readiness result, so no warming is exercised. Rename the `it(...)` case to reflect that `runAgentPassthrough` only checks Ollama model readiness before OpenClaw JSON dispatch, and keep the assertions focused on `maybeWarmOllamaAfterDaemonRestart` being called before `execJson`.
🤖 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/agent/passthrough.test.ts`:
- Around line 124-156: The test name in passthrough.test.ts overstates what the
mocked flow actually does: `maybeWarmOllamaAfterDaemonRestart` returns a skipped
readiness result, so no warming is exercised. Rename the `it(...)` case to
reflect that `runAgentPassthrough` only checks Ollama model readiness before
OpenClaw JSON dispatch, and keep the assertions focused on
`maybeWarmOllamaAfterDaemonRestart` being called before `execJson`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: abb1be81-eea3-4c94-a4bc-90bf2601d64e
📒 Files selected for processing (6)
docs/inference/use-local-inference.mdxdocs/reference/troubleshooting.mdxsrc/lib/actions/sandbox/agent/ollama-restart-recovery.test.tssrc/lib/actions/sandbox/agent/ollama-restart-recovery.tssrc/lib/actions/sandbox/agent/passthrough.test.tssrc/lib/actions/sandbox/agent/passthrough.ts
✅ Files skipped from review due to trivial changes (2)
- docs/reference/troubleshooting.mdx
- docs/inference/use-local-inference.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/agent/ollama-restart-recovery.ts
- src/lib/actions/sandbox/agent/passthrough.ts
- src/lib/actions/sandbox/agent/ollama-restart-recovery.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
821423a to
70ed6be
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
wscurran
added
area: docs
|
✨ Thanks for the fix. Adding a local cleanup fallback for Related open issues: |
wscurran
added
integration: openclaw
70ed6be to
3a1f133
Compare
Signed-off-by: Ho Lim <subhoya@gmail.com>
3a1f133 to
03f8a99
Compare
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The exact-head advisor found a recovery-path blocker: restart recovery reloads only provider and model, then falls back to process-local getResolvedOllamaHost, dropping the persisted registry endpointUrl. On fresh WSL or Windows-host routes such as host.docker.internal, it can probe and warm the wrong host and miss the required recovery. Preserve and use the persisted endpoint URL, and add that regression case. Also do not treat any nonempty /api/generate body as success; JSON error bodies are nonempty, so require exit status and valid response semantics.
|
I ported this fix onto current I am leaving this original open until #6482 reaches an exact-head green/approved state; then it can be closed as superseded. |
|
Superseded by current-main replacement #6482, which preserves the original author and adds persisted-route host selection, an explicit local-host allowlist, semantic warm-response validation, bounded failure reporting, and current passthrough coverage. Closing this conflicted duplicate now so #6482 can be the single release candidate. |
## Summary - check the registered Ollama route before OpenClaw agent passthrough - translate persisted proxy and WSL bridge routes back to an allowlisted host daemon - require a successful, semantically valid Ollama warm response while preserving OpenClaw canonical errors on failure ## Validation - `npx vitest run --project cli src/lib/actions/sandbox/agent/ollama-restart-recovery.test.ts src/lib/actions/sandbox/agent/passthrough.test.ts` - `npm run build:cli` - `npm run typecheck` - `npx @biomejs/biome check ...` (touched TypeScript files) - `npm run checks` - `npm run test-size:check` - `npm run docs:check-agent-variants` Supersedes #6076. Fixes #6039. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added best-effort Ollama “restart recovery” during OpenClaw agent passthrough, including a bounded warm-up when models may be unloaded after a daemon restart. * **Documentation** * Updated local inference and troubleshooting guidance with Ollama-specific warm-up/timeout and continued-dispatch behavior. * **Bug Fixes** * Reduces post-restart failures by safely probing model availability, warming with routed requests, and continuing dispatch when warm-up times out or fails. * **Tests** * Expanded unit, integration, and GPU end-to-end coverage for warm-up skip/warm/warn outcomes and correct recovery-before-dispatch ordering. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- Signed-off-by: cjagwani <cjagwani@nvidia.com> --------- Signed-off-by: cjagwani <cjagwani@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
## Summary - check the registered Ollama route before OpenClaw agent passthrough - translate persisted proxy and WSL bridge routes back to an allowlisted host daemon - require a successful, semantically valid Ollama warm response while preserving OpenClaw canonical errors on failure ## Validation - `npx vitest run --project cli src/lib/actions/sandbox/agent/ollama-restart-recovery.test.ts src/lib/actions/sandbox/agent/passthrough.test.ts` - `npm run build:cli` - `npm run typecheck` - `npx @biomejs/biome check ...` (touched TypeScript files) - `npm run checks` - `npm run test-size:check` - `npm run docs:check-agent-variants` Supersedes NVIDIA#6076. Fixes NVIDIA#6039. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added best-effort Ollama “restart recovery” during OpenClaw agent passthrough, including a bounded warm-up when models may be unloaded after a daemon restart. * **Documentation** * Updated local inference and troubleshooting guidance with Ollama-specific warm-up/timeout and continued-dispatch behavior. * **Bug Fixes** * Reduces post-restart failures by safely probing model availability, warming with routed requests, and continuing dispatch when warm-up times out or fails. * **Tests** * Expanded unit, integration, and GPU end-to-end coverage for warm-up skip/warm/warn outcomes and correct recovery-before-dispatch ordering. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- Signed-off-by: cjagwani <cjagwani@nvidia.com> --------- Signed-off-by: cjagwani <cjagwani@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
Summary
Adds best-effort Ollama restart recovery for OpenClaw agent passthrough by warming the registered local Ollama model when the daemon is reachable but the model runner is unloaded.
Related Issue
Closes #6039
Changes
ollama-localmodel is unloaded.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpasses —SKIP=test-cli,test-plugin npx prek run --from-ref main --to-ref HEADpasses; full coverage hooks were not run to completion locally.npm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification commands run locally:
./node_modules/.bin/vitest run --project cli src/lib/actions/sandbox/agent/ollama-restart-recovery.test.ts src/lib/actions/sandbox/agent/passthrough.test.tstest/e2e/e2e-cloud-experimental/check-docs.sh --only-links --local-only docs/inference/use-local-inference.mdx docs/reference/troubleshooting.mdxnpm run build:cli./node_modules/.bin/tsc -p tsconfig.cli.json --noEmitSKIP=test-cli,test-plugin npx prek run --from-ref main --to-ref HEADgit diff --checkSigned-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit
ollama run <model>).