From 3aeca4893e380dc5c3d05db0d2307511c494c5c8 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Tue, 4 Aug 2026 01:50:14 +0300 Subject: [PATCH] docs(sync): correct the ownership precedence the fix inverted Review found the owning documentation still describing the behaviour this branch replaced, in one case stacked directly above the new docstring saying the opposite. Holding a session proves containment, not ownership, so every text that called store membership the authoritative mapping was actively misleading for the module whose wrong answer misroutes every send. Corrected in the module docstring, the resolution module's precedence description, the sync-refs helper it points at, and the sync DOCUMENTATION.md table and rules. The debug report built its authoritative value membership-first, so for exactly the scenario this branch fixes it reported the parent directory and could raise a source-disagreement alert while routing was in fact correct. It now uses the same record-first order as the resolver. The CLI timeout comment claimed the wait and provisioning windows were additive while the code took the larger of the two. The server provisions the worktree inside session creation, before it waits for the session to go idle, so they do run in sequence: the windows are now summed and the tests pin both cases. --- packages/ui/src/lib/debug.ts | 6 +++++- packages/ui/src/sync/DOCUMENTATION.md | 4 ++-- .../ui/src/sync/session-directory-resolution.ts | 8 +++++--- packages/ui/src/sync/session-ui-store.ts | 8 +++----- packages/ui/src/sync/sync-refs.ts | 14 ++++++++------ packages/web/bin/lib/cli-control.js | 7 ++++--- packages/web/bin/lib/cli-control.test.js | 11 +++++------ 7 files changed, 32 insertions(+), 26 deletions(-) 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-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 ef450f54..dbef659c 100644 --- a/packages/ui/src/sync/session-ui-store.ts +++ b/packages/ui/src/sync/session-ui-store.ts @@ -406,14 +406,12 @@ const getAttachmentForSession = (sessionId: string | null | undefined): SessionW return useSessionWorktreeStore.getState().getAttachment(sessionId) } -/** - * 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 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 waitSeconds = Number(input?.timeout) > 0 ? Number(input.timeout) : DEFAULT_WAIT_TIMEOUT_SECONDS; const waitTimeoutMs = (waitSeconds * 1000) + WAIT_HTTP_TIMEOUT_BUFFER_MS; - // Waiting for the session and provisioning its worktree are additive, so the - // window must cover whichever is longer rather than only the wait. - return provisionsWorktree ? Math.max(waitTimeoutMs, WORKTREE_PROVISION_TIMEOUT_MS) : waitTimeoutMs; + // 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 1ae93015..5b19853e 100644 --- a/packages/web/bin/lib/cli-control.test.js +++ b/packages/web/bin/lib/cli-control.test.js @@ -28,11 +28,10 @@ describe('resolveControlTimeoutMs', () => { expect(resolveControlTimeoutMs({ worktree: ' ' }, {})).toBeUndefined(); }); - it('covers worktree provisioning even when the wait window is shorter', () => { - expect(resolveControlTimeoutMs({ wait: true, timeout: 30, worktree: 'feature' }, {})).toBe(120_000); - }); - - it('keeps a longer wait window when it outlasts worktree provisioning', () => { - expect(resolveControlTimeoutMs({ wait: true, worktree: 'feature' }, {})).toBe(630_000); + 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); }); });