From 3d525b9fff67c35093393aefb16c46d351ac34b0 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 18 Aug 2026 12:01:31 +0200 Subject: [PATCH 1/2] fix(workspace): stop a metadata-less tile leaf from freezing autosave forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single tile leaf whose session id had no row in `sessions` permanently disabled autosave on a real workspace for three weeks. Every launch journalled `expectedCount 4, resolvedCount 3, ok false`, rendered the "Autosave off" banner, and left workspace.json frozen at the moment the corruption was written. The failure is self-sealing, which is what makes it worth a targeted fix rather than waiting for #545: - `collectLiveProcessIds` counted every tile leaf, so the orphan entered `expectedSessions`. - rehydrate's respawn loop iterates `persisted.sessions`, so the orphan could never be claimed, spawned, or given an outcome. - `complete = resolvedIds.size === expectedSessions` was therefore unsatisfiable, pinning `partial-restore` and holding autosave off. - autosave is the ONLY writer of workspace.json, so the corrupt tree could never be rewritten. Restarting could not help, despite being exactly what the banner advised. Fixed on both sides of the boundary: - Read side: a leaf with no SessionMeta has no cwd and no kind, so nothing can be spawned for it. It no longer counts toward the restore gate, which makes an already-corrupt file self-heal instead of latching off forever. - Write side: `pruneOrphanTileLeaves` collapses such leaves out of the tree via the same `closeLeaf` primitive a normal pane close uses, and repoints a tab's `focusedSessionId` when it pointed at the dead id. `pruneSessionOwnership` already scrubbed every pointer aimed AT a session; tile trees were the one owner class written verbatim, and that asymmetry is how the torn state became durable. Ownership is derived FROM leaves, so the orphan is itself an owner — pruning `sessions` against owners can never remove it, and the tree has to be rewritten. Also names the unresolved ids in `rehydrate.complete`. The event previously said one pane was missing but never which, so diagnosing this meant diffing tile leaves against the sessions map by hand. Regression tests reproduce the recorded workspace.json shape: a vertical split whose `b` leaf is orphaned and whose tab focus points at it. Refs #545 Co-Authored-By: Claude Opus 5 (1M context) --- .../workspace/hook/persistence/rehydrate.ts | 11 ++ .../workspace/hook/persistence/useAutoSave.ts | 30 +++++- .../src/workspace/sessionOwnership.test.ts | 102 +++++++++++++++++- .../src/workspace/sessionOwnership.ts | 101 ++++++++++++++++- src/shared/lifecycle/events.ts | 8 ++ 5 files changed, 247 insertions(+), 5 deletions(-) diff --git a/src/renderer/src/workspace/hook/persistence/rehydrate.ts b/src/renderer/src/workspace/hook/persistence/rehydrate.ts index dd11bcad..465f19c8 100644 --- a/src/renderer/src/workspace/hook/persistence/rehydrate.ts +++ b/src/renderer/src/workspace/hook/persistence/rehydrate.ts @@ -821,10 +821,21 @@ export async function rehydrateWorkspace( // provider started. A run whose rehydrate.start has no matching complete is a // restore that never resolved, which pins autosave off and is invisible today // apart from a console.warn. + // + // WHY the unresolved ids are named and not just counted: a real workspace sat + // at `expectedCount 4, resolvedCount 3, ok false` on every launch for three + // weeks, and the journal could not say WHICH pane was missing. Diagnosing it + // required diffing tile leaves against the `sessions` map by hand. The count + // alone tells you that restore is stuck; the ids tell you why, which is the + // whole point of recording this event. + const unresolvedSessionIds = [...liveProcessIds] + .filter(id => !resolvedIds.has(id)) + .join(',') reportLifecycle('rehydrate.complete', undefined, { expectedCount: expectedSessions, resolvedCount: resolvedIds.size, ok: resolvedIds.size === expectedSessions, + unresolvedSessionIds, 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..ac0ca4c3 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 { pruneOrphanTileLeaves, pruneSessionOwnership } from '@renderer/workspace/sessionOwnership' import { withNormalizedBuiltInMcpDomains } from '@renderer/workspace/mcpDomains' import type { WorkspaceRefs } from '@renderer/workspace/hook/refs' @@ -53,6 +53,30 @@ 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. + const prunedTabs = pruneOrphanTileLeaves(s.tabs, pruned.sessions) + if (prunedTabs.droppedLeafSessionIds.length > 0) { + // eslint-disable-next-line no-console + console.warn( + '[workspace] dropping tile leaves with no session metadata during autosave:', + { leaves: prunedTabs.droppedLeafSessionIds, tabs: prunedTabs.droppedTabIds }, + ) + } + // If the active tab lost every pane it can no longer be activated; fall + // back to the first surviving tab so the next launch opens on something + // real instead of an id that resolves to nothing. + const activeTabId = prunedTabs.tabs.some(t => t.id === s.activeTabId) + ? s.activeTabId + : prunedTabs.tabs[0]?.id ?? s.activeTabId + // 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 +93,13 @@ export function useAutoSave( id => pruned.sessions[id] !== undefined, ) const persisted: PersistedWorkspace = { - tabs: s.tabs.map(t => ({ + tabs: prunedTabs.tabs.map(t => ({ id: t.id, title: t.title, focusedSessionId: t.focusedSessionId, root: t.root, })), - activeTabId: s.activeTabId, + activeTabId, dispatchMode: pruned.dispatchMode, // WHY normalize MCP domains at the persistence boundary: // diff --git a/src/renderer/src/workspace/sessionOwnership.test.ts b/src/renderer/src/workspace/sessionOwnership.test.ts index 698f3de0..40673a55 100644 --- a/src/renderer/src/workspace/sessionOwnership.test.ts +++ b/src/renderer/src/workspace/sessionOwnership.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from 'vitest' -import { pruneSessionOwnership } from '@renderer/workspace/sessionOwnership' +import { + collectLiveProcessIds, + pruneOrphanTileLeaves, + pruneSessionOwnership, +} from '@renderer/workspace/sessionOwnership' import type { TileNode, WorkspaceState } from '@renderer/workspace/types' function leaf(sessionId: string): TileNode { @@ -158,3 +162,99 @@ 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']) + }) +}) + +describe('pruneOrphanTileLeaves', () => { + it('collapses an orphaned split into its survivor and repoints tab focus', () => { + const state = makeOrphanLeafState() + + const result = pruneOrphanTileLeaves(state.tabs, { live: state.sessions.live }) + + 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('leaves healthy trees untouched', () => { + const state = makeOrphanLeafState() + state.sessions.orphan = { cwd: '/work/project-a', kind: 'claude' } + + const result = pruneOrphanTileLeaves(state.tabs, 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]) + }) + + it('drops a tab whose every leaf is orphaned', () => { + const state = makeOrphanLeafState() + state.tabs[0].root = leaf('orphan') + + const result = pruneOrphanTileLeaves(state.tabs, { live: state.sessions.live }) + + expect(result.droppedTabIds).toEqual(['tabA']) + expect(result.tabs).toEqual([]) + }) + + it('closes the loop: a pruned tree makes restore completion satisfiable again', () => { + // The end-to-end invariant. Before the fix these two numbers could never + // agree, so `complete` was false forever and autosave stayed locked. + const state = makeOrphanLeafState() + const pruned = pruneOrphanTileLeaves(state.tabs, { live: state.sessions.live }) + const expectedSessions = collectLiveProcessIds({ + tabs: pruned.tabs, + sessions: { live: state.sessions.live }, + }) + const resolvedAfterRestore = new Set(['live']) + + expect(resolvedAfterRestore.size).toBe(expectedSessions.size) + }) +}) diff --git a/src/renderer/src/workspace/sessionOwnership.ts b/src/renderer/src/workspace/sessionOwnership.ts index 346950ef..718d955a 100644 --- a/src/renderer/src/workspace/sessionOwnership.ts +++ b/src/renderer/src/workspace/sessionOwnership.ts @@ -7,7 +7,7 @@ import type { TabId, TileNode, } from '@renderer/workspace/types' -import { collectLeaves } from '@renderer/workspace/tile-tree/treeOps' +import { closeLeaf, collectLeaves } from '@renderer/workspace/tile-tree/treeOps' import { keepTiledLaneSessions } from '@renderer/workspace/dispatch/tiledDispatchSelectors' type SessionOwnershipTab = { @@ -113,6 +113,30 @@ export function collectLiveProcessIds(input: SessionOwnershipInput): Set() for (const tab of input.tabs) { for (const id of collectLeaves(tab.root)) { + // WHY a tile leaf with no SessionMeta is excluded from the live set: + // + // This set is BOTH the rehydrate spawn list and the denominator of the + // restore-completion gate (`expectedSessions` in rehydrate.ts). A leaf + // whose id has no row in `sessions` has no cwd and no kind, so there is + // literally nothing to spawn for it — rehydrate's respawn loop iterates + // `persisted.sessions` and can never even reach it. Counting it made + // `resolvedIds.size === expectedSessions` unsatisfiable FOREVER: + // restore reported `partial-restore`, autosave stayed locked to protect + // disk, and because autosave was the only writer of workspace.json the + // corrupt tree could never be rewritten. A single dangling leaf + // permanently froze a real user's workspace file for three weeks + // (observed: expectedCount 4, resolvedCount 3, ok false on every boot). + // + // Excluding it here is what makes an already-corrupt file self-heal: the + // gate becomes satisfiable, autosave unlocks, and the write-side guard + // (`pruneOrphanTileLeaves`) then serializes a repaired tree. The pane is + // not silently dropped from view either — the same guard collapses the + // leaf out of the tree, so the user never sees a pane that cannot exist. + // + // This is deliberately NOT "spawn a fresh session for the orphan": we do + // not know its cwd or provider, and inventing one would resurrect a pane + // the user never asked for, pointed at the wrong directory. + if (!input.sessions[id]) continue live.add(id) } } @@ -194,3 +218,78 @@ export function pruneSessionOwnership( droppedSessionIds, } } + + +/** + * Drop tile leaves whose session id has no `sessions` row, collapsing each + * orphaned split into its surviving sibling. + * + * 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. + * + * 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, so it is reported in `droppedTabIds` for the caller + * to log and to repair `activeTabId` against. + */ +export function pruneOrphanTileLeaves< + TTab extends SessionOwnershipTab & { focusedSessionId?: SessionId }, +>( + tabs: readonly TTab[], + sessions: Record, +): { tabs: TTab[]; droppedLeafSessionIds: SessionId[]; droppedTabIds: TabId[] } { + const droppedLeafSessionIds: SessionId[] = [] + const droppedTabIds: TabId[] = [] + const kept: TTab[] = [] + + for (const tab of tabs) { + const orphans = collectLeaves(tab.root).filter(id => !sessions[id]) + if (orphans.length === 0) { + kept.push(tab) + continue + } + droppedLeafSessionIds.push(...orphans) + + // 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. + let root: TileNode | null = tab.root + for (const orphanId of orphans) { + if (root === null) break + root = closeLeaf(root, orphanId) + } + if (root === null) { + droppedTabIds.push(tab.id) + continue + } + + const survivingLeaves = collectLeaves(root) + kept.push({ + ...tab, + root, + // focusedSessionId is a required field on the persisted tab, and it + // pointed at the orphan in the real-world case. Leaving it dangling + // would hand the next launch a focus id that resolves to no pane. + ...(tab.focusedSessionId !== undefined && !sessions[tab.focusedSessionId] + ? { focusedSessionId: survivingLeaves[0] } + : {}), + }) + } + + return { tabs: kept, 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', From 4320c6a411969fab4acf80172175cb1b56e1043f Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 18 Aug 2026 17:12:02 +0200 Subject: [PATCH 2/2] fix(workspace): split leaf ownership from the live-process set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The first commit built `collectOwnedSessionIds` on `collectLiveProcessIds`, so the two questions "whose SessionMeta must survive a save?" and "which sessions need a backend process?" shared one answer. They are not the same question, and collapsing them means any future narrowing of "needs a process" silently narrows 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 next autosave — which manufactured exactly the orphan leaf this branch repairs, and then handed the repair guard a live pane to collapse out of the user's tree. A loud freeze would have become a silent pane deletion. Ownership now flows through `collectTileLeafIds`, so the two guards compose. Other review fixes: - `hasSessionMeta` replaces bare `sessions[id]` truthiness. A plain index read walks the prototype chain, so a leaf id of `toString` or `constructor` read as "has metadata" — inverting the check and reproducing the freeze while looking healthy. - Tab focus is repaired against tree membership, not the sessions map. The invariant a tab owes is "focus names a leaf I contain"; the old predicate left focus pointing at a real session in another tab, which rehydrate does not repair either. - `activeTabId` and `tileTabs` repair moved into the same pure function (now `repairPersistedTabs`). The tab-drop branch is the only destructive path in the change and it had no test, because `useAutoSave` has no test harness anywhere in the repo; making the whole path pure makes it testable without React. - `unresolvedSessionIds` is emitted only on failure and capped at 8 ids. The journal sanitizer truncates at 300 chars, which would have sliced the last UUID in half — a half-id reads like a real one. - `summarize-lifecycle.mts` renders the new key, so the ids reach the human-facing report they were added for. - Corrected a comment that claimed the guard stops the user from seeing an impossible pane. It does not: repair applies to the serialized copy, not `state.tabs`, so the orphan still renders until relaunch. A wrong comment is worse than none. Replaced the "closes the loop" test, which asserted 1 === 1 and passed against any implementation, with a direct assertion of the restored invariant (`liveProcessIds` is a subset of the keys of `sessions`). Co-Authored-By: Claude Opus 5 (1M context) --- scripts/summarize-lifecycle.mts | 5 +- .../workspace/hook/persistence/rehydrate.ts | 16 +- .../workspace/hook/persistence/useAutoSave.ts | 35 +-- .../src/workspace/sessionOwnership.test.ts | 158 +++++++++++-- .../src/workspace/sessionOwnership.ts | 218 +++++++++++++----- 5 files changed, 340 insertions(+), 92 deletions(-) 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)) - .join(',') + const unresolved = [...liveProcessIds].filter(id => !resolvedIds.has(id)) reportLifecycle('rehydrate.complete', undefined, { expectedCount: expectedSessions, resolvedCount: resolvedIds.size, ok: resolvedIds.size === expectedSessions, - unresolvedSessionIds, + // 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 ac0ca4c3..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 { pruneOrphanTileLeaves, 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' @@ -62,20 +62,29 @@ export function useAutoSave( // 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. - const prunedTabs = pruneOrphanTileLeaves(s.tabs, pruned.sessions) - if (prunedTabs.droppedLeafSessionIds.length > 0) { + // + // 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: prunedTabs.droppedLeafSessionIds, tabs: prunedTabs.droppedTabIds }, + { leaves: repairedTabs.droppedLeafSessionIds, tabs: repairedTabs.droppedTabIds }, ) } - // If the active tab lost every pane it can no longer be activated; fall - // back to the first surviving tab so the next launch opens on something - // real instead of an id that resolves to nothing. - const activeTabId = prunedTabs.tabs.some(t => t.id === s.activeTabId) - ? s.activeTabId - : prunedTabs.tabs[0]?.id ?? s.activeTabId // Collect non-empty drafts so in-progress prompts survive crashes. const drafts: Record = {} @@ -93,13 +102,13 @@ export function useAutoSave( id => pruned.sessions[id] !== undefined, ) const persisted: PersistedWorkspace = { - tabs: prunedTabs.tabs.map(t => ({ + tabs: repairedTabs.tabs.map(t => ({ id: t.id, title: t.title, focusedSessionId: t.focusedSessionId, root: t.root, })), - activeTabId, + activeTabId: repairedTabs.activeTabId, dispatchMode: pruned.dispatchMode, // WHY normalize MCP domains at the persistence boundary: // @@ -120,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 40673a55..34b319d1 100644 --- a/src/renderer/src/workspace/sessionOwnership.test.ts +++ b/src/renderer/src/workspace/sessionOwnership.test.ts @@ -2,10 +2,16 @@ import { describe, expect, it } from 'vitest' import { collectLiveProcessIds, - pruneOrphanTileLeaves, + collectOwnedSessionIds, pruneSessionOwnership, + repairPersistedTabs, } from '@renderer/workspace/sessionOwnership' -import type { TileNode, WorkspaceState } from '@renderer/workspace/types' +import type { + SessionId, + SessionMeta, + TileNode, + WorkspaceState, +} from '@renderer/workspace/types' function leaf(sessionId: string): TileNode { return { type: 'leaf', sessionId } @@ -205,13 +211,62 @@ describe('collectLiveProcessIds', () => { 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('pruneOrphanTileLeaves', () => { - it('collapses an orphaned split into its survivor and repoints tab focus', () => { +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' } - const result = pruneOrphanTileLeaves(state.tabs, { live: state.sessions.live }) + 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([]) @@ -223,38 +278,105 @@ describe('pruneOrphanTileLeaves', () => { 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 = pruneOrphanTileLeaves(state.tabs, state.sessions) + 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('drops a tab whose every leaf is orphaned', () => { + it('reports one id when the same orphan occupies several leaves', () => { const state = makeOrphanLeafState() - state.tabs[0].root = leaf('orphan') + 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 = pruneOrphanTileLeaves(state.tabs, { live: state.sessions.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).toEqual([]) + expect(result.tabs.map(t => t.id)).toEqual(['tabB']) + expect(result.activeTabId).toBe('tabB') }) - it('closes the loop: a pruned tree makes restore completion satisfiable again', () => { - // The end-to-end invariant. Before the fix these two numbers could never - // agree, so `complete` was false forever and autosave stayed locked. + it('drops a dead tab out of tileTabs rather than persisting a dangling id', () => { const state = makeOrphanLeafState() - const pruned = pruneOrphanTileLeaves(state.tabs, { live: state.sessions.live }) - const expectedSessions = collectLiveProcessIds({ - tabs: pruned.tabs, + 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, }) - const resolvedAfterRestore = new Set(['live']) - expect(resolvedAfterRestore.size).toBe(expectedSessions.size) + // 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 718d955a..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 { 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,38 +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)) { - // WHY a tile leaf with no SessionMeta is excluded from the live set: - // - // This set is BOTH the rehydrate spawn list and the denominator of the - // restore-completion gate (`expectedSessions` in rehydrate.ts). A leaf - // whose id has no row in `sessions` has no cwd and no kind, so there is - // literally nothing to spawn for it — rehydrate's respawn loop iterates - // `persisted.sessions` and can never even reach it. Counting it made - // `resolvedIds.size === expectedSessions` unsatisfiable FOREVER: - // restore reported `partial-restore`, autosave stayed locked to protect - // disk, and because autosave was the only writer of workspace.json the - // corrupt tree could never be rewritten. A single dangling leaf - // permanently froze a real user's workspace file for three weeks - // (observed: expectedCount 4, resolvedCount 3, ok false on every boot). - // - // Excluding it here is what makes an already-corrupt file self-heal: the - // gate becomes satisfiable, autosave unlocks, and the write-side guard - // (`pruneOrphanTileLeaves`) then serializes a repaired tree. The pane is - // not silently dropped from view either — the same guard collapses the - // leaf out of the tree, so the user never sees a pane that cannot exist. - // - // This is deliberately NOT "spawn a fresh session for the orphan": we do - // not know its cwd or provider, and inventing one would resurrect a pane - // the user never asked for, pointed at the wrong directory. - if (!input.sessions[id]) continue - live.add(id) - } - } - return live + return collectTileLeafIds(input) } export function collectUnownedSessionIds(input: SessionOwnershipInput): SessionId[] { @@ -221,8 +279,9 @@ export function pruneSessionOwnership( /** - * Drop tile leaves whose session id has no `sessions` row, collapsing each - * orphaned split into its surviving sibling. + * 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: * @@ -230,10 +289,10 @@ export function pruneSessionOwnership( * 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. + * 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` @@ -241,35 +300,55 @@ export function pruneSessionOwnership( * `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, so it is reported in `droppedTabIds` for the caller - * to log and to repair `activeTabId` against. + * 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 pruneOrphanTileLeaves< +export function repairPersistedTabs< TTab extends SessionOwnershipTab & { focusedSessionId?: SessionId }, ->( - tabs: readonly TTab[], - sessions: Record, -): { tabs: TTab[]; droppedLeafSessionIds: SessionId[]; droppedTabIds: TabId[] } { - const droppedLeafSessionIds: 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 tabs) { - const orphans = collectLeaves(tab.root).filter(id => !sessions[id]) + for (const tab of input.tabs) { + const orphans = collectLeaves(tab.root).filter(id => !hasSessionMeta(sessions, id)) if (orphans.length === 0) { kept.push(tab) continue } - droppedLeafSessionIds.push(...orphans) + 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. + // 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 orphans) { + for (const orphanId of droppedLeafSessionIds) { if (root === null) break root = closeLeaf(root, orphanId) } @@ -282,14 +361,41 @@ export function pruneOrphanTileLeaves< kept.push({ ...tab, root, - // focusedSessionId is a required field on the persisted tab, and it - // pointed at the orphan in the real-world case. Leaving it dangling - // would hand the next launch a focus id that resolves to no pane. - ...(tab.focusedSessionId !== undefined && !sessions[tab.focusedSessionId] + // 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] } : {}), }) } - return { tabs: kept, droppedLeafSessionIds, droppedTabIds } + // 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, + } }