test(sandbox): add exactly-once regression for Ollama GPU unload

[laitingsheng]

Summary

Follow-up on the deferred review items from #3173. Adds the dedicated regression test for the Ollama GPU unload exactly-once invariant, threads injectable dependencies through cleanupSandboxServices, and tightens two comments per CodeRabbit / jyaunches feedback.

Related Issue

Follow-up to #3173. Related: #2717 (already closed by #3173).

Changes

• src/lib/actions/sandbox/destroy.ts
  • Export cleanupSandboxServices and add a CleanupSandboxServicesDeps shape (mirrors RemoveSandboxImageDeps/RemoveSandboxRegistryEntryDeps).
  • Resolve getSandbox, stopAll, unloadOllamaModels, runOpenshell, and rmSync from the deps argument, falling back to lazy require() so runtime call sites stay unchanged and the circular-import boundary is preserved.
• test/destroy-cleanup-sandbox-services.test.ts (new)
  • stopHostServices=true + Ollama provider — stopAll() invoked once, unloadOllamaModels() not called by destroy.ts (it runs transitively inside stopAll()).
  • stopHostServices=false + Ollama provider — unloadOllamaModels() invoked exactly once, stopAll() not called.
  • stopHostServices=false + non-Ollama provider — neither called.
  • PID directory removal and messaging-provider teardown loop run with the expected arguments.
• src/lib/onboard-ollama-proxy.ts
  • Append a sync pointer to test/ollama-gpu-cleanup.test.ts in the unloadOllamaModels JSDoc.
  • Document why -sS (rather than --fail) is intentional on the POST to /api/generate — best-effort teardown should not surface orphaned-GPU-memory failures into unrelated CLI exit codes.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Verification

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• No secrets, API keys, or credentials committed
• Docs updated for user-facing behavior changes
• make docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

• Tests

  • Added comprehensive regression tests for sandbox cleanup processes, ensuring Ollama models are properly handled during service teardown.

• Documentation

  • Clarified internal documentation regarding sandbox cleanup and Ollama model unloading behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d41f2ee6-6200-44e1-9d05-b01165459831

📥 Commits

Reviewing files that changed from the base of the PR and between da250a7 and 7529038.

📒 Files selected for processing (2)

• src/lib/actions/sandbox/destroy.ts
• src/lib/inference/ollama/proxy.ts

✅ Files skipped from review due to trivial changes (1)

• src/lib/inference/ollama/proxy.ts

---

📝 Walkthrough

Walkthrough

This PR refactors cleanupSandboxServices to support dependency injection for testability, fixes Ollama model unload deduplication by clarifying stop-vs-unload branching, and adds four regression tests verifying unloadOllamaModels() is called exactly once across different cleanup scenarios. Comments document synchronous unload requirements to avoid GPU-memory cleanup races.

Changes

Sandbox cleanup with testable dependency injection

Layer / File(s)	Summary
Dependency injection types src/lib/actions/sandbox/destroy.ts	New exported CleanupSandboxServicesDeps type and RunOpenshell function type enable injectable decoupling of cleanupSandboxServices from module requirements.
Cleanup service refactoring src/lib/actions/sandbox/destroy.ts	cleanupSandboxServices now accepts optional injected dependencies (getSandbox, stopAll, unloadOllamaModels, runOpenshell, rmSync), preserving original stop-vs-unload Ollama branching and best-effort temp directory and messaging provider cleanup.
Synchronous unload documentation src/lib/inference/ollama/proxy.ts	Comments clarify that unloadOllamaModels() must remain synchronous to avoid GPU-memory cleanup races, and that curl output suppression prevents HTTP errors from affecting unrelated CLI exit codes.
Regression test suite test/destroy-cleanup-sandbox-services.test.ts	Four tests for issue #2717 verify: (1) stopHostServices=true delegates unload to stopAll(); (2) stopHostServices=false calls unloadOllamaModels() once for Ollama; (3) non-Ollama providers skip unload; (4) filesystem and messaging provider cleanup occur as expected.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• NVIDIA/NemoClaw#3173: Modifies sandbox destroy flow to deduplicate Ollama model unloading and adjust control flow.

Suggested labels

v0.0.37

Suggested reviewers

• jyaunches
• cv

Poem

🐰 The cleanup hops with care,
Dependencies now shared fair,
Ollama models unload once (hooray!),
Tests ensure we won't replay,
Sandbox services swept away clean! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the primary change: adding a regression test for the Ollama GPU unload exactly-once invariant in the sandbox destroy flow.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-2717-followup-cleanup-tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}