diff --git a/packages/ui/src/lib/debug.ts b/packages/ui/src/lib/debug.ts index cece345d..a455d61c 100644 --- a/packages/ui/src/lib/debug.ts +++ b/packages/ui/src/lib/debug.ts @@ -441,7 +441,11 @@ export const debugUtils = { const sources = { attachment, worktreeMetadata, - authoritative: owningStoreDirectory ?? recordDirectory, + // Record first, matching the resolver: holding a session proves + // containment, not ownership, so the parent repository holds its + // worktrees' sessions too. Reporting membership first made this + // diagnostic contradict the routing it exists to explain. + authoritative: recordDirectory ?? owningStoreDirectory, selected, remembered: remembered.runtime, }; diff --git a/packages/ui/src/sync/DOCUMENTATION.md b/packages/ui/src/sync/DOCUMENTATION.md index 96761339..04dd3088 100644 --- a/packages/ui/src/sync/DOCUMENTATION.md +++ b/packages/ui/src/sync/DOCUMENTATION.md @@ -207,7 +207,7 @@ The discriminator is whether the server confirmed the path, not whether the valu | Source | Meaning | |---|---| -| `authoritative` | The child store that actually holds the session, then its own record | +| `authoritative` | The session record's own directory, then a child store that holds it | | `selected` | Server-confirmed directory captured at selection; a guessed one is never passed | | `attachment` | Worktree attachment recorded by this client; the *requested* path | | `worktree-metadata` | Worktree captured when the session was created in one; the *requested* path | @@ -215,7 +215,7 @@ The discriminator is whether the server confirmed the path, not whether the valu Rules: -1. `getSyncSessionDirectory()` is the authoritative session→directory mapping: a session lives in exactly the child store for its directory, whether or not the server populated `session.directory`. `null` means "not indexed yet", never "no directory". +1. Ownership comes from the session record's own `directory`. `getSyncSessionDirectory()` reports *containment*, not ownership, and is only the fallback for a record without a directory: a project's session list includes the sessions of its worktrees so the sidebar can group them, so the parent repository holds worktree sessions too, and reading ownership from membership routes a worktree session to its parent. `null` means "not indexed yet", never "no directory". 2. `attachment` and `worktreeMetadata` hold the worktree path this client asked for, before the server canonicalized it. They are a hint for a session sync has not indexed yet, never a correction of a confirmed directory — otherwise a stale local path re-creates the very mismatch this precedence exists to prevent. 3. Never persist or rank a guessed directory. `selectSession` may fall back to the active directory to keep routing usable, but that value is not written to runtime memory, not written to the last-active snapshot, and not passed as `selected` — a persisted guess outlives the race that produced it and survives reloads and restarts. 4. Components must not read `currentSessionDirectory` to build request or queue keys; use `getDirectoryForSession()` so every consumer resolves identically. diff --git a/packages/ui/src/sync/session-directory-adoption.test.ts b/packages/ui/src/sync/session-directory-adoption.test.ts new file mode 100644 index 00000000..4857872c --- /dev/null +++ b/packages/ui/src/sync/session-directory-adoption.test.ts @@ -0,0 +1,98 @@ +import { beforeEach, describe, expect, test } from "bun:test" + +import { ChildStoreManager } from "./child-store" +import { setSyncRefs } from "./sync-refs" +import { useSessionUIStore } from "./session-ui-store" + +/** + * Selecting a session whose directory this client has not indexed yet routes it + * through the active directory as a deliberate guess. Nothing used to settle + * that guess once the owning directory finished bootstrapping, so every fetch + * stayed addressed to a directory that does not own the session and the session + * never rendered. + * + * These tests pin both directions: a guess is promoted once the authoritative + * directory becomes readable, and a confirmed selection is never rewritten. + */ + +const PARENT = "/repo" +const WORKTREE = "/repo/.worktrees/feature" +const SESSION_ID = "ses_directory_adoption" + +const indexSessionIn = ( + manager: ChildStoreManager, + directory: string, + recordDirectory: string = directory, +): void => { + const store = manager.ensureChild(directory, { bootstrap: false }) + store.setState({ + session: [{ id: SESSION_ID, directory: recordDirectory, title: "test" } as never], + }) +} + +let manager: ChildStoreManager + +beforeEach(() => { + manager = new ChildStoreManager() + setSyncRefs({} as never, manager, PARENT) + useSessionUIStore.getState().setCurrentSession(null) +}) + +describe("adoptAuthoritativeSessionDirectory", () => { + test("promotes a guessed selection once the owning directory is indexed", () => { + useSessionUIStore.getState().setCurrentSession(SESSION_ID) + expect(useSessionUIStore.getState().currentSessionDirectory).not.toBe(WORKTREE) + + indexSessionIn(manager, WORKTREE) + useSessionUIStore.getState().adoptAuthoritativeSessionDirectory() + + expect(useSessionUIStore.getState().currentSessionDirectory).toBe(WORKTREE) + }) + + test("believes the session record over the store that merely holds it", () => { + // A project's session list includes the sessions of its worktrees so the + // sidebar can group them, so the parent store holds this session while the + // session itself reports the worktree. Ownership comes from the record. + useSessionUIStore.getState().setCurrentSession(SESSION_ID) + indexSessionIn(manager, PARENT, WORKTREE) + + useSessionUIStore.getState().adoptAuthoritativeSessionDirectory() + + expect(useSessionUIStore.getState().currentSessionDirectory).toBe(WORKTREE) + }) + + test("does nothing while the owning directory is still unknown", () => { + useSessionUIStore.getState().setCurrentSession(SESSION_ID) + const before = useSessionUIStore.getState().currentSessionDirectory + + useSessionUIStore.getState().adoptAuthoritativeSessionDirectory() + + expect(useSessionUIStore.getState().currentSessionDirectory).toBe(before) + }) + + test("never rewrites a selection that was confirmed at selection time", () => { + useSessionUIStore.getState().setCurrentSession(SESSION_ID, WORKTREE) + expect(useSessionUIStore.getState().currentSessionDirectory).toBe(WORKTREE) + + // A different directory claiming the session must not move a confirmed + // selection: the confirmed value outranks anything sync learns later. + indexSessionIn(manager, PARENT) + useSessionUIStore.getState().adoptAuthoritativeSessionDirectory() + + expect(useSessionUIStore.getState().currentSessionDirectory).toBe(WORKTREE) + }) + + test("is a no-op for a session that is no longer selected", () => { + useSessionUIStore.getState().setCurrentSession(SESSION_ID) + indexSessionIn(manager, WORKTREE) + useSessionUIStore.getState().setCurrentSession("ses_other") + + // Whatever the new selection resolved to, a late adoption for the previous + // session must not touch it. + const before = useSessionUIStore.getState().currentSessionDirectory + useSessionUIStore.getState().adoptAuthoritativeSessionDirectory(SESSION_ID) + + expect(useSessionUIStore.getState().currentSessionId).toBe("ses_other") + expect(useSessionUIStore.getState().currentSessionDirectory).toBe(before) + }) +}) diff --git a/packages/ui/src/sync/session-directory-resolution.ts b/packages/ui/src/sync/session-directory-resolution.ts index d8069176..616c4aa7 100644 --- a/packages/ui/src/sync/session-directory-resolution.ts +++ b/packages/ui/src/sync/session-directory-resolution.ts @@ -13,8 +13,10 @@ * The ordering discriminator is **whether the server confirmed the path**, not * whether the value is local or synced: * - * 1. `authoritative` — the child store that actually holds the session, then - * the session's own record. Server-backed truth for an indexed session. + * 1. `authoritative` — the session's own record, then a child store that holds + * it. Server-backed truth for an indexed session. Record first because + * holding a session proves containment, not ownership: a project's session + * list includes its worktrees' sessions so the sidebar can group them. * 2. `selected` — the directory captured when the session was selected, but * only when it came from a server response (the directory `createSession` * returned, which may be a canonicalized form of what was requested). A @@ -42,7 +44,7 @@ export type SessionDirectorySource = | 'none' export type SessionDirectorySources = { - /** Directory of the child store that holds the session, or its own record. */ + /** The session record's own directory, or a store that holds it. */ authoritative?: string | null /** Server-confirmed directory captured at selection. Never a guessed one. */ selected?: string | null diff --git a/packages/ui/src/sync/session-ui-store.ts b/packages/ui/src/sync/session-ui-store.ts index 4f65e98b..dbef659c 100644 --- a/packages/ui/src/sync/session-ui-store.ts +++ b/packages/ui/src/sync/session-ui-store.ts @@ -352,6 +352,12 @@ export type SessionUIState = { debugSessionMessages: (sessionId: string) => Promise pollForTokenUpdates: () => void setSessionDirectory: (sessionId: string, directory: string | null) => void + /** + * Replace a guessed selection directory with the authoritative one once sync + * has indexed the session. Safe to call at any time: it only ever promotes a + * guess, never overrides a confirmed selection. + */ + adoptAuthoritativeSessionDirectory: (sessionId?: string) => void } // --------------------------------------------------------------------------- @@ -401,15 +407,26 @@ const getAttachmentForSession = (sessionId: string | null | undefined): SessionW } /** - * Authoritative directory for a session: the child store that holds it, and - * only then the session record's own fields. `null` means "not indexed yet", - * never "no directory" — callers must fall back rather than treat it as empty. + * The directory that owns a session, from the two server-backed signals. + * + * `null` means "not indexed yet", never "no directory" — callers must fall back + * rather than treat it as empty. + * + * The session's own record wins. Holding a session in a child store proves + * containment, not ownership: a project's session list legitimately includes + * the sessions of its worktrees so the sidebar can group them, so the parent + * repository holds worktree sessions too. Reading ownership from store + * membership therefore reports the parent for a session that lives in a + * worktree, and every fetch is then addressed to a directory that does not own + * it. Store membership remains the fallback for a session whose record carries + * no directory. */ const getAuthoritativeSessionDirectory = (sessionId: string): string | null => { - const owningDirectory = getSyncSessionDirectory(sessionId) - if (owningDirectory) return normalizePath(owningDirectory) const target = getAllSyncSessions().find((s) => s.id === sessionId) - return target ? resolveDirectoryKey(target) : null + const recordDirectory = target ? resolveDirectoryKey(target) : null + if (recordDirectory) return normalizePath(recordDirectory) + const owningDirectory = getSyncSessionDirectory(sessionId) + return owningDirectory ? normalizePath(owningDirectory) : null } /** @@ -1739,6 +1756,26 @@ export const useSessionUIStore = create()((set, get) => ({ // Handled by sync system's SSE stream }, + adoptAuthoritativeSessionDirectory: (sessionId) => { + const target = sessionId ?? get().currentSessionId + // Only a guess is promoted. A confirmed selection outranks anything sync + // learns later, and a selection that has since moved on must not be + // rewritten by a directory that finished bootstrapping in the background. + if (!target || target !== guessedSelectionSessionId) return + if (target !== get().currentSessionId) return + + const authoritative = getAuthoritativeSessionDirectory(target) + if (!authoritative) return + + // The selection stops being a guess even when the directory is unchanged: + // the value has now been confirmed by the store that owns the session. + guessedSelectionSessionId = null + if (authoritative !== get().currentSessionDirectory) { + set({ currentSessionDirectory: authoritative }) + } + writeRuntimeSessionMemory(runtimeMemoryKey(), { sessionId: target, directory: authoritative }) + }, + setSessionDirectory: (sessionId, directory) => { const normalized = normalizePath(directory) // Callers set this from a confirmed destination (a completed move, a diff --git a/packages/ui/src/sync/sync-context.tsx b/packages/ui/src/sync/sync-context.tsx index 9e1da2b4..b4ff6c19 100644 --- a/packages/ui/src/sync/sync-context.tsx +++ b/packages/ui/src/sync/sync-context.tsx @@ -34,6 +34,7 @@ import { countSyncPerformance } from "./performance-diagnostics" import { runBackgroundNetworkTask } from "@/lib/background-network" import { setActionRefs } from "./session-actions" import { setSyncRefs, getAllSyncSessions } from "./sync-refs" +import { useSessionUIStore } from "./session-ui-store" import { stripSessionDiffSnapshots } from "./sanitize" import { applySessionEventToGlobalSessions } from "./session-event-router" import { syncDebug } from "./debug" @@ -1949,6 +1950,16 @@ export function SyncProvider(props: { const result = await runBootstrap(0) if (result === "failed") throw new Error(`Directory bootstrap failed for ${directory}`) + + // Selecting a session whose directory this client had not indexed yet + // routes it through the active directory as a documented guess. This is + // the moment that guess can be settled: the owning store now holds the + // session, so the authoritative directory is finally readable. Without + // this the guess survives, every fetch is addressed to a directory that + // does not own the session, and the session never renders. + if (result === "complete") { + useSessionUIStore.getState().adoptAuthoritativeSessionDirectory() + } }, onDispose: (directory) => { messageLoader.invalidateDirectory(directory) diff --git a/packages/ui/src/sync/sync-refs.ts b/packages/ui/src/sync/sync-refs.ts index 0d96911c..ccc236f2 100644 --- a/packages/ui/src/sync/sync-refs.ts +++ b/packages/ui/src/sync/sync-refs.ts @@ -123,13 +123,15 @@ export function getAllSyncSessionMap(): ReadonlyMap { const DEFAULT_WAIT_TIMEOUT_SECONDS = 600; const WAIT_HTTP_TIMEOUT_BUFFER_MS = 30_000; +// Provisioning a worktree is not one of the instant control calls the short +// default is sized for: it runs git against the repository and prepares a new +// directory, which on a cold path takes longer than the default allows. The +// server finishes the work regardless of the client giving up, so a client-side +// timeout here reported a failure for a worktree that was in fact created. +const WORKTREE_PROVISION_TIMEOUT_MS = 120_000; + // The control service blocks server-side while wait is set, so the client // HTTP timeout must outlive the requested wait window instead of the short // default used for instant control calls. export const resolveControlTimeoutMs = (input, options) => { if (Number.isFinite(options?.timeoutMs) && options.timeoutMs > 0) return options.timeoutMs; - if (input?.wait !== true) return undefined; + const provisionsWorktree = asNonEmptyString(input?.worktree) !== null; + if (input?.wait !== true) { + return provisionsWorktree ? WORKTREE_PROVISION_TIMEOUT_MS : undefined; + } const waitSeconds = Number(input?.timeout) > 0 ? Number(input.timeout) : DEFAULT_WAIT_TIMEOUT_SECONDS; - return (waitSeconds * 1000) + WAIT_HTTP_TIMEOUT_BUFFER_MS; + const waitTimeoutMs = (waitSeconds * 1000) + WAIT_HTTP_TIMEOUT_BUFFER_MS; + // The server provisions the worktree inside session creation, before it + // starts waiting for the session to go idle, so the two windows run in + // sequence rather than overlapping. The client window has to cover both. + return provisionsWorktree ? waitTimeoutMs + WORKTREE_PROVISION_TIMEOUT_MS : waitTimeoutMs; }; export const requestControlAction = async (port, action, input, options = {}) => { diff --git a/packages/web/bin/lib/cli-control.test.js b/packages/web/bin/lib/cli-control.test.js index 0e259e58..5b19853e 100644 --- a/packages/web/bin/lib/cli-control.test.js +++ b/packages/web/bin/lib/cli-control.test.js @@ -19,4 +19,19 @@ describe('resolveControlTimeoutMs', () => { it('never shrinks an explicitly requested HTTP timeout', () => { expect(resolveControlTimeoutMs({ wait: true, timeout: 30 }, { timeoutMs: 5000 })).toBe(5000); }); + + it('allows a worktree to be provisioned without waiting for the session', () => { + expect(resolveControlTimeoutMs({ worktree: 'feature' }, {})).toBe(120_000); + }); + + it('ignores a blank worktree name', () => { + expect(resolveControlTimeoutMs({ worktree: ' ' }, {})).toBeUndefined(); + }); + + it('covers provisioning and waiting in sequence when both are requested', () => { + // The server creates the worktree before it begins waiting for the session, + // so the client window must span both rather than the longer of the two. + expect(resolveControlTimeoutMs({ wait: true, timeout: 30, worktree: 'feature' }, {})).toBe(180_000); + expect(resolveControlTimeoutMs({ wait: true, worktree: 'feature' }, {})).toBe(750_000); + }); });