Merge pull request #336 from cloudflare/ajansson/fix/gateway-double-spawn-289

fix: prevent gateway double-spawn via port probe safety net

Author: andreasjansson

Dockerfile

@@ -12,6 +12,7 @@ RUN ARCH="$(dpkg --print-architecture)" \
          *) echo "Unsupported architecture: ${ARCH}" >&2; exit 1 ;; \
        esac \
     && apt-get update && apt-get install -y xz-utils ca-certificates \
+    && rm -rf /usr/local/lib/node_modules /usr/local/bin/node /usr/local/bin/npm /usr/local/bin/npx /usr/local/bin/corepack \
     && curl -fsSLk https://nodejs.org/dist/v${NODE_VERSION}/node-v${NODE_VERSION}-linux-${NODE_ARCH}.tar.xz -o /tmp/node.tar.xz \
     && rm -rf /usr/local/lib/node_modules /usr/local/bin/node /usr/local/bin/npm /usr/local/bin/npx /usr/local/bin/corepack \
     && tar -xJf /tmp/node.tar.xz -C /usr/local --strip-components=1 \

src/gateway/process.test.ts

@@ -1,7 +1,7 @@
 import { describe, it, expect, vi } from 'vitest';
-import { findExistingGatewayProcess } from './process';
+import { findExistingGatewayProcess, isGatewayPortOpen } from './process';
 import type { Sandbox, Process } from '@cloudflare/sandbox';
-import { createMockSandbox } from '../test-utils';
+import { createMockSandbox, createMockExecResult } from '../test-utils';
 
 function createFullMockProcess(overrides: Partial<Process> = {}): Process {
   return {
@@ -143,3 +143,29 @@ describe('findExistingGatewayProcess', () => {
     expect(result).toBeNull();
   });
 });
+
+describe('isGatewayPortOpen', () => {
+  it('returns true when port is open (nc exits 0)', async () => {
+    const { sandbox, execMock } = createMockSandbox();
+    execMock.mockResolvedValue(createMockExecResult('', { exitCode: 0 }));
+
+    const result = await isGatewayPortOpen(sandbox);
+    expect(result).toBe(true);
+    expect(execMock).toHaveBeenCalledWith('nc -z localhost 18789');
+  });
+
+  it('returns false when port is closed (nc exits non-zero)', async () => {
+    const { sandbox, execMock } = createMockSandbox();
+    execMock.mockResolvedValue(createMockExecResult('', { exitCode: 1 }));
+
+    const result = await isGatewayPortOpen(sandbox);
+    expect(result).toBe(false);
+  });
+
+  it('propagates errors from sandbox.exec', async () => {
+    const { sandbox, execMock } = createMockSandbox();
+    execMock.mockRejectedValue(new Error('container not ready'));
+
+    await expect(isGatewayPortOpen(sandbox)).rejects.toThrow('container not ready');
+  });
+});

src/gateway/process.ts

@@ -3,6 +3,15 @@ import type { OpenClawEnv } from '../types';
 import { GATEWAY_PORT, STARTUP_TIMEOUT_MS } from '../config';
 import { buildEnvVars } from './env';
 
+/**
+ * Check if the gateway port is already listening via a TCP probe.
+ * Used as a safety net when listProcesses() fails to detect the gateway.
+ */
+export async function isGatewayPortOpen(sandbox: Sandbox): Promise<boolean> {
+  const result = await sandbox.exec(`nc -z localhost ${GATEWAY_PORT}`);
+  return result.exitCode === 0;
+}
+
 /**
  * Find an existing OpenClaw gateway process
  *
@@ -50,9 +59,10 @@ export async function findExistingGatewayProcess(sandbox: Sandbox): Promise<Proc
  *
  * @param sandbox - The sandbox instance
  * @param env - Worker environment bindings
- * @returns The running gateway process
+ * @returns The running gateway process, or null if the gateway is up but we
+ *          don't have a process handle (detected via port probe only)
  */
