Merge master into the 1.41.0 release branch (faster session close, README GIFs)

test/process-exit-wait.test.ts

@@ -0,0 +1,81 @@
+/**
+ * @fileoverview The kill path's waits end when the processes do, not on a timer.
+ *
+ * Closing a session used to sleep a fixed 100 + 200 + 100 ms across its signals
+ * and then verify in 100 ms steps, so a session whose processes were gone in a
+ * few ms still took ~0.45 s to close. `waitForProcessesExit()` keeps each of
+ * those deadlines but returns as soon as nothing is left running, and
+ * `isProcessRunning()` counts a zombie as exited (it holds nothing but its pid
+ * until its reaper gets to it, and `kill(pid, 0)` cannot tell it apart).
+ *
+ * Port: N/A.
+ */
+import { spawn } from 'node:child_process';
+import { readFileSync } from 'node:fs';
+import { describe, expect, it } from 'vitest';
+
+import { isProcessRunning, waitForProcessesExit } from '../src/utils/process-exit-wait.js';
+
+describe('waitForProcessesExit', () => {
+  it('returns as soon as every pid has exited, long before the deadline', async () => {
+    let polls = 0;
+    const isRunning = (): boolean => ++polls < 4;
+    const started = Date.now();
+
+    const survivors = await waitForProcessesExit([101, 102], { timeoutMs: 5000, pollMs: 5, isRunning });
+
+    expect(survivors).toEqual([]);
+    expect(Date.now() - started).toBeLessThan(1000);
+  });
+
+  it('returns the survivors at the deadline instead of throwing', async () => {
+    const started = Date.now();
+
+    const survivors = await waitForProcessesExit([7, 8], {
+      timeoutMs: 60,
+      pollMs: 10,
+      isRunning: (pid) => pid === 8,
+    });
+
+    expect(survivors).toEqual([8]);
+    expect(Date.now() - started).toBeGreaterThanOrEqual(50);
+  });
+
+  it('does not wait at all when nothing is running', async () => {
+    const started = Date.now();
+    expect(await waitForProcessesExit([], { timeoutMs: 5000 })).toEqual([]);
+    expect(await waitForProcessesExit([9], { timeoutMs: 5000, isRunning: () => false })).toEqual([]);
+    expect(Date.now() - started).toBeLessThan(100);
+  });
+});
+
+describe('isProcessRunning', () => {
+  it('is true for a live process and false for a pid that does not exist', () => {
+    expect(isProcessRunning(process.pid)).toBe(true);
+    // Above the default pid_max on Linux and macOS alike.
+    expect(isProcessRunning(4_194_304 + 12_345)).toBe(false);
+  });
+
+  it.skipIf(process.platform !== 'linux')('counts a zombie as exited, which kill(pid, 0) cannot', async () => {
+    // `sleep 0` exits at once, and its parent then becomes `sleep 5`, which never
+    // reaps anything: the child stays a zombie until the parent itself goes.
+    const parent = spawn('sh', ['-c', 'sleep 0 & echo $!; exec sleep 5'], { stdio: ['ignore', 'pipe', 'ignore'] });
+    try {
+      const zombie = await new Promise<number>((resolve, reject) => {
+        parent.stdout.once('data', (chunk: Buffer) => resolve(parseInt(chunk.toString(), 10)));
+        parent.once('error', reject);
+      });
+      const state = (): string => {
+        const stat = readFileSync(`/proc/${zombie}/stat`, 'utf8');
+        return stat.charAt(stat.lastIndexOf(')') + 2);
+      };
+      for (let i = 0; i < 100 && state() !== 'Z'; i++) await new Promise((r) => setTimeout(r, 10));
+      expect(state()).toBe('Z');
+
+      expect(() => process.kill(zombie, 0)).not.toThrow();
+      expect(isProcessRunning(zombie)).toBe(false);
+    } finally {
+      parent.kill('SIGKILL');
+    }
+  });
+});

test/session-close-fallback.test.ts

@@ -16,6 +16,11 @@
  * leaves the active-session handoff alone for a close this tab started. A delete
  * from anywhere else still lands on the welcome screen.
  *
+ * The close is also OPTIMISTIC: the tab goes and the next one is selected before
+ * the DELETE is even sent (the server's kill takes a few hundred ms, and a tab
+ * sitting there that long read as a dead button). A refused delete puts the row
+ * back where it was; one whose reply was lost is checked with a GET first.
+ *
  * Loaded via `vm` with a stubbed context (no jsdom), like input-send-order.test.ts.
  * Port: N/A.
  */
