From 7f25d4786bb73c1c54c633ca74de4e5fc9f564eb Mon Sep 17 00:00:00 2001
From: Garry Tan <garrytan@gmail.com>
Date: Sun, 5 Apr 2026 22:58:37 -0700
Subject: [PATCH] fix: verify tunnel is alive before returning URL to
 pair-agent

Root cause: when ngrok dies externally (pkill, crash, timeout), the server
still reports tunnelActive=true with a dead URL. pair-agent prints an
instruction block pointing at a dead tunnel. The remote agent gets
"endpoint offline" and the user has to manually restart everything.

Three-layer fix:
- Server /pair endpoint: probes tunnel URL before returning it. If dead,
  resets tunnelActive/tunnelUrl and returns null (triggers CLI restart).
- Server /tunnel/start: probes cached tunnel before returning already_active.
  If dead, falls through to restart ngrok automatically.
- CLI pair-agent: double-checks tunnel URL from server before printing
  instruction block. Falls through to auto-start on failure.

4 regression tests verify all three probe points + CLI verification.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 browse/src/cli.ts               | 19 +++++++++++++
 browse/src/server.ts            | 47 +++++++++++++++++++++++++++++----
 browse/test/server-auth.test.ts | 44 ++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/browse/src/cli.ts b/browse/src/cli.ts
index 575bec1b..5a7a8f2b 100644
--- a/browse/src/cli.ts
+++ b/browse/src/cli.ts
@@ -622,6 +622,25 @@ async function handlePairAgent(state: ServerState, args: string[]): Promise<void
 
   // Determine the URL to use
   let serverUrl: string;
+  if (pairData.tunnel_url) {
+    // Server already verified the tunnel is alive, but double-check from CLI side
+    // in case of race condition between server probe and our request
+    try {
+      const cliProbe = await fetch(`${pairData.tunnel_url}/health`, {
+        headers: { 'ngrok-skip-browser-warning': 'true' },
+        signal: AbortSignal.timeout(5000),
+      });
+      if (cliProbe.ok) {
+        serverUrl = pairData.tunnel_url;
+      } else {
+        console.warn(`[browse] Tunnel returned HTTP ${cliProbe.status}, attempting restart...`);
+        pairData.tunnel_url = null; // fall through to restart logic
+      }
+    } catch {
+      console.warn('[browse] Tunnel unreachable from CLI, attempting restart...');
+      pairData.tunnel_url = null; // fall through to restart logic
+    }
+  }
   if (pairData.tunnel_url) {
     serverUrl = pairData.tunnel_url;
   } else if (!localHost) {
diff --git a/browse/src/server.ts b/browse/src/server.ts
index f2ff82ea..fe242683 100644
--- a/browse/src/server.ts
+++ b/browse/src/server.ts
@@ -1445,11 +1445,34 @@ async function start() {
             domains: pairBody.domains,
             rateLimit: pairBody.rateLimit,
           });
+          // Verify tunnel is actually alive before reporting it (ngrok may have died externally)
+          let verifiedTunnelUrl: string | null = null;
+          if (tunnelActive && tunnelUrl) {
+            try {
+              const probe = await fetch(`${tunnelUrl}/health`, {
+                headers: { 'ngrok-skip-browser-warning': 'true' },
+                signal: AbortSignal.timeout(5000),
+              });
+              if (probe.ok) {
+                verifiedTunnelUrl = tunnelUrl;
+              } else {
+                console.warn(`[browse] Tunnel probe failed (HTTP ${probe.status}), marking tunnel as dead`);
+                tunnelActive = false;
+                tunnelUrl = null;
+                tunnelListener = null;
+              }
+            } catch {
+              console.warn('[browse] Tunnel probe timed out or unreachable, marking tunnel as dead');
+              tunnelActive = false;
+              tunnelUrl = null;
+              tunnelListener = null;
+            }
+          }
           return new Response(JSON.stringify({
             setup_key: setupKey.token,
             expires_at: setupKey.expiresAt,
             scopes: setupKey.scopes,
-            tunnel_url: tunnelActive ? tunnelUrl : null,
+            tunnel_url: verifiedTunnelUrl,
             server_url: `http://127.0.0.1:${server?.port || 0}`,
           }), { status: 200, headers: { 'Content-Type': 'application/json' } });
         } catch {
@@ -1466,10 +1489,24 @@ async function start() {
             status: 403, headers: { 'Content-Type': 'application/json' },
           });
         }