-export async function ensureGateway(sandbox: Sandbox, env: OpenClawEnv): Promise<Process> {
+export async function ensureGateway(sandbox: Sandbox, env: OpenClawEnv): Promise<Process | null> {
   // Check if gateway is already running or starting
   const existingProcess = await findExistingGatewayProcess(sandbox);
   if (existingProcess) {
@@ -83,6 +93,20 @@ export async function ensureGateway(sandbox: Sandbox, env: OpenClawEnv): Promise
     }
   }
 
+  // Safety net: the process wasn't found by listProcesses() (e.g. the command
+  // string didn't match any known pattern), but the gateway may still be running.
+  // Probe the port directly — if it's open, the gateway is up and we're done.
+  try {
+    if (await isGatewayPortOpen(sandbox)) {
+      console.log(
+        `Port ${GATEWAY_PORT} already open — gateway running but undetected by listProcesses(), skipping spawn`,
+      );
+      return null;
+    }
+  } catch (e) {
+    console.log('Port probe failed, proceeding to start gateway:', e);
+  }
+
   // Start a new OpenClaw gateway
   console.log('Starting new OpenClaw gateway...');
   const envVars = buildEnvVars(env);

src/index.ts

@@ -139,13 +139,11 @@ app.use('*', async (c, next) => {
   const sandbox = getSandbox(c.env.Sandbox, 'openclaw', options);
   c.set('sandbox', sandbox);
 
-  // Restore from backup on first access (idempotent, once per Worker isolate)
-  try {
-    await restoreIfNeeded(sandbox, c.env.BACKUP_BUCKET);
-  } catch (err) {
-    console.error('[middleware] Backup restore failed:', err);
-    // Continue anyway — fresh container is better than no container
-  }
+  // NOTE: restoreIfNeeded is NOT called here in the global middleware.
+  // It's called only from the catch-all route (gateway proxy) and /api/status.
+  // Calling it on admin routes (sync, debug/cli) would mount a FUSE overlay
+  // that interferes with createBackup — the SDK resets the overlay on backup,
+  // wiping any upper-layer writes made since the last restore.
 
   await next();
 });
@@ -243,6 +241,16 @@ app.all('*', async (c) => {
 
   console.log('[PROXY] Handling request:', url.pathname);
 
+  // Restore from backup before starting the gateway.
+  // This is only called here (catch-all) and from /api/status — NOT from admin
+  // routes like sync or debug/cli, because the SDK resets the FUSE overlay on
+  // createBackup, wiping upper-layer writes.
+  try {
+    await restoreIfNeeded(sandbox, c.env.BACKUP_BUCKET);
+  } catch (err) {
+    console.error('[PROXY] Backup restore failed:', err);
+  }
+
   // Check if gateway is already running
   const existingProcess = await findExistingGatewayProcess(sandbox);
   const isGatewayReady = existingProcess !== null && existingProcess.status === 'running';

src/persistence.ts

@@ -28,7 +28,12 @@ async function deleteHandle(bucket: R2Bucket): Promise<void> {
 
 /**
  * Restore the most recent backup if one exists and hasn't been restored yet.
- * Called on every request before proxying to the gateway.
+ *
+ * IMPORTANT: This must only be called from the catch-all route (gateway proxy)
+ * and /api/status — NOT from admin routes like sync or debug/cli. The Sandbox
+ * SDK's createBackup() resets the FUSE overlay, wiping any upper-layer writes.
+ * If restoreIfNeeded mounts an overlay before createBackup runs, the backup
+ * will lose files written to the upper layer.
  *
  * The backup handle is read from R2 (persisted across Worker isolate restarts).
  * An in-memory flag prevents redundant restores within the same isolate.
@@ -43,6 +48,16 @@ export async function restoreIfNeeded(sandbox: Sandbox, bucket: R2Bucket): Promi
     return;
   }
 
+  // Unmount any existing FUSE overlay before restoring. If the Worker isolate
+  // recycled, a previous restore's overlay may still be mounted with stale
+  // upper-layer state (e.g. deleted files via whiteout entries). A fresh
+  // mount from the backup gives us a clean lower layer.
+  try {
+    await sandbox.exec(`umount ${BACKUP_DIR} 2>/dev/null; true`);
+  } catch {
+    // May not be mounted
+  }
+
   console.log(`[persistence] Restoring backup ${handle.id}...`);
   const t0 = Date.now();
   try {

src/routes/api.ts

@@ -248,28 +248,38 @@ adminApi.post('/gateway/restart', async (c) => {
     // Find and kill the existing gateway process
     const existingProcess = await findExistingGatewayProcess(sandbox);
 
+    // Kill via the Process API first
+    if (existingProcess) {
+      console.log('[Restart] Killing via Process API:', existingProcess.id);
+      try {
+        await existingProcess.kill();
+      } catch {
+        // Ignore
+      }
+    }
+
     // Force kill the gateway via exec — more reliable than Process.kill()
     // because start-openclaw.sh execs into "openclaw" which forks
     // "openclaw-gateway". Process.kill() only kills the tracked shell PID,
     // but the forked child keeps port 18789. (Credit: dalexeenko #261)
     //
-    // The actual process names are "openclaw" and "openclaw-gateway" (hyphenated).
+    // Use multiple strategies since we don't know what tools are available.
     try {
-      await sandbox.exec(
-        'kill -9 $(pgrep -x "openclaw-gateway" 2>/dev/null) $(pgrep -x "openclaw" 2>/dev/null) 2>/dev/null; true',
+      const killResult = await sandbox.exec(
+        [
+          // Strategy 1: pgrep by exact name (most precise)
+          'kill -9 $(pgrep -x "openclaw-gateway" 2>/dev/null) $(pgrep -x "openclaw" 2>/dev/null) 2>/dev/null',
+          // Strategy 2: pkill by pattern (broader match)
+          'pkill -9 -f "openclaw" 2>/dev/null',
+          // Strategy 3: find by port (most reliable but needs ss/fuser)
+          'kill -9 $(ss -tlnp sport = :18789 2>/dev/null | grep -oP "pid=\\K[0-9]+") 2>/dev/null',
+          // Always succeed
+          'true',
+        ].join('; '),
       );
-    } catch {
-      // Process may not exist or pgrep not available
-    }
-
-    // Also kill via the Process API for completeness
-    if (existingProcess) {
-      console.log('Also killing via Process API:', existingProcess.id);
-      try {
-        await existingProcess.kill();
-      } catch {
-        // Ignore
-      }
+      console.log('[Restart] Kill result:', killResult.stdout?.trim(), killResult.stderr?.trim());
+    } catch (e) {
+      console.error('[Restart] Kill exec failed:', e);
     }
 
     // Clean up lock files that prevent restart
@@ -281,13 +291,11 @@ adminApi.post('/gateway/restart', async (c) => {
       // Ignore
     }
 
-    // Wait for process to fully die and verify port is free
-    await new Promise((r) => setTimeout(r, 2000));
+    // Wait for process to fully die and verify
+    await new Promise((r) => setTimeout(r, 3000));
     try {
-      const check = await sandbox.exec(
-        'pgrep -x "openclaw-gateway" && echo "STILL ALIVE" || echo "dead"',
-      );
-      console.log('[Restart] Process check after kill:', check.stdout?.trim());
+      const check = await sandbox.exec('ps aux | grep -v grep | grep openclaw || echo "ALL DEAD"');
+      console.log('[Restart] Surviving processes:', check.stdout?.trim());
     } catch {
       // Ignore
     }

src/routes/public.ts

@@ -2,6 +2,7 @@ import { Hono } from 'hono';
 import type { AppEnv } from '../types';
 import { GATEWAY_PORT } from '../config';
 import { findExistingGatewayProcess, ensureGateway } from '../gateway';
+import { restoreIfNeeded } from '../persistence';
 
 /**
  * Public routes - NO Cloudflare Access authentication required
@@ -34,6 +35,13 @@ publicRoutes.get('/logo-small.png', (c) => {
 publicRoutes.get('/api/status', async (c) => {
   const sandbox = c.get('sandbox');
 
+  // Restore from backup before checking/starting the gateway
+  try {
+    await restoreIfNeeded(sandbox, c.env.BACKUP_BUCKET);
+  } catch (err) {
+    console.error('[api/status] Backup restore failed:', err);
+  }
+
   try {
     let process = await findExistingGatewayProcess(sandbox);
     if (!process) {

start-openclaw.sh

@@ -88,6 +88,9 @@ config.gateway.port = 18789;
 config.gateway.mode = 'local';
 config.gateway.trustedProxies = ['10.1.0.0'];
 
+config.gateway.controlUi = config.gateway.controlUi || {};
+config.gateway.controlUi.allowedOrigins = ['*'];
+
 if (process.env.OPENCLAW_GATEWAY_TOKEN) {
     config.gateway.auth = config.gateway.auth || {};
     config.gateway.auth.token = process.env.OPENCLAW_GATEWAY_TOKEN;

test/e2e/r2_persistence.txt

@@ -60,8 +60,6 @@ create workspace marker file
 %require
 ===
 WORKER_URL=$(cat "$CCTR_FIXTURE_DIR/worker-url.txt")
-# Write directly to /home/openclaw (the backup dir) rather than via
-# the /root/clawd symlink, to avoid FUSE overlay resolution issues.
 ./curl-auth -s "$WORKER_URL/debug/cli?cmd=echo+e2e-persistence-test+%3E+/home/openclaw/clawd/e2e-marker.txt+%26%26+cat+/home/openclaw/clawd/e2e-marker.txt" | jq .
 ---
 {{ result: json object }}
@@ -86,7 +84,6 @@ delete marker file locally and confirm gone
 %require
 ===
 WORKER_URL=$(cat "$CCTR_FIXTURE_DIR/worker-url.txt")
-# Use rm -f and verify absence separately to tolerate FUSE overlay quirks
 ./curl-auth -s "$WORKER_URL/debug/cli?cmd=rm+-f+/home/openclaw/clawd/e2e-marker.txt+%26%26+test+!+-f+/home/openclaw/clawd/e2e-marker.txt+%26%26+echo+deleted-and-gone" | jq .
 ---
 {{ result: json object }}
@@ -111,9 +108,6 @@ wait for gateway to restart after restore
 %require
 ===
 WORKER_URL=$(cat "$CCTR_FIXTURE_DIR/worker-url.txt")
-# The restart handler kills the old process (waiting for it to die) and
-# clears the persistence cache. Hitting /api/status starts a new gateway
-# (after running restoreIfNeeded). Poll until the gateway is up.
 for i in $(seq 1 30); do
     STATUS=$(./curl-auth -s "$WORKER_URL/api/status" 2>/dev/null || echo "")
     OK=$(echo "$STATUS" | jq -r '.ok // false' 2>/dev/null)