fix(sync): render sessions in worktrees created while the client is running #2603
This commit is contained in:
@@ -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,
|
||||
};
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
|
||||
@@ -352,6 +352,12 @@ export type SessionUIState = {
|
||||
debugSessionMessages: (sessionId: string) => Promise<void>
|
||||
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<SessionUIState>()((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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 session→directory 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
|
||||
|
||||
@@ -10,14 +10,28 @@ const asNonEmptyString = (value) => {
|
||||
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 = {}) => {
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user