fix: clean up Docker volumes after failed gateway start

[WuKongAI-CMU]

Summary

• Extract destroyGateway() helper that runs openshell gateway destroy AND removes orphaned openshell-cluster-nemoclaw Docker volumes
• Call it before gateway start (existing pre-cleanup), after failed start (new), and after failed health check (new)
• Error messages now tell users to simply rerun the installer instead of requiring manual docker volume rm

Fixes #17

Root cause

openshell gateway destroy does not always remove the Docker volumes it created. When a subsequent openshell gateway start finds these volumes, it fails with "Corrupted cluster state". Users had to discover docker volume rm on their own.

Before

Gateway failed to start. Run: openshell gateway info
# Next run:
Error: Corrupted cluster state. Please manually remove Docker volumes.

After

Gateway failed to start. Cleaning up stale state...
Stale state removed. Please rerun the installer.
If the error persists, run: openshell gateway info

Test plan

• All 39 tests pass
• Verify cleanup removes volumes after intentionally failed gateway start
• Verify rerun succeeds after cleanup

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Added automatic gateway cleanup on startup, failed startups, and health check failures
  • New volume management utilities for handling stale gateway resources
  • Manual cleanup command generation for recovery scenarios

• Tests

  • Added comprehensive test coverage for gateway cleanup functionality

[kjw3]

Scope check after review: PR #95 should stay focused on fixing #17. Related issues #120 and #311 are separate and already have their own open PRs (#121 and #310). Issue #23 is broader; #95 addresses one concrete slice of it but should not absorb the full rerun/idempotency scope.

I rebased the #17 fix onto current origin/main, resolved the bin/lib/onboard.js overlap, added cleanup fallback tests, and pushed the result to origin branch fix/gateway-cleanup-mainline at commit 4b123ef for validation across macOS, Brev, and Spark.

[coderabbitai]

📝 Walkthrough

Walkthrough

This change introduces a gateway lifecycle cleanup system that automatically removes stale Docker volumes when gateway startup fails or the gateway is destroyed. It adds five helper functions to handle volume enumeration, inspection, removal, and error reporting, along with enhanced startup logic that triggers cleanup on failure scenarios.

Changes

Cohort / File(s)	Summary
Gateway Cleanup Lifecycle bin/lib/onboard.js	Added destroyGateway() wrapper that orchestrates gateway destruction and volume cleanup. Introduced gatewayVolumeCandidates() to identify Docker volumes, cleanupGatewayVolumes() to remove volumes and track failures, manualGatewayVolumeCleanupCommand() to format recovery commands, and reportGatewayCleanupResult() to emit cleanup guidance. Enhanced startup logic to call destroyGateway() before fresh start and on failure/health-check paths. Replaced hard-coded gateway name with DEFAULT_GATEWAY_NAME constant. Exported all new helpers.
Cleanup Function Tests test/onboard.test.js	Added comprehensive test suite covering volume candidate enumeration, successful and failed volume removal scenarios, and manual recovery command generation for leftover volumes.

Sequence Diagram

sequenceDiagram
    actor User
    participant Gateway Startup
    participant destroyGateway
    participant Docker Volumes
    participant Cleanup Reporter
    
    User->>Gateway Startup: Start gateway
    Gateway Startup->>destroyGateway: Teardown old state
    destroyGateway->>Docker Volumes: List and remove stale volumes
    Docker Volumes-->>destroyGateway: Removal results (success/failed)
    destroyGateway->>Cleanup Reporter: Report cleanup outcome
    Cleanup Reporter-->>Gateway Startup: Cleanup status
    
    alt Startup succeeds
        Gateway Startup->>User: Gateway ready
    else Startup fails
        Gateway Startup->>destroyGateway: Trigger cleanup
        destroyGateway->>Docker Volumes: Remove volumes
        Docker Volumes-->>destroyGateway: Results with failures
        destroyGateway->>Cleanup Reporter: Report + show recovery command
        Cleanup Reporter-->>Gateway Startup: Cleanup summary
        Gateway Startup->>User: Error + recovery instructions
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit hops through Docker's land,
Sweeping volumes with careful hand,
When gateways stumble and fail to start,
I clean the mess—a tidy art!
No manual toil, no mess so grand, 🧹✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% 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 PR title accurately describes the main change: implementing cleanup of Docker volumes after failed gateway starts.
Linked Issues check	✅ Passed	The PR implements all key requirements from issue #17: new cleanup functions prevent lingering volumes, destroyGateway() is called on failures, error messages guide users to rerun, and tests validate the cleanup behavior.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing issue #17: adding gateway cleanup functions, integrating them into startup/failure paths, and testing the new cleanup logic.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

