Browse Source

fix: resolve multiplexer server URLs lazily

highcoldddd 3 weeks ago
parent
commit
7948bc1989

+ 39 - 11
docs/multiplexer-integration.md

@@ -27,18 +27,46 @@ When OpenCode launches child agent sessions, oh-my-opencode-slim can open panes
 
 *OpenCode running in tmux with live subagent panes.*
 
-> ⚠️ **Current workaround:** Start OpenCode with `--port` to enable multiplexer integration. The port must match the `OPENCODE_PORT` environment variable. This is required until [opencode#9099](https://github.com/anomalyco/opencode/issues/9099) is resolved.
-
-If you open multiple OpenCode sessions, use a random high port for each launch instead of hard-coding `4096`.
-
-**Bash helper:**
-
-```bash
+OpenCode 1.17.18's normal default (`port 0`) does not expose a TCP listener that
+another `opencode attach` process can use from a multiplexer pane. Start
+OpenCode with an explicit `--port`, but do not hard-code `4096` when running
+multiple instances. The plugin now reads `ctx.serverUrl` only when checking,
+spawning, or polling, which avoids snapshotting the temporary startup URL; it
+cannot create a listener that OpenCode did not start.
+
+This zsh helper preserves an explicit `--port` and exports the matching
+`OPENCODE_PORT`. Otherwise, it asks Python to select an available loopback port
+and starts OpenCode with that port explicitly:
+
+```zsh
 omos() {
-  local port
-  port=$(jot -r 1 49152 65535)
-  OPENCODE_PORT="$port" \
-  opencode --port "$port" "$@"
+  local port arg
+
+  for arg in "$@"; do
+    if [[ "$arg" == --port=* ]]; then
+      port="${arg#--port=}"
+      break
+    fi
+  done
+
+  if [[ -z "$port" ]]; then
+    local -a args=("$@")
+    local -i index
+    for ((index = 1; index <= ${#args}; index++)); do
+      if [[ "${args[index]}" == --port ]]; then
+        port="${args[index + 1]}"
+        break
+      fi
+    done
+  fi
+
+  if [[ -n "$port" ]]; then
+    OPENCODE_PORT="$port" command opencode "$@"
+    return
+  fi
+
+  port=$(python3 -c 'import socket; s = socket.socket(); s.bind(("127.0.0.1", 0)); print(s.getsockname()[1]); s.close()') || return
+  OPENCODE_PORT="$port" command opencode --port "$port" "$@"
 }
 ```
 

+ 12 - 5
src/multiplexer/cmux/session-lifecycle.test.ts

@@ -31,7 +31,7 @@ describe('CmuxSessionLifecycle races', () => {
     const lifecycle = new CmuxSessionLifecycle(
       'owner',
       mux,
-      'http://server',
+      () => 'http://server',
       '/repo',
       undefined,
       { isServerRunning: async () => true },
@@ -63,7 +63,7 @@ describe('CmuxSessionLifecycle races', () => {
     const lifecycle = new CmuxSessionLifecycle(
       'owner',
       mux,
-      'http://server',
+      () => 'http://server',
       '/repo',
       undefined,
       {
@@ -105,9 +105,16 @@ describe('CmuxSessionLifecycle races', () => {
     });
     const mux = multiplexer();
     mux.closePane.mockResolvedValue(false);
-    new CmuxSessionLifecycle('new', mux, 'http://server', '/repo', undefined, {
-      closeRetryMaxAttempts: 1,
-    });
+    new CmuxSessionLifecycle(
+      'new',
+      mux,
+      () => 'http://server',
+      '/repo',
+      undefined,
+      {
+        closeRetryMaxAttempts: 1,
+      },
+    );
     await Promise.resolve();
     await Promise.resolve();
     expect(mux.closePane).toHaveBeenCalledTimes(1);

+ 22 - 4
src/multiplexer/cmux/session-lifecycle.ts

@@ -1,4 +1,5 @@
 import { POLL_INTERVAL_BACKGROUND_MS } from '../../config';
+import { log } from '../../utils/logger';
 import type { Multiplexer } from '../types';
 import { isServerRunning } from '../types';
 import { CmuxClosePolicy, type CmuxCloseReason } from './close-policy';
@@ -50,6 +51,13 @@ const ACTIVITY_EVENTS = new Set([
 const MIN_LIFETIME_MS = 10_000;
 const IDLE_CONFIRMATIONS = 3;
 
+class ServerUrlUnavailableError extends Error {
+  constructor() {
+    super('OpenCode server URL is unavailable');
+    this.name = 'ServerUrlUnavailableError';
+  }
+}
+
 export class CmuxSessionLifecycle {
   private readonly store = new CmuxSessionStore();
   private readonly policy: CmuxClosePolicy;
@@ -74,7 +82,7 @@ export class CmuxSessionLifecycle {
   constructor(
     private readonly owner: string,
     private readonly multiplexer: Multiplexer,
-    private readonly serverUrl: string,
+    private readonly resolveServerUrl: () => string | null,
     private readonly defaultDirectory: string,
     private readonly backgroundJobs?: BackgroundJobs,
     options: CmuxSessionLifecycleOptions = {},
@@ -252,14 +260,19 @@ export class CmuxSessionLifecycle {
   }
 
   private async spawnOperation(record: CmuxSessionRecord) {
-    if (!(await this.serverCheck(this.serverUrl))) {
+    const serverUrl = this.resolveServerUrl();
+    if (!serverUrl) {
+      log('[cmux-session-lifecycle] no valid server URL; skipping spawn');
+      return { success: false, error: 'unavailable' as const };
+    }
+    if (!(await this.serverCheck(serverUrl))) {
       return { success: false, error: 'unavailable' as const };
     }
     try {
       return await this.multiplexer.spawnPane(
         record.session,
         record.title,
-        this.serverUrl,
+        serverUrl,
         record.directory,
       );
     } catch {
@@ -559,7 +572,12 @@ export class CmuxSessionLifecycle {
   }
 
   private async loadStatuses(): Promise<Record<string, { type: string }>> {
-    const response = await fetch(new URL('/session/status', this.serverUrl), {
+    const serverUrl = this.resolveServerUrl();
+    if (!serverUrl) {
+      log('[cmux-session-lifecycle] no valid server URL; skipping poll');
+      throw new ServerUrlUnavailableError();
+    }
+    const response = await fetch(new URL('/session/status', serverUrl), {
       signal: AbortSignal.timeout(2_000),
     });
     if (!response.ok)

+ 331 - 0
src/multiplexer/session-manager.test.ts

@@ -1410,6 +1410,337 @@ describe('MultiplexerSessionManager', () => {
   describe('cmux lifecycle', () => {
     const cmuxConfig = { ...defaultMultiplexerConfig, type: 'cmux' as const };
 
+    test('resolves a dynamic serverUrl after manager construction', async () => {
+      mockMultiplexerType = 'cmux';
+      let serverUrl = new URL('http://localhost:4096/');
+      let getterCalls = 0;
+      const ctx = createMockContext();
+      Object.defineProperty(ctx, 'serverUrl', {
+        configurable: true,
+        get: () => {
+          getterCalls += 1;
+          return serverUrl;
+        },
+      });
+      const serverCheck = mock(async () => true);
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        cmuxConfig,
+        undefined,
+        {
+          isServerRunning: serverCheck,
+        },
+      );
+      expect(getterCalls).toBe(0);
+
+      serverUrl = new URL('http://127.0.0.1:63871/');
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'dynamic-port', parentID: 'parent' } },
+      });
+      setMockSessionStatuses({ 'dynamic-port': { type: 'busy' } });
+      await (manager as any).pollSessions();
+
+      expect(serverCheck).toHaveBeenCalledWith('http://127.0.0.1:63871/');
+      expect(mockMultiplexer.spawnPane).toHaveBeenCalledWith(
+        'dynamic-port',
+        'Subagent',
+        'http://127.0.0.1:63871/',
+        '/test/directory',
+      );
+      expect(mockFetch).toHaveBeenCalledWith(
+        new URL('http://127.0.0.1:63871/session/status'),
+        expect.any(Object),
+      );
+      expect(serverCheck).not.toHaveBeenCalledWith('http://localhost:4096/');
+      expect(mockFetch).not.toHaveBeenCalledWith(
+        new URL('http://localhost:4096/session/status'),
+        expect.any(Object),
+      );
+    });
+
+    test('pins the URL selected before an awaited health check', async () => {
+      mockMultiplexerType = 'cmux';
+      let serverUrl = new URL('http://127.0.0.1:63871/');
+      const ctx = createMockContext();
+      Object.defineProperty(ctx, 'serverUrl', { get: () => serverUrl });
+      const health = createDeferred<boolean>();
+      const serverCheck = mock(() => health.promise);
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        cmuxConfig,
+        undefined,
+        {
+          isServerRunning: serverCheck,
+        },
+      );
+
+      const creating = manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'pinned-url', parentID: 'parent' } },
+      });
+      await Promise.resolve();
+      serverUrl = new URL('http://127.0.0.1:63872/');
+      health.resolve(true);
+      await creating;
+
+      expect(mockMultiplexer.spawnPane).toHaveBeenCalledWith(
+        'pinned-url',
+        'Subagent',
+        'http://127.0.0.1:63871/',
+        '/test/directory',
+      );
+    });
+
+    test('deferred spawn retry resolves the latest URL', async () => {
+      mockMultiplexerType = 'cmux';
+      let serverUrl = new URL('http://127.0.0.1:63871/');
+      const ctx = createMockContext();
+      Object.defineProperty(ctx, 'serverUrl', { get: () => serverUrl });
+      const retry = createDeferred<void>();
+      const serverCheck = mock(async () => true);
+      mockMultiplexer.spawnPane
+        .mockResolvedValueOnce({ success: false, error: 'unavailable' })
+        .mockResolvedValueOnce({ success: true, paneId: 'retried-pane' });
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        cmuxConfig,
+        undefined,
+        {
+          isServerRunning: serverCheck,
+          delay: () => retry.promise,
+        },
+      );
+
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'deferred-url', parentID: 'parent' } },
+      });
+      serverUrl = new URL('http://127.0.0.1:63872/');
+      retry.resolve();
+      await flushPromises();
+
+      expect(serverCheck).toHaveBeenLastCalledWith('http://127.0.0.1:63872/');
+      expect(mockMultiplexer.spawnPane).toHaveBeenLastCalledWith(
+        'deferred-url',
+        'Subagent',
+        'http://127.0.0.1:63872/',
+        '/test/directory',
+      );
+    });
+
+    test('busy respawn resolves the latest URL', async () => {
+      mockMultiplexerType = 'cmux';
+      let serverUrl = new URL('http://127.0.0.1:63871/');
+      const ctx = createMockContext();
+      Object.defineProperty(ctx, 'serverUrl', { get: () => serverUrl });
+      const manager = new MultiplexerSessionManager(ctx, cmuxConfig);
+      mockMultiplexer.spawnPane
+        .mockResolvedValueOnce({ success: true, paneId: 'first-pane' })
+        .mockResolvedValueOnce({ success: true, paneId: 'second-pane' });
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'busy-url', parentID: 'parent' } },
+      });
+      await manager.closeSessionFromCoordinator('busy-url');
+      serverUrl = new URL('http://127.0.0.1:63872/');
+      await manager.onSessionStatus({
+        type: 'session.status',
+        properties: { sessionID: 'busy-url', status: { type: 'busy' } },
+      });
+
+      expect(mockMultiplexer.spawnPane).toHaveBeenLastCalledWith(
+        'busy-url',
+        'Subagent',
+        'http://127.0.0.1:63872/',
+        '/test/directory',
+      );
+    });
+
+    test('each poll resolves the latest URL', async () => {
+      mockMultiplexerType = 'cmux';
+      let serverUrl = new URL('http://127.0.0.1:63871/');
+      const ctx = createMockContext();
+      Object.defineProperty(ctx, 'serverUrl', { get: () => serverUrl });
+      const manager = new MultiplexerSessionManager(ctx, cmuxConfig);
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'poll-url', parentID: 'parent' } },
+      });
+      await (manager as any).pollSessions();
+      serverUrl = new URL('http://127.0.0.1:63872/');
+      await (manager as any).pollSessions();
+
+      expect(mockFetch.mock.calls.at(-2)?.[0]).toEqual(
+        new URL('http://127.0.0.1:63871/session/status'),
+      );
+      expect(mockFetch.mock.calls.at(-1)?.[0]).toEqual(
+        new URL('http://127.0.0.1:63872/session/status'),
+      );
+    });
+
+    test('temporary missing URL never advances missing grace or closes a pane', async () => {
+      mockMultiplexerType = 'cmux';
+      let now = 0;
+      let serverUrl: URL | undefined = new URL('http://127.0.0.1:63871/');
+      const ctx = createMockContext();
+      Object.defineProperty(ctx, 'serverUrl', { get: () => serverUrl });
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        cmuxConfig,
+        undefined,
+        {
+          now: () => now,
+          missingGraceMs: 10,
+        },
+      );
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'temporary-url', parentID: 'parent' } },
+      });
+
+      serverUrl = undefined;
+      for (now = 10; now <= 40; now += 10)
+        await (manager as any).pollSessions();
+      expect(mockMultiplexer.closePane).not.toHaveBeenCalled();
+
+      serverUrl = new URL('http://127.0.0.1:63872/');
+      setMockSessionStatuses({ 'temporary-url': { type: 'busy' } });
+      await (manager as any).pollSessions();
+      expect(mockFetch).toHaveBeenLastCalledWith(
+        new URL('http://127.0.0.1:63872/session/status'),
+        expect.any(Object),
+      );
+      expect(mockMultiplexer.closePane).not.toHaveBeenCalled();
+    });
+
+    test('uses the SDK client baseUrl when ctx.serverUrl is missing', async () => {
+      mockMultiplexerType = 'cmux';
+      const ctx = createMockContext();
+      ctx.serverUrl = undefined;
+      ctx.client._client = {
+        getConfig: () => ({ baseUrl: 'http://127.0.0.1:63872/' }),
+      };
+      const serverCheck = mock(async () => true);
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        cmuxConfig,
+        undefined,
+        {
+          isServerRunning: serverCheck,
+        },
+      );
+
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'client-url', parentID: 'parent' } },
+      });
+
+      expect(serverCheck).toHaveBeenCalledWith('http://127.0.0.1:63872/');
+      expect(mockMultiplexer.spawnPane).toHaveBeenCalledWith(
+        'client-url',
+        'Subagent',
+        'http://127.0.0.1:63872/',
+        '/test/directory',
+      );
+    });
+
+    test('does not health check, spawn, or fall back to 4096 without a URL', async () => {
+      mockMultiplexerType = 'cmux';
+      const ctx = createMockContext();
+      ctx.serverUrl = undefined;
+      const manager = new MultiplexerSessionManager(ctx, cmuxConfig);
+
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'no-url', parentID: 'parent' } },
+      });
+      await (manager as any).pollSessions();
+
+      expect(mockIsServerRunning).not.toHaveBeenCalled();
+      expect(mockMultiplexer.spawnPane).not.toHaveBeenCalled();
+      expect(mockFetch).not.toHaveBeenCalled();
+    });
+
+    test('malicious client reflection cannot break initialization or spawning', async () => {
+      mockMultiplexerType = 'cmux';
+      for (const client of [
+        new Proxy(
+          {},
+          {
+            has: () => true,
+            get: () => {
+              throw new Error('get');
+            },
+          },
+        ),
+        Object.defineProperty({}, '_client', {
+          get: () => {
+            throw new Error('accessor');
+          },
+        }),
+        {
+          _client: Object.defineProperty({}, 'getConfig', {
+            get: () => {
+              throw new Error('getConfig accessor');
+            },
+          }),
+        },
+        {
+          _client: {
+            getConfig: () =>
+              Object.defineProperty({}, 'baseUrl', {
+                get: () => {
+                  throw new Error('baseUrl accessor');
+                },
+              }),
+          },
+        },
+      ]) {
+        const ctx = createMockContext();
+        ctx.serverUrl = undefined;
+        ctx.client = client;
+        const manager = new MultiplexerSessionManager(ctx, cmuxConfig);
+        await expect(
+          manager.onSessionCreated({
+            type: 'session.created',
+            properties: {
+              info: { id: `proxy-${Math.random()}`, parentID: 'p' },
+            },
+          }),
+        ).resolves.toBeUndefined();
+      }
+      expect(mockIsServerRunning).not.toHaveBeenCalled();
+      expect(mockMultiplexer.spawnPane).not.toHaveBeenCalled();
+    });
+
+    test('keeps an explicit fixed 4096 serverUrl working', async () => {
+      mockMultiplexerType = 'cmux';
+      const ctx = createMockContext({ serverUrl: 'http://localhost:4096/' });
+      const serverCheck = mock(async () => true);
+      const manager = new MultiplexerSessionManager(
+        ctx,
+        cmuxConfig,
+        undefined,
+        {
+          isServerRunning: serverCheck,
+        },
+      );
+
+      await manager.onSessionCreated({
+        type: 'session.created',
+        properties: { info: { id: 'fixed-port', parentID: 'parent' } },
+      });
+
+      expect(serverCheck).toHaveBeenCalledWith('http://localhost:4096/');
+      expect(mockMultiplexer.spawnPane).toHaveBeenCalledWith(
+        'fixed-port',
+        'Subagent',
+        'http://localhost:4096/',
+        '/test/directory',
+      );
+    });
+
     test('requires lifetime, three idle polls, and a final idle recheck', async () => {
       mockMultiplexerType = 'cmux';
       let now = 0;

+ 84 - 13
src/multiplexer/session-manager.ts

@@ -90,6 +90,50 @@ export function resetMultiplexerSessionManagerState(): void {
 
 export type MultiplexerSessionManagerOptions = CmuxSessionLifecycleOptions;
 
+function validServerUrl(value: unknown): string | null {
+  if (typeof value !== 'string' && !(value instanceof URL)) return null;
+  try {
+    const url = new URL(value.toString());
+    return url.protocol === 'http:' || url.protocol === 'https:'
+      ? url.toString()
+      : null;
+  } catch {
+    return null;
+  }
+}
+
+function clientBaseUrl(client: unknown): string | null {
+  try {
+    if (!client || typeof client !== 'object' || !('_client' in client))
+      return null;
+    const internal = client._client;
+    if (!internal || typeof internal !== 'object' || !('getConfig' in internal))
+      return null;
+    const getConfig = internal.getConfig;
+    if (typeof getConfig !== 'function') return null;
+    const config: unknown = getConfig.call(internal);
+    if (!config || typeof config !== 'object' || !('baseUrl' in config))
+      return null;
+    return validServerUrl(config.baseUrl);
+  } catch {
+    return null;
+  }
+}
+
+function createServerUrlResolver(ctx: PluginInput): () => string | null {
+  return () => {
+    try {
+      const serverUrl = validServerUrl(ctx.serverUrl);
+      if (serverUrl) return serverUrl;
+    } catch {}
+    try {
+      return clientBaseUrl(ctx.client);
+    } catch {
+      return null;
+    }
+  };
+}
+
 /**
  * Tracks child sessions and spawns/closes multiplexer panes for them.
  *
@@ -98,7 +142,7 @@ export type MultiplexerSessionManagerOptions = CmuxSessionLifecycleOptions;
  */
 export class MultiplexerSessionManager {
   private instanceId = Math.random().toString(36).slice(2, 8);
-  private serverUrl: string;
+  private readonly resolveServerUrl: () => string | null;
   private directory: string;
   private multiplexer: Multiplexer | null = null;
   private sessions: SharedSessionState['sessions'];
@@ -122,9 +166,7 @@ export class MultiplexerSessionManager {
     this.closingSessions = sharedState.closingSessions;
 
     this.directory = ctx.directory;
-    const defaultPort = process.env.OPENCODE_PORT ?? '4096';
-    this.serverUrl =
-      ctx.serverUrl?.toString() ?? `http://localhost:${defaultPort}`;
+    this.resolveServerUrl = createServerUrlResolver(ctx);
 
     this.multiplexer = getMultiplexer(config);
     this.enabled =
@@ -135,7 +177,7 @@ export class MultiplexerSessionManager {
       this.cmuxLifecycle = new CmuxSessionLifecycle(
         this.instanceId,
         this.multiplexer,
-        this.serverUrl,
+        this.resolveServerUrl,
         this.directory,
         this.backgroundJobBoard,
         options,
@@ -146,7 +188,7 @@ export class MultiplexerSessionManager {
       instanceId: this.instanceId,
       enabled: this.enabled,
       type: config.type,
-      serverUrl: this.serverUrl,
+      serverUrl: 'dynamic',
       trackedSessions: this.sessions.size,
       knownSessions: this.knownSessions.size,
     });
@@ -189,11 +231,22 @@ export class MultiplexerSessionManager {
     this.spawningSessions.add(sessionId);
 
     try {
-      const serverRunning = await isServerRunning(this.serverUrl);
+      const serverUrl = this.resolveServerUrl();
+      if (!serverUrl) {
+        log(
+          '[multiplexer-session-manager] no valid server URL, skipping spawn',
+          {
+            instanceId: this.instanceId,
+            sessionId,
+          },
+        );
+        return;
+      }
+      const serverRunning = await isServerRunning(serverUrl);
       if (!serverRunning) {
         log('[multiplexer-session-manager] server not running, skipping', {
           instanceId: this.instanceId,
-          serverUrl: this.serverUrl,
+          serverUrl,
         });
         return;
       }
@@ -213,7 +266,7 @@ export class MultiplexerSessionManager {
       );
 
       const paneResult = await this.multiplexer
-        .spawnPane(sessionId, title, this.serverUrl, directory)
+        .spawnPane(sessionId, title, serverUrl, directory)
         .catch((err) => {
           log('[multiplexer-session-manager] failed to spawn pane', {
             instanceId: this.instanceId,
@@ -409,7 +462,14 @@ export class MultiplexerSessionManager {
   private async fetchSessionStatuses(): Promise<
     Record<string, { type: string }>
   > {
-    const url = new URL('/session/status', this.serverUrl);
+    const serverUrl = this.resolveServerUrl();
+    if (!serverUrl) {
+      log('[multiplexer-session-manager] no valid server URL, skipping poll', {
+        instanceId: this.instanceId,
+      });
+      return {};
+    }
+    const url = new URL('/session/status', serverUrl);
     const response = await fetch(url, { signal: AbortSignal.timeout(2_000) });
 
     if (!response.ok) {
@@ -541,13 +601,24 @@ export class MultiplexerSessionManager {
     this.spawningSessions.add(sessionId);
 
     try {
-      const serverRunning = await isServerRunning(this.serverUrl);
+      const serverUrl = this.resolveServerUrl();
+      if (!serverUrl) {
+        log(
+          '[multiplexer-session-manager] no valid server URL, skipping respawn',
+          {
+            instanceId: this.instanceId,
+            sessionId,
+          },
+        );
+        return;
+      }
+      const serverRunning = await isServerRunning(serverUrl);
       if (!serverRunning) {
         log(
           '[multiplexer-session-manager] server not running, skipping busy respawn',
           {
             instanceId: this.instanceId,
-            serverUrl: this.serverUrl,
+            serverUrl,
             sessionId,
           },
         );
@@ -569,7 +640,7 @@ export class MultiplexerSessionManager {
       );
 
       const paneResult = await this.multiplexer
-        .spawnPane(sessionId, known.title, this.serverUrl, known.directory)
+        .spawnPane(sessionId, known.title, serverUrl, known.directory)
         .catch((err) => {
           log('[multiplexer-session-manager] failed to respawn pane', {
             instanceId: this.instanceId,