From a35ff1032c9b54380a3dc5412bff79b13a51c646 Mon Sep 17 00:00:00 2001 From: mattv8 Date: Tue, 25 Aug 2026 09:57:23 -0600 Subject: [PATCH] fix(sessions): avoid staged-change rollback conflicts --- .../session/sidebar/DOCUMENTATION.md | 6 +- .../lib/worktrees/sessionWorktreeMove.test.ts | 197 +++++++++++------- .../src/lib/worktrees/sessionWorktreeMove.ts | 27 ++- 3 files changed, 136 insertions(+), 94 deletions(-) diff --git a/packages/ui/src/components/session/sidebar/DOCUMENTATION.md b/packages/ui/src/components/session/sidebar/DOCUMENTATION.md index 874da7e0..1a77301e 100644 --- a/packages/ui/src/components/session/sidebar/DOCUMENTATION.md +++ b/packages/ui/src/components/session/sidebar/DOCUMENTATION.md @@ -16,8 +16,10 @@ kept at this root in `types.ts` and `utils.tsx`. target disabled and a separate `New worktree...` action. Opening the submenu refreshes the worktree topology. Moving transfers the full idle subtree. Clean and non-Git sources move session-only; a dirty Git source prompts to move only - the session, move all source changes, or cancel. Only the root session carries - source changes during a subtree move. + the session, move all source changes, or cancel. Descendants move first without + changes and roll back session-only if a later descendant fails. The root moves + last and carries source changes once, which prevents rollback from replaying the + transferred patch into the source. `MainLayout` and `VSCodeLayout` call `useSessionListSync({ isVSCode })` unconditionally. The hook publishes complete directory bootstrap demand, diff --git a/packages/ui/src/lib/worktrees/sessionWorktreeMove.test.ts b/packages/ui/src/lib/worktrees/sessionWorktreeMove.test.ts index 125a5c93..07485e59 100644 --- a/packages/ui/src/lib/worktrees/sessionWorktreeMove.test.ts +++ b/packages/ui/src/lib/worktrees/sessionWorktreeMove.test.ts @@ -1,5 +1,9 @@ import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test'; import type { Session, SessionStatus } from '@opencode-ai/sdk/v2'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import type { State } from '@/sync/types'; import type { WorktreeMetadata } from '@/types/worktree'; import type { ProjectRef } from '@/lib/worktrees/worktreeManager'; @@ -54,6 +58,7 @@ const toastSuccesses: string[] = []; const toastErrors: Array<{ title: string; description?: string }> = []; const directoryStates = new Map(); const storedMetadata = new Map(); +const tempDirectories: string[] = []; const originalConsoleWarn = console.warn; type SessionUIState = { availableWorktrees: WorktreeMetadata[]; @@ -281,6 +286,33 @@ const deferred = (): DeferredVoid => { return { promise, resolve, reject }; }; +const runGit = (directory: string, args: string[], input?: string): string => + execFileSync('git', args, { + cwd: directory, + encoding: 'utf8', + input, + stdio: ['pipe', 'pipe', 'pipe'], + }); + +const createStagedChangeWorktrees = () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'openchamber-staged-move-')); + tempDirectories.push(root); + const source = path.join(root, 'source'); + const destination = path.join(root, 'destination'); + fs.mkdirSync(source); + runGit(source, ['init', '-b', 'main']); + runGit(source, ['config', 'user.email', 'test@example.com']); + runGit(source, ['config', 'user.name', 'Test']); + runGit(source, ['config', 'core.autocrlf', 'false']); + fs.writeFileSync(path.join(source, 'file.txt'), 'base\n'); + runGit(source, ['add', 'file.txt']); + runGit(source, ['commit', '--no-gpg-sign', '-m', 'init']); + runGit(source, ['worktree', 'add', '--detach', destination, 'HEAD']); + fs.writeFileSync(path.join(source, 'file.txt'), 'staged\n'); + runGit(source, ['add', 'file.txt']); + return { source, destination }; +}; + const getIncompleteRollbackCause = (error: Error): IncompleteRollbackCause => { const cause = error.cause; if (!cause || !(cause instanceof Object)) { @@ -348,9 +380,12 @@ describe('moveSessionTreeToExistingWorktree', () => { afterEach(() => { console.warn = originalConsoleWarn; + for (const directory of tempDirectories.splice(0)) { + fs.rmSync(directory, { recursive: true, force: true }); + } }); - test('moves the root before descendants, only transfers changes once, and refreshes both directories', async () => { + test('moves descendants before the root, only transfers changes once, and refreshes both directories', async () => { const root = makeSession('root'); const child = makeSession('child'); const previousRootMetadata = makeWorktreeMetadata({ path: '/old-root', label: 'Old root' }); @@ -370,12 +405,12 @@ describe('moveSessionTreeToExistingWorktree', () => { expect(result).toBe('/destination'); expect(moveCalls).toEqual([ - { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: true }, { sessionId: 'child', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, + { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: true }, ]); expect(metadataWrites).toEqual([ - { sessionId: 'root', metadata: latestMetadataResult }, { sessionId: 'child', metadata: latestMetadataResult }, + { sessionId: 'root', metadata: latestMetadataResult }, ]); expect(refreshCalls).toEqual([['/source', '/destination']]); expect(removeWorktreeCalls).toEqual([]); @@ -498,17 +533,13 @@ describe('moveSessionTreeToExistingWorktree', () => { })).rejects.toThrow('child-b failed'); expect(moveCalls).toEqual([ - { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: true }, { sessionId: 'child-a', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, { sessionId: 'child-b', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, { sessionId: 'child-a', sourceDirectory: '/destination', destinationDirectory: '/source', moveChanges: false }, - { sessionId: 'root', sourceDirectory: '/destination', destinationDirectory: '/source', moveChanges: true }, ]); expect(metadataWrites).toEqual([ - { sessionId: 'root', metadata: latestMetadataResult }, { sessionId: 'child-a', metadata: latestMetadataResult }, { sessionId: 'child-a', metadata: previousChildAMetadata }, - { sessionId: 'root', metadata: previousRootMetadata }, ]); expect(storedMetadata.get(root.id)).toBe(previousRootMetadata); expect(storedMetadata.get(childA.id)).toBe(previousChildAMetadata); @@ -517,67 +548,104 @@ describe('moveSessionTreeToExistingWorktree', () => { expect(refreshCalls).toEqual([]); }); - test('rolls back the root and never moves a child that becomes busy after the root move starts', async () => { + test('does not replay transferred staged changes when a descendant move fails', async () => { + const { source, destination } = createStagedChangeWorktrees(); + const root = makeSession('root', source); + const child = makeSession('child', source); + setStatuses(source, { root: 'idle', child: 'idle' }); + moveSessionImplementation = async (session, sourceDirectory, destinationDirectory, moveChanges) => { + if (session.id === 'child' && sourceDirectory === source) { + throw new Error('child failed'); + } + if (!moveChanges) return; + + const patch = runGit(sourceDirectory, ['diff', '--binary', 'HEAD']); + runGit(destinationDirectory, ['apply', '-'], patch); + runGit(sourceDirectory, ['checkout', '--', 'file.txt']); + }; + + const error = await moveSessionTreeToExistingWorktree({ + root, + descendants: [child], + sourceDirectory: source, + destination: makeWorktreeMetadata({ path: destination }), + moveChanges: true, + }).catch((rejection) => rejection); + + expect(error).toEqual(new Error('child failed')); + expect(moveCalls).toEqual([ + { sessionId: 'child', sourceDirectory: source, destinationDirectory: destination, moveChanges: false }, + ]); + expect(runGit(source, ['status', '--short'])).toBe('M file.txt\n'); + expect(fs.readFileSync(path.join(destination, 'file.txt'), 'utf8')).toBe('base\n'); + }); + + test('rolls back an earlier child and never moves a later descendant that becomes busy', async () => { const root = makeSession('root'); - const child = makeSession('child'); - const rootMove = deferred(); + const childA = makeSession('child-a'); + const childB = makeSession('child-b'); + const childAMove = deferred(); const previousRootMetadata = makeWorktreeMetadata({ path: '/old-root', label: 'Old root' }); - const previousChildMetadata = makeWorktreeMetadata({ path: '/old-child', label: 'Old child' }); - setStatuses('/source', { root: 'idle', child: 'idle' }); + const previousChildAMetadata = makeWorktreeMetadata({ path: '/old-child-a', label: 'Old child A' }); + const previousChildBMetadata = makeWorktreeMetadata({ path: '/old-child-b', label: 'Old child B' }); + setStatuses('/source', { root: 'idle', 'child-a': 'idle', 'child-b': 'idle' }); setStatuses('/destination', {}); storedMetadata.set(root.id, previousRootMetadata); - storedMetadata.set(child.id, previousChildMetadata); + storedMetadata.set(childA.id, previousChildAMetadata); + storedMetadata.set(childB.id, previousChildBMetadata); moveSessionImplementation = async (session, sourceDirectory) => { - if (session.id === 'root' && sourceDirectory === '/source') { - return rootMove.promise; + if (session.id === 'child-a' && sourceDirectory === '/source') { + return childAMove.promise; } }; const movePromise = moveSessionTreeToExistingWorktree({ root, - descendants: [child], + descendants: [childA, childB], sourceDirectory: '/source', destination: makeWorktreeMetadata(), moveChanges: true, }); await waitFor(() => moveCalls.length === 1); - setStatuses('/source', { root: 'idle', child: 'busy' }); - setStatuses('/destination', { root: 'idle' }); - rootMove.resolve(); + setStatuses('/source', { root: 'idle', 'child-a': 'idle', 'child-b': 'busy' }); + setStatuses('/destination', { 'child-a': 'idle' }); + childAMove.resolve(); await expect(movePromise).rejects.toThrow('Session is not idle'); expect(moveCalls).toEqual([ - { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: true }, - { sessionId: 'root', sourceDirectory: '/destination', destinationDirectory: '/source', moveChanges: true }, + { sessionId: 'child-a', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, + { sessionId: 'child-a', sourceDirectory: '/destination', destinationDirectory: '/source', moveChanges: false }, ]); expect(metadataWrites).toEqual([ - { sessionId: 'root', metadata: latestMetadataResult }, - { sessionId: 'root', metadata: previousRootMetadata }, + { sessionId: 'child-a', metadata: latestMetadataResult }, + { sessionId: 'child-a', metadata: previousChildAMetadata }, ]); expect(storedMetadata.get(root.id)).toBe(previousRootMetadata); - expect(storedMetadata.get(child.id)).toBe(previousChildMetadata); + expect(storedMetadata.get(childA.id)).toBe(previousChildAMetadata); + expect(storedMetadata.get(childB.id)).toBe(previousChildBMetadata); expect(removeWorktreeCalls).toEqual([]); expect(refreshCalls).toEqual([]); }); test('reports an incomplete rollback explicitly and still does not remove the existing destination', async () => { const root = makeSession('root'); - const child = makeSession('child'); - setStatuses('/source', { root: 'idle', child: 'idle' }); + const childA = makeSession('child-a'); + const childB = makeSession('child-b'); + setStatuses('/source', { root: 'idle', 'child-a': 'idle', 'child-b': 'idle' }); moveSessionImplementation = async (session, sourceDirectory) => { - if (session.id === 'child' && sourceDirectory === '/source') { - throw new Error('child failed'); + if (session.id === 'child-b' && sourceDirectory === '/source') { + throw new Error('child-b failed'); } - if (session.id === 'root' && sourceDirectory === '/destination') { + if (session.id === 'child-a' && sourceDirectory === '/destination') { throw new Error('rollback failed'); } }; const error = await moveSessionTreeToExistingWorktree({ root, - descendants: [child], + descendants: [childA, childB], sourceDirectory: '/source', destination: makeWorktreeMetadata(), moveChanges: true, @@ -589,47 +657,48 @@ describe('moveSessionTreeToExistingWorktree', () => { } expect(error.message.includes('could not be fully rolled back')).toBe(true); const cause = getIncompleteRollbackCause(error); - expect(cause.moveError.message).toBe('child failed'); - expect(cause.rollbackFailures).toEqual([{ sessionId: 'root', error: new Error('rollback failed') }]); + expect(cause.moveError.message).toBe('child-b failed'); + expect(cause.rollbackFailures).toEqual([{ sessionId: 'child-a', error: new Error('rollback failed') }]); expect(removeWorktreeCalls).toEqual([]); }); const expectBusyOrRetryRollbackBlock = async (status: Extract): Promise => { const root = makeSession('root'); - const child = makeSession('child'); - setStatuses('/source', { root: 'idle', child: 'idle' }); + const childA = makeSession('child-a'); + const childB = makeSession('child-b'); + setStatuses('/source', { root: 'idle', 'child-a': 'idle', 'child-b': 'idle' }); setStatuses('/destination', {}); moveSessionImplementation = async (session, sourceDirectory) => { - if (sourceDirectory === '/source' && session.id === 'root') { - setStatuses('/destination', { root: status }); + if (sourceDirectory === '/source' && session.id === 'child-a') { + setStatuses('/destination', { 'child-a': status }); return; } - if (sourceDirectory === '/source' && session.id === 'child') { - throw new Error('child failed'); + if (sourceDirectory === '/source' && session.id === 'child-b') { + throw new Error('child-b failed'); } }; await expect(moveSessionTreeToExistingWorktree({ root, - descendants: [child], + descendants: [childA, childB], sourceDirectory: '/source', destination: makeWorktreeMetadata(), moveChanges: true, })).rejects.toThrow('could not be fully rolled back'); expect(moveCalls).toEqual([ - { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: true }, - { sessionId: 'child', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, + { sessionId: 'child-a', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, + { sessionId: 'child-b', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, ]); expect(removeWorktreeCalls).toEqual([]); }; - test('does not attempt rollback for a moved root that becomes busy in the destination', async () => { + test('does not attempt rollback for a moved child that becomes busy in the destination', async () => { await expectBusyOrRetryRollbackBlock('busy'); }); - test('does not attempt rollback for a moved root that becomes retry in the destination', async () => { + test('does not attempt rollback for a moved child that becomes retry in the destination', async () => { await expectBusyOrRetryRollbackBlock('retry'); }); @@ -851,18 +920,18 @@ describe('moveSessionTreeToExistingWorktree', () => { expect(getSessionTreeMoveConfirmation()).toBeNull(); expect(moveCalls).toEqual([ - { - sessionId: 'root', - sourceDirectory: '/source', - destinationDirectory: '/created-worktree', - moveChanges: true, - }, { sessionId: 'child', sourceDirectory: '/source', destinationDirectory: '/created-worktree', moveChanges: false, }, + { + sessionId: 'root', + sourceDirectory: '/source', + destinationDirectory: '/created-worktree', + moveChanges: true, + }, ]); }); @@ -891,7 +960,7 @@ describe('moveSessionTreeToExistingWorktree', () => { expect(moveCalls).toEqual([]); }); - test('uses session-only mode when rolling back a moved root', async () => { + test('does not move the root when a descendant fails in session-only mode', async () => { const root = makeSession('root'); const child = makeSession('child'); setStatuses('/source', { root: 'idle', child: 'idle' }); @@ -911,35 +980,7 @@ describe('moveSessionTreeToExistingWorktree', () => { })).rejects.toThrow('child failed'); expect(moveCalls).toEqual([ - { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, { sessionId: 'child', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, - { sessionId: 'root', sourceDirectory: '/destination', destinationDirectory: '/source', moveChanges: false }, - ]); - }); - - test('uses all-changes mode when rolling back a moved root after a full transfer', async () => { - const root = makeSession('root'); - const child = makeSession('child'); - setStatuses('/source', { root: 'idle', child: 'idle' }); - setStatuses('/destination', { root: 'idle' }); - moveSessionImplementation = async (session, sourceDirectory) => { - if (session.id === 'child' && sourceDirectory === '/source') { - throw new Error('child failed'); - } - }; - - await expect(moveSessionTreeToExistingWorktree({ - root, - descendants: [child], - sourceDirectory: '/source', - destination: makeWorktreeMetadata(), - moveChanges: true, - })).rejects.toThrow('child failed'); - - expect(moveCalls).toEqual([ - { sessionId: 'root', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: true }, - { sessionId: 'child', sourceDirectory: '/source', destinationDirectory: '/destination', moveChanges: false }, - { sessionId: 'root', sourceDirectory: '/destination', destinationDirectory: '/source', moveChanges: true }, ]); }); diff --git a/packages/ui/src/lib/worktrees/sessionWorktreeMove.ts b/packages/ui/src/lib/worktrees/sessionWorktreeMove.ts index 5ea8b8ed..4f3b842e 100644 --- a/packages/ui/src/lib/worktrees/sessionWorktreeMove.ts +++ b/packages/ui/src/lib/worktrees/sessionWorktreeMove.ts @@ -150,11 +150,9 @@ const isSessionBusyOrRetrying = (session: Session, directory: string): boolean = const rollbackMovedSessions = async ( sessions: Session[], - rootSessionId: string, sourceDirectory: string, worktreeDirectory: string, previousMetadata: ReadonlyMap, - moveChanges: boolean, ): Promise => { const failures: RollbackFailure[] = []; for (const session of [...sessions].reverse()) { @@ -163,12 +161,12 @@ const rollbackMovedSessions = async ( continue; } try { - await moveSessionToDirectory( - session, - worktreeDirectory, - sourceDirectory, - session.id === rootSessionId && moveChanges, - ); + await moveSessionToDirectory( + session, + worktreeDirectory, + sourceDirectory, + false, + ); useSessionUIStore.getState().setWorktreeMetadata(session.id, previousMetadata.get(session.id) ?? null); } catch (error) { failures.push({ @@ -212,7 +210,7 @@ const moveSessionTreeTransaction = async ( setSessionMovePending(input.root.id, true); try { - const sessions = [input.root, ...input.descendants]; + const sessions = [...input.descendants, input.root]; const previousMetadata = new Map( sessions.map((session) => [ session.id, @@ -227,27 +225,27 @@ const moveSessionTreeTransaction = async ( destination = await prepareDestination(); for (const [index, session] of sessions.entries()) { // Setup and earlier moves can take long enough for a not-yet-moved - // descendant to start running, so re-check the remaining source tree - // immediately before each move. + // session to start running, so re-check the remaining source tree + // immediately before each move. The root moves last so no later + // descendant failure can require replaying a transferred patch. assertSessionsIdle(sessions.slice(index), input.sourceDirectory); await moveSessionToDirectory( session, input.sourceDirectory, destination.directory, - index === 0 && input.moveChanges, + session.id === input.root.id && input.moveChanges, ); moved.push(session); + if (session.id === input.root.id) continue; useSessionUIStore.getState().setWorktreeMetadata(session.id, getLatestWorktreeMetadata(destination.metadata)); } } catch (error) { const moveError = error instanceof Error ? error : new Error(String(error)); const rollbackFailures = await rollbackMovedSessions( moved, - input.root.id, input.sourceDirectory, destination?.directory ?? input.sourceDirectory, previousMetadata, - input.moveChanges, ); if (rollbackFailures.length > 0) { throw createIncompleteRollbackError(moveError, rollbackFailures); @@ -257,6 +255,7 @@ const moveSessionTreeTransaction = async ( } throw moveError; } + useSessionUIStore.getState().setWorktreeMetadata(input.root.id, getLatestWorktreeMetadata(destination.metadata)); try { await refreshGlobalSessionsForDirectories([input.sourceDirectory, destination.directory]);