mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
fix(status-bar): invalidate the CLI session count on kill and restart (#12468)
* fix(status-bar): invalidate the CLI session count on kill and restart `pty:management:killOne` / `killAll` / `restart` tear sessions down via `adapter.shutdown()` and broadcast nothing — unlike `pty:kill`, which ends in `sendPtyExitToRenderer`. The status-bar count is an event-sourced cache, so killing sessions from Manage Sessions or "Kill all terminals" left the `>_ N` chip frozen until the popover was opened, which itself triggers a refresh. > [!NOTE] > The dual-source split described in the issue text was already fixed by merged #9387. This closes a *different* remaining invalidation gap that produces the same reported symptom. Broadcast the teardown so the chip updates without needing the popover opened. Fixes #8372 Co-authored-by: Orca <help@stably.ai> * test(e2e): add recordable proof for status-bar-cli-session-count Fails on origin/main, passes on this branch. Test: drops after Manage Sessions kills a foreign daemon session, popover never opened Co-authored-by: Orca <help@stably.ai> * fix(status-bar): avoid duplicate inventory refresh after kill all --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
Orca
@@ -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"
|
||||
]
|
||||
},
|
||||
{
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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<DaemonKillAllResult>
|
||||
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)
|
||||
|
||||
@@ -824,9 +824,6 @@ export function ResourceUsageStatusSegment({
|
||||
clearSessionsError()
|
||||
void fetchSnapshot()
|
||||
void refreshSessions()
|
||||
},
|
||||
onKillAllSettled: () => {
|
||||
void refreshSessions()
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
@@ -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))
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<number | null> {
|
||||
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<string | null> {
|
||||
const text = await resourceChip(page).locator('span.tabular-nums').last().textContent()
|
||||
return text?.trim() ?? null
|
||||
}
|
||||
|
||||
async function listDaemonSessionIds(page: Page): Promise<string[]> {
|
||||
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<void> {
|
||||
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()
|
||||
}
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user