diff --git a/package.json b/package.json index 2bf90c3ea..7a2c012c0 100644 --- a/package.json +++ b/package.json @@ -15,10 +15,11 @@ "testing:rendering": "npm run testing:rendering:build && electron testing/rendering/out/main/main.js", "testing:rendering:build": "electron-vite build --config testing/rendering/electron.vite.config.ts", "testing:rendering:preview": "electron testing/rendering/out/main/main.js", - "test:review-fixes": "npm run test:settings-coerce && npm run test:prompt-templates", + "test:review-fixes": "npm run test:settings-coerce && npm run test:prompt-templates && npm run test:session-ownership", "test:settings-coerce": "tsx --tsconfig tsconfig.web.json scripts/test-settings-coerce.ts", "test:prompt-templates": "tsx --tsconfig tsconfig.web.json scripts/test-prompt-templates.ts", "test:work-context": "tsx --tsconfig tsconfig.web.json scripts/test-work-context.ts", + "test:session-ownership": "tsx --tsconfig tsconfig.web.json scripts/test-session-ownership.ts", "test:worktree-activity": "tsx --tsconfig tsconfig.node.json scripts/test-worktree-activity-index.ts", "test:ghost-fallback": "tsx --tsconfig tsconfig.web.json scripts/test-ghost-fallback.ts", "test:dictation": "tsx --tsconfig tsconfig.node.json scripts/test-dictation-imports.ts", diff --git a/scripts/test-session-ownership.ts b/scripts/test-session-ownership.ts new file mode 100644 index 000000000..c090c6f72 --- /dev/null +++ b/scripts/test-session-ownership.ts @@ -0,0 +1,144 @@ +import assert from 'node:assert/strict' + +import { + collectOwnedSessionIds, + collectUnownedSessionIds, + pickOwnedSessions, + pruneSessionOwnership, +} from '../src/renderer/src/workspace/sessionOwnership' +import type { + BuriedPaneRecord, + DetachedSessionRecord, + SessionMeta, + Tab, +} from '../src/renderer/src/workspace/types' + +const meta = (cwd: string, kind: SessionMeta['kind'] = 'codex'): SessionMeta => ({ + cwd, + kind, + providerSessionId: `${cwd}-provider`, +}) + +const tab: Tab = { + id: 'tab-a', + title: 'Project A', + focusedSessionId: 'visible-a', + root: { + type: 'split', + direction: 'vertical', + ratio: 0.5, + a: { type: 'leaf', sessionId: 'visible-a' }, + b: { type: 'leaf', sessionId: 'visible-b' }, + }, +} + +const detached: DetachedSessionRecord = { + sessionId: 'detached-a', + surface: 'dispatch', + projectTabId: 'tab-a', + projectTabTitle: 'Project A', + projectTabIndex: 0, + detachedAt: 1, +} + +const buried: BuriedPaneRecord = { + id: 'buried-a', + sessionId: 'buried-a', + sessionMeta: meta('/buried'), + buriedAt: 2, + sourceTabId: 'tab-a', + sourceTabTitle: 'Project A', + sourceTabIndex: 0, +} + +const sessions: Record = { + 'visible-a': meta('/visible-a'), + 'visible-b': meta('/visible-b'), + 'detached-a': meta('/detached-a'), + 'buried-a': meta('/buried-a'), + 'stale-a': meta('/stale-a'), +} + +{ + const owned = collectOwnedSessionIds({ + tabs: [tab], + sessions, + detachedSessions: { 'old-key': detached }, + buried: [buried], + }) + + assert.deepEqual( + [...owned].sort(), + ['buried-a', 'detached-a', 'visible-a', 'visible-b'], + ) +} + +{ + assert.deepEqual( + collectUnownedSessionIds({ + tabs: [tab], + sessions, + detachedSessions: { 'detached-a': detached }, + buried: [buried], + }), + ['stale-a'], + ) +} + +{ + const owned = new Set(['visible-a', 'buried-a']) + assert.deepEqual(Object.keys(pickOwnedSessions(sessions, owned)).sort(), [ + 'buried-a', + 'visible-a', + ]) +} + +{ + const pruned = pruneSessionOwnership({ + tabs: [tab], + sessions: { + ...sessions, + 'missing-detached-meta': meta('/missing-detached-meta'), + }, + detachedSessions: { + 'stale-key': detached, + 'missing-owner': { + ...detached, + sessionId: 'owner-without-meta', + }, + }, + buried: [ + buried, + { + ...buried, + id: 'buried-without-meta', + sessionId: 'buried-without-meta', + }, + ], + dispatchMode: { + scope: 'project', + focusedSessionId: 'stale-a', + }, + }) + + // WHY this assertion matters: + // + // The regression was not "visible panes disappear"; it was the opposite: + // invisible metadata survived and became real processes. This fixture keeps + // all legitimate hidden owners (detached + buried), normalizes detached keys + // to their session id, and proves that focus alone does not keep stale-a + // alive. + assert.deepEqual(Object.keys(pruned.sessions).sort(), [ + 'buried-a', + 'detached-a', + 'visible-a', + 'visible-b', + ]) + assert.deepEqual(Object.keys(pruned.detachedSessions), ['detached-a']) + assert.deepEqual(pruned.buried.map(entry => entry.sessionId), ['buried-a']) + assert.equal(pruned.dispatchMode?.focusedSessionId, undefined) + assert.deepEqual(pruned.droppedSessionIds.sort(), [ + 'missing-detached-meta', + 'stale-a', + ]) +} diff --git a/src/renderer/src/workspace/hook/actions/session.ts b/src/renderer/src/workspace/hook/actions/session.ts index cd0e0f02f..6bc342c59 100644 --- a/src/renderer/src/workspace/hook/actions/session.ts +++ b/src/renderer/src/workspace/hook/actions/session.ts @@ -18,6 +18,11 @@ import type { import type { WorkspaceRefs } from '@renderer/workspace/hook/refs' import { loadInitialHistoryForSession } from '@renderer/workspace/hook/actions/initialHistory' import { commandTargetSessionIdForState } from '@renderer/workspace/hook/selectors/commandTargetSessionId' +import { + collectOwnedSessionIds, + collectUnownedSessionIds, + pickOwnedSessions, +} from '@renderer/workspace/sessionOwnership' // ----------------------------------------------------------------------------- // Session lifecycle actions. @@ -426,7 +431,23 @@ export function useSessionActions( const reloadAgentSessions = useCallback( async (dangerousMode = refs.dangerousAgentsRef.current) => { const current = refs.stateRef.current - const agentEntries = Object.entries(current.sessions).filter(([, meta]) => { + const ownedIds = collectOwnedSessionIds(current) + const staleIds = collectUnownedSessionIds(current) + if (staleIds.length > 0) { + // WHY reload prunes but does not kill unowned ids directly: + // + // `reloadAgentSessions` is a user-visible policy change ("restart the + // owned agents with the current dangerous-mode setting"), not a + // workspace garbage collector. If an unowned runtime process exists, + // it has already escaped the UI ownership model; respawning it here is + // the bug. Dropping the metadata prevents the stale row from surviving + // the reload/autosave cycle, while process cleanup can stay best-effort + // at the explicit kill/session-manager layer. + // eslint-disable-next-line no-console + console.warn('[workspace] dropping unowned sessions during agent reload:', staleIds) + } + const agentEntries = Object.entries(current.sessions).filter(([id, meta]) => { + if (!ownedIds.has(id)) return false const kind = meta.kind ?? 'claude' return kind === 'claude' || kind === 'codex' }) @@ -509,7 +530,10 @@ export function useSessionActions( }) setState(prev => { - const nextSessions = { ...prev.sessions } + const nextSessions = pickOwnedSessions( + prev.sessions, + collectOwnedSessionIds(prev), + ) for (const [oldId] of agentEntries) delete nextSessions[oldId] for (const [newId, meta] of Object.entries(freshSessions)) { nextSessions[newId] = meta diff --git a/src/renderer/src/workspace/hook/persistence/rehydrate.ts b/src/renderer/src/workspace/hook/persistence/rehydrate.ts index b99418fad..ed652c36a 100644 --- a/src/renderer/src/workspace/hook/persistence/rehydrate.ts +++ b/src/renderer/src/workspace/hook/persistence/rehydrate.ts @@ -11,6 +11,10 @@ import type { import { collectLeaves } from '@renderer/workspace/tile-tree/treeOps' import { sanitizeTileTabsState } from '@renderer/workspace/layout/helpers' import type { PersistedWorkspace } from '@renderer/workspace/persistence' +import { + collectOwnedSessionIds, + collectUnownedSessionIds, +} from '@renderer/workspace/sessionOwnership' import type { WorkspaceSetRuntimes, @@ -64,6 +68,22 @@ export async function rehydrateWorkspace( }) const idMap = new Map() const freshSessions: Record = {} + const ownedIds = collectOwnedSessionIds(persisted) + const staleIds = collectUnownedSessionIds(persisted) + + if (staleIds.length > 0) { + // WHY log and drop instead of trying to repair by providerSessionId: + // + // Local SessionIds are the workspace ownership keys. providerSessionId is + // provider history identity and can legitimately be duplicated by clone, + // rewind, or failed restore paths. Using it as a repair key risks attaching + // a hidden stale row to the wrong visible pane. The only safe restore set is + // the ids already owned by tab leaves, detached sessions, or buried panes. + // Dropping stale metadata here prevents invisible persisted rows from + // becoming real backend processes/proxies during startup. + // eslint-disable-next-line no-console + console.warn('[workspace] dropping unowned persisted sessions during rehydrate:', staleIds) + } const remapNode = (n: TileNode): TileNode => { if (n.type === 'leaf') { @@ -290,53 +310,55 @@ export async function rehydrateWorkspace( // slow respawn must not block the entire tab strip from coming // back. await Promise.all( - Object.entries(persisted.sessions).map(async ([oldId, meta]) => { - const restoreSpan = perf.span('workspace.rehydrate.session', { - oldId, - kind: meta.kind ?? 'claude', - hasProviderSessionId: Boolean(meta.providerSessionId), - hasTmuxName: Boolean(meta.tmuxName), - }) - try { - const kind: SessionKind = meta.kind ?? 'claude' - // For terminal sessions with a persisted tmuxName, pass it - // as recoverTmuxName so main re-attaches the alive tmux - // session (or falls back to fresh spawn if it died). Agents - // ignore recoverTmuxName at the main side; safe to omit. - const { sessionId: newId, tmuxName: nextTmuxName } = await window.api.spawnSession({ - kind, - cwd: meta.cwd, - resumeSessionId: kind !== 'terminal' ? meta.providerSessionId : undefined, - dangerousMode: kind !== 'terminal' ? refs.dangerousAgentsRef.current : undefined, - useProxy: kind !== 'terminal' ? refs.useProxyStreamingRef.current : undefined, - recoverTmuxName: kind === 'terminal' ? meta.tmuxName : undefined, + Object.entries(persisted.sessions) + .filter(([oldId]) => ownedIds.has(oldId)) + .map(async ([oldId, meta]) => { + const restoreSpan = perf.span('workspace.rehydrate.session', { + oldId, + kind: meta.kind ?? 'claude', + hasProviderSessionId: Boolean(meta.providerSessionId), + hasTmuxName: Boolean(meta.tmuxName), }) - idMap.set(oldId, newId) - // Carry the full meta forward — kind + providerSessionId + - // tmuxName — so the next save cycle doesn't drop these and - // cause the session to degrade on the NEXT reload. - // tmuxName is replaced with whatever main reported - // (recovered name when alive, fresh name when respawned). - freshSessions[newId] = { - ...meta, - ...(nextTmuxName ? { tmuxName: nextTmuxName } : {}), - } - commitRehydratedState() - if (kind !== 'terminal' && freshSessions[newId].providerSessionId) { - void loadInitialHistoryForSession({ - sessionId: newId, - meta: freshSessions[newId], - refs, - setRuntimes, + try { + const kind: SessionKind = meta.kind ?? 'claude' + // For terminal sessions with a persisted tmuxName, pass it + // as recoverTmuxName so main re-attaches the alive tmux + // session (or falls back to fresh spawn if it died). Agents + // ignore recoverTmuxName at the main side; safe to omit. + const { sessionId: newId, tmuxName: nextTmuxName } = await window.api.spawnSession({ + kind, + cwd: meta.cwd, + resumeSessionId: kind !== 'terminal' ? meta.providerSessionId : undefined, + dangerousMode: kind !== 'terminal' ? refs.dangerousAgentsRef.current : undefined, + useProxy: kind !== 'terminal' ? refs.useProxyStreamingRef.current : undefined, + recoverTmuxName: kind === 'terminal' ? meta.tmuxName : undefined, }) + idMap.set(oldId, newId) + // Carry the full meta forward — kind + providerSessionId + + // tmuxName — so the next save cycle doesn't drop these and + // cause the session to degrade on the NEXT reload. + // tmuxName is replaced with whatever main reported + // (recovered name when alive, fresh name when respawned). + freshSessions[newId] = { + ...meta, + ...(nextTmuxName ? { tmuxName: nextTmuxName } : {}), + } + commitRehydratedState() + if (kind !== 'terminal' && freshSessions[newId].providerSessionId) { + void loadInitialHistoryForSession({ + sessionId: newId, + meta: freshSessions[newId], + refs, + setRuntimes, + }) + } + restoreSpan.end({ newId }) + } catch (err) { + restoreSpan.fail(err) + // eslint-disable-next-line no-console + console.warn(`[workspace] failed to respawn ${meta.cwd}:`, err) } - restoreSpan.end({ newId }) - } catch (err) { - restoreSpan.fail(err) - // eslint-disable-next-line no-console - console.warn(`[workspace] failed to respawn ${meta.cwd}:`, err) - } - }), + }), ) if (!commitRehydratedState()) { diff --git a/src/renderer/src/workspace/hook/persistence/useAutoSave.ts b/src/renderer/src/workspace/hook/persistence/useAutoSave.ts index d73207f5b..23c295469 100644 --- a/src/renderer/src/workspace/hook/persistence/useAutoSave.ts +++ b/src/renderer/src/workspace/hook/persistence/useAutoSave.ts @@ -2,6 +2,7 @@ import { useCallback, useEffect } from 'react' import type { PersistedWorkspace } from '@renderer/workspace/persistence' import type { SessionId, WorkspaceState } from '@renderer/workspace/types' +import { pruneSessionOwnership } from '@renderer/workspace/sessionOwnership' import type { WorkspaceRefs } from '@renderer/workspace/hook/refs' import * as perf from '@renderer/performance/client' @@ -38,10 +39,23 @@ export function useAutoSave( const flushSave = useCallback(() => { const saveSpan = perf.span('workspace.autosave.flush') const s = refs.latestStateRef.current + const pruned = pruneSessionOwnership(s) + if (pruned.droppedSessionIds.length > 0) { + // WHY autosave prunes instead of faithfully serializing runtime state: + // + // Autosave is the durability boundary. If an action accidentally leaves + // an unowned row in `state.sessions`, writing it to workspace.json turns a + // transient invariant violation into a startup respawn on every future + // launch. Pruning here is the last line of defense: owned hidden sessions + // (detached/buried) still persist, but metadata with no owner cannot make + // itself durable. + // eslint-disable-next-line no-console + console.warn('[workspace] dropping unowned sessions during autosave:', pruned.droppedSessionIds) + } // Collect non-empty drafts so in-progress prompts survive crashes. const drafts: Record = {} for (const [id, rt] of Object.entries(refs.latestRuntimesRef.current)) { - if (rt.draftInput) drafts[id] = rt.draftInput + if (pruned.sessions[id] && rt.draftInput) drafts[id] = rt.draftInput } const persisted: PersistedWorkspace = { tabs: s.tabs.map(t => ({ @@ -51,10 +65,10 @@ export function useAutoSave( root: t.root, })), activeTabId: s.activeTabId, - dispatchMode: s.dispatchMode, - sessions: s.sessions, - detachedSessions: s.detachedSessions, - buried: s.buried, + dispatchMode: pruned.dispatchMode, + sessions: pruned.sessions, + detachedSessions: pruned.detachedSessions, + buried: pruned.buried, tileTabs: refs.latestTileTabsRef.current, drafts: Object.keys(drafts).length > 0 ? drafts : undefined, } diff --git a/src/renderer/src/workspace/sessionOwnership.ts b/src/renderer/src/workspace/sessionOwnership.ts new file mode 100644 index 000000000..274b380a2 --- /dev/null +++ b/src/renderer/src/workspace/sessionOwnership.ts @@ -0,0 +1,131 @@ +import type { + BuriedPaneRecord, + DetachedSessionRecord, + DispatchModeState, + SessionId, + SessionMeta, + TileNode, +} from '@renderer/workspace/types' +import { collectLeaves } from '@renderer/workspace/tile-tree/treeOps' + +type SessionOwnershipTab = { + root: TileNode +} + +export type SessionOwnershipInput = { + tabs: SessionOwnershipTab[] + sessions: Record + detachedSessions?: Record + buried?: BuriedPaneRecord[] +} + +export type PrunedSessionOwnership = { + sessions: Record + detachedSessions: Record + buried: BuriedPaneRecord[] + dispatchMode: DispatchModeState | null | undefined + droppedSessionIds: SessionId[] +} + +// WHY this helper exists: +// +// Workspace persistence has three independent ownership surfaces for a live +// session: visible tile leaves, detached non-grid surfaces, and buried panes. +// The `sessions` map is deliberately only metadata for those owners. Treating +// the map itself as authority creates a fourth hidden state: "metadata exists +// but no UI/surface owns it". Startup and agent reload turn that hidden state +// into real backend processes, which is how stale Codex rows can respawn into +// dozens of invisible proxies and push Electron toward OOM. +// +// Keeping ownership collection centralized makes every persistence/reload path +// answer the same question the same way: "what session ids are allowed to stay +// live?" Dispatch focus is intentionally excluded. It is a selection pointer, +// not ownership; allowing it to keep a session alive would let a stale focus id +// resurrect work the user can no longer see or manage. +export function collectOwnedSessionIds(input: SessionOwnershipInput): Set { + const owned = new Set() + + for (const tab of input.tabs) { + for (const id of collectLeaves(tab.root)) { + owned.add(id) + } + } + + for (const entry of Object.values(input.detachedSessions ?? {})) { + owned.add(entry.sessionId) + } + + for (const entry of input.buried ?? []) { + owned.add(entry.sessionId) + } + + return owned +} + +export function collectUnownedSessionIds(input: SessionOwnershipInput): SessionId[] { + const owned = collectOwnedSessionIds(input) + return Object.keys(input.sessions).filter(id => !owned.has(id)) +} + +export function pickOwnedSessions( + sessions: Record, + ownedIds: Set, +): Record { + const out: Record = {} + for (const [id, meta] of Object.entries(sessions)) { + if (ownedIds.has(id)) out[id] = meta + } + return out +} + +export function pruneSessionOwnership( + input: SessionOwnershipInput & { + dispatchMode?: DispatchModeState | null + }, +): PrunedSessionOwnership { + const ownedIds = collectOwnedSessionIds(input) + const sessions = pickOwnedSessions(input.sessions, ownedIds) + const liveIds = new Set(Object.keys(sessions)) + + // WHY filter owner records after filtering `sessions`: + // + // A corrupted workspace can fail both directions. The OOM bug came from + // metadata without an owner, but the inverse is also possible after a failed + // rehydrate or hand-edited workspace.json: an owner points at missing + // metadata. Persisting that shape means the next load has to reason about a + // pane whose cwd/kind no longer exists. Pruning owner records to ids that + // survived in `sessions` keeps the serialized model closed under restore. + // + // Detached records are also normalized by session id while we are here. The + // object key is a lookup convenience, not user data; keeping an old runtime + // key around a remapped record makes later lifecycle actions target the wrong + // entry. + const detachedSessions: Record = {} + for (const entry of Object.values(input.detachedSessions ?? {})) { + if (!liveIds.has(entry.sessionId)) continue + const sessionId = entry.sessionId + detachedSessions[sessionId] = { + ...entry, + sessionId, + } + } + + const buried = (input.buried ?? []).filter(entry => liveIds.has(entry.sessionId)) + const focusedSessionId = input.dispatchMode?.focusedSessionId + const dispatchMode = input.dispatchMode + ? { + ...input.dispatchMode, + focusedSessionId: focusedSessionId && liveIds.has(focusedSessionId) + ? focusedSessionId + : undefined, + } + : input.dispatchMode + + return { + sessions, + detachedSessions, + buried, + dispatchMode, + droppedSessionIds: Object.keys(input.sessions).filter(id => !liveIds.has(id)), + } +}