test/onboard.test.js (1)

12-53: Add a regression test for Docker-unavailable cleanup behavior.

Current tests validate remove success/failure, but not the case where Docker itself is unavailable. That path is important for avoiding false “cleanup succeeded” messaging.

🧪 Suggested test case

 describe("gateway cleanup helpers", () => {
+  it("marks cleanup as failed when Docker is unavailable", () => {
+    const runFn = (cmd) => {
+      if (cmd.startsWith("docker info")) return { status: 1 };
+      return { status: 1 };
+    };
+
+    const result = cleanupGatewayVolumes(runFn);
+    assert.deepEqual(result, {
+      removedVolumes: [],
+      failedVolumes: ["openshell-cluster-nemoclaw"],
+    });
+  });
+
   it("uses the known OpenShell volume name for the default gateway", () => {
     assert.deepEqual(gatewayVolumeCandidates(), ["openshell-cluster-nemoclaw"]);
   });

🤖 Prompt for AI Agents

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

In `@test/onboard.test.js` around lines 12 - 53, Add a new test case in
onboard.test.js that simulates Docker being entirely unavailable by having the
runFn return a non-zero status (e.g., {status: 1}) for the "docker volume
inspect" command; call cleanupGatewayVolumes(runFn) and assert it reports
removedVolumes: [] and failedVolumes includes the known gateway name (from
gatewayVolumeCandidates()/openshell-cluster-nemoclaw), then verify
manualGatewayVolumeCleanupCommand(result.failedVolumes) returns the exact manual
removal string; locate and update the existing describe("gateway cleanup
helpers") block and reuse the same helper functions (cleanupGatewayVolumes and
manualGatewayVolumeCleanupCommand) to add this regression test.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@bin/lib/onboard.js`:
- Around line 98-107: cleanupGatewayVolumes currently treats any non-zero result
from runFn(`docker volume inspect ...`) as "volume absent", which hides
Docker-daemon/CLI failures; change the logic in cleanupGatewayVolumes (and its
use of runFn and gatewayVolumeCandidates/DEFAULT_GATEWAY_NAME) so that when
inspectResult.status !== 0 you first verify Docker is reachable (e.g., call
runFn('docker info' or 'docker version') and check its status/output) and if
Docker is unavailable return/throw an error (or propagate failure) instead of
continuing; only treat non-zero inspect as "absent" when Docker is confirmed
working. Ensure callers handle the propagated error or failure status.
- Around line 134-136: The command string in destroyGateway interpolates
gatewayName directly; change it to shell-quote gatewayName before interpolation
to avoid injection — e.g., escape any single quotes in gatewayName and wrap it
in single quotes when building the command passed to runFn (reference function
destroyGateway and runFn); leave the call to cleanupGatewayVolumes(gatewayName)
unchanged.

---

Nitpick comments:
In `@test/onboard.test.js`:
- Around line 12-53: Add a new test case in onboard.test.js that simulates
Docker being entirely unavailable by having the runFn return a non-zero status
(e.g., {status: 1}) for the "docker volume inspect" command; call
cleanupGatewayVolumes(runFn) and assert it reports removedVolumes: [] and
failedVolumes includes the known gateway name (from
gatewayVolumeCandidates()/openshell-cluster-nemoclaw), then verify
manualGatewayVolumeCleanupCommand(result.failedVolumes) returns the exact manual
removal string; locate and update the existing describe("gateway cleanup
helpers") block and reuse the same helper functions (cleanupGatewayVolumes and
manualGatewayVolumeCleanupCommand) to add this regression test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6e05a240-9cab-4301-ad5a-86e1106e305c

📥 Commits

Reviewing files that changed from the base of the PR and between 1e23347 and 4b123ef.

📒 Files selected for processing (2)

• bin/lib/onboard.js
• test/onboard.test.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Handle Docker-unavailable cases explicitly before reporting cleanup success.

Right now, a non-zero docker volume inspect is treated as "volume absent" (Line 107), so Docker daemon/CLI failures can end up reported as successful stale-state cleanup.

🔧 Proposed fix

 function cleanupGatewayVolumes(runFn = run, gatewayName = DEFAULT_GATEWAY_NAME) {
   const removedVolumes = [];
   const failedVolumes = [];
+  const dockerReady = runFn("docker info >/dev/null 2>&1", {
+    ignoreError: true,
+    stdio: "ignore",
+  });
+  if (dockerReady.status !== 0) {
+    return { removedVolumes, failedVolumes: gatewayVolumeCandidates(gatewayName) };
+  }
 
   for (const volumeName of gatewayVolumeCandidates(gatewayName)) {
     const inspectResult = runFn(`docker volume inspect ${shellQuote(volumeName)} >/dev/null 2>&1`, {
       ignoreError: true,
       stdio: "ignore",
     });

🤖 Prompt for AI Agents

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

In `@bin/lib/onboard.js` around lines 98 - 107, cleanupGatewayVolumes currently
treats any non-zero result from runFn(`docker volume inspect ...`) as "volume
absent", which hides Docker-daemon/CLI failures; change the logic in
cleanupGatewayVolumes (and its use of runFn and
gatewayVolumeCandidates/DEFAULT_GATEWAY_NAME) so that when inspectResult.status
!== 0 you first verify Docker is reachable (e.g., call runFn('docker info' or
'docker version') and check its status/output) and if Docker is unavailable
return/throw an error (or propagate failure) instead of continuing; only treat
non-zero inspect as "absent" when Docker is confirmed working. Ensure callers
handle the propagated error or failure status.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Quote gatewayName in destroyGateway command construction.

gatewayName is interpolated directly into a shell command on Line 135. Even if current callers use a constant, this exported helper should be safe by default.

🔧 Proposed fix

 function destroyGateway(runFn = run, gatewayName = DEFAULT_GATEWAY_NAME) {
-  runFn(`openshell gateway destroy -g ${gatewayName} 2>/dev/null || true`, { ignoreError: true });
+  runFn(`openshell gateway destroy -g ${shellQuote(gatewayName)} 2>/dev/null || true`, { ignoreError: true });
   return cleanupGatewayVolumes(runFn, gatewayName);
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	function destroyGateway(runFn = run, gatewayName = DEFAULT_GATEWAY_NAME) {
	runFn(`openshell gateway destroy -g ${gatewayName} 2>/dev/null || true`, { ignoreError: true });
	return cleanupGatewayVolumes(runFn, gatewayName);
	function destroyGateway(runFn = run, gatewayName = DEFAULT_GATEWAY_NAME) {
	runFn(`openshell gateway destroy -g ${shellQuote(gatewayName)} 2>/dev/null || true`, { ignoreError: true });
	return cleanupGatewayVolumes(runFn, gatewayName);
	}

🤖 Prompt for AI Agents

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

In `@bin/lib/onboard.js` around lines 134 - 136, The command string in
destroyGateway interpolates gatewayName directly; change it to shell-quote
gatewayName before interpolation to avoid injection — e.g., escape any single
quotes in gatewayName and wrap it in single quotes when building the command
passed to runFn (reference function destroyGateway and runFn); leave the call to
cleanupGatewayVolumes(gatewayName) unchanged.