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.
This commit is contained in:
Bohdan Triapitsyn
2026-08-04 01:50:14 +03:00
parent b6c58df949
commit 3aeca4893e
7 changed files with 32 additions and 26 deletions
+5 -1
View File
@@ -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,
};
+2 -2
View File
@@ -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.
@@ -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
+3 -5
View File
@@ -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
+8 -6
View File
@@ -123,13 +123,15 @@ export function getAllSyncSessionMap(): ReadonlyMap<string, State["session"][num
}
/**
* Directory of the child store that actually holds this session.
* Directory of a child store that holds this session.
*
* This is the authoritative sessiondirectory mapping: a session is present in
* exactly the store for the directory it belongs to, regardless of whether the
* server populated `session.directory` on the record itself. Returns `null`
* when no initialized child store contains the session, which means "unknown",
* never "no directory".
* This reports containment, not ownership, and a session can be held by more
* than one store: 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. Ownership comes from the session record's own
* directory; this is the fallback for a record that carries none. Returns
* `null` when no initialized child store contains the session, which means
* "unknown", never "no directory".
*/
export function getSyncSessionDirectory(sessionId: string): string | null {
if (!sessionId) return null
+4 -3
View File
@@ -28,9 +28,10 @@ export const resolveControlTimeoutMs = (input, options) => {
}
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 = {}) => {
+5 -6
View File
@@ -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);
});
});