fix: close stale WebSocket on session switch to prevent double output and crash loop

Before: openTerminal() never closed the previous WebSocket. After A→B→A, two WS connections to session A both wrote to _term → double keystroke echoes. Rapid switches triggered a CONNECTING-state send error and reconnect loop. After: openTerminal() nulls _currentSession, clears reconnect timer, and closes _ws before opening a new connection. connect() event handlers now capture the WS instance in a local const and check 'ws !== _ws' on entry so stale connections silently ignore all events. Tests: three new regression tests verify: - openTerminal() calls close() on the previous WS (Bug 1) - stale open handler does not send on new WS after switch (Bug 2) - stale close handler does not schedule reconnect after switch (Bug 2)
2026-03-28 01:25:29 -07:00
parent e2d4282b5d
commit 26e2baa89c
2 changed files with 208 additions and 8 deletions
@@ -46,22 +46,30 @@ function connectWebSocket(name) {
    // 'tty' subprotocol is REQUIRED — without it ttyd never starts the PTY.
    // Confirmed via raw Python WebSocket tests: ttyd accepts the TCP upgrade but
    // sits completely silent (no child process spawned) when subprotocol is omitted.
-    _ws = new WebSocket(url, ['tty']);
-    _ws.binaryType = 'arraybuffer';
+    //
+    // Local const `ws` captures this specific instance so each handler can check
+    // `if (ws !== _ws) return;` (stale guard). Without it, rapid reconnects or
+    // session switches cause old handlers to fire on the new _ws while it is still
+    // CONNECTING → send error → close → reconnect → infinite loop (Bug 2).
+    const ws = new WebSocket(url, ['tty']);
+    _ws = ws;
+    ws.binaryType = 'arraybuffer';

-    _ws.addEventListener('open', function() {
+    ws.addEventListener('open', function() {
+      if (ws !== _ws) return; // stale connection — superseded by a newer one, ignore
      if (reconnectOverlay) reconnectOverlay.classList.add('hidden');
      // Step 1: TEXT frame auth handshake — ttyd checks AuthToken before starting PTY
-      _ws.send(JSON.stringify({ AuthToken: '' }));
+      ws.send(JSON.stringify({ AuthToken: '' }));
      // Step 2: BINARY frame with initial terminal dimensions — [0x31] + JSON({columns, rows})
      if (_term) {
-        _ws.send(encodePayload(0x31, JSON.stringify({ columns: _term.cols, rows: _term.rows })));
+        ws.send(encodePayload(0x31, JSON.stringify({ columns: _term.cols, rows: _term.rows })));
      }
      // Auto-focus the terminal so user can type immediately without clicking
      if (_term) _term.focus();
    });

-    _ws.addEventListener('message', function(e) {
+    ws.addEventListener('message', function(e) {
+      if (ws !== _ws) return; // stale connection — superseded by a newer one, ignore
      if (!_term) return;
      if (e.data instanceof ArrayBuffer) {
        var msg = new Uint8Array(e.data);
@@ -77,13 +85,15 @@ function connectWebSocket(name) {
      }
    });

-    _ws.addEventListener('close', function() {
+    ws.addEventListener('close', function() {
+      if (ws !== _ws) return; // stale connection — don't reconnect for old sockets
      if (!_currentSession) return; // intentional close — don't reconnect
      if (reconnectOverlay) reconnectOverlay.classList.remove('hidden');
      _reconnectTimer = setTimeout(connect, 2000);
    });

-    _ws.addEventListener('error', function() {
+    ws.addEventListener('error', function() {
+      if (ws !== _ws) return; // stale connection — ignore
      console.warn('tmux-web: WebSocket error on', url);
    });
  }
@@ -153,6 +163,22 @@ function createTerminal() {
 * @param {string} sessionName
 */
 function openTerminal(sessionName) {
+  // Null _currentSession first so any in-flight close handler on the old WS won't
+  // schedule a reconnect (it checks `if (!_currentSession) return;`).
+  _currentSession = null;
+
+  // Cancel any pending reconnect timer from the previous session.
+  if (_reconnectTimer) {
+    clearTimeout(_reconnectTimer);
+    _reconnectTimer = null;
+  }
+
+  // Close existing WebSocket so it can't write to the new terminal (Bug 1 fix).
+  if (_ws) {
+    _ws.close();
+    _ws = null;
+  }
+
  _currentSession = sessionName;

  const container = document.getElementById('terminal-container');
@@ -288,6 +288,180 @@ test('initVisualViewport returns early without error when window.visualViewport
  globalThis.setTimeout = orig;
 });

+// ─── Multi-session helpers ────────────────────────────────────────────────────
+
+/**
+ * Load a fresh terminal.js with a multi-WS-instance-aware environment.
+ * Unlike loadTerminal(), this tracks ALL WebSocket instances in order so tests
+ * can inspect individual connections after multiple openTerminal() calls.
+ */
+function createMultiSessionEnv() {
+  const modulePath = join(__dirname, '..', 'terminal.js');
+  delete require.cache[require.resolve(modulePath)];
+
+  const wsInstances = [];   // all WS objects created, in order
+  const termInstances = []; // all Terminal objects created, in order
+
+  class MockWS {
+    constructor(url, protocols) {
+      this.url = url;
+      this.protocols = protocols;
+      this.readyState = 1; // OPEN
+      this.binaryType = '';
+      this._handlers = {};
+      this.closeCalled = false;
+      this.sentMessages = [];
+      wsInstances.push(this);
+    }
+    addEventListener(event, fn) { this._handlers[event] = fn; }
+    fire(event, arg) { if (this._handlers[event]) this._handlers[event](arg); }
+    close() { this.closeCalled = true; }
+    send(data) { this.sentMessages.push(data); }
+  }
+  MockWS.OPEN = 1;
+  MockWS.CONNECTING = 0;
+
+  function makeMockTerm() {
+    const t = {
+      cols: 80, rows: 24,
+      open: () => {},
+      onData: () => {},
+      onResize: () => {},
+      loadAddon: () => {},
+      dispose: () => {},
+      focus: () => {},
+      writeMessages: [],
+    };
+    t.write = (data) => t.writeMessages.push(data);
+    termInstances.push(t);
+    return t;
+  }
+
+  let capturedReconnectFn = null;
+  const origSetTimeout = globalThis.setTimeout;
+  globalThis.setTimeout = (fn, _ms) => { capturedReconnectFn = fn; return 0; };
+  globalThis.WebSocket = MockWS;
+  globalThis.location = { protocol: 'http:', host: 'localhost' };
+  globalThis.document = {
+    getElementById: (id) => {
+      if (id === 'terminal-container') return { appendChild: () => {} };
+      if (id === 'reconnect-overlay') return { classList: { add: () => {}, remove: () => {} } };
+      return null;
+    },
+    querySelector: () => null,
+    querySelectorAll: () => [],
+    addEventListener: () => {},
+    createElement: () => ({ style: {}, classList: { add: () => {}, remove: () => {} } }),
+  };
+  globalThis.window = {
+    addEventListener: () => {},
+    location: { href: '' },
+    innerWidth: 1024,
+    Terminal: function() { return makeMockTerm(); },
+    FitAddon: { FitAddon: function() { return { fit: () => {} }; } },
+    _openTerminal: undefined,
+    _closeTerminal: undefined,
+  };
+
+  require(modulePath);
+  globalThis.setTimeout = origSetTimeout;
+
+  const env = {
+    get wsInstances() { return wsInstances; },
+    get termInstances() { return termInstances; },
+    get capturedReconnectFn() { return capturedReconnectFn; },
+
+    /** Call fn() with setTimeout mocked so reconnect timers are captured but not auto-run. */
+    withTimeout(fn) {
+      const orig = globalThis.setTimeout;
+      globalThis.setTimeout = (cb, _ms) => { capturedReconnectFn = cb; return 0; };
+      fn();
+      globalThis.setTimeout = orig;
+    },
+
+    openTerminal(name) { env.withTimeout(() => globalThis.window._openTerminal(name)); },
+    closeTerminal() { globalThis.window._closeTerminal(); },
+
+    /** Fire the pending reconnect callback (if any), capturing any new reconnect it schedules. */
+    fireReconnect() {
+      if (!capturedReconnectFn) return;
+      const fn = capturedReconnectFn;
+      capturedReconnectFn = null;
+      env.withTimeout(() => fn());
+    },
+  };
+
+  return env;
+}
+
+// ─── Bug-fix regression tests ─────────────────────────────────────────────────
+// Bug 1 — double keystrokes on switch-away-and-back
+// Bug 2 — "Still in CONNECTING state" crash loop
+
+test('openTerminal closes previous WebSocket before opening new connection (bug: stale WS double output)', () => {
+  const env = createMultiSessionEnv();
+
+  env.openTerminal('session-a');
+  assert.strictEqual(env.wsInstances.length, 1, 'First openTerminal should create exactly 1 WS');
+  const ws1 = env.wsInstances[0];
+
+  env.openTerminal('session-b');
+
+  // Bug 1: without the fix, ws1.close() is never called — the old socket stays alive and
+  // both WS1 and WS2 write to the same xterm terminal, producing doubled keystrokes.
+  assert.ok(ws1.closeCalled,
+    'Bug 1: openTerminal must call close() on the previous WebSocket to prevent stale writes');
+  assert.strictEqual(env.wsInstances.length, 2, 'Second openTerminal should have created a second WS');
+});
+
+test('stale open handler is a no-op after session switch (bug: crash loop)', () => {
+  const env = createMultiSessionEnv();
+
+  env.openTerminal('session-a');
+  const ws1 = env.wsInstances[0];
+  // Capture WS1's open handler before the switch displaces it
+  const openHandler1 = ws1._handlers['open'];
+  assert.ok(openHandler1, 'WS1 must have had an open handler registered');
+
+  env.openTerminal('session-b');
+  const ws2 = env.wsInstances[1];
+
+  // Simulate WS1's open event arriving late (browser timing — arrives after WS2 is live).
+  // Bug 2: without the stale guard, the handler does _ws.send() where _ws is now WS2
+  // (which is CONNECTING) → WebSocket error → WS2 close → reconnect → infinite loop.
+  if (openHandler1) openHandler1();
+
+  assert.strictEqual(ws2.sentMessages.length, 0,
+    'Bug 2: stale open handler for WS1 must not send auth/resize on the new WS2');
+});
+
+test('stale close handler does not trigger reconnect after session switch (bug: crash loop)', () => {
+  const env = createMultiSessionEnv();
+
+  env.openTerminal('session-a');
+  const ws1 = env.wsInstances[0];
+  const closeHandler1 = ws1._handlers['close'];
+  assert.ok(closeHandler1, 'WS1 must have had a close handler registered');
+
+  env.openTerminal('session-b');
+
+  // After the switch: _ws = WS2, _currentSession = 'session-b'
+  // Simulate WS1's close event arriving late (server finishes closing the old socket).
+  let reconnectScheduled = false;
+  const origSetTimeout = globalThis.setTimeout;
+  globalThis.setTimeout = (_fn, _ms) => { reconnectScheduled = true; return 0; };
+
+  if (closeHandler1) closeHandler1();
+
+  globalThis.setTimeout = origSetTimeout;
+
+  // Bug 2: without stale guard, !_currentSession is false ('session-b' is set), so the
+  // handler schedules connect() — a fresh WS replaces _ws while WS2 is CONNECTING → loop.
+  // With stale guard: ws1 !== _ws (WS2) → return early → no reconnect.
+  assert.ok(!reconnectScheduled,
+    'Bug 2: stale close handler for WS1 must not schedule a reconnect after switching sessions');
+});
+
 // ─── ttyd protocol tests ──────────────────────────────────────────────────────
 // ttyd 1.7.7 requires:
 //   1. WebSocket subprotocol 'tty' — without it ttyd never starts the PTY