-        if (tunnelActive) {
-          return new Response(JSON.stringify({ url: tunnelUrl, already_active: true }), {
-            status: 200, headers: { 'Content-Type': 'application/json' },
-          });
+        if (tunnelActive && tunnelUrl) {
+          // Verify tunnel is still alive before returning cached URL
+          try {
+            const probe = await fetch(`${tunnelUrl}/health`, {
+              headers: { 'ngrok-skip-browser-warning': 'true' },
+              signal: AbortSignal.timeout(5000),
+            });
+            if (probe.ok) {
+              return new Response(JSON.stringify({ url: tunnelUrl, already_active: true }), {
+                status: 200, headers: { 'Content-Type': 'application/json' },
+              });
+            }
+          } catch {}
+          // Tunnel is dead, reset and fall through to restart
+          console.warn('[browse] Cached tunnel is dead, restarting...');
+          tunnelActive = false;
+          tunnelUrl = null;
+          tunnelListener = null;
         }
         try {
           // Read ngrok authtoken: env var > ~/.gstack/ngrok.env > ngrok native config
diff --git a/browse/test/server-auth.test.ts b/browse/test/server-auth.test.ts
index d8e4fad2..94c4d262 100644
--- a/browse/test/server-auth.test.ts
+++ b/browse/test/server-auth.test.ts
@@ -10,6 +10,7 @@ import * as fs from 'fs';
 import * as path from 'path';
 
 const SERVER_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/server.ts'), 'utf-8');
+const CLI_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/cli.ts'), 'utf-8');
 
 // Helper: extract a block of source between two markers
 function sliceBetween(source: string, startMarker: string, endMarker: string): string {
@@ -188,4 +189,47 @@ describe('Server auth security', () => {
     const commandStartBlock = sliceBetween(SERVER_SRC, "Activity: emit command_start", "try {");
     expect(commandStartBlock).toContain('clientId: tokenInfo?.clientId');
   });
+
+  // ─── Tunnel liveness verification ─────────────────────────────
+
+  // Test 11a: /pair endpoint probes tunnel before returning tunnel_url
+  test('/pair verifies tunnel is alive before returning tunnel_url', () => {
+    const pairBlock = sliceBetween(SERVER_SRC, "url.pathname === '/pair'", "url.pathname === '/tunnel/start'");
+    // Must probe the tunnel URL
+    expect(pairBlock).toContain('verifiedTunnelUrl');
+    expect(pairBlock).toContain('Tunnel probe failed');
+    expect(pairBlock).toContain('marking tunnel as dead');
+    // Must reset tunnel state on failure
+    expect(pairBlock).toContain('tunnelActive = false');
+    expect(pairBlock).toContain('tunnelUrl = null');
+  });
+
+  // Test 11b: /pair returns null tunnel_url when tunnel is dead
+  test('/pair returns verified tunnel URL, not raw tunnelActive flag', () => {
+    const pairBlock = sliceBetween(SERVER_SRC, "url.pathname === '/pair'", "url.pathname === '/tunnel/start'");
+    // Should use verifiedTunnelUrl (probe result), not raw tunnelUrl
+    expect(pairBlock).toContain('tunnel_url: verifiedTunnelUrl');
+    // Must NOT use raw tunnelActive check for the response
+    expect(pairBlock).not.toContain('tunnel_url: tunnelActive ? tunnelUrl');
+  });
+
+  // Test 11c: /tunnel/start probes cached tunnel before returning already_active
+  test('/tunnel/start verifies cached tunnel is alive before returning already_active', () => {
+    const tunnelBlock = sliceBetween(SERVER_SRC, "url.pathname === '/tunnel/start'", "url.pathname === '/refs'");
+    // Must probe before returning cached URL
+    expect(tunnelBlock).toContain('Cached tunnel is dead');
+    expect(tunnelBlock).toContain('tunnelActive = false');
+    // Must fall through to restart when dead
+    expect(tunnelBlock).toContain('restarting');
+  });
+
+  // Test 11d: CLI verifies tunnel_url from server before printing instruction block
+  test('CLI probes tunnel_url before using it in instruction block', () => {
+    const pairSection = sliceBetween(CLI_SRC, 'Determine the URL to use', 'local HOST: write config');
+    // Must probe the tunnel URL
+    expect(pairSection).toContain('cliProbe');
+    expect(pairSection).toContain('Tunnel unreachable from CLI');
+    // Must fall through to restart logic on failure
+    expect(pairSection).toContain('attempting restart');
+  });
 });