@@ -74,7 +79,11 @@ function makeApp(active: string | null, order = [A, B]): TestApp {
   app.isSoloWindow = false;
   app._wsSessionId = null;
   app.terminal = { clear: vi.fn() };
-  app._apiDelete = vi.fn(async () => ({ success: true }));
+  // Shaped like the Response the real helper resolves with (or null: no answer).
+  app._apiDelete = vi.fn(async () => ({ ok: true, status: 200 }));
+  // The failure path's "is it still there?" check: yes, by default.
+  app._api = vi.fn(async () => ({ ok: true, status: 200 }));
+  app.saveSessionOrder = vi.fn();
   // The real one touches ~20 maps; the parts this behavior depends on are the
   // session map and the tab order, so those are pruned for real.
   app._cleanupSessionData = vi.fn((id: string) => {
@@ -101,7 +110,7 @@ describe('closing the active session', () => {
     // delete lands before the request resolves.
     app._apiDelete = vi.fn(async () => {
       app._onSessionDeleted({ id: A });
-      return { success: true };
+      return { ok: true, status: 200 };
     });
 
     await app.closeSession(A);
@@ -174,3 +183,84 @@ describe('closing the active session', () => {
     expect(app.showToast).toHaveBeenCalledWith('Failed to close session', 'error');
   });
 });
+
+describe('closing is optimistic', () => {
+  it('drops the tab and selects the next one BEFORE the server answers', async () => {
+    const app = makeApp(A);
+    let answer: (value: unknown) => void = () => {};
+    app._apiDelete = vi.fn(() => new Promise((r) => (answer = r)));
+
+    const closing = app.closeSession(A);
+
+    expect(app.sessions.has(A)).toBe(false);
+    expect(app.selectSession).toHaveBeenCalledWith(B, { auto: true });
+    // Rendered now, not on the debounce that every session update restarts.
+    expect(app.renderSessionTabs).toHaveBeenCalledWith({ immediate: true });
+    expect(app.showToast).not.toHaveBeenCalled();
+
+    answer({ ok: true, status: 200 });
+    await closing;
+    expect(app.showToast).toHaveBeenCalledWith('Session closed and tmux killed', 'success');
+  });
+
+  it('a refused delete puts the row back where it was, and says so', async () => {
+    const app = makeApp(B, [A, B]);
+    app._apiDelete = vi.fn(async () => ({ ok: false, status: 500 }));
+
+    await app.closeSession(A);
+
+    expect(app._api).toHaveBeenCalledWith(`/api/sessions/${A}`);
+    expect(app.sessions.has(A)).toBe(true);
+    expect(app.sessionOrder).toEqual([A, B]);
+    expect(app.showToast).toHaveBeenCalledWith('Failed to close session', 'error');
+  });
+
+  it('a delete that landed but lost its reply counts as closed', async () => {
+    const app = makeApp(A);
+    app._apiDelete = vi.fn(async () => null);
+    app._api = vi.fn(async () => ({ ok: false, status: 404 }));
+
+    await app.closeSession(A);
+
+    expect(app.sessions.has(A)).toBe(false);
+    expect(app.showToast).toHaveBeenCalledWith('Session closed and tmux killed', 'success');
+  });
+
+  it('a 404 from the delete is a close that already happened', async () => {
+    const app = makeApp(A);
+    app._apiDelete = vi.fn(async () => ({ ok: false, status: 404 }));
+
+    await app.closeSession(A);
+
+    expect(app._api).not.toHaveBeenCalled();
+    expect(app.sessions.has(A)).toBe(false);
+    expect(app.showToast).toHaveBeenCalledWith('Session closed and tmux killed', 'success');
+  });
+
+  it('a session update arriving mid-close does not bring the tab back', async () => {
+    const app = makeApp(A);
+    app.updateCost = vi.fn();
+    app.updateSubagentParentNames = vi.fn();
+    app._apiDelete = vi.fn(async () => {
+      (app as unknown as { _onSessionUpdated: (d: unknown) => void })._onSessionUpdated({ id: A, status: 'idle' });
+      return { ok: true, status: 200 };
+    });
+
+    await app.closeSession(A);
+
+    expect(app.sessions.has(A)).toBe(false);
+  });
+
+  it('a second close of the same tab while the first is in flight is a no-op', async () => {
+    const app = makeApp(A);
+    let answer: (value: unknown) => void = () => {};
+    app._apiDelete = vi.fn(() => new Promise((r) => (answer = r)));
+
+    const first = app.closeSession(A);
+    await app.closeSession(A);
+    answer({ ok: true, status: 200 });
+    await first;
+
+    expect(app._apiDelete).toHaveBeenCalledTimes(1);
+  });
+});

test/subagent-watcher.test.ts

@@ -68,6 +68,7 @@ import * as fs from 'fs';
 import * as fsPromises from 'node:fs/promises';
 import { createInterface } from 'readline';
 import { execSync } from 'child_process';
+import { execFile as nodeExecFile } from 'node:child_process';
 
 /**
  * Flush the microtask queue to allow async scanForSubagents() to complete.
@@ -1628,6 +1629,66 @@ describe('SubagentWatcher', () => {
       expect(result).toBe(true);
       expect(completedHandler).toHaveBeenCalled();
     });
+
+    it("killSubagentsForSession scans the process table ONCE for all of a session's agents", async () => {
+      // Closing a session ran a full `pgrep -f claude` + /proc read per recently
+      // active subagent (~85ms each on a busy box); one scan now serves them all.
+      // One readline per transcript: discovery reads each agent's file in turn.
+      const rls: ReturnType<typeof createMockRl>[] = [];
+      mockCreateInterface.mockImplementation(() => {
+        const rl = createMockRl();
+        rls.push(rl);
+        return rl;
+      });
+      mockCreateReadStream.mockReturnValue({ destroy: vi.fn() });
+      mockExistsSync.mockReturnValue(true);
+      mockReaddirSync.mockImplementation((path: string) => {
+        if (path.includes('subagents')) return ['agent-k1.jsonl', 'agent-k2.jsonl', 'agent-k3.jsonl'];
+        if (path.includes('session1')) return ['subagents'];
+        if (path.includes('-home-user-project')) return ['session1'];
+        return ['-home-user-project'];
+      });
+      mockStatSync.mockReturnValue({ isDirectory: () => true, birthtime: new Date(), mtime: new Date(), size: 100 });
+      mockReadFileSync.mockReturnValue(createUserEntry('Test subagent task'));
+
+      watcher.start();
+      for (let i = 0; i < 3; i++) {
+        await flushAsyncScan();
+        rls.forEach((rl) => rl.emit('close'));
+        await vi.advanceTimersByTimeAsync(100);
+      }
+      const agents = watcher.getSubagentsForSession('/home/user/project');
+      expect(agents.map((a) => a.agentId).sort()).toEqual(['k1', 'k2', 'k3']);
+
+      const execFileMock = vi.mocked(nodeExecFile) as unknown as Mock;
+      const originalExecFile = execFileMock.getMockImplementation();
+      let pgrepCalls = 0;
+      execFileMock.mockImplementation(
+        (cmd: string, _args: string[], _opts: unknown, cb: (err: Error | null, stdout: string) => void) => {
+          if (cmd === 'pgrep') pgrepCalls++;
+          cb(null, '4242\n4343\n');
+        }
+      );
+      mockReadFile.mockImplementation(async (path: string) => {
+        if (path === '/proc/4242/environ') return 'HOME=/x\0PARENT=session1\0';
+        if (path.startsWith('/proc/')) return 'HOME=/x\0';
+        return mockReadFileSync(path);
+      });
+      const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true);
+      let killCalls: unknown[][] = [];
+      try {
+        await watcher.killSubagentsForSession('/home/user/project', 'session1');
+        killCalls = [...killSpy.mock.calls];
+      } finally {
+        killSpy.mockRestore();
+        execFileMock.mockImplementation(originalExecFile as never);
+      }
+
+      expect(pgrepCalls).toBe(1);
+      expect(agents.every((a) => a.status === 'completed')).toBe(true);
+      // Only the matching process, and only once.
+      expect(killCalls).toEqual([[4242, 'SIGTERM']]);
+    });
   });
 
   describe('Error Handling', () => {

test/tile-grid-close-fallback.test.ts

@@ -7,9 +7,9 @@
  * pick that, with the grid open, would be refused (auto never collapses the
  * grid) and leave nothing focused. So the fallback is grid-aware, and it lives
  * IN closeSession: the delete broadcast routinely lands while the request is in
- * flight, and the delete handlers skip ids in `_closingSessions`. The neighbour
- * is captured BEFORE the await, like `wasActive`, because that broadcast may
- * already have removed the tile.
+ * flight, and the delete handlers skip ids in `_closingSessions`. The close is
+ * optimistic, so the tile goes and the neighbour takes focus before the request
+ * is even sent; the broadcast then finds nothing left to do.
  *
  * - closing the focused tile: next tile in grid order, else the previous one;
  *   `s-other` is FIRST in sessionOrder and never tiled, so the old pick would
@@ -33,7 +33,8 @@ function setup(ids = IDS, focus = ids[0]) {
   app.selectSession = vi.fn();
   app.markIdleAlertSeen.mockClear();
   let finish: () => void = () => {};
-  app._apiDelete = vi.fn(() => new Promise<void>((r) => (finish = r)));
+  // Resolves like the real helper's Response once `finish()` is called.
+  app._apiDelete = vi.fn(() => new Promise((r) => (finish = () => r({ ok: true, status: 200 }))));
   // The real cleanup touches a lot of panels; what the fallback reads is the session list.
   app._cleanupSessionData = vi.fn((id: string) => {
     app.sessions.delete(id);
@@ -77,11 +78,12 @@ describe('closeSession on the focused tile', () => {
     const { app, finish } = setup(IDS, 's-b');
     const closing = app.closeSession('s-b');
     await settle();
+    // The close already moved focus to the neighbour before the request went
+    // out; the broadcast for it must not move it again.
+    expect(app.activeSessionId).toBe('s-c');
     app._onSessionDeleted({ id: 's-b' });
-    // Only the tile went; closeSession owns the follow-up (as the split's
-    // wrapper does for ids in _closingSessions), so focus has not moved yet.
     expect(app._tileGrid.ids).toEqual(['s-a', 's-c']);
-    expect(app.activeSessionId).toBe('s-b');
+    expect(app.activeSessionId).toBe('s-c');
     expect(app.showWelcome).not.toHaveBeenCalled();
     finish();
     await closing;