fix(uninstall): clean up orphaned openshell processes on uninstall (#1940) by yanyunl1991 · Pull Request #1957 · NVIDIA/NemoClaw

yanyunl1991 · 2026-04-16T08:05:35Z

Summary

nemoclaw uninstall only kills openshell forward processes but leaves behind openshell sandbox create, openshell ssh-proxy, and their child ssh sessions. These orphaned processes accumulate across onboard/destroy cycles.

Fix

Add stop_orphaned_openshell_processes() to the uninstall flow (step 1) that finds and kills all openshell-related processes:

openshell sandbox create and openshell ssh-proxy via pgrep -f
ssh sessions spawned by openshell (verified via ps -p <PID> -o args= containing "openshell")

Reproduction & Verification

Before fix — orphaned processes remain after uninstall:

  $ nemoclaw uninstall
  [1/6] Stopping services                                                                                                                                                                                                                                                                                                    
  [uninstall] No local OpenShell forward processes found
  ...                                                                                                                                                                                                                                                                                                                        
  $ ps -ef | grep openshell | grep -v grep
  openshell sandbox create --from ... --name lyy ...                                                                                                                                                                                                                                                                         
  ssh -o ProxyCommand=/...openshell ssh-proxy ...   
  openshell ssh-proxy --gateway ...                                                                                                                                                                                                                                                                                          
  (18 orphaned processes)

After fix — all processes cleaned up:

  $ nemoclaw uninstall
  [1/6] Stopping services                                                                                                                                                                                                                                                                                                    
  [uninstall] No local OpenShell forward processes found
  [uninstall] Stopped orphaned openshell process 350520 
  [uninstall] Stopped orphaned openshell process 355434
  ... (18 processes killed)                                                                                                                                                                                                                                                                                                  
  $ ps -ef | grep openshell | grep -v grep
  (empty — all cleaned up)

Test plan

Reproduced on Ubuntu 24.04 (NemoClaw v0.1.0, OpenShell 0.0.26)
Verified 18 orphaned openshell processes cleaned up after uninstall
Verified ps -ef | grep openshell returns empty after fix
Only kills openshell-related processes (ssh sessions verified via cmdline)
Existing test suite passes

Summary by CodeRabbit

Bug Fixes
- Uninstall now stops remaining helper services and removes related temporary service artifacts.
- Detects and terminates orphaned OpenShell-related background processes owned by the invoking user; deduplicates targets, attempts graceful shutdown before forcing termination, and provides clearer info/warn logging of outcomes.

Signed-off-by: Yanyun Liao yanyunl@nvidia.com

…1940) `nemoclaw uninstall` only killed `openshell forward` processes but left behind `openshell sandbox create`, `openshell ssh-proxy`, and their child `ssh` sessions. These orphaned processes accumulate across onboard/destroy cycles. Add `stop_orphaned_openshell_processes()` to the uninstall flow that finds and kills all openshell-related processes: - `openshell sandbox create` and `openshell ssh-proxy` via pgrep - `ssh` sessions spawned by openshell (verified via ps command line) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

coderabbitai · 2026-04-16T08:06:10Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: dfb4e9a0-7c92-44cb-9ffd-247652c8e1ce

📥 Commits

Reviewing files that changed from the base of the PR and between 1fafefd and af0d2fe.

📒 Files selected for processing (1)

uninstall.sh

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

uninstall.sh

📝 Walkthrough

Walkthrough

Added two routines to uninstall.sh: stop_helper_services() to stop NemoClaw helper services and remove ${TMP_ROOT}/nemoclaw-services-*, and stop_orphaned_openshell_processes() to locate and terminate leftover OpenShell-related processes; main() now calls the orphan cleanup after stopping forward processes.

Changes

