fix(e2e): restore protected Ollama service lifecycle - #8741
Merged
Conversation
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:
📝 WalkthroughWalkthroughThe PR adds validated Ollama cleanup and protected startup scripts. Cleanup stops services and processes, checks port 11434, and reports failures. Protected startup supports service and manual fallback paths with readiness checks. Tests cover success and failure paths. ChangesOllama lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cleanupOllama
participant ollamaCleanupScript
participant OllamaServices
participant OllamaProcesses
participant Port11434
cleanupOllama->>ollamaCleanupScript: execute cleanup script
ollamaCleanupScript->>OllamaServices: stop user and system services
ollamaCleanupScript->>OllamaProcesses: terminate Ollama and auth-proxy processes
ollamaCleanupScript->>Port11434: verify no listener remains
Port11434-->>cleanupOllama: return cleanup status
sequenceDiagram
participant startProtectedOllama
participant protectedOllamaStartScript
participant OllamaServices
participant Ollama
startProtectedOllama->>protectedOllamaStartScript: execute startup script
protectedOllamaStartScript->>OllamaServices: restart an available service
OllamaServices->>Ollama: start Ollama daemon
protectedOllamaStartScript->>Ollama: poll readiness
Ollama-->>protectedOllamaStartScript: return readiness or diagnostics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/e2e/support/gpu-e2e-helpers.test.ts (1)
141-143: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the synchronous child processes.
None of the four new
execFileSynccalls set a timeout.protectedOllamaStartScriptpolls readiness for up to 120 iterations, so a stub that behaves unexpectedly blocks the Vitest worker instead of failing. Add a positivetimeoutandkillSignal: "SIGKILL"at Lines 141, 170, 200, and 233.🔧 Proposed fix for this call site
execFileSync("bash", ["-c", ollamaCleanupScript()], { env: { ...process.env, FAKE_CALLS: calls, PATH: `${bin}:${process.env.PATH}` }, + killSignal: "SIGKILL", + timeout: 20_000, });Based on path instructions for
test/e2e/**and the referencedtest/e2e/README.md: "synchronous commands need a positive timeout shorter than the first heartbeat and killSignal "SIGKILL"".🤖 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/e2e/support/gpu-e2e-helpers.test.ts` around lines 141 - 143, Update all four new execFileSync calls in the test helpers, including the call in ollamaCleanupScript setup, to specify a positive timeout shorter than the first heartbeat and killSignal: "SIGKILL" in their options. Apply the same bounded-process configuration at each referenced call site while preserving the existing environment and command arguments.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 `@test/e2e/live/gpu-e2e-helpers.ts`:
- Around line 271-284: Replace the node -e listener probe in the Ollama cleanup
check with a bash-native TCP connection probe to 127.0.0.1:11434. Ensure the
conditional distinguishes a successful connection from a refused or timed-out
connection, so missing node cannot cause cleanup to report success while the
port remains bound; preserve the existing error message and exit behavior when a
listener is detected.
In `@test/e2e/live/managed-image-protected-runtime-helpers.ts`:
- Around line 175-191: Reduce the readiness loop in the generated startup script
around the curl probe so its maximum curl and sleep time fits within
startProtectedOllama’s 150,000 ms timeout. Preserve the existing readiness check
and failure diagnostics, ensuring the loop completes and invokes diagnose_ollama
before ShellProbe can terminate it.
- Around line 154-173: Update the system-service detection in the restart flow
around system_service and ollama.service so service existence is probed
independently of root or sudo availability. If ollama.service exists but the
caller lacks privilege to restart it, fail immediately with the established
fatal cleanup-compatible behavior; only attempt the system restart when
permitted, otherwise continue to user-service or manual startup only when no
system service is installed.
- Around line 129-136: Update protectedOllamaStartScript to encode logPath as a
Bash single-quoted literal rather than using JSON.stringify, escaping embedded
single quotes with the standard Bash-safe form. Keep the existing absolute-path
and control-character validation, and use the escaped literal in the log_path
assignment so characters such as $, backticks, and ! cannot be interpreted by
the shell.
In `@test/e2e/support/gpu-e2e-helpers.test.ts`:
- Around line 141-147: Add an `id` stub that returns a non-zero UID to the stub
tables in test/e2e/support/gpu-e2e-helpers.test.ts at lines 141-147, 187-205,
and 232-242, alongside the existing command stubs. This must force all three
lifecycle tests through the non-root sudo paths so `calls.log` is created and
the existing assertions remain valid.
---
Nitpick comments:
In `@test/e2e/support/gpu-e2e-helpers.test.ts`:
- Around line 141-143: Update all four new execFileSync calls in the test
helpers, including the call in ollamaCleanupScript setup, to specify a positive
timeout shorter than the first heartbeat and killSignal: "SIGKILL" in their
options. Apply the same bounded-process configuration at each referenced call
site while preserving the existing environment and command arguments.
🪄 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: 145f0a07-092b-441f-81c5-4d27147b5d78
📒 Files selected for processing (3)
test/e2e/live/gpu-e2e-helpers.tstest/e2e/live/managed-image-protected-runtime-helpers.tstest/e2e/support/gpu-e2e-helpers.test.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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/support/gpu-e2e-helpers.test.ts`:
- Around line 270-274: Update the execFileSync invocation in the
protectedOllamaStartScript test to include a positive timeout shorter than the
first heartbeat and configure SIGKILL as the kill signal, ensuring a stalled
generated script cannot block the Vitest worker indefinitely.
🪄 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: b6db993a-08f0-41d5-9fe9-49837c03e368
📒 Files selected for processing (2)
test/e2e/live/managed-image-protected-runtime-helpers.tstest/e2e/support/gpu-e2e-helpers.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/e2e/support/gpu-e2e-helpers.test.ts`:
- Around line 332-340: Update the command stubs in the lifecycle test around the
command setup loop so the setsid stub invokes a fake ollama serve process that
creates a readiness marker, rather than only recording the command. Change the
curl stub to succeed only when that marker exists, and add assertions verifying
both fake daemon execution and the readiness probe were reached, preserving the
existing failure behavior otherwise.
- Around line 347-350: Update the execFileSync call that runs
protectedOllamaStartScript to include a positive timeout shorter than the
script’s first heartbeat and set killSignal to "SIGKILL", while preserving the
existing encoding and environment options.
🪄 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: b02b4d65-5c1c-4869-bf77-6c3ff731ddf9
📒 Files selected for processing (1)
test/e2e/support/gpu-e2e-helpers.test.ts
wscurran
added
area: e2e
23 tasks
cv
added a commit
that referenced
this pull request
Aug 10, 2026
<!-- markdownlint-disable MD041 --> ## Summary Protected Ollama and vLLM readiness probes used login shells, so a runner-side login/logout hook could turn a successful probe into a false nonzero exit. Use non-login shells with the existing explicit environment and retain bounded, redacted failure diagnostics with a stable final failure marker. ## Related Issue Refs #7744 Supports #8658 ## Changes - Run only the protected Ollama and vLLM readiness commands with `bash -c`; service lifecycle and ownership behavior are unchanged. - Keep at most the final 4 MiB of readiness output, tail at most 200 diagnostic lines, tolerate diagnostic-read failures, emit content-free readiness markers after diagnostics, and keep child diagnostics out of assertion output while pointing to the redacted artifact. - Add support tests for hostile login-shell hooks, successful attempt markers, redacted and bounded failure output, oversized log lines, failing vLLM log capture, and containers that stop during readiness. ## Type of Change - [x] 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes only the trusted live-E2E readiness harness and its support tests; supported product behavior is unchanged. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent Codex Desktop code and security reviews passed after the bounded-diagnostic, stopped-runtime, current-main #8741 lifecycle-integration, and content-free assertion fixes; the combined overlap and assertion suite passed 49/49 with no remaining actionable findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The branch changes only live-E2E readiness execution and capture plus support tests; it does not change supported or user-visible behavior. - Agent: Codex Desktop <!-- docs-review-head-sha: 6c3a24d --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project e2e-support test/e2e/support/gpu-e2e-helpers.test.ts test/e2e/support/managed-image-protected-runtime-readiness.test.ts` (49/49); `npm run typecheck:cli` and `npm run validate:pr` passed at exact head `6c3a24dd3`, including the E2E semantic phase gate. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved protected managed-image readiness checks for Ollama and vLLM. * Added clearer failure diagnostics, including recent container logs and readiness attempt counts. * Prevented oversized logs and output from overwhelming readiness reports. * Preserved useful failure details while redacting sensitive information. * Added handling for containers that stop during readiness checks. * Improved reporting for successful and unsuccessful startup readiness states. * Exposed result artifact locations for investigating protected startup failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
23 tasks
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
Restores the protected managed-image GPU test's Ollama lifecycle after the official installer creates a system service. Startup now uses that installed service as the single authority and fails with diagnostics if it cannot restart; cleanup stops supported service forms and rejects any remaining daemon process or TCP listener.
Related Issue
Part of #7744.
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededtest/e2e/live/gpu-e2e-helpers.ts,test/e2e/live/gpu-e2e.test.ts,test/e2e/live/managed-image-protected-runtime-helpers.ts, andtest/e2e/support/gpu-e2e-helpers.test.ts. These changes affect E2E lifecycle verification only. Focused E2E-support tests passed 40/40; semantic phase tests passed 20/20; the conditional-growth scan, Biome, repository checks,git diff --check, and normal commit hooks passed./root/docs_review_ollama_lifecycle)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 unavailablenpx vitest run --project e2e-support test/e2e/support/gpu-e2e-helpers.test.ts(40 passed);npx vitest run --project e2e-support test/e2e/support/e2e-semantic-phase-check.test.ts(20 passed);npm run test-conditionals:scan -- --top 25, Biome, andnpm run checks:repositorypassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; this change is isolated to live E2E helpers and their support tests.2698df348. The protected workflow executes its qualification helper from trustedmain, so this pre-merge run validates exact candidate images but cannot exercise the PR's helper change; post-mergemainevidence remains required.npm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests