diff --git a/scripts/summarize-lifecycle.mts b/scripts/summarize-lifecycle.mts index 74700610..e79f77cf 100644 --- a/scripts/summarize-lifecycle.mts +++ b/scripts/summarize-lifecycle.mts @@ -112,7 +112,10 @@ async function readLifecycleEvents(runsDir: string, runId: string): Promise !resolvedIds.has(id)) reportLifecycle('rehydrate.complete', undefined, { expectedCount: expectedSessions, resolvedCount: resolvedIds.size, ok: resolvedIds.size === expectedSessions, + // Only on a failure: a healthy boot would otherwise record an empty string + // on every launch, and "the field is absent" already means "none". + // + // Capped at 8 because the journal sanitizer truncates strings over 300 + // chars, which at ~37 bytes per comma-joined UUID would slice the last id + // in half — a half-id in a diagnostic is worse than a missing one, since + // it reads like a real id. `expectedCount`/`resolvedCount` still carry the + // true totals. + ...(unresolved.length > 0 + ? { unresolvedSessionIds: unresolved.slice(0, 8).join(',') } + : {}), durationMs: Date.now() - rehydrateStartedAt, }) return { diff --git a/src/renderer/src/workspace/hook/persistence/useAutoSave.ts b/src/renderer/src/workspace/hook/persistence/useAutoSave.ts index 67f8a877..6338d5a4 100644 --- a/src/renderer/src/workspace/hook/persistence/useAutoSave.ts +++ b/src/renderer/src/workspace/hook/persistence/useAutoSave.ts @@ -2,7 +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 { pruneSessionOwnership, repairPersistedTabs } from '@renderer/workspace/sessionOwnership' import { withNormalizedBuiltInMcpDomains } from '@renderer/workspace/mcpDomains' import type { WorkspaceRefs } from '@renderer/workspace/hook/refs' @@ -53,6 +53,39 @@ export function useAutoSave( // eslint-disable-next-line no-console console.warn('[workspace] dropping unowned sessions during autosave:', pruned.droppedSessionIds) } + // Repair the tile trees BEFORE serializing them. `pruneSessionOwnership` + // above scrubs every pointer that aims at a session, but the trees are + // themselves an ownership surface and used to be written verbatim — so a + // leaf whose metadata had already been removed from `state.sessions` could + // become durable. That shape is not merely untidy: rehydrate counts such a + // leaf as a pane it must restore, can never restore it, and therefore + // reports `partial-restore` and holds autosave off on every subsequent + // launch. Since autosave is the only writer of workspace.json, the file + // then cannot be repaired by the app at all. + // + // WHY `pruned.sessions` and not `s.sessions`: they agree on exactly the + // question being asked. `pruned.sessions` keeps `ownedIds ∩ own keys of + // s.sessions`, and every tile leaf with metadata is owned by construction, + // so a leaf is missing here if and only if it was already an orphan in + // this same snapshot. Nothing `pruneSessionOwnership` drops for an + // unrelated reason (unowned metadata, a detached record whose parent tab + // is gone, a buried pane) can ever be a tile leaf — which is what stops + // this from deleting a live pane. If that ever stops holding, this call + // becomes destructive, so keep the two in step. + const repairedTabs = repairPersistedTabs({ + tabs: s.tabs, + sessions: pruned.sessions, + activeTabId: s.activeTabId, + tileTabs: refs.latestTileTabsRef.current, + }) + if (repairedTabs.droppedLeafSessionIds.length > 0) { + // eslint-disable-next-line no-console + console.warn( + '[workspace] dropping tile leaves with no session metadata during autosave:', + { leaves: repairedTabs.droppedLeafSessionIds, tabs: repairedTabs.droppedTabIds }, + ) + } + // Collect non-empty drafts so in-progress prompts survive crashes. const drafts: Record = {} for (const [id, rt] of Object.entries(refs.latestRuntimesRef.current)) { @@ -69,13 +102,13 @@ export function useAutoSave( id => pruned.sessions[id] !== undefined, ) const persisted: PersistedWorkspace = { - tabs: s.tabs.map(t => ({ + tabs: repairedTabs.tabs.map(t => ({ id: t.id, title: t.title, focusedSessionId: t.focusedSessionId, root: t.root, })), - activeTabId: s.activeTabId, + activeTabId: repairedTabs.activeTabId, dispatchMode: pruned.dispatchMode, // WHY normalize MCP domains at the persistence boundary: // @@ -96,7 +129,7 @@ export function useAutoSave( pinnedSessionIds: persistedPinnedSessionIds.length > 0 ? persistedPinnedSessionIds : undefined, - tileTabs: refs.latestTileTabsRef.current, + tileTabs: repairedTabs.tileTabs, drafts: Object.keys(drafts).length > 0 ? drafts : undefined, } let json = '' diff --git a/src/renderer/src/workspace/sessionOwnership.test.ts b/src/renderer/src/workspace/sessionOwnership.test.ts index 698f3de0..34b319d1 100644 --- a/src/renderer/src/workspace/sessionOwnership.test.ts +++ b/src/renderer/src/workspace/sessionOwnership.test.ts @@ -1,7 +1,17 @@ import { describe, expect, it } from 'vitest' -import { pruneSessionOwnership } from '@renderer/workspace/sessionOwnership' -import type { TileNode, WorkspaceState } from '@renderer/workspace/types' +import { + collectLiveProcessIds, + collectOwnedSessionIds, + pruneSessionOwnership, + repairPersistedTabs, +} from '@renderer/workspace/sessionOwnership' +import type { + SessionId, + SessionMeta, + TileNode, + WorkspaceState, +} from '@renderer/workspace/types' function leaf(sessionId: string): TileNode { return { type: 'leaf', sessionId } @@ -158,3 +168,215 @@ describe('pruneSessionOwnership', () => { expect(result.droppedSessionIds).toHaveLength(83) }) }) + + +// Regression fixture for the "Autosave off" freeze. +// +// Recorded from a real ~/.config/agent-code/workspace.json: tab "agent-code" +// held a vertical split whose `b` leaf pointed at a session id that had no row +// in `sessions`, and the tab's focusedSessionId pointed at that same dead id. +// Every launch afterwards journalled `expectedCount 4, resolvedCount 3, ok +// false` and refused to autosave, which meant the file could never be +// repaired. The exact shape matters, so it is reproduced rather than +// paraphrased. +function makeOrphanLeafState(): WorkspaceState { + const state = makeState() + state.tabs = [ + { + id: 'tabA', + title: 'agent-code', + root: { + type: 'split', + direction: 'vertical', + ratio: 0.5, + a: leaf('live'), + b: leaf('orphan'), + }, + focusedSessionId: 'orphan', + }, + ] + return state +} + +describe('collectLiveProcessIds', () => { + it('excludes tile leaves that have no session metadata', () => { + // The gate denominator must only count panes that CAN be restored. + // Counting the orphan is what made restore completion unsatisfiable. + expect([...collectLiveProcessIds(makeOrphanLeafState())]).toEqual(['live']) + }) + + it('still counts every leaf that does have metadata', () => { + const state = makeOrphanLeafState() + state.sessions.orphan = { cwd: '/work/project-a', kind: 'claude' } + + expect([...collectLiveProcessIds(state)].sort()).toEqual(['live', 'orphan']) + }) + + it('restores the gate invariant: every live id is a key of sessions', () => { + // This is the property whose absence froze the workspace — the gate + // compares |resolvedIds| to |liveProcessIds| while resolvedIds can only + // ever contain keys of `sessions`, so the comparison is satisfiable only + // when liveProcessIds is a subset of those keys. Asserted directly rather + // than restating a fixture's expected size, so it holds for any input. + const state = makeOrphanLeafState() + const live = collectLiveProcessIds(state) + + expect([...live].every(id => + Object.prototype.hasOwnProperty.call(state.sessions, id))).toBe(true) + expect(live.has('orphan')).toBe(false) + }) + + it('does not read metadata through the prototype chain', () => { + // A leaf id like `toString` resolves to an inherited function under a bare + // index read, which would classify a genuine orphan as healthy and + // reproduce the freeze on a hand-edited workspace.json. + const state = makeOrphanLeafState() + state.tabs[0].root = { type: 'leaf', sessionId: 'toString' } + state.tabs[0].focusedSessionId = 'toString' + + expect([...collectLiveProcessIds(state)]).toEqual([]) + }) +}) + +describe('collectOwnedSessionIds', () => { + it('keeps tile leaves owned independently of the live-process set', () => { + // Ownership and "needs a process" are different questions. A pane kind + // that deliberately spawns nothing (extension views) narrows the live set; + // if ownership were derived from that narrowed set, autosave would drop the + // pane's SessionMeta and manufacture the very orphan leaf this module + // repairs. Pinning them as separate sources keeps that impossible. + const state = makeOrphanLeafState() + state.sessions.orphan = { cwd: '/work/project-a', kind: 'claude' } + + expect([...collectOwnedSessionIds(state)].sort()).toEqual(['live', 'orphan']) + }) +}) + +describe('repairPersistedTabs', () => { + function repair( + state: WorkspaceState, + sessions: Record = { live: state.sessions.live }, + ) { + return repairPersistedTabs({ + tabs: state.tabs, + sessions, + activeTabId: state.activeTabId, + tileTabs: null, + }) + } + + it('collapses an orphaned split into its survivor and repoints tab focus', () => { + const result = repair(makeOrphanLeafState()) + + expect(result.droppedLeafSessionIds).toEqual(['orphan']) + expect(result.droppedTabIds).toEqual([]) + expect(result.tabs).toHaveLength(1) + // The split is gone entirely — the survivor is promoted to root, exactly + // as a normal pane close would have left it. + expect(result.tabs[0].root).toEqual({ type: 'leaf', sessionId: 'live' }) + expect(result.tabs[0].focusedSessionId).toBe('live') + expect(result.tabs[0].title).toBe('agent-code') + }) + + it('repoints focus that names a session outside this tab', () => { + // The invariant a tab owes is "focus names a leaf I contain". Testing + // against the sessions map instead would leave this dangling, and rehydrate + // does not repair it either because the id resolves fine. + const state = makeOrphanLeafState() + state.tabs[0].focusedSessionId = 'elsewhere' + + const result = repair(state, { live: state.sessions.live, elsewhere: state.sessions.live }) + + expect(result.tabs[0].focusedSessionId).toBe('live') + }) + + it('leaves healthy trees untouched', () => { + const state = makeOrphanLeafState() + state.sessions.orphan = { cwd: '/work/project-a', kind: 'claude' } + + const result = repair(state, state.sessions) + + expect(result.droppedLeafSessionIds).toEqual([]) + // Identity, not just equality: a healthy save must not churn the tree. + expect(result.tabs[0]).toBe(state.tabs[0]) + expect(result.activeTabId).toBe(state.activeTabId) + }) + + it('reports one id when the same orphan occupies several leaves', () => { + const state = makeOrphanLeafState() + state.tabs[0].root = { + type: 'split', + direction: 'vertical', + ratio: 0.5, + a: leaf('orphan'), + b: { type: 'split', direction: 'horizontal', ratio: 0.5, a: leaf('orphan'), b: leaf('live') }, + } + + const result = repair(state) + + expect(result.droppedLeafSessionIds).toEqual(['orphan']) + expect(result.tabs[0].root).toEqual({ type: 'leaf', sessionId: 'live' }) + }) + + it('drops a tab whose every leaf is orphaned and repoints activeTabId', () => { + // The one destructive branch in the whole change. + const state = makeOrphanLeafState() + state.tabs = [ + { id: 'tabA', title: 'agent-code', root: leaf('orphan'), focusedSessionId: 'orphan' }, + { id: 'tabB', title: 'other', root: leaf('live'), focusedSessionId: 'live' }, + ] + state.activeTabId = 'tabA' + + const result = repair(state) + + expect(result.droppedTabIds).toEqual(['tabA']) + expect(result.tabs.map(t => t.id)).toEqual(['tabB']) + expect(result.activeTabId).toBe('tabB') + }) + + it('drops a dead tab out of tileTabs rather than persisting a dangling id', () => { + const state = makeOrphanLeafState() + state.tabs = [ + { id: 'tabA', title: 'agent-code', root: leaf('orphan'), focusedSessionId: 'orphan' }, + { id: 'tabB', title: 'b', root: leaf('live'), focusedSessionId: 'live' }, + { id: 'tabC', title: 'c', root: leaf('live'), focusedSessionId: 'live' }, + ] + + const result = repairPersistedTabs({ + tabs: state.tabs, + sessions: { live: state.sessions.live }, + activeTabId: 'tabB', + tileTabs: { + tabIds: ['tabA', 'tabB', 'tabC'], + focusedTabId: 'tabA', + direction: 'vertical', + ratios: [0.34, 0.33, 0.33], + }, + }) + + expect(result.tileTabs?.tabIds).toEqual(['tabB', 'tabC']) + // Focus pointed at the dropped tab, and ratios must match the new count. + expect(result.tileTabs?.focusedTabId).toBe('tabB') + expect(result.tileTabs?.ratios).toHaveLength(2) + }) + + it('keeps every tab when nothing was dropped', () => { + const state = makeOrphanLeafState() + const tileTabs = { + tabIds: ['tabA', 'tabB'], + focusedTabId: 'tabA', + direction: 'vertical' as const, + ratios: [0.5, 0.5], + } + + const result = repairPersistedTabs({ + tabs: state.tabs, + sessions: { live: state.sessions.live }, + activeTabId: state.activeTabId, + tileTabs, + }) + + // Only a dropped TAB can invalidate tileTabs; a collapsed split cannot. + expect(result.tileTabs).toBe(tileTabs) + }) +}) diff --git a/src/renderer/src/workspace/sessionOwnership.ts b/src/renderer/src/workspace/sessionOwnership.ts index 346950ef..685d2abc 100644 --- a/src/renderer/src/workspace/sessionOwnership.ts +++ b/src/renderer/src/workspace/sessionOwnership.ts @@ -6,8 +6,10 @@ import type { SessionMeta, TabId, TileNode, + TileTabsState, } from '@renderer/workspace/types' -import { collectLeaves } from '@renderer/workspace/tile-tree/treeOps' +import { closeLeaf, collectLeaves } from '@renderer/workspace/tile-tree/treeOps' +import { sanitizeTileTabsState } from '@renderer/workspace/layout/helpers' import { keepTiledLaneSessions } from '@renderer/workspace/dispatch/tiledDispatchSelectors' type SessionOwnershipTab = { @@ -73,8 +75,70 @@ export type PrunedSessionOwnership = { // Dispatch focus is intentionally excluded from both sets. 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. +/** + * Does `sessions` actually carry metadata for this id? + * + * WHY an own-property check and not a bare `sessions[id]` truthiness test: a plain + * index read walks the prototype chain, so a leaf id of `toString`, + * `constructor`, or `valueOf` resolves to an inherited function and reads as + * "has metadata". That is precisely inverted from what every caller here + * wants, and it would make such a leaf invisible to BOTH the restore gate and + * the repair guard — i.e. it would reproduce the permanent-freeze bug while + * looking healthy. Session ids are `randomUUID()` today, so this needs a + * hand-edited workspace.json to reach; hand-edited files are named as an + * explicit threat model throughout this module, so the check should be total. + * + * The value must also be truthy, not merely present: rehydrate decides what to + * restore with a truthiness test of its own (`freshSessions[id] ?`), so an own + * key holding `undefined` has to read as "no metadata" on this side too or the + * two halves disagree about what a pane is. + */ +function hasSessionMeta( + sessions: Record, + id: SessionId, +): boolean { + // `Object.prototype.hasOwnProperty.call` rather than `Object.hasOwn`: this + // project's TS lib target predates ES2022, and a own-property check is not + // worth moving the whole compiler target for. + return Object.prototype.hasOwnProperty.call(sessions, id) && Boolean(sessions[id]) +} + +/** + * Every tile leaf that has real metadata — the ownership half of "visible". + * + * WHY this is separate from `collectLiveProcessIds`, which on this branch + * returns the same thing: + * + * Ownership answers "whose SessionMeta must survive a save?" while the live set + * answers "which sessions need a backend process?". Those coincide today, and + * `collectOwnedSessionIds` used to be built directly on the live set because of + * that. But the sets are not the same question, and collapsing them makes any + * future narrowing of "needs a process" silently narrow ownership too — which + * deletes user data. + * + * That is not hypothetical. The in-flight extension-view work adds panes that + * are real tile leaves with real metadata but deliberately spawn NO process, by + * skipping them in `collectLiveProcessIds`. Built on the old shape, that skip + * also removed them from the owned set, so `pickOwnedSessions` dropped their + * metadata on the very next autosave — turning them into exactly the orphan + * leaves this module now repairs, and handing the repair guard a live pane to + * collapse out of the user's tree. Splitting the two sets here is what makes + * "excluded from spawning" and "excluded from persistence" independent, so a + * process-less pane kind is safe to add in one place. + */ +export function collectTileLeafIds(input: SessionOwnershipInput): Set { + const leaves = new Set() + for (const tab of input.tabs) { + for (const id of collectLeaves(tab.root)) { + if (!hasSessionMeta(input.sessions, id)) continue + leaves.add(id) + } + } + return leaves +} + export function collectOwnedSessionIds(input: SessionOwnershipInput): Set { - const owned = collectLiveProcessIds(input) + const owned = collectTileLeafIds(input) const existingTabIds = new Set(input.tabs.map(tab => tab.id)) for (const entry of Object.values(input.detachedSessions ?? {})) { @@ -109,14 +173,32 @@ export function collectOwnedSessionIds(input: SessionOwnershipInput): Set { - const live = new Set() - for (const tab of input.tabs) { - for (const id of collectLeaves(tab.root)) { - live.add(id) - } - } - return live + return collectTileLeafIds(input) } export function collectUnownedSessionIds(input: SessionOwnershipInput): SessionId[] { @@ -194,3 +276,126 @@ export function pruneSessionOwnership( droppedSessionIds, } } + + +/** + * Repair the tab structures that autosave is about to serialize: drop tile + * leaves whose session id has no `sessions` row, collapsing each orphaned split + * into its surviving sibling, and fix up the pointers that repair invalidates. + * + * WHY this belongs at the autosave boundary and not in a close/kill path: + * + * `pruneSessionOwnership` already claims to keep the serialized model "closed + * under restore", and it scrubs every pointer that aims AT a session — + * `sessions`, `detachedSessions`, `buried`, dispatch focus, tiled lanes. Tile + * trees were the one owner class it never validated, because `useAutoSave` + * serialized `state.tabs` verbatim. That asymmetry is what let a torn in-memory + * state (a leaf whose metadata had already been removed) become durable, and + * durable corruption here is uniquely bad: it disables the very autosave that + * would fix it. + * + * There is a self-reference that makes this the ONLY place the repair can + * happen. Ownership is *derived from* tile leaves — `collectOwnedSessionIds` + * walks the trees — so an orphan leaf can never be removed by pruning + * `sessions` against owners. The orphan IS an owner; there is nothing for + * `pickOwnedSessions` to drop. The tree itself has to be rewritten. + * + * WHAT THIS DOES NOT DO — do not let the next reader assume otherwise: it + * repairs the object being SERIALIZED, not `state.tabs`. For the rest of the + * session the orphan leaf stays in the live tree and still renders, as a + * default-provider pane stuck idle with a `?` label (TileTree renderWorkspaceLeaf + * falls back to DEFAULT_PROVIDER and an empty runtime). So on-screen and + * on-disk deliberately diverge until the next launch, which is the trade this + * whole change makes: a pane that cannot be restored must not be allowed to + * hold the user's entire workspace file hostage. + * + * A tab that loses every leaf is dropped: its root would be empty, which + * `TileNode` cannot represent and no pane could render. That is the one + * destructive branch here, which is why `activeTabId` and `tileTabs` are + * repaired in the same pure function rather than at the call site — it keeps + * the whole destructive path testable without a React harness. + */ +export function repairPersistedTabs< + TTab extends SessionOwnershipTab & { focusedSessionId?: SessionId }, +>(input: { + tabs: readonly TTab[] + sessions: Record + activeTabId: TabId + tileTabs: TileTabsState | null +}): { + tabs: TTab[] + activeTabId: TabId + tileTabs: TileTabsState | null + droppedLeafSessionIds: SessionId[] + droppedTabIds: TabId[] +} { + const { sessions } = input + const droppedLeafSessionIds = new Set() + const droppedTabIds: TabId[] = [] + const kept: TTab[] = [] + + for (const tab of input.tabs) { + const orphans = collectLeaves(tab.root).filter(id => !hasSessionMeta(sessions, id)) + if (orphans.length === 0) { + kept.push(tab) + continue + } + for (const id of orphans) droppedLeafSessionIds.add(id) + + // closeLeaf is the same primitive the user-facing pane close uses, so a + // repaired tree has exactly the shape it would have had if the pane had + // been closed normally — splits collapse into the survivor, ratios of + // untouched splits are preserved. It nulls EVERY matching leaf in one + // pass, so iterating the deduped orphan set is sufficient. + let root: TileNode | null = tab.root + for (const orphanId of droppedLeafSessionIds) { + if (root === null) break + root = closeLeaf(root, orphanId) + } + if (root === null) { + droppedTabIds.push(tab.id) + continue + } + + const survivingLeaves = collectLeaves(root) + kept.push({ + ...tab, + root, + // WHY the test is tree membership and not "is focus still in `sessions`": + // the invariant a tab owes is that its focus names a leaf IT CONTAINS. + // Checking the sessions map instead would leave focus pointing at a real + // session that lives in another tab — rehydrate would not repair that + // either, because its `idMap.get(focusedSessionId) ?? leaves[0]` fallback + // only fires for an id it cannot resolve at all. + ...(tab.focusedSessionId !== undefined + && !survivingLeaves.includes(tab.focusedSessionId) + ? { focusedSessionId: survivingLeaves[0] } + : {}), + }) + } + + // Pointers that only a dropped TAB can invalidate. Both self-heal on read, + // but this function's whole claim is that what it returns is closed under + // restore, and leaving a known-dangling id behind would make that a lie. + const activeTabId = kept.some(t => t.id === input.activeTabId) + ? input.activeTabId + : kept[0]?.id ?? input.activeTabId + const survivingTabIds = new Set(kept.map(t => t.id)) + const tileTabs = input.tileTabs === null || droppedTabIds.length === 0 + ? input.tileTabs + // sanitizeTileTabsState re-picks focus, re-derives ratios to match the new + // tab count, and collapses to null below two tabs — so filtering the ids is + // all this needs to do. + : sanitizeTileTabsState({ + ...input.tileTabs, + tabIds: input.tileTabs.tabIds.filter(id => survivingTabIds.has(id)), + }) + + return { + tabs: kept, + activeTabId, + tileTabs, + droppedLeafSessionIds: [...droppedLeafSessionIds], + droppedTabIds, + } +} diff --git a/src/shared/lifecycle/events.ts b/src/shared/lifecycle/events.ts index 6b9b8a38..8338468f 100644 --- a/src/shared/lifecycle/events.ts +++ b/src/shared/lifecycle/events.ts @@ -245,6 +245,14 @@ export const SESSION_LIFECYCLE_DATA_KEYS = [ 'buried', 'expectedCount', 'resolvedCount', + // Comma-joined session ids that were expected but never resolved. Ids are + // already first-class in this stream (`ids.sessionId`), so this adds no new + // category of data — it answers "which pane" for an event that previously + // only said "one pane", which cost three weeks of a frozen workspace to + // diagnose by hand. Joined into a string because payload values must stay + // flat: the sanitizer only inspects top-level keys, so an array would sail + // past the allowlist. + 'unresolvedSessionIds', 'entryCount', 'suppressed',