Cohort / File(s)	Summary
Uninstall script changes `uninstall.sh`	Added `stop_helper_services()` to stop NemoClaw helper services via `scripts/start-services.sh --stop` (when available) and remove `${TMP_ROOT}/nemoclaw-services-*`. Added `stop_orphaned_openshell_processes()` which derives the real user (via `SUDO_USER`/`LOGNAME`/`id -un`), collects PIDs from `pgrep` for `openshell (sandbox create

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I sniff the sockets, chase each stray,
A polite SIGTERM shows them the way,
If stubborn ghosts refuse to part,
A firmer nudge restores the heart,
Temp files cleared, I hop away.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(uninstall): clean up orphaned openshell processes on uninstall' clearly and concisely summarizes the main change: adding cleanup of orphaned OpenShell processes to the uninstall flow.

_{✏️ 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/uninstall-cleanup-openshell-processes-1940

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

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

uninstall.sh (1)
286-334: Add targeted tests for PID collection, dedupe, and kill fallback paths.

test/uninstall.test.ts (Line 1-Line 71) currently mocks pgrep as a no-op and does not validate this new function’s matching logic (sandbox create, ssh-proxy, ssh cmdline filtering) or kill fallback behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@uninstall.sh` around lines 286 - 334, Add unit tests exercising
stop_orphaned_openshell_processes: mock pgrep to return PIDs for the -f
"openshell (sandbox create|ssh-proxy)" case and for pgrep -x ssh, mock ps -p to
return an ssh process line that includes "openshell" so the ssh PID is
collected, include duplicate PIDs in the mocked lists to validate deduplication,
and stub kill to first fail and then succeed to exercise the kill fallback (kill
then kill -9) paths; assert that the collected unique PIDs are attempted to be
killed and that the test verifies the expected info/warn log messages emitted by
stop_orphaned_openshell_processes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@uninstall.sh`:
- Around line 296-310: The pgrep calls that collect PIDs for "openshell (sandbox
create|ssh-proxy)" and for ssh are unscoped and can match other users' processes
when run with elevated privileges; update both pgrep invocations to restrict
matches to the invoking user by adding a -u filter using the effective invoking
user variable (use SUDO_USER if set, otherwise USER), e.g. replace the bare
pgrep commands inside the two process-collection loops with calls that include
-u "${SUDO_USER:-$USER}" so only the invoking user's openshell/ssh processes are
considered before appending to pids[].

---

Nitpick comments:
In `@uninstall.sh`:
- Around line 286-334: Add unit tests exercising
stop_orphaned_openshell_processes: mock pgrep to return PIDs for the -f
"openshell (sandbox create|ssh-proxy)" case and for pgrep -x ssh, mock ps -p to
return an ssh process line that includes "openshell" so the ssh PID is
collected, include duplicate PIDs in the mocked lists to validate deduplication,
and stub kill to first fail and then succeed to exercise the kill fallback (kill
then kill -9) paths; assert that the collected unique PIDs are attempted to be
killed and that the test verifies the expected info/warn log messages emitted by
stop_orphaned_openshell_processes.

🪄 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: Pro Plus

Run ID: 70976c0b-e7df-4f58-99be-f205c074902d

📥 Commits

Reviewing files that changed from the base of the PR and between ec4d058 and 7b5123c.

📒 Files selected for processing (1)

uninstall.sh

Add `-u $(id -un)` to pgrep calls to avoid killing openshell/ssh processes belonging to other users on shared systems when running uninstall via sudo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

uninstall.sh (1)
297-304: ⚠️ Potential issue | 🟠 Major

Use the original caller for pgrep -u, not the effective user.

Line 300 resolves id -un, which becomes root under sudo. In the common sudo ./uninstall.sh flow, that scopes discovery to root-owned processes and misses the actual user's orphaned OpenShell/ssh sessions.
🔧 Proposed fix
-  local _user
-  _user="$(id -un 2>/dev/null || echo "")"
+  local _user="${SUDO_UID:-${UID:-}}"
   local -a _pgrep_user=()
   if [ -n "$_user" ]; then
     _pgrep_user=(-u "$_user")
   fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@uninstall.sh` around lines 297 - 304, The script currently sets _user via id
