From 70af9c847d25feb9bfa9c2a192de03db0b3ecd15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Andr=C3=A9?= Date: Fri, 18 Sep 2026 10:09:02 +0200 Subject: [PATCH 1/3] fix(server): bound worktree ownership miss refreshes Keep the native event stream responsive when the shared daemon emits events from many unrelated directories. Sequential ownership misses previously replaced the inventory and its negative cache for every new directory, repeatedly running full worktree discovery and blocking the serial event bridge. Allow one miss-triggered refresh per inventory cache lifetime. Further foreign directories share that refreshed snapshot, while TTL expiry and explicit worktree invalidation still discover external changes. Preserve concurrent singleflight loads and canonical path ownership checks. Add a regression for twenty distinct foreign directories, negative-result reuse, explicit invalidation, and TTL expiry. It reproduces 21 inventory loads before the fix and requires two afterward. Validated the 63 targeted routing, workspace-manager, and directory tests, server typecheck, and emitted server build; measured the installed application's event backlog before and after a graceful relaunch. --- .../src/workspaces/worktree-directory.test.ts | 35 +++++++++++++++++++ .../src/workspaces/worktree-directory.ts | 10 ++++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/packages/server/src/workspaces/worktree-directory.test.ts b/packages/server/src/workspaces/worktree-directory.test.ts index e14736d3c..d202a8071 100644 --- a/packages/server/src/workspaces/worktree-directory.test.ts +++ b/packages/server/src/workspaces/worktree-directory.test.ts @@ -79,3 +79,38 @@ test("resolves nested and junction paths to their canonical owning worktree", as assert.equal(isPathWithinWorktree("\\\\wsl.localhost\\Ubuntu\\repo\\Foo", "\\\\wsl.localhost\\Ubuntu\\repo\\Foo\\nested"), true) assert.equal(isPathWithinWorktree("\\\\WSL.LOCALHOST\\ubuntu\\repo\\Foo", "\\\\wsl.localhost\\Ubuntu\\repo\\Foo\\nested"), true) }) + +test("distinct foreign event directories share one ownership refresh until invalidation", async (t) => { + t.mock.timers.enable({ apis: ["Date"], now: Date.now() }) + const temp = mkdtempSync(path.join(tmpdir(), "codenomad-foreign-events-")) + t.after(() => { invalidateWorktreeCache(temp); rmSync(temp, { recursive: true, force: true }) }) + const root = path.join(temp, "repo") + mkdirSync(root) + let loads = 0 + const inventory = [{ slug: "root", directory: root, kind: "root" as const }] + const params = { + workspaceId: temp, workspacePath: root, + loadWorktrees: async () => { loads++; return inventory }, + } + await resolveOwnedWorktreePath({ ...params, directory: root }) + // The global native stream contains unrelated projects and temporary checkouts. + // Its serial router must not reload every repository's inventory for each one. + const foreign = Array.from({ length: 20 }, (_, i) => path.join(temp, `foreign-${i}`)) + for (const directory of foreign) { + mkdirSync(directory) + assert.equal(await resolveOwnedWorktreePath({ ...params, directory }), null) + } + assert.equal(loads, 2, "at most one miss refresh for the current inventory") + assert.equal(await resolveOwnedWorktreePath({ ...params, directory: foreign[0] }), null) + assert.equal(loads, 2) + // A native worktree event invalidates the snapshot, including negative matches. + inventory.push({ slug: "linked", directory: foreign[0], kind: "root" }) + invalidateWorktreeCache(temp) + assert.equal((await resolveOwnedWorktreePath({ ...params, directory: foreign[0] }))?.slug, "linked") + assert.equal(loads, 3) + // An external change without an event is still discovered after the cache TTL. + inventory.push({ slug: "external", directory: foreign[1], kind: "root" }) + t.mock.timers.tick(10_001) + assert.equal((await resolveOwnedWorktreePath({ ...params, directory: foreign[1] }))?.slug, "external") + assert.equal(loads, 4) +}) diff --git a/packages/server/src/workspaces/worktree-directory.ts b/packages/server/src/workspaces/worktree-directory.ts index 6f5ca3dd0..52e56c1a8 100644 --- a/packages/server/src/workspaces/worktree-directory.ts +++ b/packages/server/src/workspaces/worktree-directory.ts @@ -7,6 +7,7 @@ type WorktreeSource = { loadWorktrees: () => Promise } type WorktreeCacheEntry = { expiresAt: number + refreshedOnMiss: boolean worktrees: Array<{ slug: string; directory: string; normalizedDirectory: string; worktreeDirectory: string }> resolvedDirectories: Map } @@ -40,6 +41,7 @@ async function getCachedWorktrees(params: WorktreeSource & { workspaceId: string const worktrees = await params.loadWorktrees() const entry: WorktreeCacheEntry = { expiresAt: Date.now() + WORKTREE_CACHE_TTL_MS, + refreshedOnMiss: false, worktrees: await Promise.all( worktrees.map(async (wt) => ({ slug: wt.slug, @@ -182,11 +184,13 @@ export async function resolveOwnedWorktreePath(params: WorktreeSource & { let entry = await getCachedWorktrees(params) if (entry.resolvedDirectories.has(target)) return entry.resolvedDirectories.get(target)! let match = find(entry.worktrees) - if (!match || (match.slug === "root" && match.normalizedDirectory !== target)) { - // Several foreign-location events can miss the same snapshot concurrently. - // Refresh that snapshot once; never discard another caller's pending load. + if (!entry.refreshedOnMiss && (!match || (match.slug === "root" && match.normalizedDirectory !== target))) { + // Refresh once for this cache lifetime, including sequential misses from + // distinct foreign event locations. Otherwise every miss discards the last + // negative result and stalls the serial native event bridge on inventory I/O. if (worktreeCache.get(params.workspaceId) === entry) worktreeCache.delete(params.workspaceId) entry = await getCachedWorktrees(params) + entry.refreshedOnMiss = true match = find(entry.worktrees) } const resolved = match ? { slug: match.slug, directory: target, worktreeDirectory: match.worktreeDirectory } : null From 4ba01d11fbf8106b7f3ccb5a728cbe639be160b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Andr=C3=A9?= Date: Fri, 18 Sep 2026 11:13:46 +0200 Subject: [PATCH 2/3] fix(worktrees): cache inventories and lazily refresh display reads Retain successful native worktree inventories rather than only sharing in-flight requests. Serve warm display snapshots immediately and refresh stale data on demand in one shared background scan, without polling. Fence invalidated or disposed scans and retain the last display snapshot with retry backoff after failures. Keep directory authorization on validated snapshots and force fresh family transaction inventories. Publish completed inventory changes to existing UI consumers and serialize their reload behind older HTTP responses. Open the worktree selector without waiting for I/O and suppress duplicate selection notifications so reconciliation cannot close it or trigger a family move. Cover cache expiry, concurrency, mutation races, failures and disposal; real native rename/cache behavior with an isolated OpenCode 2.0.7 daemon; and Chromium selector updates, blocked refresh dismissal and pointer/keyboard/touch gestures. Passed 110 targeted server tests, five UI store tests, four browser tests, server/UI typechecks and builds. Cold scan cost and authoritative event-routing stalls remain separate from warm display caching. --- AGENTS.md | 1 + packages/server/src/api-types.ts | 2 + packages/server/src/events/bus.ts | 2 + .../server/src/server/routes/workspaces.ts | 2 +- .../server/src/server/routes/worktrees.ts | 5 +- .../server/src/workspaces/instance-events.ts | 3 +- packages/server/src/workspaces/manager.ts | 37 +++-- .../src/workspaces/worktree-inventory.test.ts | 153 ++++++++++++++++++ .../src/workspaces/worktree-inventory.ts | 88 ++++++++++ .../ui/src/components/worktree-selector.tsx | 4 +- packages/ui/src/stores/worktrees.ts | 10 ++ .../ui/tests/browser/fixtures/worktrees.tsx | 42 ++++- packages/ui/tests/browser/worktrees.test.ts | 29 ++++ scripts/test-native-worktree-management.mjs | 38 +++++ 14 files changed, 394 insertions(+), 22 deletions(-) create mode 100644 packages/server/src/workspaces/worktree-inventory.test.ts create mode 100644 packages/server/src/workspaces/worktree-inventory.ts diff --git a/AGENTS.md b/AGENTS.md index 37d83ae9c..75c061b95 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,6 +26,7 @@ ## Coding Principles - Worktree discovery/create/remove use `workspaces/native-worktrees.ts` and the native OpenCode worktree API. CodeNomad supplies the `.codenomad/worktrees` default, named-branch policy and verified family transactions. Git common-directory identity scopes the native inventory to the opened local repository; opaque worktree identifiers are separate from mutable branch labels. Validate through `scripts/test-opencode-location-native.mjs` with an isolated CLI and `tests/browser/worktrees.test.ts` for selector gestures. +- Worktree inventory snapshots live in `workspaces/worktree-inventory.ts`: display reads serve cached data and lazily revalidate, directory authorization uses validated reads, and family transactions force fresh reads. Invalidation retains display data and fences pending scans; `workspace.worktreesChanged` refreshes existing UI consumers after a changed snapshot is published. Keep selector opening independent of refresh completion and suppress duplicate selection events during inventory reconciliation. - Session pruning is a narrow V2 plugin/RPC exception under `packages/server/src/opencode/session-pruning/`; see `dev-docs/SESSION_PRUNING_RPC.md`. Bundle it with the shared server for both desktop hosts and provision through normal native plugin discovery. RPC registrations follow backend presence; clean shutdown removes that backend's lease and crashes expire. Loading never deletes content. Deletion occurs only on an explicit pruning request, without an extra enable-write switch or beta-number gate. Keep generic RPC proxy access closed. Writes validate actual storage, a fresh daemon-storage identity challenge and the native durable execution claim inside a synchronous SQLite transaction. Run isolated native concurrency/payload and client-cache regressions; tests must never target the shared daemon or a user's database. - Favor KISS by keeping modules narrowly scoped and limiting public APIs to what callers actually need. diff --git a/packages/server/src/api-types.ts b/packages/server/src/api-types.ts index a3d028749..60fff4963 100644 --- a/packages/server/src/api-types.ts +++ b/packages/server/src/api-types.ts @@ -468,6 +468,7 @@ export type WorkspaceEventType = | "workspace.error" | "workspace.stopped" | "workspace.log" + | "workspace.worktreesChanged" | "sidecar.updated" | "sidecar.removed" | "storage.configChanged" @@ -484,6 +485,7 @@ export type WorkspaceEventPayload = | { type: "workspace.error"; workspace: WorkspaceDescriptor } | { type: "workspace.stopped"; workspaceId: string; reason?: "deleted" | "stopped" } | { type: "workspace.log"; entry: WorkspaceLogEntry } + | { type: "workspace.worktreesChanged"; workspaceId: string } | { type: "sidecar.updated"; sidecar: SideCar } | { type: "sidecar.removed"; sidecarId: string } | { type: "storage.configChanged"; owner: SettingsOwner; value: SettingsBucket } diff --git a/packages/server/src/events/bus.ts b/packages/server/src/events/bus.ts index 637aad1d2..8950c48ab 100644 --- a/packages/server/src/events/bus.ts +++ b/packages/server/src/events/bus.ts @@ -35,6 +35,7 @@ export class EventBus extends EventEmitter { this.on("workspace.error", handler) this.on("workspace.stopped", handler) this.on("workspace.log", handler) + this.on("workspace.worktreesChanged", handler) this.on("sidecar.updated", handler) this.on("sidecar.removed", handler) this.on("storage.configChanged", handler) @@ -51,6 +52,7 @@ export class EventBus extends EventEmitter { this.off("workspace.error", handler) this.off("workspace.stopped", handler) this.off("workspace.log", handler) + this.off("workspace.worktreesChanged", handler) this.off("sidecar.updated", handler) this.off("sidecar.removed", handler) this.off("storage.configChanged", handler) diff --git a/packages/server/src/server/routes/workspaces.ts b/packages/server/src/server/routes/workspaces.ts index 05e91a52b..f97333e0c 100644 --- a/packages/server/src/server/routes/workspaces.ts +++ b/packages/server/src/server/routes/workspaces.ts @@ -361,7 +361,7 @@ async function resolveGitWorktreeDirectory( workspaceId: workspace.id, workspacePath: workspace.path, worktreeSlug, - loadWorktrees: async () => (await workspaceManager.getWorktrees(workspace.id)).worktrees, + loadWorktrees: async () => (await workspaceManager.getWorktrees(workspace.id, "validated")).worktrees, logger, }) if (!directory) { diff --git a/packages/server/src/server/routes/worktrees.ts b/packages/server/src/server/routes/worktrees.ts index 34be72e23..5052bd4ba 100644 --- a/packages/server/src/server/routes/worktrees.ts +++ b/packages/server/src/server/routes/worktrees.ts @@ -47,7 +47,6 @@ export function registerWorktreeRoutes(app: FastifyInstance, deps: RouteDeps) { try { const response: WorktreeListResponse = await deps.workspaceManager.getWorktrees(workspace.id) - invalidateWorktreeCache(workspace.id) return response } catch (error) { return handleError(error, reply) @@ -85,7 +84,7 @@ export function registerWorktreeRoutes(app: FastifyInstance, deps: RouteDeps) { return { error: "Workspace is not a Git repository" } } - const catalogue = await deps.workspaceManager.getWorktrees(workspace.id) + const catalogue = await deps.workspaceManager.getWorktrees(workspace.id, "fresh") const source = catalogue.worktrees.find(entry => entry.slug === (body.fromSlug ?? "root")) if (!source) throw new ProjectSessionError("Source worktree not found", 404) const identities = await Promise.all(catalogue.worktrees.map(entry => ( @@ -265,7 +264,7 @@ export function registerWorktreeRoutes(app: FastifyInstance, deps: RouteDeps) { } function strictWorktrees(manager: WorkspaceManager, workspaceId: string) { - return manager.getWorktrees(workspaceId).then(result => result.worktrees).catch((error) => { + return manager.getWorktrees(workspaceId, "fresh").then(result => result.worktrees).catch((error) => { throw new ProjectSessionError(error instanceof Error ? error.message : "Unable to read native worktree inventory", 502) }) } diff --git a/packages/server/src/workspaces/instance-events.ts b/packages/server/src/workspaces/instance-events.ts index 8d23316d0..aea4d5963 100644 --- a/packages/server/src/workspaces/instance-events.ts +++ b/packages/server/src/workspaces/instance-events.ts @@ -4,7 +4,6 @@ import { EventBus } from "../events/bus" import { Logger } from "../logger" import { WorkspaceManager } from "./manager" import { InstanceStreamStatus } from "../api-types" -import { invalidateWorktreeCache } from "./worktree-directory" const RECONNECT_DELAY_MS = 1000 const LOCATION_OWNER_CACHE_MS = 2000 @@ -110,7 +109,7 @@ export class InstanceEventBridge { private async publishEvent(event: OpenCodeEvent) { if (event.type === "worktree.updated") { - invalidateWorktreeCache() + this.options.workspaceManager.invalidateWorktrees() this.locationOwners.clear() } const sessionId = this.sessionId(event) diff --git a/packages/server/src/workspaces/manager.ts b/packages/server/src/workspaces/manager.ts index 46f14ac49..27e709825 100644 --- a/packages/server/src/workspaces/manager.ts +++ b/packages/server/src/workspaces/manager.ts @@ -32,6 +32,7 @@ import { import { WslOpenCodeService } from "./wsl-opencode-service" import { invalidateWorktreeCache, isPathOwnedByWorktree, resolveOwnedWorktreePath } from "./worktree-directory" import { listNativeWorktrees, createNativeWorktree, removeNativeWorktree } from "./native-worktrees" +import { WorktreeInventory } from "./worktree-inventory" import { resolveRepoRoot } from "./git-worktrees" import { locationRequestOptions, readLocationRef, sameLocation } from "../opencode/compatibility/location" @@ -317,25 +318,33 @@ export class WorkspaceManager { } } - private readonly worktreeInventoryRequests = new Map>() + private readonly worktreeInventory = new WorktreeInventory({ + load: (id) => this.nativeWorktreeContext(id).then(listNativeWorktrees), + changed: (id) => { + invalidateWorktreeCache(id) + this.options.eventBus.publish({ type: "workspace.worktreesChanged", workspaceId: id }) + }, + failed: (id, error) => this.options.logger.warn({ workspaceId: id, err: error }, "Failed to refresh worktree inventory"), + now: () => this.now(), + }) - async getWorktrees(id: string) { - const pending = this.worktreeInventoryRequests.get(id) - if (pending) return pending - const task = this.nativeWorktreeContext(id).then(listNativeWorktrees) - this.worktreeInventoryRequests.set(id, task) - try { return await task } - finally { if (this.worktreeInventoryRequests.get(id) === task) this.worktreeInventoryRequests.delete(id) } + getWorktrees(id: string, mode: "cached" | "validated" | "fresh" = "cached") { + return this.worktreeInventory.read(id, mode) + } + + invalidateWorktrees(): void { + this.worktreeInventory.invalidate() + invalidateWorktreeCache() } async createWorktree(id: string, branch: string, fromSlug?: string) { try { return await createNativeWorktree(await this.nativeWorktreeContext(id), branch, fromSlug) } - finally { invalidateWorktreeCache() } + finally { this.invalidateWorktrees() } } async removeWorktree(id: string, serviceDirectory: string, force: boolean) { try { return await removeNativeWorktree(await this.nativeWorktreeContext(id), serviceDirectory, force) } - finally { invalidateWorktreeCache() } + finally { this.invalidateWorktrees() } } private async ownsHostDirectory(record: WorkspaceRecord, directory: string): Promise { @@ -347,7 +356,7 @@ export class WorkspaceManager { workspaceId: record.id, workspacePath: record.path, directory, - loadWorktrees: async () => (await this.getWorktrees(record.id)).worktrees, + loadWorktrees: async () => (await this.getWorktrees(record.id, "validated")).worktrees, logger: this.options.logger, })) !== null } @@ -372,7 +381,7 @@ export class WorkspaceManager { workspaceId: record.id, workspacePath: record.path, directory: hostDirectory, - loadWorktrees: async () => (await this.getWorktrees(record.id)).worktrees, + loadWorktrees: async () => (await this.getWorktrees(record.id, "validated")).worktrees, logger: this.options.logger, }) } @@ -391,7 +400,7 @@ export class WorkspaceManager { workspaceId: record.id, workspacePath: record.path, candidate, - loadWorktrees: async () => (await this.getWorktrees(record.id)).worktrees, + loadWorktrees: async () => (await this.getWorktrees(record.id, "validated")).worktrees, logger: this.options.logger, }) } @@ -1042,6 +1051,8 @@ export class WorkspaceManager { ): void { if (this.workspaces.get(id) !== record) return this.workspaces.delete(id) + this.worktreeInventory.forget(id) + invalidateWorktreeCache(id) clearWorkspaceSearchCache(record.path) if (publishStopped) this.publishStopped(record, reason) } diff --git a/packages/server/src/workspaces/worktree-inventory.test.ts b/packages/server/src/workspaces/worktree-inventory.test.ts new file mode 100644 index 000000000..f50949f98 --- /dev/null +++ b/packages/server/src/workspaces/worktree-inventory.test.ts @@ -0,0 +1,153 @@ +import assert from "node:assert/strict" +import { it } from "node:test" +import type { WorktreeListResponse } from "../api-types" +import { WorktreeInventory } from "./worktree-inventory" + +function deferred() { + let resolve!: (value: T) => void + let reject!: (error: Error) => void + const promise = new Promise((yes, no) => { resolve = yes; reject = no }) + return { promise, resolve, reject } +} + +const snapshot = (branch: string): WorktreeListResponse => ({ + isGitRepo: true, + worktrees: [{ slug: "root", directory: "/repo", kind: "root", branch }], +}) + +function fixture() { + let now = 1 + const scans: ReturnType>[] = [] + const changes: string[] = [] + const failures: unknown[] = [] + const cache = new WorktreeInventory({ + now: () => now, + load: async () => { + const scan = deferred() + scans.push(scan) + return scan.promise + }, + changed: id => { changes.push(id) }, + failed: (_id, error) => { failures.push(error) }, + }) + return { cache, scans, changes, failures, advance: () => { now += 10_001 } } +} + +const turn = () => new Promise(resolve => setImmediate(resolve)) + +async function seed(f: ReturnType) { + const read = f.cache.read("repo") + await turn() + f.scans[0].resolve(snapshot("main")) + await read +} + +it("shares cold reads and retains their result across sequential requests", async () => { + const f = fixture() + const reads = [f.cache.read("repo"), f.cache.read("repo", "validated")] + await turn() + assert.equal(f.scans.length, 1) + f.scans[0].resolve(snapshot("main")) + assert.deepEqual(await Promise.all(reads), [snapshot("main"), snapshot("main")]) + assert.deepEqual(await f.cache.read("repo"), snapshot("main")) + assert.equal(f.scans.length, 1) + assert.deepEqual(f.changes, []) +}) + +it("returns expired data immediately and runs one lazy refresh for all consumers", async () => { + const f = fixture() + await seed(f) + f.advance() + await turn() + assert.equal(f.scans.length, 1, "expiry alone must not start Git") + const reads = await Promise.all(Array.from({ length: 20 }, () => f.cache.read("repo"))) + assert.ok(reads.every(value => value.worktrees[0].branch === "main")) + assert.equal(f.scans.length, 2) + let authorized = false + const validation = f.cache.read("repo", "validated").then(value => { authorized = true; return value }) + await turn() + assert.equal(authorized, false, "authorization must not use expired display data") + f.scans[1].resolve(snapshot("feature")) + assert.deepEqual(await validation, snapshot("feature")) + assert.deepEqual(await f.cache.read("repo"), snapshot("feature")) + assert.deepEqual(f.changes, ["repo"]) + assert.equal(f.scans.length, 2) +}) + +it("invalidates lazily and forces fresh family reads even inside the TTL", async () => { + const f = fixture() + await seed(f) + f.cache.invalidate() + assert.equal(f.scans.length, 1) + assert.deepEqual(await f.cache.read("repo"), snapshot("main")) + const validated = f.cache.read("repo", "validated") + f.scans[1].resolve(snapshot("feature")) + await validated + const fresh = f.cache.read("repo", "fresh") + await turn() + assert.equal(f.scans.length, 3) + f.scans[2].resolve(snapshot("feature")) + await fresh + assert.deepEqual(f.changes, ["repo"], "unchanged scans must not cause an SSE reload loop") +}) + +it("fences a scan overtaken by a mutation and serializes a trailing validation", async () => { + const f = fixture() + await seed(f) + const fresh = f.cache.read("repo", "fresh") + await turn() + f.cache.invalidate("repo") + const afterMutation = f.cache.read("repo", "validated") + assert.equal(f.scans.length, 2) + f.scans[1].resolve(snapshot("obsolete")) + await turn() + assert.deepEqual(f.changes, []) + assert.equal(f.scans.length, 3) + assert.deepEqual(await f.cache.read("repo"), snapshot("main")) + f.scans[2].resolve(snapshot("new")) + assert.deepEqual(await Promise.all([fresh, afterMutation]), [snapshot("new"), snapshot("new")]) + assert.deepEqual(f.changes, ["repo"]) +}) + +it("keeps the last good display snapshot on failure with demand-driven retry backoff", async () => { + const f = fixture() + await seed(f) + f.advance() + await f.cache.read("repo") + await f.cache.read("repo") + const validated = f.cache.read("repo", "validated") + const rejected = assert.rejects(validated, /offline/) + f.scans[1].reject(new Error("offline")) + await rejected + await turn() + assert.deepEqual(await f.cache.read("repo"), snapshot("main")) + assert.equal(f.failures.length, 1) + assert.equal(f.scans.length, 2) + f.advance() + await f.cache.read("repo") + const retry = f.cache.read("repo", "validated") + f.scans[2].resolve(snapshot("recovered")) + await retry + assert.deepEqual(f.changes, ["repo"]) +}) + +it("never republishes a scan after workspace disposal and isolates workspace keys", async () => { + const f = fixture() + await seed(f) + const fresh = f.cache.read("repo", "fresh") + const rejected = assert.rejects(fresh, /disposed/) + await turn() + f.cache.forget("repo") + const other = f.cache.read("other") + await turn() + f.scans[1].resolve(snapshot("obsolete")) + f.scans[2].resolve(snapshot("other")) + await rejected + assert.deepEqual(await other, snapshot("other")) + assert.deepEqual(f.changes, []) + const reopened = f.cache.read("repo") + await turn() + assert.equal(f.scans.length, 4) + f.scans[3].resolve(snapshot("reopened")) + assert.deepEqual(await reopened, snapshot("reopened")) +}) diff --git a/packages/server/src/workspaces/worktree-inventory.ts b/packages/server/src/workspaces/worktree-inventory.ts new file mode 100644 index 000000000..9e05537ae --- /dev/null +++ b/packages/server/src/workspaces/worktree-inventory.ts @@ -0,0 +1,88 @@ +import type { WorktreeListResponse } from "../api-types" + +type ReadMode = "cached" | "validated" | "fresh" +type Entry = { + value?: WorktreeListResponse + expiresAt: number + retryAt: number + revision: number + pending?: Promise +} + +const MAX_AGE_MS = 10_000 + +/** Demand-driven inventory: display reads keep the last snapshot while refreshing. + * Directory authorization waits for validation; family transactions force a scan. + * No timers, and no rejected/obsolete scan can replace the last good snapshot. + */ +export class WorktreeInventory { + private readonly entries = new Map() + + constructor(private readonly options: { + load: (id: string) => Promise + changed: (id: string) => void + failed: (id: string, error: unknown) => void + now?: () => number + }) {} + + async read(id: string, mode: ReadMode = "cached"): Promise { + let entry = this.entries.get(id) + if (!entry) { + entry = { expiresAt: 0, retryAt: 0, revision: 0 } + this.entries.set(id, entry) + } + const now = this.now() + if (mode !== "fresh" && entry.value && entry.expiresAt > now) return entry.value + if (mode === "cached" && entry.value) { + if (!entry.pending && entry.retryAt <= now) void this.load(id, entry).catch(error => this.options.failed(id, error)) + return entry.value + } + return this.load(id, entry) + } + + // Invalidation is lazy: retain the display snapshot, but fence any running scan. + invalidate(id?: string): void { + const entries = id === undefined ? this.entries.values() : [this.entries.get(id)] + for (const entry of entries) { + if (!entry) continue + entry.revision += 1 + entry.expiresAt = 0 + entry.retryAt = 0 + } + } + + forget(id: string): void { + this.entries.delete(id) + } + + private load(id: string, entry: Entry): Promise { + if (entry.pending) return entry.pending + const revision = entry.revision + const previous = entry.value + const task = Promise.resolve().then(() => this.options.load(id)).then(async value => { + if (this.entries.get(id) !== entry) throw new Error("Workspace inventory was disposed") + if (revision !== entry.revision) { + // A mutation overtook the scan. Serialize one trailing validation rather + // than publishing obsolete data or starting overlapping Git process fans. + entry.pending = undefined + return this.load(id, entry) + } + entry.value = value + entry.expiresAt = this.now() + MAX_AGE_MS + entry.retryAt = 0 + if (previous && JSON.stringify(previous) !== JSON.stringify(value)) this.options.changed(id) + return value + }).catch(error => { + if (revision === entry.revision) entry.retryAt = this.now() + MAX_AGE_MS + throw error + }).finally(() => { + if (entry.pending === task) entry.pending = undefined + }) + entry.pending = task + return task + } + + private now(): number { + return (this.options.now ?? Date.now)() + } +} diff --git a/packages/ui/src/components/worktree-selector.tsx b/packages/ui/src/components/worktree-selector.tsx index 062f3cf85..b61f33367 100644 --- a/packages/ui/src/components/worktree-selector.tsx +++ b/packages/ui/src/components/worktree-selector.tsx @@ -317,14 +317,16 @@ export default function WorktreeSelector(props: WorktreeSelectorProps) { open={isOpen()} onOpenChange={(open) => { if (!open) { setIsOpen(false); return } + setIsOpen(true) void reloadWorktrees(props.instanceId).catch(error => log.warn("Failed to refresh worktrees", error)) - .then(() => setIsOpen(true)) }} value={selectedOption() ?? null} onChange={(value) => { void handleChange(value).catch((error) => log.warn("Failed to change worktree", error)) }} options={worktreeOptions()} + // Inventory reconciliation is not a user selection and must not close the menu. + allowDuplicateSelectionEvents={false} optionValue="key" optionTextValue={(opt) => (opt.kind === "action" ? opt.label : opt.raw.label ?? opt.slug)} placeholder={t("sessionList.sort.worktree")} diff --git a/packages/ui/src/stores/worktrees.ts b/packages/ui/src/stores/worktrees.ts index 5c2602dd6..56d1c1975 100644 --- a/packages/ui/src/stores/worktrees.ts +++ b/packages/ui/src/stores/worktrees.ts @@ -1,6 +1,7 @@ import { createSignal } from "solid-js" import type { WorktreeDescriptor } from "../../../server/src/api-types" import { serverApi } from "../lib/api-client" +import { serverEvents } from "../lib/server-events" import { getSessionRoot, sessions } from "./session-state" import { getLogger } from "../lib/logger" import type { WorktreeReadyEvent } from "../lib/sse-manager" @@ -84,6 +85,15 @@ async function reloadWorktrees(instanceId: string): Promise { await queueWorktreeRequest(instanceId, false) } +serverEvents.on("workspace.worktreesChanged", (event) => { + if (event.type !== "workspace.worktreesChanged") return + const id = event.workspaceId + // Refresh consumers that already requested this inventory. Queue behind an + // older HTTP response so it cannot overwrite the completed background scan. + if (!worktreesByInstance().has(id) && !worktreeRequests.has(id)) return + void reloadWorktrees(id).catch(error => log.warn("Failed to receive refreshed worktrees", { instanceId: id, error })) +}) + async function handleWorktreeReady( instanceId: string, event: WorktreeReadyEvent, diff --git a/packages/ui/tests/browser/fixtures/worktrees.tsx b/packages/ui/tests/browser/fixtures/worktrees.tsx index d9e27fd33..b1e326a41 100644 --- a/packages/ui/tests/browser/fixtures/worktrees.tsx +++ b/packages/ui/tests/browser/fixtures/worktrees.tsx @@ -6,7 +6,8 @@ import { serverApi } from "../../../src/lib/api-client" import { sdkManager } from "../../../src/lib/sdk-manager" import { addInstance } from "../../../src/stores/instances" import { setSessions } from "../../../src/stores/session-state" -import { ensureWorktreesLoaded } from "../../../src/stores/worktrees" +import { ensureWorktreesLoaded, reloadWorktrees, getWorktrees } from "../../../src/stores/worktrees" +import { serverEvents } from "../../../src/lib/server-events" import "../../../src/index.css" const id = "worktree-fixture" @@ -43,4 +44,41 @@ render(() =>
, document.getElementById("root")!) await updatePreferences({ locale: "en" }) -;(window as any).fixture = { calls, location: () => session.location.directory } +;(window as any).fixture = { + calls, + location: () => session.location.directory, + worktrees: () => getWorktrees(id), + holdRefresh: () => { + let release!: () => void + const pending = new Promise(resolve => { release = resolve }) + serverApi.fetchWorktrees = async () => { + await pending + return { isGitRepo: true, worktrees: entries.map(entry => ({ ...entry })) } + } + ;(window as any).fixture.releaseRefresh = async () => { + release() + await reloadWorktrees(id) + } + }, + backgroundUpdate: async () => { + const old = entries.map(entry => ({ ...entry })) + let release!: () => void + const pending = new Promise(resolve => { release = resolve }) + let requests = 0 + serverApi.fetchWorktrees = async () => { + requests++ + if (requests === 1) { + await pending + return { isGitRepo: true, worktrees: old } + } + return { isGitRepo: true, worktrees: entries } + } + const initial = reloadWorktrees(id) + await Promise.resolve() + entries[1] = { ...entries[1], label: "renamed in background" } + // Exercise the production dispatcher while an older HTTP reply is pending. + ;(serverEvents as any).dispatchBatch([{ type: "workspace.worktreesChanged", workspaceId: id }]) + release() + await initial + }, +} diff --git a/packages/ui/tests/browser/worktrees.test.ts b/packages/ui/tests/browser/worktrees.test.ts index 186807c9a..161a7d162 100644 --- a/packages/ui/tests/browser/worktrees.test.ts +++ b/packages/ui/tests/browser/worktrees.test.ts @@ -61,6 +61,35 @@ test("worktree actions use click/keyboard/touch without selecting or moving the } finally { await context.close() } }) +test("background inventory completion updates the selector after an older HTTP response", async () => { + const page = await browser.newPage() + try { + await prepare(page) + await page.locator(".selector-trigger").click() + await page.getByRole("option", { name: /feature/ }).waitFor() + await page.evaluate(() => (window as any).fixture.backgroundUpdate()) + await page.waitForFunction(() => (window as any).fixture.worktrees().some((entry: any) => entry.label === "renamed in background")) + await page.getByRole("option", { name: /renamed in background/ }).waitFor() + assert.deepEqual(await page.evaluate(() => (window as any).fixture.calls), []) + assert.equal(await page.evaluate(() => (window as any).fixture.location()), "/repo") + } finally { await page.close() } +}) + +test("opens cached options before refresh completes and keeps a dismissed menu closed", async () => { + const page = await browser.newPage() + try { + await prepare(page) + await page.evaluate(() => (window as any).fixture.holdRefresh()) + await page.locator(".selector-trigger").click() + await page.getByRole("option", { name: /feature/ }).waitFor() + await page.keyboard.press("Escape") + await page.getByRole("listbox").waitFor({ state: "hidden" }) + await page.evaluate(() => (window as any).fixture.releaseRefresh()) + assert.equal(await page.locator(".selector-trigger").getAttribute("aria-expanded"), "false") + assert.deepEqual(await page.evaluate(() => (window as any).fixture.calls), []) + } finally { await page.close() } +}) + test("creation uses the selected source and the returned stable worktree ID", async () => { const page = await browser.newPage() try { diff --git a/scripts/test-native-worktree-management.mjs b/scripts/test-native-worktree-management.mjs index 7379cbe81..64cb7a581 100644 --- a/scripts/test-native-worktree-management.mjs +++ b/scripts/test-native-worktree-management.mjs @@ -7,6 +7,7 @@ import { tsImport } from "tsx/esm/api" // Invoked by the isolated daemon fixture only. No service discovery/user state. export async function testNativeWorktreeManagement({ client, root }) { const { listNativeWorktrees, createNativeWorktree, removeNativeWorktree } = await tsImport("../packages/server/src/workspaces/native-worktrees.ts", import.meta.url) + const { WorktreeInventory } = await tsImport("../packages/server/src/workspaces/worktree-inventory.ts", import.meta.url) const repo = path.join(root, "worktree-policy") const external = path.join(root, "agent-created") const clone = path.join(root, "independent-clone") @@ -64,5 +65,42 @@ export async function testNativeWorktreeManagement({ client, root }) { const fromLinked = await listNativeWorktrees({ ...context, workspacePath: external, location: { directory: external } }) assert.equal(fromLinked.defaultDirectory, catalogue.defaultDirectory, "opening a linked checkout must not nest the default parent") assert.equal(fromLinked.worktrees.find(entry => entry.branch === "main").removable, false) + let scans = 0 + const changed = [] + const inventory = new WorktreeInventory({ + load: async () => { assert.ok(++scans < 8, "native refresh events must not cause a scan loop"); return listNativeWorktrees(context) }, + changed: id => changed.push(id), + failed: (_id, error) => { throw error }, + }) + const controller = new AbortController() + let connected + const ready = new Promise(resolve => { connected = resolve }) + const events = (async () => { + for await (const event of client.event.subscribe({ signal: controller.signal })) { + if (event.type === "server.connected") connected() + if (event.type === "worktree.updated") inventory.invalidate() + } + })() + try { + await ready + const cached = await inventory.read("fixture") + assert.equal(await inventory.read("fixture"), cached) + assert.equal(scans, 1, "sequential display reads must reuse the completed native scan") + git(external, "branch", "-m", "agent-renamed") + inventory.invalidate("fixture") + assert.equal(scans, 1, "invalidation must stay lazy") + assert.equal(await inventory.read("fixture"), cached, "display must not wait for background Git") + const updated = await inventory.read("fixture", "validated") + assert.equal(updated.worktrees.find(entry => entry.slug === source.slug).branch, "agent-renamed") + assert.deepEqual(changed, ["fixture"]) + assert.equal(scans, 2) + await inventory.read("fixture", "fresh") + assert.equal(scans, 3, "family validation must bypass a warm display cache") + assert.deepEqual(changed, ["fixture"], "an unchanged scan must not create a reload feedback loop") + console.log("PASS: native worktree cache reuse, lazy rename refresh and forced family validation") + } finally { + controller.abort() + await events.catch(error => { if (!controller.signal.aborted) throw error }) + } console.log("PASS: native worktree discovery/create/remove, clone scope, selected HEAD, default parent, named branches, stable identity, nested paths and dirty/checked-out guards") } From d46bdbc15d63e6051d22c4ffd320d00c6d143893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Andr=C3=A9?= Date: Fri, 18 Sep 2026 13:21:04 +0200 Subject: [PATCH 3/3] fix(worktrees): address cache authority and selector review findings Require post-mutation inventory validation before returning display reads, propagate ownership misses through the lower inventory cache, and reject directory resolutions invalidated while loading. Preserve lazy display snapshots for passive refreshes while keeping create/remove read-your-writes semantics. Coalesce UI refresh bursts into one in-flight read and a trailing refresh, retaining the last successful snapshot on reload errors. Keep selector options and inline actions focused through background updates and dismiss explicit reselection without moving the session. Document refresh conventions against existing catalogue, Git and virtualization behavior. Add layered-cache and invalidation race regressions, native isolated manager create/remove coverage, and browser tests for stale HTTP responses, refresh bursts, keyboard focus and menu gestures. Synchronize the Git singleflight test mock with native ESM imports. Validation: 110 server tests, 7 UI store tests, 7 Chromium tests, isolated OpenCode 2.0.7 fixture, server/UI typechecks and UI build. --- AGENTS.md | 2 + dev-docs/CACHE_REFRESH_CONVENTIONS.md | 41 ++++++++++++++ .../server/src/server/routes/workspaces.ts | 2 +- .../server/src/workspaces/git-status.test.ts | 8 ++- packages/server/src/workspaces/manager.ts | 14 ++--- .../src/workspaces/worktree-directory.test.ts | 55 +++++++++++++++++++ .../src/workspaces/worktree-directory.ts | 20 ++++--- .../src/workspaces/worktree-inventory.test.ts | 15 +++++ .../src/workspaces/worktree-inventory.ts | 7 ++- .../ui/src/components/worktree-selector.tsx | 36 +++++++++++- packages/ui/src/stores/worktree-ready.test.ts | 31 ++++++++++- packages/ui/src/stores/worktrees.ts | 27 +++++++-- .../ui/tests/browser/fixtures/worktrees.tsx | 16 ++++++ packages/ui/tests/browser/worktrees.test.ts | 47 ++++++++++++++++ scripts/test-native-worktree-management.mjs | 33 +++++++++++ 15 files changed, 326 insertions(+), 28 deletions(-) create mode 100644 dev-docs/CACHE_REFRESH_CONVENTIONS.md diff --git a/AGENTS.md b/AGENTS.md index 75c061b95..775d0e1d6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,6 +25,8 @@ ## Coding Principles +- Follow `dev-docs/CACHE_REFRESH_CONVENTIONS.md` for display snapshots, coalesced trailing refreshes, stale-response fencing and authoritative mutation reads. Check existing feature semantics before adding another cache or refresh policy. + - Worktree discovery/create/remove use `workspaces/native-worktrees.ts` and the native OpenCode worktree API. CodeNomad supplies the `.codenomad/worktrees` default, named-branch policy and verified family transactions. Git common-directory identity scopes the native inventory to the opened local repository; opaque worktree identifiers are separate from mutable branch labels. Validate through `scripts/test-opencode-location-native.mjs` with an isolated CLI and `tests/browser/worktrees.test.ts` for selector gestures. - Worktree inventory snapshots live in `workspaces/worktree-inventory.ts`: display reads serve cached data and lazily revalidate, directory authorization uses validated reads, and family transactions force fresh reads. Invalidation retains display data and fences pending scans; `workspace.worktreesChanged` refreshes existing UI consumers after a changed snapshot is published. Keep selector opening independent of refresh completion and suppress duplicate selection events during inventory reconciliation. diff --git a/dev-docs/CACHE_REFRESH_CONVENTIONS.md b/dev-docs/CACHE_REFRESH_CONVENTIONS.md new file mode 100644 index 000000000..e545f0521 --- /dev/null +++ b/dev-docs/CACHE_REFRESH_CONVENTIONS.md @@ -0,0 +1,41 @@ +# Cache and refresh conventions + +Use consistent refresh semantics across features. Keep domain-specific implementations +where their authority, lifetime or runtime differs. + +## Common rules + +- Keep the last successful display snapshot while a passive refresh runs. +- Share concurrent reads for the same identity. During a read, coalesce refresh + requests into one pending follow-up rather than queueing one request per event. +- Invalidate on relevant events or mutations; a TTL expiring does not itself start + work. Avoid background polling merely to keep a hidden view warm. +- A response must still belong to the current identity/generation before publication. + Serialize follow-up reads so an older response cannot overwrite their result. +- Preserve the last good snapshot on refresh failure and permit recovery. Surface + failures for explicit operations rather than reporting stale data as success. +- A successful mutation must be visible to its next dependent read. Display-cache + policies must not weaken ownership checks or transactional revalidation. +- Updating a displayed collection is not a user selection. Preserve its open state, + stable-key keyboard target and inline actions through background refreshes. + +## Current implementations and limits + +| Area | Current behaviour | Implementation | +| --- | --- | --- | +| Providers/models | Retained signals; shared in-flight catalogue load; dirty-bit trailing refresh; instance, location and request-generation checks. No completed-result TTL inside `fetchProviders` itself. | `packages/ui/src/stores/session-api.ts` | +| Git changes | Filesystem events debounce for 100 ms; one passive refresh plus a pending follow-up; hidden tab marked stale; request versions protect status/diff. Server shares concurrent status requests, not completed results. | `useGitChanges.ts`, `filesystem-events.ts`, server `workspaces/git-status.ts` | +| Worktree display | Last successful server snapshot; demand-driven refresh after 10 s or invalidation; one scan per workspace; obsolete scans discarded and followed by validation; UI requests coalesced. | server `workspaces/worktree-inventory.ts`, UI `stores/worktrees.ts` | +| Worktree authority | Validated reads await stale-inventory revalidation; family transactions force scans. Ownership misses can bypass a warm inventory once per directory-cache lifetime. Create/remove requires a validated next display read. | server `workspaces/worktree-directory.ts`, `manager.ts` | +| Render cache | Explicit versioned values scoped to instance/session; no network scheduler or TTL policy. | UI `lib/global-cache.ts` | +| Virtualized lists | Session list, transcript and timeline use `virtua/solid`; virtualization limits rendered rows, not network refreshes. | UI `session-list.tsx`, `virtual-follow-list.tsx`, `message-timeline.tsx` | + +Git updates are regulated, but not incremental: every new server status calculation +runs five Git commands plus untracked-file processing, and the UI also requests +native status. Continuous activity can sustain repeated full calculations and +selected-diff reads. This is not a completed-result cache. + +The worktree cache is in memory. It does not reduce the first native inventory scan, +the cost of mandatory authoritative scans, or all latency in the serial event relay. +An isolated native fixture covers warm-cache create/remove visibility; browser +fixtures cover menu updates, focus, old responses and refresh bursts. diff --git a/packages/server/src/server/routes/workspaces.ts b/packages/server/src/server/routes/workspaces.ts index f97333e0c..2c524089f 100644 --- a/packages/server/src/server/routes/workspaces.ts +++ b/packages/server/src/server/routes/workspaces.ts @@ -361,7 +361,7 @@ async function resolveGitWorktreeDirectory( workspaceId: workspace.id, workspacePath: workspace.path, worktreeSlug, - loadWorktrees: async () => (await workspaceManager.getWorktrees(workspace.id, "validated")).worktrees, + loadWorktrees: async (refresh) => (await workspaceManager.getWorktrees(workspace.id, refresh ? "fresh" : "validated")).worktrees, logger, }) if (!directory) { diff --git a/packages/server/src/workspaces/git-status.test.ts b/packages/server/src/workspaces/git-status.test.ts index 365a52f0c..df1a2f150 100644 --- a/packages/server/src/workspaces/git-status.test.ts +++ b/packages/server/src/workspaces/git-status.test.ts @@ -2,6 +2,7 @@ import assert from "node:assert/strict" import { mkdtemp, rm } from "node:fs/promises" import fs from "node:fs/promises" import { tmpdir } from "node:os" +import { syncBuiltinESMExports } from "node:module" import path from "node:path" import { describe, it } from "node:test" @@ -20,13 +21,16 @@ describe("worktree git status singleflight", () => { let canonicalized = 0 let ready!: () => void const bothCanonicalized = new Promise((resolve) => { ready = resolve }) - t.mock.method(fs, "realpath", async (value: string) => { + const canonicalization = t.mock.method(fs, "realpath", async (value: string) => { const result = await realpath(value) canonicalized += 1 if (canonicalized === 2) ready() await bothCanonicalized return result }) + // Native ESM named imports do not see default-export monkey patches until + // synchronized. The CI runner uses ESM even when a local tsx run uses CJS. + syncBuiltinESMExports() const run = async () => { calls += 1 await blocked @@ -49,6 +53,8 @@ describe("worktree git status singleflight", () => { assert.equal(calls, 10) } finally { release() + canonicalization.mock.restore() + syncBuiltinESMExports() await rm(directory, { recursive: true, force: true }) } }) diff --git a/packages/server/src/workspaces/manager.ts b/packages/server/src/workspaces/manager.ts index 27e709825..24523e3ea 100644 --- a/packages/server/src/workspaces/manager.ts +++ b/packages/server/src/workspaces/manager.ts @@ -332,19 +332,19 @@ export class WorkspaceManager { return this.worktreeInventory.read(id, mode) } - invalidateWorktrees(): void { - this.worktreeInventory.invalidate() + invalidateWorktrees(mode: "lazy" | "blocking" = "lazy"): void { + this.worktreeInventory.invalidate(undefined, mode) invalidateWorktreeCache() } async createWorktree(id: string, branch: string, fromSlug?: string) { try { return await createNativeWorktree(await this.nativeWorktreeContext(id), branch, fromSlug) } - finally { this.invalidateWorktrees() } + finally { this.invalidateWorktrees("blocking") } } async removeWorktree(id: string, serviceDirectory: string, force: boolean) { try { return await removeNativeWorktree(await this.nativeWorktreeContext(id), serviceDirectory, force) } - finally { this.invalidateWorktrees() } + finally { this.invalidateWorktrees("blocking") } } private async ownsHostDirectory(record: WorkspaceRecord, directory: string): Promise { @@ -356,7 +356,7 @@ export class WorkspaceManager { workspaceId: record.id, workspacePath: record.path, directory, - loadWorktrees: async () => (await this.getWorktrees(record.id, "validated")).worktrees, + loadWorktrees: async (refresh) => (await this.getWorktrees(record.id, refresh ? "fresh" : "validated")).worktrees, logger: this.options.logger, })) !== null } @@ -381,7 +381,7 @@ export class WorkspaceManager { workspaceId: record.id, workspacePath: record.path, directory: hostDirectory, - loadWorktrees: async () => (await this.getWorktrees(record.id, "validated")).worktrees, + loadWorktrees: async (refresh) => (await this.getWorktrees(record.id, refresh ? "fresh" : "validated")).worktrees, logger: this.options.logger, }) } @@ -400,7 +400,7 @@ export class WorkspaceManager { workspaceId: record.id, workspacePath: record.path, candidate, - loadWorktrees: async () => (await this.getWorktrees(record.id, "validated")).worktrees, + loadWorktrees: async (refresh) => (await this.getWorktrees(record.id, refresh ? "fresh" : "validated")).worktrees, logger: this.options.logger, }) } diff --git a/packages/server/src/workspaces/worktree-directory.test.ts b/packages/server/src/workspaces/worktree-directory.test.ts index d202a8071..a664cf5c7 100644 --- a/packages/server/src/workspaces/worktree-directory.test.ts +++ b/packages/server/src/workspaces/worktree-directory.test.ts @@ -7,6 +7,7 @@ import path from "node:path" import { test } from "node:test" import { invalidateWorktreeCache, isPathWithinWorktree, resolveOwnedWorktreePath } from "./worktree-directory" import { fixtureCatalogue } from "./__tests__/native-worktree-fixture" +import { WorktreeInventory } from "./worktree-inventory" test("concurrent ownership misses share a refresh and cache negative results until invalidation", async (t) => { const temp = mkdtempSync(path.join(tmpdir(), "codenomad-inventory-misses-")) @@ -114,3 +115,57 @@ test("distinct foreign event directories share one ownership refresh until inval assert.equal((await resolveOwnedWorktreePath({ ...params, directory: foreign[1] }))?.slug, "external") assert.equal(loads, 4) }) + +test("an ownership miss bypasses the warm lower inventory cache once", async t => { + const temp = mkdtempSync(path.join(tmpdir(), "codenomad-layered-cache-")) + t.after(() => { invalidateWorktreeCache(temp); rmSync(temp, { recursive: true, force: true }) }) + const root = path.join(temp, "repo") + const nested = path.join(root, "linked") + mkdirSync(nested, { recursive: true }) + const worktrees = [{ slug: "root", directory: root, kind: "root" as const }] + let scans = 0 + const inventory = new WorktreeInventory({ + load: async () => { scans++; return { isGitRepo: true, worktrees: [...worktrees] } }, + changed: () => invalidateWorktreeCache(temp), + failed: () => {}, + }) + const params = { + workspaceId: temp, workspacePath: root, + loadWorktrees: async (refresh?: boolean) => (await inventory.read(temp, refresh ? "fresh" : "validated")).worktrees, + } + assert.equal((await resolveOwnedWorktreePath({ ...params, directory: root }))?.slug, "root") + worktrees.push({ slug: "linked", directory: nested, kind: "root" }) + // No native update event yet. Resolving the nested checkout must not collapse + // it to the parent repository's mutation/deletion identity. + assert.equal((await resolveOwnedWorktreePath({ ...params, directory: nested }))?.slug, "linked") + assert.equal(scans, 2) + for (let i = 0; i < 20; i++) { + assert.equal(await resolveOwnedWorktreePath({ ...params, directory: path.join(temp, `foreign-${i}`) }), null) + } + assert.equal(scans, 2) +}) + +test("an invalidated directory load cannot return obsolete ownership to its awaiting caller", async t => { + const temp = mkdtempSync(path.join(tmpdir(), "codenomad-directory-generation-")) + t.after(() => { invalidateWorktreeCache(temp); rmSync(temp, { recursive: true, force: true }) }) + const root = path.join(temp, "repo") + mkdirSync(root) + let release!: () => void + let started!: () => void + const ready = new Promise(resolve => { started = resolve }) + const blocked = new Promise(resolve => { release = resolve }) + let calls = 0 + const read = resolveOwnedWorktreePath({ + workspaceId: temp, workspacePath: root, directory: root, + loadWorktrees: async () => { + if (++calls !== 1) return [] + started() + await blocked + return [{ slug: "root", directory: root, kind: "root" }] + }, + }) + await ready + invalidateWorktreeCache(temp) + release() + assert.equal(await read, null) +}) diff --git a/packages/server/src/workspaces/worktree-directory.ts b/packages/server/src/workspaces/worktree-directory.ts index 52e56c1a8..d26ebe727 100644 --- a/packages/server/src/workspaces/worktree-directory.ts +++ b/packages/server/src/workspaces/worktree-directory.ts @@ -3,7 +3,7 @@ import path from "node:path" import type { LogLike } from "./git-worktrees" import type { WorktreeDescriptor } from "../api-types" -type WorktreeSource = { loadWorktrees: () => Promise } +type WorktreeSource = { loadWorktrees: (refresh?: boolean) => Promise } type WorktreeCacheEntry = { expiresAt: number @@ -26,7 +26,10 @@ async function normalizeDirectoryPath(directory: string): Promise { } } -async function getCachedWorktrees(params: WorktreeSource & { workspaceId: string; workspacePath: string; logger?: LogLike }) { +async function getCachedWorktrees( + params: WorktreeSource & { workspaceId: string; workspacePath: string; logger?: LogLike }, + refresh = false, +): Promise { const cached = worktreeCache.get(params.workspaceId) const now = Date.now() if (cached && cached.expiresAt > now) { @@ -38,7 +41,7 @@ async function getCachedWorktrees(params: WorktreeSource & { workspaceId: string let load!: Promise load = (async () => { - const worktrees = await params.loadWorktrees() + const worktrees = await params.loadWorktrees(refresh) const entry: WorktreeCacheEntry = { expiresAt: Date.now() + WORKTREE_CACHE_TTL_MS, refreshedOnMiss: false, @@ -52,7 +55,10 @@ async function getCachedWorktrees(params: WorktreeSource & { workspaceId: string ), resolvedDirectories: new Map(), } - if (worktreeLoads.get(params.workspaceId) === load) worktreeCache.set(params.workspaceId, entry) + // A native snapshot update or mutation can invalidate during load/realpath. + // Never return the obsolete ownership to callers already awaiting this load. + if (worktreeLoads.get(params.workspaceId) !== load) return getCachedWorktrees(params) + worktreeCache.set(params.workspaceId, entry) return entry })() worktreeLoads.set(params.workspaceId, load) @@ -96,7 +102,7 @@ export async function resolveWorktreeDirectory(params: WorktreeSource & { workspacePath: params.workspacePath, logger: params.logger, loadWorktrees: params.loadWorktrees, - }) + }, true) return refreshed.worktrees.find((wt) => wt.slug === params.worktreeSlug)?.directory ?? null } @@ -126,7 +132,7 @@ export async function resolveWorktreeSlugForDirectory(params: WorktreeSource & { workspacePath: params.workspacePath, logger: params.logger, loadWorktrees: params.loadWorktrees, - }) + }, true) return refreshed.worktrees.find((wt) => wt.normalizedDirectory === target)?.slug ?? null } @@ -189,7 +195,7 @@ export async function resolveOwnedWorktreePath(params: WorktreeSource & { // distinct foreign event locations. Otherwise every miss discards the last // negative result and stalls the serial native event bridge on inventory I/O. if (worktreeCache.get(params.workspaceId) === entry) worktreeCache.delete(params.workspaceId) - entry = await getCachedWorktrees(params) + entry = await getCachedWorktrees(params, true) entry.refreshedOnMiss = true match = find(entry.worktrees) } diff --git a/packages/server/src/workspaces/worktree-inventory.test.ts b/packages/server/src/workspaces/worktree-inventory.test.ts index f50949f98..eb7150365 100644 --- a/packages/server/src/workspaces/worktree-inventory.test.ts +++ b/packages/server/src/workspaces/worktree-inventory.test.ts @@ -151,3 +151,18 @@ it("never republishes a scan after workspace disposal and isolates workspace key f.scans[3].resolve(snapshot("reopened")) assert.deepEqual(await reopened, snapshot("reopened")) }) + +it("waits for the post-mutation inventory instead of returning a pre-create display snapshot", async () => { + const f = fixture() + await seed(f) + f.cache.invalidate(undefined, "blocking") + let settled = false + const read = f.cache.read("repo").then(value => { settled = true; return value }) + await turn() + assert.equal(settled, false) + const created = snapshot("main") + created.worktrees.push({ slug: "new", directory: "/repo/new", kind: "worktree" }) + f.scans[1].resolve(created) + assert.deepEqual(await read, created) + assert.deepEqual(f.changes, ["repo"]) +}) diff --git a/packages/server/src/workspaces/worktree-inventory.ts b/packages/server/src/workspaces/worktree-inventory.ts index 9e05537ae..2a04a3dc9 100644 --- a/packages/server/src/workspaces/worktree-inventory.ts +++ b/packages/server/src/workspaces/worktree-inventory.ts @@ -6,6 +6,7 @@ type Entry = { expiresAt: number retryAt: number revision: number + requireValidation?: boolean pending?: Promise } @@ -33,7 +34,7 @@ export class WorktreeInventory { } const now = this.now() if (mode !== "fresh" && entry.value && entry.expiresAt > now) return entry.value - if (mode === "cached" && entry.value) { + if (mode === "cached" && entry.value && !entry.requireValidation) { if (!entry.pending && entry.retryAt <= now) void this.load(id, entry).catch(error => this.options.failed(id, error)) return entry.value } @@ -41,13 +42,14 @@ export class WorktreeInventory { } // Invalidation is lazy: retain the display snapshot, but fence any running scan. - invalidate(id?: string): void { + invalidate(id?: string, mode: "lazy" | "blocking" = "lazy"): void { const entries = id === undefined ? this.entries.values() : [this.entries.get(id)] for (const entry of entries) { if (!entry) continue entry.revision += 1 entry.expiresAt = 0 entry.retryAt = 0 + if (mode === "blocking") entry.requireValidation = true } } @@ -70,6 +72,7 @@ export class WorktreeInventory { entry.value = value entry.expiresAt = this.now() + MAX_AGE_MS entry.retryAt = 0 + entry.requireValidation = false if (previous && JSON.stringify(previous) !== JSON.stringify(value)) this.options.changed(id) return value }).catch(error => { diff --git a/packages/ui/src/components/worktree-selector.tsx b/packages/ui/src/components/worktree-selector.tsx index b61f33367..0b0ea0ecd 100644 --- a/packages/ui/src/components/worktree-selector.tsx +++ b/packages/ui/src/components/worktree-selector.tsx @@ -1,6 +1,6 @@ import { Select } from "@kobalte/core/select" import { Dialog } from "@kobalte/core/dialog" -import { Show, createMemo, createSignal, createUniqueId } from "solid-js" +import { Show, createMemo, createSignal, createUniqueId, untrack } from "solid-js" import { ChevronDown, Copy, FolderOpen, Trash2 } from "lucide-solid" import type { WorktreeDescriptor } from "../../../server/src/api-types" import { getLogger } from "../lib/logger" @@ -148,9 +148,29 @@ export default function WorktreeSelector(props: WorktreeSelectorProps) { const gitRepoStatus = createMemo(() => getGitRepoStatus(props.instanceId)) const worktreesUnavailable = createMemo(() => gitRepoStatus() === false) const dropdownDisabled = createMemo(() => isChildSession() || worktreesUnavailable()) + let listbox: HTMLUListElement | undefined const worktreeOptions = createMemo(() => { const list = getWorktrees(props.instanceId) + // Kobalte recreates option DOM when its collection changes. Keep the user's + // keyboard target (including an inline action) through lazy inventory updates. + const focused = document.activeElement as HTMLElement | null + const option = focused?.closest("[role=option]") + const key = option?.dataset.key + const actionLabel = focused?.closest("button")?.getAttribute("aria-label") + if (key && listbox?.contains(focused) && untrack(isOpen)) { + queueMicrotask(() => { + if (!isOpen() || focused?.isConnected || !listbox?.isConnected) return + if (document.activeElement !== document.body && !listbox.contains(document.activeElement)) return + const replacement = Array.from(listbox.querySelectorAll("[role=option]")).find(item => item.dataset.key === key) + const action = actionLabel ? Array.from(replacement?.querySelectorAll("button") ?? []).find(button => button.getAttribute("aria-label") === actionLabel) : undefined + const target = action ?? replacement + // Update Kobalte's focused key before focusing a nested button; a button + // focus alone leaves the previous option as its keyboard target. + if (action) replacement?.focus({ preventScroll: true }) + ;(target ?? listbox).focus({ preventScroll: true }) + }) + } const mapped: WorktreeOption[] = list.map((wt) => ({ kind: "worktree", key: wt.slug, @@ -347,7 +367,17 @@ export default function WorktreeSelector(props: WorktreeSelectorProps) { } return ( - + { if (opt.slug === currentSlug()) setIsOpen(false) }} + onKeyDown={(event) => { + if (opt.slug === currentSlug() && (event.key === "Enter" || event.key === " ")) { + event.preventDefault() + setIsOpen(false) + } + }} + >
@@ -470,7 +500,7 @@ export default function WorktreeSelector(props: WorktreeSelectorProps) { - + diff --git a/packages/ui/src/stores/worktree-ready.test.ts b/packages/ui/src/stores/worktree-ready.test.ts index 67886ca53..999edcb68 100644 --- a/packages/ui/src/stores/worktree-ready.test.ts +++ b/packages/ui/src/stores/worktree-ready.test.ts @@ -7,6 +7,31 @@ import type { Session } from "../types/session.ts" import { sessions, setSessions } from "./session-state.ts" describe("handleWorktreeReady", () => { + it("reports a failed trailing reload and retains the successful initial snapshot", async () => { + const original = serverApi.fetchWorktrees + let release!: () => void + const gate = new Promise(resolve => { release = resolve }) + let calls = 0 + serverApi.fetchWorktrees = async () => { + if (++calls > 1) throw new Error("trailing refresh failed") + await gate + return { isGitRepo: true, worktrees: [{ slug: "root", directory: "/repo", kind: "root" }] } + } + try { + const initial = ensureWorktreesLoaded("failed-trailing-read") + await Promise.resolve() + const reload = reloadWorktrees("failed-trailing-read") + const rejected = assert.rejects(Promise.all([initial, reload]), /trailing refresh failed/) + release() + await rejected + assert.equal(calls, 2) + assert.equal(getWorktrees("failed-trailing-read")[0]?.slug, "root") + } finally { + release() + serverApi.fetchWorktrees = original + } + }) + it("refreshes worktrees", async () => { const calls: string[] = [] @@ -106,6 +131,7 @@ describe("handleWorktreeReady", () => { try { const initial = ensureWorktreesLoaded(instanceId) + await Promise.resolve() const reload = reloadWorktrees(instanceId) await Promise.resolve() @@ -115,8 +141,7 @@ describe("handleWorktreeReady", () => { isGitRepo: true, worktrees: [{ slug: "root", directory: "/repo", kind: "root" }], }) - await initial - await Promise.resolve() + await new Promise(resolve => setImmediate(resolve)) assert.equal(requestCount, 2) @@ -127,7 +152,7 @@ describe("handleWorktreeReady", () => { { slug: "feature", directory: "/repo-feature", kind: "worktree" }, ], }) - await reload + await Promise.all([initial, reload]) assert.deepEqual(getWorktrees(instanceId).map((worktree) => worktree.slug), ["root", "feature"]) } finally { diff --git a/packages/ui/src/stores/worktrees.ts b/packages/ui/src/stores/worktrees.ts index 56d1c1975..fa2280adf 100644 --- a/packages/ui/src/stores/worktrees.ts +++ b/packages/ui/src/stores/worktrees.ts @@ -15,6 +15,7 @@ const [worktreesByInstance, setWorktreesByInstance] = createSignal>(new Map()) const worktreeRequests = new Map>() +const pendingWorktreeRefreshes = new Set() const worktreeReadyRefreshes = new Map>() const familyMoveRequests = new Map>() const defaultDirectories = new Map() @@ -22,8 +23,12 @@ const defaultDirectories = new Map() type WorktreeReadyRefresh = (instanceId: string) => Promise async function queueWorktreeRequest(instanceId: string, initial: boolean): Promise { - const previous = worktreeRequests.get(instanceId) - const task = (previous?.catch(() => undefined) ?? Promise.resolve()).then(async () => { + const existing = worktreeRequests.get(instanceId) + if (existing) { + if (!initial) pendingWorktreeRefreshes.add(instanceId) + return existing + } + const load = async (initialRead: boolean) => { try { const response = await serverApi.fetchWorktrees(instanceId) if (response.defaultDirectory) defaultDirectories.set(instanceId, response.defaultDirectory) @@ -40,8 +45,8 @@ async function queueWorktreeRequest(instanceId: string, initial: boolean): Promi return next }) } catch (error) { - log.warn(initial ? "Failed to load worktrees" : "Failed to reload worktrees", { instanceId, error }) - if (!initial) throw error + log.warn(initialRead ? "Failed to load worktrees" : "Failed to reload worktrees", { instanceId, error }) + if (!initialRead) throw error setWorktreesByInstance((prev) => { const next = new Map(prev) @@ -57,6 +62,20 @@ async function queueWorktreeRequest(instanceId: string, initial: boolean): Promi return next }) } + } + // Like the provider/model catalogue, retain one in-flight read and one dirty + // bit. A burst requests one trailing read, not an unbounded HTTP queue. + const task = Promise.resolve().then(async () => { + let initialRead = initial + do { + pendingWorktreeRefreshes.delete(instanceId) + try { + await load(initialRead) + } catch (error) { + if (!pendingWorktreeRefreshes.has(instanceId)) throw error + } + initialRead = false + } while (pendingWorktreeRefreshes.has(instanceId)) }) worktreeRequests.set(instanceId, task) diff --git a/packages/ui/tests/browser/fixtures/worktrees.tsx b/packages/ui/tests/browser/fixtures/worktrees.tsx index b1e326a41..4410531f7 100644 --- a/packages/ui/tests/browser/fixtures/worktrees.tsx +++ b/packages/ui/tests/browser/fixtures/worktrees.tsx @@ -48,6 +48,22 @@ await updatePreferences({ locale: "en" }) calls, location: () => session.location.directory, worktrees: () => getWorktrees(id), + refreshBurst: async () => { + let release!: () => void + const gate = new Promise(resolve => { release = resolve }) + let requests = 0 + serverApi.fetchWorktrees = async () => { + requests++ + if (requests === 1) await gate + return { isGitRepo: true, worktrees: entries } + } + const first = reloadWorktrees(id) + await Promise.resolve() + const burst = Array.from({ length: 40 }, () => reloadWorktrees(id)) + release() + await Promise.all([first, ...burst]) + return requests + }, holdRefresh: () => { let release!: () => void const pending = new Promise(resolve => { release = resolve }) diff --git a/packages/ui/tests/browser/worktrees.test.ts b/packages/ui/tests/browser/worktrees.test.ts index 161a7d162..e053745f7 100644 --- a/packages/ui/tests/browser/worktrees.test.ts +++ b/packages/ui/tests/browser/worktrees.test.ts @@ -67,14 +67,61 @@ test("background inventory completion updates the selector after an older HTTP r await prepare(page) await page.locator(".selector-trigger").click() await page.getByRole("option", { name: /feature/ }).waitFor() + await page.getByRole("option", { name: /feature/ }).focus() + const focused = await page.evaluate(() => (document.activeElement as HTMLElement)?.dataset.key) await page.evaluate(() => (window as any).fixture.backgroundUpdate()) await page.waitForFunction(() => (window as any).fixture.worktrees().some((entry: any) => entry.label === "renamed in background")) await page.getByRole("option", { name: /renamed in background/ }).waitFor() + assert.ok(focused) + assert.equal(await page.evaluate(() => (document.activeElement as HTMLElement)?.dataset.key), focused) assert.deepEqual(await page.evaluate(() => (window as any).fixture.calls), []) assert.equal(await page.evaluate(() => (window as any).fixture.location()), "/repo") } finally { await page.close() } }) +test("selecting the current worktree dismisses the menu without moving the session", async () => { + const page = await browser.newPage() + try { + await prepare(page) + const trigger = page.locator(".selector-trigger") + await trigger.click() + await page.getByRole("option", { name: "Workspace", exact: true }).click() + await page.getByRole("listbox").waitFor({ state: "hidden" }) + assert.deepEqual(await page.evaluate(() => (window as any).fixture.calls), []) + await trigger.focus() + await page.keyboard.press("Enter") + await page.getByRole("listbox").waitFor() + await page.keyboard.press("Enter") + await page.getByRole("listbox").waitFor({ state: "hidden" }) + assert.deepEqual(await page.evaluate(() => (window as any).fixture.calls), []) + } finally { await page.close() } +}) + +test("coalesces a refresh burst into one trailing HTTP read", async () => { + const page = await browser.newPage() + try { + await prepare(page) + assert.equal(await page.evaluate(() => (window as any).fixture.refreshBurst()), 2) + } finally { await page.close() } +}) + +test("keeps an inline action focused through a background rename", async () => { + const page = await browser.newPage() + try { + await prepare(page) + await page.locator(".selector-trigger").click() + await page.getByRole("option", { name: /feature/ }).getByRole("button", { name: "Copy path", exact: true }).focus() + assert.equal(await page.evaluate(() => document.activeElement?.getAttribute("aria-label")), "Copy path", "inline action must be focusable before refresh") + await page.evaluate(() => (window as any).fixture.backgroundUpdate()) + const copy = page.getByRole("option", { name: /renamed in background/ }).getByRole("button", { name: "Copy path", exact: true }) + await copy.waitFor() + assert.equal(await copy.evaluate(element => element === document.activeElement), true, await page.evaluate(() => document.activeElement?.outerHTML)) + await page.keyboard.press("Enter") + await page.waitForFunction(() => (window as any).copied === "/repo/.codenomad/worktrees/feature") + assert.deepEqual(await page.evaluate(() => (window as any).fixture.calls), []) + } finally { await page.close() } +}) + test("opens cached options before refresh completes and keeps a dismissed menu closed", async () => { const page = await browser.newPage() try { diff --git a/scripts/test-native-worktree-management.mjs b/scripts/test-native-worktree-management.mjs index 64cb7a581..24a73a068 100644 --- a/scripts/test-native-worktree-management.mjs +++ b/scripts/test-native-worktree-management.mjs @@ -3,6 +3,7 @@ import { execFileSync } from "node:child_process" import { mkdir, writeFile, rm, realpath } from "node:fs/promises" import path from "node:path" import { tsImport } from "tsx/esm/api" +import pino from "pino" // Invoked by the isolated daemon fixture only. No service discovery/user state. export async function testNativeWorktreeManagement({ client, root }) { @@ -102,5 +103,37 @@ export async function testNativeWorktreeManagement({ client, root }) { controller.abort() await events.catch(error => { if (!controller.signal.aborted) throw error }) } + const { WorkspaceManager } = await tsImport("../packages/server/src/workspaces/manager.ts", import.meta.url) + const { EventBus } = await tsImport("../packages/server/src/events/bus.ts", import.meta.url) + // Exercise the production manager's post-mutation invalidation using only + // this fixture's authenticated client. No native discovery/user service. + const manager = new WorkspaceManager({ + rootDir: root, + settings: { getOwner: () => ({ environmentVariables: {} }) }, + binaryResolver: { resolveDefault: () => ({ path: process.execPath, label: "Isolated fixture" }) }, + eventBus: new EventBus(), + logger: pino({ level: "silent" }), + sharedService: { + client: async () => client, + headers: async () => ({}), + validateLocation: async location => client.location.get({ location }), + shutdown: async () => {}, + }, + }) + try { + const { workspace } = await manager.create(repo) + const before = await manager.getWorktrees(workspace.id) + const added = await manager.createWorktree(workspace.id, "cached-create") + const afterCreate = await manager.getWorktrees(workspace.id) + assert.equal(afterCreate.worktrees.length, before.worktrees.length + 1) + assert.ok(afterCreate.worktrees.some(entry => entry.slug === added.slug), "create-and-use must receive the new ID immediately") + await manager.removeWorktree(workspace.id, added.serviceRoot, false) + const afterRemove = await manager.getWorktrees(workspace.id) + assert.equal(afterRemove.worktrees.length, before.worktrees.length) + assert.ok(!afterRemove.worktrees.some(entry => entry.slug === added.slug), "post-delete refresh must not resurrect a cached checkout") + console.log("PASS: production manager warm-cache create/remove read-your-writes") + } finally { + await manager.shutdown() + } console.log("PASS: native worktree discovery/create/remove, clone scope, selected HEAD, default parent, named branches, stable identity, nested paths and dirty/checked-out guards") }