diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 0a010442e1f..4abdba712a0 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -4435,7 +4435,7 @@ "providers": ["local", "daemon", "ssh", "wsl", "remote-runtime"], "coveredPlatforms": ["macos"], "coveredProviders": [], - "coverageNotes": "Platform-neutral unit coverage proves the Resource Manager closed badge performs one readiness seed, coalesces unknown spawn signals, skips known-session reattach signals, and installs no interval. Broader terminal interaction coverage remains on the pending reliability stack.", + "coverageNotes": "Platform-neutral unit coverage proves the Resource Manager closed badge performs one readiness seed, coalesces unknown spawn signals, skips known-session reattach signals, installs no interval, and re-reads only once per explicit daemon-management kill/restart (which emits no pty:exit). Broader terminal interaction coverage remains on the pending reliability stack.", "motivatingLinks": [ "https://github.com/stablyai/orca/pull/7002", "https://github.com/stablyai/orca/pull/6858", @@ -4464,7 +4464,8 @@ "spawn signals during a slow inventory read never overlap provider-wide scans and cause at most one required follow-up", "unknown sessions that exit before reconciliation cancel their queued inventory read", "unmount during a slow inventory read cannot schedule follow-up work", - "exit and out-of-order refresh races cannot resurrect stale sessions" + "exit and out-of-order refresh races cannot resurrect stale sessions", + "an explicit daemon-management invalidation performs exactly one inventory read, and none before readiness or after unmount" ] }, { diff --git a/src/renderer/src/components/settings/ManageSessionsSection.tsx b/src/renderer/src/components/settings/ManageSessionsSection.tsx index de48b535e3d..d0f39bbeec6 100644 --- a/src/renderer/src/components/settings/ManageSessionsSection.tsx +++ b/src/renderer/src/components/settings/ManageSessionsSection.tsx @@ -9,6 +9,7 @@ import { activateTabAndFocusPane } from '@/lib/activate-tab-and-focus-pane' import { useDaemonActions, DaemonActionDialog } from '../shared/useDaemonActions' import { ManageSessionKillDialog } from './ManageSessionKillDialog' import { ManageSessionsTable } from './ManageSessionsTable' +import { notifyDaemonSessionInventoryInvalidated } from '../status-bar/daemon-session-inventory-invalidation' import { translate } from '@/i18n/i18n' type ConfirmKind = 'killOne' @@ -122,6 +123,7 @@ export function ManageSessionsSection(): React.JSX.Element { void refresh() }, onRestartSettled: () => { + notifyDaemonSessionInventoryInvalidated() void refresh() } }) @@ -150,6 +152,7 @@ export function ManageSessionsSection(): React.JSX.Element { ) } mutationInFlight.current = false + notifyDaemonSessionInventoryInvalidated() await refresh() } catch (err) { toast.error( diff --git a/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts b/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts index 2ec53edce39..35b77f6fcb2 100644 --- a/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts +++ b/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts @@ -339,6 +339,26 @@ describe('runKillAllTerminalSurfaces', () => { expect(killPty).not.toHaveBeenCalled() }) + it('invalidates cached session inventories once the daemon sweep settles either way', async () => { + const notifyInventoryInvalidated = vi.fn() + + await runKillAllTerminalSurfaces([], { + getState: () => state(), + killDaemonSessions: vi.fn().mockResolvedValue({ killedCount: 3, remainingCount: 0 }), + notifyInventoryInvalidated, + reportSummary: vi.fn() + }) + expect(notifyInventoryInvalidated).toHaveBeenCalledTimes(1) + + await runKillAllTerminalSurfaces([], { + getState: () => state(), + killDaemonSessions: vi.fn().mockRejectedValue(new Error('daemon gone')), + notifyInventoryInvalidated, + reportSummary: vi.fn() + }) + expect(notifyInventoryInvalidated).toHaveBeenCalledTimes(2) + }) + it('uses one daemon management call and never inventories sessions afterward', async () => { const killAll = vi.fn().mockResolvedValue({ killedCount: 0, remainingCount: 0 }) const listSessions = vi.fn() diff --git a/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts b/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts index 8d280ba3f73..311b69c3592 100644 --- a/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts +++ b/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts @@ -9,6 +9,7 @@ import { type TerminalTabRetirementState } from '@/store/slices/terminal-tab-retirement' import { reserveTerminalRetirementTeardowns } from '@/store/slices/terminal-retirement-teardown-reservation' +import { notifyDaemonSessionInventoryInvalidated } from '../status-bar/daemon-session-inventory-invalidation' const CLOSE_BATCH_SIZE = 2 @@ -42,6 +43,7 @@ export type KillAllTerminalSurfacesSummary = { type KillAllTerminalSurfaceDependencies = { getState: () => KillAllTerminalSurfaceState killDaemonSessions: () => Promise + notifyInventoryInvalidated: () => void closeSurface: ( tabId: string, options: { @@ -127,6 +129,7 @@ function createDefaultDependencies(): KillAllTerminalSurfaceDependencies { return { getState: useAppStore.getState, killDaemonSessions: () => window.api.pty.management.killAll(), + notifyInventoryInvalidated: notifyDaemonSessionInventoryInvalidated, closeSurface: closeTerminalTab, killPty: (ptyId) => window.api.pty.kill(ptyId), now: () => globalThis.performance?.now() ?? Date.now(), @@ -157,6 +160,13 @@ export async function runKillAllTerminalSurfaces( } catch { daemon = { status: 'rejected' } } + try { + // Why: the management sweep ends sessions without per-PTY pty:exit events, + // so cached inventories (status-bar badge) must be told to re-read. + deps.notifyInventoryInvalidated() + } catch { + // A stale badge must not abort the destructive action already in progress. + } const cleanupState = deps.getState() const remainingTargetIds = new Set(targetIds) diff --git a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx index a8ece07593d..fe0f5e1a297 100644 --- a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx +++ b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx @@ -824,9 +824,6 @@ export function ResourceUsageStatusSegment({ clearSessionsError() void fetchSnapshot() void refreshSessions() - }, - onKillAllSettled: () => { - void refreshSessions() } }) diff --git a/src/renderer/src/components/status-bar/daemon-session-inventory-invalidation.ts b/src/renderer/src/components/status-bar/daemon-session-inventory-invalidation.ts new file mode 100644 index 00000000000..b63ff78ed2f --- /dev/null +++ b/src/renderer/src/components/status-bar/daemon-session-inventory-invalidation.ts @@ -0,0 +1,18 @@ +// Why: daemon-session kills routed through pty:management:* emit no pty:exit, +// so the status-bar inventory needs an explicit signal to stop showing a stale count. +const listeners = new Set<() => void>() + +export function subscribeDaemonSessionInventoryInvalidated(listener: () => void): () => void { + listeners.add(listener) + return () => { + listeners.delete(listener) + } +} + +export function notifyDaemonSessionInventoryInvalidated(): void { + // Copy so a listener unsubscribing during dispatch cannot skip its neighbours. + const snapshot = [...listeners] + for (const listener of snapshot) { + listener() + } +} diff --git a/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx b/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx index 936f83c3178..5112e613672 100644 --- a/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx +++ b/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx @@ -1,7 +1,8 @@ // @vitest-environment happy-dom -import { act, renderHook, waitFor } from '@testing-library/react' +import { act, cleanup, renderHook, waitFor } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { DaemonSession } from './resource-usage-merge-types' +import { notifyDaemonSessionInventoryInvalidated } from './daemon-session-inventory-invalidation' import { useResourceSessionInventory } from './use-resource-session-inventory' function session(id: string): DaemonSession { @@ -45,6 +46,8 @@ describe('useResourceSessionInventory', () => { }) afterEach(() => { + // Unmount leftover hooks so their module-level invalidation subscriptions cannot bleed into the next test. + cleanup() delete (window as unknown as { api?: unknown }).api }) @@ -254,6 +257,39 @@ describe('useResourceSessionInventory', () => { expect(result.current.sessionInventory.count).toBe(2) }) + it('re-reads the inventory once when a management kill invalidates it without a pty exit', async () => { + listSessions + .mockResolvedValueOnce([session('one'), session('two')]) + .mockResolvedValue([session('one')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(2)) + + await act(async () => { + notifyDaemonSessionInventoryInvalidated() + }) + + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + expect(listSessions).toHaveBeenCalledTimes(2) + }) + + it('ignores inventory invalidation before session restore is ready and after unmount', async () => { + listSessions.mockResolvedValue([session('one')]) + const { unmount } = renderHook(({ ready }) => useResourceSessionInventory(ready), { + initialProps: { ready: false } + }) + + await act(async () => { + notifyDaemonSessionInventoryInvalidated() + }) + expect(listSessions).not.toHaveBeenCalled() + + unmount() + await act(async () => { + notifyDaemonSessionInventoryInvalidated() + }) + expect(listSessions).not.toHaveBeenCalled() + }) + it('unsubscribes from lifecycle events on unmount', () => { listSessions.mockResolvedValue([]) const { unmount } = renderHook(() => useResourceSessionInventory(true)) diff --git a/src/renderer/src/components/status-bar/use-resource-session-inventory.ts b/src/renderer/src/components/status-bar/use-resource-session-inventory.ts index dc0b5febc1f..dd28e4b0398 100644 --- a/src/renderer/src/components/status-bar/use-resource-session-inventory.ts +++ b/src/renderer/src/components/status-bar/use-resource-session-inventory.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { useMountedRef } from '@/hooks/useMountedRef' +import { subscribeDaemonSessionInventoryInvalidated } from './daemon-session-inventory-invalidation' import { EMPTY_DAEMON_SESSION_INVENTORY, inventoryFromSessions, @@ -123,6 +124,17 @@ export function useResourceSessionInventory(ready: boolean): ResourceSessionInve void refreshSessions() }, [ready, refreshSessions]) + useEffect(() => { + if (!ready) { + return + } + // Why: management kills/restarts destroy daemon sessions without a pty:exit, + // so the closed badge would keep the pre-kill count until the popover opens. + return subscribeDaemonSessionInventoryInvalidated(() => { + void refreshSessions() + }) + }, [ready, refreshSessions]) + useEffect(() => { if (!ready) { return diff --git a/tests/e2e/status-bar-session-count-management-kill.spec.ts b/tests/e2e/status-bar-session-count-management-kill.spec.ts new file mode 100644 index 00000000000..45d1cd337d0 --- /dev/null +++ b/tests/e2e/status-bar-session-count-management-kill.spec.ts @@ -0,0 +1,193 @@ +/** + * E2E regression for #8372 — the status-bar CLI session count froze after a Manage Sessions kill. + * + * `pty:management:killOne` tears a session down with `adapter.shutdown()`. The daemon only fans + * `exit` to the clients attached to that session, so when the killed session belongs to *another* + * daemon client (a previous app generation, `orca serve`, a second Orca client) this window's main + * process never emits `pty:exit`. The status-bar chip is an event-sourced cache: with no lifecycle + * event and no interval it kept painting the pre-kill count until the Resource Manager popover was + * opened — and opening the popover refreshes, which is exactly why this spec never opens it. + * + * The spec creates that foreign session the way the daemon protocol really does it: a second + * DaemonClient connected to the app's own daemon socket. The app then sees three live sessions + * (its two panes plus the foreign one), the third of which Manage Sessions lists as unbound. + * + * Visible evidence: the `>_ N` number in the status bar. It must go 3 -> 2 after the row kill, + * with the popover closed the whole time. + */ + +import path from 'node:path' +import { randomUUID } from 'node:crypto' +import type { Page } from '@stablyai/playwright-test' +import { expect, test } from './helpers/orca-app' +import { DaemonClient } from '../../src/main/daemon/client' +import { PROTOCOL_VERSION } from '../../src/main/daemon/types' +import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { + splitActiveTerminalPane, + waitForActiveTerminalManager, + waitForPaneCount, + waitForPaneIdentitySnapshot +} from './helpers/terminal' + +/** The closed status-bar chip. Its accessible name carries the same count it paints. */ +function resourceChip(page: Page) { + return page.getByRole('button', { name: /^Resource Manager, \d+ terminal session/ }) +} + +/** The count as the chip's accessible name reports it. */ +async function readChipAriaCount(page: Page): Promise { + const label = await resourceChip(page).getAttribute('aria-label') + const match = /(\d+) terminal session/.exec(label ?? '') + return match ? Number(match[1]) : null +} + +/** The count as a human reads it off the chip: the digits next to the terminal glyph. */ +async function readChipVisibleCount(page: Page): Promise { + const text = await resourceChip(page).locator('span.tabular-nums').last().textContent() + return text?.trim() ?? null +} + +async function listDaemonSessionIds(page: Page): Promise { + return page.evaluate(async () => (await window.api.pty.listSessions()).map(({ id }) => id)) +} + +/** The daemon's session set has to hold the expected size across consecutive reads. */ +async function waitForStableSessionIds(page: Page, expected: number): Promise { + await expect + .poll( + async () => { + const first = await listDaemonSessionIds(page) + await page.waitForTimeout(750) + const second = await listDaemonSessionIds(page) + return ( + first.length === expected && + second.length === expected && + first.every((id) => second.includes(id)) + ) + }, + { timeout: 30_000, message: `The daemon session set never settled at ${expected}` } + ) + .toBe(true) +} + +test.describe('Status bar CLI session count', () => { + test('drops after Manage Sessions kills a foreign daemon session, popover never opened', async ({ + orcaPage: page, + electronApp + }) => { + test.skip( + process.platform === 'win32', + 'Named-pipe daemon endpoints need a different socket path derivation.' + ) + + await waitForSessionReady(page) + await waitForActiveWorktree(page) + await ensureTerminalVisible(page) + + const hasPaneManager = await waitForActiveTerminalManager(page, 30_000) + .then(() => true) + .catch(() => false) + test.skip( + !hasPaneManager, + 'Electron automation in this environment never mounts the TerminalPane manager.' + ) + await waitForPaneCount(page, 1, 30_000) + + // A second daemon client, connected to the app's own daemon exactly as another Orca client + // would be. Its session is live and listed, but this app never attaches to it. + const userDataDir = await electronApp.evaluate(({ app }) => app.getPath('userData')) + const runtimeDir = path.join(userDataDir, 'daemon') + const foreignClient = new DaemonClient({ + socketPath: path.join(runtimeDir, `daemon-v${PROTOCOL_VERSION}.sock`), + tokenPath: path.join(runtimeDir, `daemon-v${PROTOCOL_VERSION}.token`) + }) + const foreignSessionId = `${randomUUID()}::${userDataDir}` + + try { + await foreignClient.ensureConnected() + const created = await foreignClient.request<{ isNew: boolean; pid: number }>( + 'createOrAttach', + { sessionId: foreignSessionId, cols: 80, rows: 24, cwd: userDataDir, env: {} } + ) + expect(created.isNew, 'the foreign daemon session was not created').toBe(true) + + // Split after the foreign session exists: the new pane's spawn event is the app's one + // natural inventory re-read, so the chip starts out agreeing with the daemon. + await splitActiveTerminalPane(page, 'vertical') + await waitForPaneCount(page, 2, 30_000) + + // Both panes must be fully bound and settled before Settings unmounts the terminal view: + // parking a still-spawning pane tears its PTY down, which would move the count for a + // reason that has nothing to do with the kill under test. + await waitForPaneIdentitySnapshot(page, 2) + await waitForStableSessionIds(page, 3) + + const baselineIds = await listDaemonSessionIds(page) + expect(baselineIds, 'the foreign session is not live on the daemon').toContain( + foreignSessionId + ) + const baseline = baselineIds.length + expect(baseline, 'expected the two panes plus the foreign session').toBe(3) + + // Baseline: the chip must already agree with the daemon before the kill, otherwise a + // post-kill mismatch would prove nothing about invalidation. + await expect(resourceChip(page)).toBeVisible() + await expect + .poll(async () => readChipAriaCount(page), { + timeout: 30_000, + message: 'The status-bar chip never caught up with the live session count' + }) + .toBe(baseline) + expect(await readChipVisibleCount(page)).toMatch(new RegExp(`^${baseline}\\b`)) + + // Real UI kill path: Settings > Terminal > Manage Sessions, row kill + confirm. + await page.evaluate(() => { + const state = window.__store?.getState() + if (!state) { + throw new Error('store unavailable') + } + state.openSettingsTarget({ pane: 'terminal', repoId: null }) + state.openSettingsPage() + }) + + const killRowButton = page.getByRole('button', { name: `Kill session ${foreignSessionId}` }) + await expect(killRowButton).toBeVisible({ timeout: 30_000 }) + await killRowButton.click() + + const confirmButton = page.getByRole('button', { name: 'Kill session', exact: true }) + await expect(confirmButton).toBeVisible() + await confirmButton.click() + + // The kill itself must succeed identically on both branches; only the chip differs. + await expect + .poll(async () => (await listDaemonSessionIds(page)).includes(foreignSessionId), { + timeout: 30_000, + message: 'The daemon never dropped the killed session' + }) + .toBe(false) + + // Guard the whole point of the bug: the popover refreshes on open, so it must stay closed. + await expect(page.locator('[data-radix-popper-content-wrapper]')).toHaveCount(0) + + // The regression: with no pty:exit and no invalidation the closed chip keeps the stale count. + await expect + .poll(async () => readChipAriaCount(page), { + timeout: 15_000, + message: `The status-bar chip stayed at the pre-kill count instead of dropping to ${baseline - 1}` + }) + .toBe(baseline - 1) + expect( + await readChipVisibleCount(page), + 'The number painted on the chip did not follow its accessible name' + ).toMatch(new RegExp(`^${baseline - 1}\\b`)) + + await expect(page.locator('[data-radix-popper-content-wrapper]')).toHaveCount(0) + } finally { + await foreignClient + .request('kill', { sessionId: foreignSessionId, immediate: true }) + .catch(() => {}) + foreignClient.disconnect() + } + }) +})