-un (the effective user), which becomes root under sudo and mis-scopes
_pgrep_user; change the assignment to prefer the original caller (use the
SUDO_USER env var, then LOGNAME, then fallback to id -un) so _pgrep_user=(-u
"$_user") targets the invoking user; update the _user assignment near the
existing _user and _pgrep_user variables to use something like
_user="${SUDO_USER:-${LOGNAME:-$(id -un 2>/dev/null || echo "")}}".

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@uninstall.sh`:
- Around line 312-321: The substring check for "openshell" on cmd can falsely
match unrelated SSH sessions; instead update the loop that reads pids (the while
IFS= read -r pid; do ... cmd="$(ps -p "$pid" -o args= 2>/dev/null)" ... done <
<(pgrep "${_pgrep_user[@]}" -x ssh 2>/dev/null || true)) to test for the
specific hostname pattern "openshell-" (e.g., match "openshell-" with the
hyphen) when deciding to append to pids, and if you decide to keep the broader
user-scoped pgrep ("${_pgrep_user[@]}") add a short comment explaining that user
scoping mitigates false positives; ensure references to variables pids and cmd
and the pgrep invocation remain intact.

---

Duplicate comments:
In `@uninstall.sh`:
- Around line 297-304: The script currently sets _user via id -un (the effective
user), which becomes root under sudo and mis-scopes _pgrep_user; change the
assignment to prefer the original caller (use the SUDO_USER env var, then
LOGNAME, then fallback to id -un) so _pgrep_user=(-u "$_user") targets the
invoking user; update the _user assignment near the existing _user and
_pgrep_user variables to use something like _user="${SUDO_USER:-${LOGNAME:-$(id
-un 2>/dev/null || echo "")}}".

🪄 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: Pro Plus

Run ID: 519f8ed6-79f6-4faf-bdfc-28703256ee10

📥 Commits

Reviewing files that changed from the base of the PR and between 0626164 and b979209.

📒 Files selected for processing (1)

uninstall.sh

#1940) - Use SUDO_USER/LOGNAME instead of id -un so pgrep targets the invoking user (not root) when running under sudo. - Tighten ssh process matching from broad "*openshell*" to specific "*openshell ssh-proxy*" or "*openshell-*" patterns to avoid false positives on unrelated ssh sessions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

coderabbitai

🧹 Nitpick comments (1)

uninstall.sh (1)
286-348: Well-implemented orphan process cleanup.

The function correctly addresses the prior review feedback:

User-scoped process discovery via SUDO_USER/LOGNAME/id -un fallback chain prevents killing other users' processes on shared systems.

SSH matching uses specific patterns (openshell ssh-proxy and openshell- prefix) to avoid false positives on unrelated SSH sessions.

The deduplication logic and graceful-then-forced kill pattern are appropriate.

Minor style note (optional): The local cmd declaration at line 319 is inside the while loop. While valid in Bash, moving it before the loop would be slightly more conventional:
♻️ Optional refactor
   # Also collect ssh processes whose command line references openshell.
   # Match "openshell ssh-proxy" or "openshell-" (gateway name pattern) to
   # avoid false positives on unrelated ssh sessions. User scoping via
   # _pgrep_user provides an additional safety net.
+  local cmd
   while IFS= read -r pid; do
     [ -n "$pid" ] || continue
-    local cmd
     cmd="$(ps -p "$pid" -o args= 2>/dev/null)" || continue
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@uninstall.sh` around lines 286 - 348, The local variable declaration for cmd
is currently inside the second while loop in stop_orphaned_openshell_processes;
move the declaration (local cmd) just before that while < <(pgrep
"${_pgrep_user[@]}" -x ssh 2>/dev/null || true) loop so the variable is declared
once in the function scope rather than repeatedly in the loop body, leaving the
rest of the loop logic (ps -p "$pid" -o args= and the pattern checks for
"openshell ssh-proxy" / "openshell-") unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@uninstall.sh`:
- Around line 286-348: The local variable declaration for cmd is currently
inside the second while loop in stop_orphaned_openshell_processes; move the
declaration (local cmd) just before that while < <(pgrep "${_pgrep_user[@]}" -x
ssh 2>/dev/null || true) loop so the variable is declared once in the function
scope rather than repeatedly in the loop body, leaving the rest of the loop
logic (ps -p "$pid" -o args= and the pattern checks for "openshell ssh-proxy" /
"openshell-") unchanged.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3f25bd32-8376-4770-95ad-69ce50721d07

📥 Commits

Reviewing files that changed from the base of the PR and between b979209 and 1fafefd.

📒 Files selected for processing (1)

uninstall.sh

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

wscurran · 2026-04-16T15:21:27Z

✨ Thanks for submitting this PR that proposes a fix to clean up orphaned OpenShell processes on uninstall, which could help improve the uninstall process and resolve the issue with leftover processes.

Possibly related open issues:

#1940 nemoclaw uninstall does not clear all the openshell process

…) (#1975) ## Summary `nemoclaw list` and `nemoclaw status` show incorrect model/provider information for multi-sandbox setups because (1) `setupInference()` writes the registry update to the gateway name `"nemoclaw"` instead of the actual sandbox name, silently no-oping, and (2) `registerSandbox()` in `createSandbox()` never includes `model` or `provider` in the initial entry. This PR fixes both issues and also fixes a Bash 3.2 compatibility regression in `uninstall.sh` introduced by #1957. ## Related Issue Fixes #1689 ## Changes - **`src/lib/onboard.ts` (line ~5397):** Pass `sandboxName` instead of `GATEWAY_NAME` to `setupInference()` so the internal `registry.updateSandbox()` call writes model/provider to the correct sandbox entry. - **`src/lib/onboard.ts` (line ~3005):** Include `model` and `provider` in the `registerSandbox()` call inside `createSandbox()` so the initial registry entry is born with the correct inference config. - **`uninstall.sh` (line ~327):** Replace `local -A` (Bash 4+ associative array) with a portable string-based dedup that works on macOS Bash 3.2, and guard empty-array iteration against `set -u` unbound variable errors. ## 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) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] 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](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## AI Disclosure - [x] AI-assisted — tool: Claude Code (pi agent) --- Signed-off-by: Brandon Pelfrey <bpelfrey@nvidia.com>  ## Summary by CodeRabbit * **Improvements** * Enhanced sandbox configuration tracking to capture model and provider information * Updated inference setup process for improved sandbox identification * Refined system process cleanup during uninstallation for better compatibility  --------- Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>

coderabbitai Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread uninstall.sh Outdated

yanyunl1991 and others added 2 commits April 16, 2026 16:12

docs(uninstall): add function comments for docstring coverage

b979209

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

coderabbitai Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread uninstall.sh Outdated

coderabbitai Bot reviewed Apr 16, 2026

View reviewed changes

style(uninstall): move local cmd declaration outside loop (#1940)

af0d2fe

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

wscurran added the fix label Apr 16, 2026

wscurran added NemoClaw CLI labels Apr 16, 2026

cv approved these changes Apr 16, 2026

View reviewed changes

cv merged commit 1f62f4f into main Apr 16, 2026
13 checks passed

brandonpelfrey mentioned this pull request Apr 16, 2026

fix(onboard): persist model and provider per sandbox in registry (#1689) #1975

Merged

13 tasks

prekshivyas mentioned this pull request Apr 16, 2026

nemoclaw uninstall does not clear all the openshell process #1940

Closed

2 tasks

wscurran added area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression and removed NemoClaw CLI labels Jun 3, 2026

cv deleted the fix/uninstall-cleanup-openshell-processes-1940 branch June 28, 2026 00:22

Navigation Menu

Uh oh!

fix(uninstall): clean up orphaned openshell processes on uninstall (#1940) - #1957

fix(uninstall): clean up orphaned openshell processes on uninstall (#1940)#1957
cv merged 5 commits into
mainfrom
fix/uninstall-cleanup-openshell-processes-1940

yanyunl1991 commented Apr 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 16, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

wscurran commented Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

yanyunl1991 commented Apr 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Reproduction & Verification

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

wscurran commented Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yanyunl1991 commented Apr 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 16, 2026 •

edited

Loading