From 83ce4bd16d07c9c75bba1954d5fbd9bd52f47323 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 19:12:50 -0700 Subject: [PATCH 1/3] fix(workspace): a corrupt container is not an empty workspace, and a hibernated terminal's tmux dies with it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four follow-ups filed as #1030 from #1013's persistence review. Each is a fail-first test against the RECORDED live workspace (testing/fixtures/workspace-v2), degraded in exactly one field. 1. Item 3 — data loss. A `tabs` or `projects` field that is PRESENT but not a list migrated to an empty pool; rehydrate then minted a fresh tab and reported `complete`, which unlocks autosave, so the next 400 ms tick wrote an empty workspace over the real file. v2 threw here, and the throw is what puts bootstrap into its locked fallback with the disk file untouched. `undefined` stays legal: a v2 file has no `projects`, a v3 file has no `tabs`, and `tabs: []` is a writer's empty workspace rather than corruption. 2. Item 2 — data loss. A v2 file with zero tabs still carried its buried panes, because burial was independent of tabs there. Here every session needs a project, so with none left the re-parent target was '' and every buried row was dropped — including rows whose only copy of the session metadata was the buried record itself. The migration now mints one project to receive them, titled from the first such session's cwd. 3. Item 4 — resource leak. Closing a hibernated terminal left its tmux session running until the next launch's sweep: boot hibernates every pane but the focused lane, so main has no row to tear down, and the renderer's persisted metadata is the only place the name survives. The close now carries `tmuxName`, and main kills it only when the registry minted the name (`ownsSessionName`), which is the same prefix filter `listManagedSessions` applies. Item 1 (carrying v2 bury notes) is left open on #1030: v3 has no parked- note surface, so where the text would be shown is a product decision rather than a migration fix. Co-Authored-By: Claude Opus 5 (1M context) --- src/main/sessionManager.recover.test.ts | 27 +++++++++ src/main/sessionManager.ts | 18 ++++++ src/main/tmux/TmuxRegistry.ts | 13 +++++ .../src/workspace/hook/actions/session.ts | 6 +- .../src/workspace/workspaceShape.test.ts | 57 +++++++++++++++++++ src/renderer/src/workspace/workspaceShape.ts | 42 +++++++++++++- src/shared/types/session.ts | 15 ++++- 7 files changed, 174 insertions(+), 4 deletions(-) diff --git a/src/main/sessionManager.recover.test.ts b/src/main/sessionManager.recover.test.ts index d415763ba..e3b85cad4 100644 --- a/src/main/sessionManager.recover.test.ts +++ b/src/main/sessionManager.recover.test.ts @@ -704,6 +704,33 @@ describe('SessionManager recover', () => { expect(manager.getBackendSnapshot(owner.sessionId)).toBeNull() }) + it('closes a HIBERNATED terminal by killing the tmux session its pane still names (#1030 item 4)', async () => { + // Boot hibernates every pane but the focused lane, so main holds no row + // for this terminal. The close used to find nothing, return false, and + // leave the shell running until the next launch's reconciliation swept + // it. The renderer's persisted metadata is the only place the name still + // exists, so main takes it — but only when the registry minted it. + const { SessionManager } = await import('./sessionManager') + const killed: string[] = [] + const registry = { + ownsSessionName: (name: string) => name.startsWith('agentcode-'), + killSession: async (name: string) => { killed.push(name) }, + isAvailable: () => true, + } + const manager = new SessionManager(registry as never) + const hibernated = { sessionId: 'hibernated-terminal', kind: 'terminal' as const, cwd: '/tmp/project' } + + await expect(manager.killOwned({ ...hibernated, tmuxName: 'agentcode-abc123' })).resolves.toBe(true) + expect(killed).toEqual(['agentcode-abc123']) + + // A name this registry did not mint is not ours to kill, whatever a + // renderer says. + await expect(manager.killOwned({ ...hibernated, tmuxName: 'someone-elses-session' })).resolves.toBe(false) + // And with no name at all there is still nothing to tear down. + await expect(manager.killOwned(hibernated)).resolves.toBe(false) + expect(killed).toEqual(['agentcode-abc123']) + }) + it('keeps readiness revisions monotonic after the bounded known-id cache evicts old ids', async () => { const { SessionManager } = await import('./sessionManager') const manager = new SessionManager() diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 246924fea..77f9b01ab 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -4533,6 +4533,24 @@ export class SessionManager extends EventEmitter { // this close reached it. Report the cancellation as handled so renderer // close does not mistake an idempotent joined stop for a stale miss. return true + } else if ( + kind === 'terminal' && + typeof options.tmuxName === 'string' && + this.tmuxRegistry?.ownsSessionName(options.tmuxName) === true + ) { + // A HIBERNATED terminal (#1030 item 4): boot hibernates every pane but + // the focused lane, so main has no row for it, yet its tmux session is + // alive and holds the user's shell. Closing the pane used to return + // false here and leave that session running until the next launch's + // reconciliation swept it — with more hibernated terminals than ever, + // that is a growing pile of orphaned shells. + // + // The name is renderer-supplied, so the registry's prefix is the + // ownership proof: it is the same filter listManagedSessions applies, + // and nothing else can mint one. killSession is a no-op when the name + // is already gone, which makes a double close harmless. + await this.tmuxRegistry.killSession(options.tmuxName) + return true } else { // During destructive handoff the predecessor row is intentionally gone, // and during compensation preflight the recovery claim is the owner. The diff --git a/src/main/tmux/TmuxRegistry.ts b/src/main/tmux/TmuxRegistry.ts index 7560018e7..6057a98a9 100644 --- a/src/main/tmux/TmuxRegistry.ts +++ b/src/main/tmux/TmuxRegistry.ts @@ -189,6 +189,19 @@ export class TmuxRegistry { } } + /** + * Is this a name THIS registry mints? + * + * WHY a caller needs to ask (#1030 item 4): closing a hibernated terminal + * hands main a tmux name from persisted renderer metadata, with no live + * session row to prove ownership. The prefix is the proof that the name is + * ours to kill: `listManagedSessions` filters on exactly this, and + * `createSession` mints exactly this. + */ + ownsSessionName(name: string): boolean { + return name.startsWith(this.namePrefix) + } + /** Kill a session by name. No-op if it doesn't exist. */ async killSession(name: string): Promise { if (!(await this.sessionExists(name))) return diff --git a/src/renderer/src/workspace/hook/actions/session.ts b/src/renderer/src/workspace/hook/actions/session.ts index d4cfcba9e..0ad632f02 100644 --- a/src/renderer/src/workspace/hook/actions/session.ts +++ b/src/renderer/src/workspace/hook/actions/session.ts @@ -129,7 +129,7 @@ export type SessionWakeOptions = { export async function killSessionBackendIfOwned( refs: WorkspaceRefs, sessionId: SessionId, - capturedOwner?: Pick, + capturedOwner?: Pick, ): Promise { // Spawn cleanup may run before React refreshes stateRef. Its caller already // knows the exact scope it just created; main still performs the atomic @@ -145,6 +145,10 @@ export async function killSessionBackendIfOwned( kind: meta.kind, providerRuntime: meta.providerRuntime, cwd: meta.cwd, + // Only a terminal has one, and main only acts on it when the pane has no + // live backend to tear down instead (#1030 item 4): a hibernated + // terminal's tmux session is otherwise orphaned until the next launch. + ...(meta.tmuxName ? { tmuxName: meta.tmuxName } : {}), }) } diff --git a/src/renderer/src/workspace/workspaceShape.test.ts b/src/renderer/src/workspace/workspaceShape.test.ts index f570f0de3..8a87c4c7d 100644 --- a/src/renderer/src/workspace/workspaceShape.test.ts +++ b/src/renderer/src/workspace/workspaceShape.test.ts @@ -5,11 +5,14 @@ import type { SessionId, TabId } from '@renderer/workspace/types' import { isStageWorkspace, liveWorkspaceFromPersisted, + MalformedWorkspaceContainerError, migrateWorkspaceToStage, } from '@renderer/workspace/workspaceShape' import { resolveTabSessions } from '@renderer/workspace/queries' import { collectOwnedSessionIds } from '@renderer/workspace/sessionOwnership' import { ownerV2Workspace } from '@renderer/workspace/workspaceShape.ownerV2Fixture' +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' // The migration contract (plan 2026-09-17-unified-stage-layout.md §6, §10). // Every fixture class here is either recorded from a real workspace.json or @@ -547,3 +550,57 @@ describe('migrateWorkspaceToStage — v3 and hybrid files', () => { expect(migrated.sessions[S('a1')]?.joinedAt).toBe(0) }) }) + +// #1030 items 2 and 3, against the RECORDED live workspace +// (testing/fixtures/workspace-v2/2026-09-19-live-workspace.sanitized.json: +// one window, 3 tabs, 27 sessions) rather than an invented shape. Each case +// below is that file with exactly one field degraded, which is how both +// states arise in the wild: a hand-edited file, or a writer that emptied the +// tabs while parked rows remained. +function recordedLiveWorkspace(): PersistedWorkspace { + const file = JSON.parse(readFileSync( + resolve(__dirname, '../../../../testing/fixtures/workspace-v2/2026-09-19-live-workspace.sanitized.json'), + 'utf8', + )) as { windows: Array<{ workspace: PersistedWorkspace }> } + return file.windows[0]!.workspace +} + +describe('migrateWorkspaceToStage — a corrupt container is never an empty workspace (#1030)', () => { + it.each(['tabs', 'projects'] as const)('refuses a %s field that is present but not a list', field => { + // Migrating this to an empty pool let rehydrate mint a fresh tab and + // report `complete`, which unlocks autosave — so the next tick wrote an + // empty workspace over the real file. Throwing is what puts bootstrap in + // its locked fallback, which is what v2 did. + const degraded = { ...recordedLiveWorkspace(), [field]: null } as unknown as PersistedWorkspace + expect(() => migrateWorkspaceToStage(degraded)).toThrow(MalformedWorkspaceContainerError) + }) + + it('still migrates the recording itself, and a file that is simply empty', () => { + expect(migrateWorkspaceToStage(recordedLiveWorkspace()).projects).toHaveLength(3) + // `tabs: []` is a writer's empty workspace, not corruption. + expect(migrateWorkspaceToStage({ tabs: [], sessions: {} }).projects).toEqual([]) + }) + + it('keeps buried rows when the file has no tabs left, instead of dropping them', () => { + // v2 kept buried panes whether or not a tab survived; here every session + // needs a project, so with none left the re-parent target was '' and the + // rows — sometimes the only copy of that session's metadata — were + // dropped on upgrade. + const recorded = recordedLiveWorkspace() + const [sessionId, meta] = Object.entries(recorded.sessions)[0]! + const buriedOnly: PersistedWorkspace = { + ...recorded, + tabs: [], + sessions: {}, + // A v2 buried record carries its own copy of the meta (`sessionMeta`), + // which is exactly why dropping it can lose the only copy. + buried: [{ + id: 'buried-1', sessionId: sessionId as SessionId, sessionMeta: meta, buriedAt: 1, + sourceTabId: recorded.tabs![0]!.id, sourceTabTitle: 'gone', sourceTabIndex: 0, + } as never], + } + const migrated = migrateWorkspaceToStage(buriedOnly, () => 'recovered-project' as TabId) + expect(migrated.projects).toEqual([expect.objectContaining({ id: 'recovered-project' })]) + expect(migrated.sessions[sessionId as SessionId]).toMatchObject({ projectId: 'recovered-project' }) + }) +}) diff --git a/src/renderer/src/workspace/workspaceShape.ts b/src/renderer/src/workspace/workspaceShape.ts index 2c817f205..01022f864 100644 --- a/src/renderer/src/workspace/workspaceShape.ts +++ b/src/renderer/src/workspace/workspaceShape.ts @@ -18,6 +18,7 @@ import { scrubGridRowMetadata, } from '@renderer/workspace/dispatch/tiledDispatchSelectors' import { normalizeGridShape } from '@renderer/workspace/dispatch/gridShape' +import { titleFromCwd } from '@renderer/workspace/layout/helpers' // --------------------------------------------------------------------------- // Read-time normalization of workspace.json into the unified shape (#992). @@ -120,7 +121,28 @@ export function defaultSeededStage(seed: SessionId | null): TiledDispatchState { * migration can never resurrect a row into a backend process (the #258 * fork-bomb shape). */ -export function migrateWorkspaceToStage(persisted: PersistedWorkspace): StageWorkspace { +export class MalformedWorkspaceContainerError extends Error {} + +export function migrateWorkspaceToStage( + persisted: PersistedWorkspace, + // Only a file with zero projects AND parked sessions to rehome needs this; + // injected so the test does not have to match a random id. + mintProjectId: () => TabId = () => crypto.randomUUID(), +): StageWorkspace { + // Rule 8 (#1030 item 3): a container that is PRESENT but not a list is + // corruption, not an empty workspace. Migrating it to an empty pool let + // rehydrate mint a fresh tab and report `complete`, which unlocks autosave — + // so the next 400 ms tick overwrote whatever the real file held. v2 threw + // here, and the throw is what put bootstrap into its locked fallback with + // the disk file untouched. `undefined` is not corruption: a v2 file has no + // `projects`, and a v3 file has no `tabs`. + for (const [field, value] of [['projects', persisted.projects], ['tabs', persisted.tabs]] as const) { + if (value !== undefined && !Array.isArray(value)) { + throw new MalformedWorkspaceContainerError( + `workspace.json has a malformed \`${field}\`; refusing to migrate it to an empty pool`, + ) + } + } // --- Rule 1. const projects: ProjectRef[] = Array.isArray(persisted.projects) ? persisted.projects @@ -136,12 +158,28 @@ export function migrateWorkspaceToStage(persisted: PersistedWorkspace): StageWor // a hand-emptied file must still migrate to something renderable. '' is // never a project id; readers treat it as "no active project". const recordedActive = persisted.activeProjectId ?? persisted.activeTabId ?? '' - const activeProjectId = projectIds.has(recordedActive) + let activeProjectId = projectIds.has(recordedActive) ? recordedActive : (projects[0]?.id ?? '') // --- Rules 2, 3, 7. const legacy = legacyMemberships(persisted) + // Rule 9 (#1030 item 2): a v2 file with zero tabs still carried its buried + // panes — burial was independent of tabs there. Here every session needs a + // project to live in, so with none left the re-parent target was '' and + // every buried row was dropped: the one place some sessions' metadata + // existed, deleted on upgrade. Mint one project to receive them instead. + // Only for files that genuinely have parked rows; an empty file stays empty. + if (projects.length === 0) { + const rehomed = [...legacy.values()].find(membership => membership.restoredMeta) + if (rehomed) { + const id = mintProjectId() + const cwd = rehomed.restoredMeta?.cwd + projects.push({ id, title: typeof cwd === 'string' ? titleFromCwd(cwd) : '', ...(typeof cwd === 'string' ? { cwd } : {}) }) + projectIds.add(id) + activeProjectId = id + } + } const sessions: Record = {} const candidateIds = new Set([ ...Object.keys(persisted.sessions ?? {}), diff --git a/src/shared/types/session.ts b/src/shared/types/session.ts index 49277ccba..569c19fb9 100644 --- a/src/shared/types/session.ts +++ b/src/shared/types/session.ts @@ -119,7 +119,20 @@ export type SessionRecoverOptions = { export type SessionOwnershipOptions = Pick< SessionRecoverOptions, 'sessionId' | 'kind' | 'providerRuntime' | 'cwd' -> +> & { + /** + * The tmux session this pane owns, for a TERMINAL the caller may be closing + * while it is hibernated (#1030 item 4). + * + * WHY the caller has to supply it: a hibernated terminal has no row in main + * — boot hibernates everything but the focused lane — so a close finds + * nothing to tear down and the tmux session survived until the next + * launch's sweep. The renderer's persisted metadata is the only place that + * name still exists. Main does not trust it blindly: it kills the name only + * when the registry minted it (`ownsSessionName`). + */ + tmuxName?: string +} export type SessionRecoveryCancellationOptions = SessionOwnershipOptions & { recoveryToken: string From 85e4ff1cbb757337133a92ae0045dc3023b9d482 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 19:38:39 -0700 Subject: [PATCH 2/3] fix(workspace): recover buried rows whose metadata never left, and drop the tmux kill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #1048: 1. P1. The burial recovery keyed on `restoredMeta`, which legacyMemberships sets ONLY when the metadata is missing from `sessions` — it exists to carry the copy a buried record holds. The common shape is the opposite: v2 buries a pane and leaves its `sessions` row in place, so exactly those rows were still dropped, which is the bug the clause exists to fix. Any parked membership now mints the project, and the metadata is resolved from either source. 2. P1. The hibernated-terminal tmux kill is REMOVED from this PR. A shared name prefix does not prove ownership: an unknown session id and an arbitrary cwd could authorize killing any `agentcode-…` name, including one minted by another install — and tmux targets match as patterns, so the review killed a foreign session with `kill-session -t agentcode-` on an isolated server. Its safe form needs a main-authoritative persisted ownership binding plus exact `=name` targeting, which is its own change; #1030 item 4 stays open with that design recorded. 3. P2. A migration throw escaped `adoptWorkspace`, and the callers only logged it — so main, which had already transferred session routing and recorded a pending bequest, never heard a refusal and those sessions stayed pinned to a window that would never display them. Adoption now refuses on a migration failure exactly as it does on unreadable JSON. Co-Authored-By: Claude Opus 5 (1M context) --- src/main/sessionManager.recover.test.ts | 27 ------------------- src/main/sessionManager.ts | 18 ------------- src/main/tmux/TmuxRegistry.ts | 13 --------- .../src/workspace/hook/actions/session.ts | 6 +---- .../hook/ipc/useWorkspaceAdoption.ts | 16 ++++++++++- .../src/workspace/workspaceShape.test.ts | 19 +++++++++++++ src/renderer/src/workspace/workspaceShape.ts | 15 +++++++++-- src/shared/types/session.ts | 15 +---------- 8 files changed, 49 insertions(+), 80 deletions(-) diff --git a/src/main/sessionManager.recover.test.ts b/src/main/sessionManager.recover.test.ts index e3b85cad4..d415763ba 100644 --- a/src/main/sessionManager.recover.test.ts +++ b/src/main/sessionManager.recover.test.ts @@ -704,33 +704,6 @@ describe('SessionManager recover', () => { expect(manager.getBackendSnapshot(owner.sessionId)).toBeNull() }) - it('closes a HIBERNATED terminal by killing the tmux session its pane still names (#1030 item 4)', async () => { - // Boot hibernates every pane but the focused lane, so main holds no row - // for this terminal. The close used to find nothing, return false, and - // leave the shell running until the next launch's reconciliation swept - // it. The renderer's persisted metadata is the only place the name still - // exists, so main takes it — but only when the registry minted it. - const { SessionManager } = await import('./sessionManager') - const killed: string[] = [] - const registry = { - ownsSessionName: (name: string) => name.startsWith('agentcode-'), - killSession: async (name: string) => { killed.push(name) }, - isAvailable: () => true, - } - const manager = new SessionManager(registry as never) - const hibernated = { sessionId: 'hibernated-terminal', kind: 'terminal' as const, cwd: '/tmp/project' } - - await expect(manager.killOwned({ ...hibernated, tmuxName: 'agentcode-abc123' })).resolves.toBe(true) - expect(killed).toEqual(['agentcode-abc123']) - - // A name this registry did not mint is not ours to kill, whatever a - // renderer says. - await expect(manager.killOwned({ ...hibernated, tmuxName: 'someone-elses-session' })).resolves.toBe(false) - // And with no name at all there is still nothing to tear down. - await expect(manager.killOwned(hibernated)).resolves.toBe(false) - expect(killed).toEqual(['agentcode-abc123']) - }) - it('keeps readiness revisions monotonic after the bounded known-id cache evicts old ids', async () => { const { SessionManager } = await import('./sessionManager') const manager = new SessionManager() diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 77f9b01ab..246924fea 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -4533,24 +4533,6 @@ export class SessionManager extends EventEmitter { // this close reached it. Report the cancellation as handled so renderer // close does not mistake an idempotent joined stop for a stale miss. return true - } else if ( - kind === 'terminal' && - typeof options.tmuxName === 'string' && - this.tmuxRegistry?.ownsSessionName(options.tmuxName) === true - ) { - // A HIBERNATED terminal (#1030 item 4): boot hibernates every pane but - // the focused lane, so main has no row for it, yet its tmux session is - // alive and holds the user's shell. Closing the pane used to return - // false here and leave that session running until the next launch's - // reconciliation swept it — with more hibernated terminals than ever, - // that is a growing pile of orphaned shells. - // - // The name is renderer-supplied, so the registry's prefix is the - // ownership proof: it is the same filter listManagedSessions applies, - // and nothing else can mint one. killSession is a no-op when the name - // is already gone, which makes a double close harmless. - await this.tmuxRegistry.killSession(options.tmuxName) - return true } else { // During destructive handoff the predecessor row is intentionally gone, // and during compensation preflight the recovery claim is the owner. The diff --git a/src/main/tmux/TmuxRegistry.ts b/src/main/tmux/TmuxRegistry.ts index 6057a98a9..7560018e7 100644 --- a/src/main/tmux/TmuxRegistry.ts +++ b/src/main/tmux/TmuxRegistry.ts @@ -189,19 +189,6 @@ export class TmuxRegistry { } } - /** - * Is this a name THIS registry mints? - * - * WHY a caller needs to ask (#1030 item 4): closing a hibernated terminal - * hands main a tmux name from persisted renderer metadata, with no live - * session row to prove ownership. The prefix is the proof that the name is - * ours to kill: `listManagedSessions` filters on exactly this, and - * `createSession` mints exactly this. - */ - ownsSessionName(name: string): boolean { - return name.startsWith(this.namePrefix) - } - /** Kill a session by name. No-op if it doesn't exist. */ async killSession(name: string): Promise { if (!(await this.sessionExists(name))) return diff --git a/src/renderer/src/workspace/hook/actions/session.ts b/src/renderer/src/workspace/hook/actions/session.ts index 0ad632f02..d4cfcba9e 100644 --- a/src/renderer/src/workspace/hook/actions/session.ts +++ b/src/renderer/src/workspace/hook/actions/session.ts @@ -129,7 +129,7 @@ export type SessionWakeOptions = { export async function killSessionBackendIfOwned( refs: WorkspaceRefs, sessionId: SessionId, - capturedOwner?: Pick, + capturedOwner?: Pick, ): Promise { // Spawn cleanup may run before React refreshes stateRef. Its caller already // knows the exact scope it just created; main still performs the atomic @@ -145,10 +145,6 @@ export async function killSessionBackendIfOwned( kind: meta.kind, providerRuntime: meta.providerRuntime, cwd: meta.cwd, - // Only a terminal has one, and main only acts on it when the pane has no - // live backend to tear down instead (#1030 item 4): a hibernated - // terminal's tmux session is otherwise orphaned until the next launch. - ...(meta.tmuxName ? { tmuxName: meta.tmuxName } : {}), }) } diff --git a/src/renderer/src/workspace/hook/ipc/useWorkspaceAdoption.ts b/src/renderer/src/workspace/hook/ipc/useWorkspaceAdoption.ts index c4de66b13..721622da6 100644 --- a/src/renderer/src/workspace/hook/ipc/useWorkspaceAdoption.ts +++ b/src/renderer/src/workspace/hook/ipc/useWorkspaceAdoption.ts @@ -121,7 +121,21 @@ export function useWorkspaceAdoption( return } - const adoption = adoptWorkspace(refs.latestStateRef.current, incoming) + // WHY the migration is inside the same guard as a JSON failure (#1048 + // Codex review): adoptWorkspace migrates the incoming document, and that + // now THROWS on a corrupt project container rather than silently + // producing an empty pool. An escaping exception was only logged by the + // callers, so main — which has already transferred session routing here + // and recorded a pending bequest — never heard a refusal, and those + // sessions stayed pinned to a window that would never display them. + let adoption: ReturnType + try { + adoption = adoptWorkspace(refs.latestStateRef.current, incoming) + } catch (err) { + console.warn('[workspace] unmigratable adoption payload:', err) + await window.api.refuseWorkspaceAdoption(windowId) + return + } if (!adoption.ok) { // Refusing tells main to leave the slice on disk AND to roll back the // session routing it moved here optimistically. Staying silent would diff --git a/src/renderer/src/workspace/workspaceShape.test.ts b/src/renderer/src/workspace/workspaceShape.test.ts index 8a87c4c7d..3980decc0 100644 --- a/src/renderer/src/workspace/workspaceShape.test.ts +++ b/src/renderer/src/workspace/workspaceShape.test.ts @@ -603,4 +603,23 @@ describe('migrateWorkspaceToStage — a corrupt container is never an empty work expect(migrated.projects).toEqual([expect.objectContaining({ id: 'recovered-project' })]) expect(migrated.sessions[sessionId as SessionId]).toMatchObject({ projectId: 'recovered-project' }) }) + + it('keeps them when the buried row is ALSO still listed in sessions (#1048 review)', () => { + // The common shape, and the one the first fix missed: v2 buries a pane + // without removing its `sessions` row, so the buried record carries no + // metadata copy — and keying the recovery on that copy dropped exactly + // these rows. + const recorded = recordedLiveWorkspace() + const [sessionId, meta] = Object.entries(recorded.sessions)[0]! + const migrated = migrateWorkspaceToStage({ + ...recorded, + tabs: [], + sessions: { [sessionId]: meta }, + buried: [{ + id: 'buried-1', sessionId: sessionId as SessionId, sessionMeta: meta, buriedAt: 1, + sourceTabId: recorded.tabs![0]!.id, sourceTabTitle: 'gone', sourceTabIndex: 0, + } as never], + }, () => 'recovered-project' as TabId) + expect(migrated.sessions[sessionId as SessionId]).toMatchObject({ projectId: 'recovered-project' }) + }) }) diff --git a/src/renderer/src/workspace/workspaceShape.ts b/src/renderer/src/workspace/workspaceShape.ts index 01022f864..9c98ce8aa 100644 --- a/src/renderer/src/workspace/workspaceShape.ts +++ b/src/renderer/src/workspace/workspaceShape.ts @@ -171,10 +171,21 @@ export function migrateWorkspaceToStage( // existed, deleted on upgrade. Mint one project to receive them instead. // Only for files that genuinely have parked rows; an empty file stays empty. if (projects.length === 0) { - const rehomed = [...legacy.values()].find(membership => membership.restoredMeta) + // WHY membership rather than `restoredMeta` (#1048 Codex review): + // `restoredMeta` is set ONLY when the metadata is missing from `sessions`, + // because it exists to carry the copy a buried record holds. The common + // case is the opposite — the row is buried AND still listed in `sessions` + // — and keying the mint on `restoredMeta` dropped exactly those rows, + // which is the bug this clause exists to fix. Any parked membership at + // all is enough; the metadata is then resolved from either source, as the + // loop below already does. + const rehomed = [...legacy.entries()].find(([sessionId, membership]) => + membership.restoredMeta !== undefined || hasSessionMeta(persisted.sessions ?? {}, sessionId)) if (rehomed) { + const [sessionId, membership] = rehomed + const meta = membership.restoredMeta ?? (persisted.sessions ?? {})[sessionId] const id = mintProjectId() - const cwd = rehomed.restoredMeta?.cwd + const cwd = meta?.cwd projects.push({ id, title: typeof cwd === 'string' ? titleFromCwd(cwd) : '', ...(typeof cwd === 'string' ? { cwd } : {}) }) projectIds.add(id) activeProjectId = id diff --git a/src/shared/types/session.ts b/src/shared/types/session.ts index 569c19fb9..49277ccba 100644 --- a/src/shared/types/session.ts +++ b/src/shared/types/session.ts @@ -119,20 +119,7 @@ export type SessionRecoverOptions = { export type SessionOwnershipOptions = Pick< SessionRecoverOptions, 'sessionId' | 'kind' | 'providerRuntime' | 'cwd' -> & { - /** - * The tmux session this pane owns, for a TERMINAL the caller may be closing - * while it is hibernated (#1030 item 4). - * - * WHY the caller has to supply it: a hibernated terminal has no row in main - * — boot hibernates everything but the focused lane — so a close finds - * nothing to tear down and the tmux session survived until the next - * launch's sweep. The renderer's persisted metadata is the only place that - * name still exists. Main does not trust it blindly: it kills the name only - * when the registry minted it (`ownsSessionName`). - */ - tmuxName?: string -} +> export type SessionRecoveryCancellationOptions = SessionOwnershipOptions & { recoveryToken: string From 6416fa70b735ba1dd8bf1bfc78d55631c78b4d09 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 20:26:22 -0700 Subject: [PATCH 3/3] fix(workspace): mint a recovery project only for rows that can live in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review finding. The recovery mint fired for ANY parked membership, but a minted project only ever receives rows that re-parent into it — and only a membership whose own project is gone (`projectId: null`) does that. A membership naming a live v2 tab keeps that name, which is not a project id, so it is dropped further down no matter what we mint. The shape that exposed it is a hybrid file: `projects: []` beside stale v2 tabs and their sessions. Migration minted a project, then dropped every session, leaving an empty nameless project — and a file with one project no longer looks empty to bootstrap, so the user lost the first-run path as well. The new test builds that hybrid from the recorded live workspace and fails without the restriction. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/workspace/workspaceShape.test.ts | 16 ++++++++++++++++ src/renderer/src/workspace/workspaceShape.ts | 14 +++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/workspace/workspaceShape.test.ts b/src/renderer/src/workspace/workspaceShape.test.ts index 3980decc0..d4849e486 100644 --- a/src/renderer/src/workspace/workspaceShape.test.ts +++ b/src/renderer/src/workspace/workspaceShape.test.ts @@ -622,4 +622,20 @@ describe('migrateWorkspaceToStage — a corrupt container is never an empty work }, () => 'recovered-project' as TabId) expect(migrated.sessions[sessionId as SessionId]).toMatchObject({ projectId: 'recovered-project' }) }) + + it('mints nothing for rows that cannot be re-homed into it (#1048 re-review)', () => { + // A hybrid file: `projects: []` (so nothing is derived from the v2 tabs) + // beside those tabs and their sessions. Those memberships still name their + // old TAB ids, which are not project ids, so every one of them is dropped + // further down whatever we mint. Minting anyway left an empty, nameless + // phantom project — and, worse, a file with one project no longer looks + // empty to bootstrap, so the user lost the first-run path too. + const recorded = recordedLiveWorkspace() + const migrated = migrateWorkspaceToStage( + { ...recorded, projects: [] } as PersistedWorkspace, + () => 'phantom-project' as TabId, + ) + expect(migrated.projects).toEqual([]) + expect(Object.keys(migrated.sessions)).toEqual([]) + }) }) diff --git a/src/renderer/src/workspace/workspaceShape.ts b/src/renderer/src/workspace/workspaceShape.ts index 9c98ce8aa..30e09aab5 100644 --- a/src/renderer/src/workspace/workspaceShape.ts +++ b/src/renderer/src/workspace/workspaceShape.ts @@ -179,8 +179,20 @@ export function migrateWorkspaceToStage( // which is the bug this clause exists to fix. Any parked membership at // all is enough; the metadata is then resolved from either source, as the // loop below already does. + // + // WHY `projectId === null` and not "any membership" (#1048 re-review): a + // minted project only ever receives rows that RE-PARENT into it, and only + // a membership whose own project is gone does that (`null` is exactly that + // state; a membership naming a live v2 tab keeps that name and is dropped + // below when the name is not a project). A hybrid file — `projects: []` + // beside stale v2 tabs — satisfied the looser test, so migration minted a + // project, then dropped every session because their tab ids still were not + // project ids. The result was an empty phantom project, which also hid the + // file from bootstrap's empty-workspace fallback: the user got a nameless + // project instead of the first-run path. const rehomed = [...legacy.entries()].find(([sessionId, membership]) => - membership.restoredMeta !== undefined || hasSessionMeta(persisted.sessions ?? {}, sessionId)) + (membership.projectId ?? null) === null + && (membership.restoredMeta !== undefined || hasSessionMeta(persisted.sessions ?? {}, sessionId))) if (rehomed) { const [sessionId, membership] = rehomed const meta = membership.restoredMeta ?? (persisted.sessions ?? {})[sessionId]