From 3e6a62d1925e4eeeea8ff932faf7b6ef38884157 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 31 Jul 2026 02:48:39 -0700 Subject: [PATCH] review: reassert host truth on unchanged polls; content-address snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client — the `changed` gate meant an unchanged poll skipped setWorktrees / setLastKnownWorktrees / setCachedWorktrees, so optimistic local edits (togglePin, handleDeleteWorktree's failure re-add) and the #8498 cache guard were no longer repaired while the host catalog was stable. The gate bought nothing: setCachedWorktrees is an in-memory Map write and areWorktreeListsEqual already ran every poll, so the steady state still short-circuits on array identity. All wire savings are unaffected. admit() now just returns the confirmed rows and HostScreen applies them exactly as it did pre-PR. Also on the client: - a stale response from a superseded client/host no longer clears the token the current client/host just established - discriminate on `worktrees` rather than on `'unchanged' in response`, so a future catalog field named `unchanged` can't reclassify a full response - useRef over useMemo for the snapshot client; React may discard memoized values - hoist WORKTREE_PS_FULL_LIMIT so the truncates-at-200 rationale travels with it Host — replace the per-limit snapshot cache with a content-addressed id (ETag semantics). Ownership lives in the id, so concurrent clients, differing limits, and runtime restarts are correct by construction; this drops the LRU, the eviction policy, the per-runtime WeakMap, and the retention of up to 8 full catalogs. The remaining cache is a pure memo: because ids derive from content, dropping or thrashing it costs CPU and nothing else. Keeping the memo also keeps the measured steady-state cost — hashing every poll instead measured 2.24ms vs 0.75ms for the compare on a 310KB catalog. Verified: mobile 2784 passed / 3 skipped, src/main/runtime/rpc 1064 passed, node + mobile typechecks, oxlint, oxfmt, max-lines ratchet. --- mobile/app/h/[hostId]/index.tsx | 46 +++++----- mobile/app/index.tsx | 4 +- .../worktree-catalog-snapshot-client.test.ts | 57 +++++++++--- .../worktree-catalog-snapshot-client.ts | 59 ++++++------- src/main/runtime/rpc/methods/worktree.ts | 3 +- .../rpc/worktree-catalog-snapshot.test.ts | 88 +++++++------------ .../runtime/rpc/worktree-catalog-snapshot.ts | 67 ++++++-------- 7 files changed, 162 insertions(+), 162 deletions(-) diff --git a/mobile/app/h/[hostId]/index.tsx b/mobile/app/h/[hostId]/index.tsx index 01ba9c7a963..c7052a39777 100644 --- a/mobile/app/h/[hostId]/index.tsx +++ b/mobile/app/h/[hostId]/index.tsx @@ -142,7 +142,9 @@ export function HostScreen({ const lastConnectedAt = useLastConnectedAt(hostId) const clientRef = useRef(null) const fetchWorktreesInFlightRef = useRef(false) - const worktreeCatalog = useMemo(() => new WorktreeCatalogSnapshotClient(), []) + // Why: useRef, not useMemo — React may discard memoized values, which would silently + // reset the snapshot token this object exists to own. + const worktreeCatalogRef = useRef(new WorktreeCatalogSnapshotClient()) const fetchRepoMetadataInFlightRef = useRef(new WeakSet()) const fetchRepoMetadataPendingRef = useRef(new WeakSet()) const repoMetadataFetchedAtRef = useRef(0) @@ -425,32 +427,32 @@ export function HostScreen({ const requestHostId = hostId try { - const pendingCatalog = await worktreeCatalog.fetch(requestClient, requestHostId) + const pendingCatalog = await worktreeCatalogRef.current.fetch(requestClient, requestHostId) if (clientRef.current !== requestClient || hostId !== requestHostId) { return } if (!options.allowDuringModal && newWorktreeModalVisibleRef.current) { return } - const result = worktreeCatalog.admit(pendingCatalog) - if (result) { - if (result.changed) { - // Why: reuse the existing array on identical snapshots to keep SectionList/sort rebuilds off the tap path. - setWorktrees((current) => - areWorktreeListsEqual(current, result.worktrees) ? current : result.worktrees - ) - setLastKnownWorktrees((current) => - areWorktreeListsEqual(current, result.worktrees) ? current : result.worktrees - ) - setWorktreesLoaded(true) - // Why (#8498): overwrite the home-written cache with the confirmed snapshot so a reconnect/remount can't serve a stale list. - if (hostId) { - setCachedWorktrees(hostId, result.worktrees) - } + // Why: unchanged responses still yield the confirmed rows, so every poll reasserts + // host truth over optimistic local edits regardless of payload size. + const confirmed = worktreeCatalogRef.current.admit(pendingCatalog) + if (confirmed) { + // Why: reuse the existing array on identical snapshots to keep SectionList/sort rebuilds off the tap path. + setWorktrees((current) => + areWorktreeListsEqual(current, confirmed) ? current : confirmed + ) + setLastKnownWorktrees((current) => + areWorktreeListsEqual(current, confirmed) ? current : confirmed + ) + setWorktreesLoaded(true) + // Why (#8498): overwrite the home-written cache with the confirmed snapshot so a reconnect/remount can't serve a stale list. + if (hostId) { + setCachedWorktrees(hostId, confirmed) } // Drop the optimistic active override once the host reports it active, so later desktop changes win. setOptimisticActiveWorktreeId((pending) => - pending && result.worktrees.some((w) => w.worktreeId === pending && w.isActive) + pending && confirmed.some((w) => w.worktreeId === pending && w.isActive) ? null : pending ) @@ -462,7 +464,7 @@ export function HostScreen({ } const still = new Set() for (const id of prev) { - const wt = result.worktrees.find((w) => w.worktreeId === id) + const wt = confirmed.find((w) => w.worktreeId === id) if (wt && wt.liveTerminalCount > 0) { still.add(id) } @@ -471,9 +473,7 @@ export function HostScreen({ }) // Sync pin state from server so desktop-initiated pins reflect without relying on stale AsyncStorage. - const serverPinned = new Set( - result.worktrees.filter((w) => w.isPinned).map((w) => w.worktreeId) - ) + const serverPinned = new Set(confirmed.filter((w) => w.isPinned).map((w) => w.worktreeId)) setPinnedIds((prev) => { if (serverPinned.size === prev.size && [...serverPinned].every((id) => prev.has(id))) { return prev @@ -490,7 +490,7 @@ export function HostScreen({ fetchWorktreesInFlightRef.current = false } }, - [client, connState, hostId, worktreeCatalog] + [client, connState, hostId] ) useFocusEffect( diff --git a/mobile/app/index.tsx b/mobile/app/index.tsx index 4000906c933..b4c9d123c5f 100644 --- a/mobile/app/index.tsx +++ b/mobile/app/index.tsx @@ -29,6 +29,7 @@ import { loadHosts } from '../src/transport/host-store' import { navigateToMobileHostEdit } from '../src/transport/host-edit-navigation' import { removeHostAndCloseClient } from '../src/transport/host-removal-lifecycle' import { pickResumeWorktree } from '../src/worktree/resume-worktree' +import { WORKTREE_PS_FULL_LIMIT } from '../src/worktree/worktree-catalog-snapshot-client' import type { RpcClient } from '../src/transport/rpc-client' import { sendSingleFlightRequest } from '../src/transport/request-single-flight' import { @@ -185,8 +186,7 @@ function fetchWorktreeInfo( }) } - // Why: worktree.ps defaults to 200 and silently truncates; request all so counts are accurate. - sendSingleFlightRequest(client, hostId, 'worktree.ps', { limit: 10000 }) + sendSingleFlightRequest(client, hostId, 'worktree.ps', { limit: WORKTREE_PS_FULL_LIMIT }) .then((response) => { if (disposed()) { return diff --git a/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts b/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts index 4565c605d66..35eb414dbd2 100644 --- a/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts +++ b/mobile/src/worktree/worktree-catalog-snapshot-client.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' import { admitWorktreeCatalogResponse, + WORKTREE_PS_FULL_LIMIT, WorktreeCatalogSnapshotClient } from './worktree-catalog-snapshot-client' @@ -34,6 +35,19 @@ describe('admitWorktreeCatalogResponse', () => { }) }) + it('classifies by rows, so a future `unchanged` catalog field cannot hide a full response', () => { + expect( + admitWorktreeCatalogResponse( + { worktrees: [{ id: 'worktree-1' }], unchanged: false, snapshotId: 'snapshot-1' }, + 'snapshot-1' + ) + ).toEqual({ + kind: 'full', + snapshotId: 'snapshot-1', + worktrees: [{ id: 'worktree-1' }] + }) + }) + it('rejects unchanged responses for a snapshot the client does not own', () => { expect( admitWorktreeCatalogResponse({ unchanged: true, snapshotId: 'snapshot-2' }, 'snapshot-1') @@ -45,9 +59,12 @@ describe('admitWorktreeCatalogResponse', () => { it('rejects malformed success payloads', () => { expect(admitWorktreeCatalogResponse(null, 'snapshot-1')).toEqual({ kind: 'invalid' }) - expect( - admitWorktreeCatalogResponse({ unchanged: false, snapshotId: 'snapshot-1' }, 'snapshot-1') - ).toEqual({ kind: 'invalid' }) + expect(admitWorktreeCatalogResponse({ unchanged: false }, 'snapshot-1')).toEqual({ + kind: 'invalid' + }) + }) + + it('accepts a full response but ignores an out-of-bounds snapshot id', () => { expect( admitWorktreeCatalogResponse({ worktrees: [], snapshotId: 'x'.repeat(129) }, null) ).toEqual({ kind: 'full', snapshotId: null, worktrees: [] }) @@ -69,9 +86,10 @@ function clientWithResults(...results: unknown[]): RpcClient { } describe('WorktreeCatalogSnapshotClient', () => { - it('retains confirmed rows while admitting unchanged responses', async () => { + it('returns the confirmed rows on unchanged responses so callers can reassert them', async () => { + const rows = [{ worktreeId: 'worktree-1' }] const client = clientWithResults( - { worktrees: [{ worktreeId: 'worktree-1' }], snapshotId: 'snapshot-1' }, + { worktrees: rows, snapshotId: 'snapshot-1' }, { unchanged: true, snapshotId: 'snapshot-1' } ) const snapshots = new WorktreeCatalogSnapshotClient() @@ -79,8 +97,8 @@ describe('WorktreeCatalogSnapshotClient', () => { const first = snapshots.admit(await snapshots.fetch(client, 'host-1')) const second = snapshots.admit(await snapshots.fetch(client, 'host-1')) - expect(first).toMatchObject({ changed: true }) - expect(second).toEqual({ changed: false, worktrees: first?.worktrees }) + expect(first).toEqual(rows) + expect(second).toEqual(rows) }) it('does not advance the token until the caller admits a response', async () => { @@ -94,7 +112,7 @@ describe('WorktreeCatalogSnapshotClient', () => { snapshots.admit(await snapshots.fetch(client, 'host-1')) expect(client.sendRequest).toHaveBeenNthCalledWith(2, 'worktree.ps', { - limit: 10_000, + limit: WORKTREE_PS_FULL_LIMIT, afterSnapshotId: null }) }) @@ -108,7 +126,7 @@ describe('WorktreeCatalogSnapshotClient', () => { await snapshots.fetch(client, 'host-1') expect(client.sendRequest).toHaveBeenNthCalledWith(2, 'worktree.ps', { - limit: 10_000, + limit: WORKTREE_PS_FULL_LIMIT, afterSnapshotId: 'snapshot-1' }) }) @@ -126,7 +144,7 @@ describe('WorktreeCatalogSnapshotClient', () => { await snapshots.fetch(client, 'host-1') expect(client.sendRequest).toHaveBeenNthCalledWith(3, 'worktree.ps', { - limit: 10_000, + limit: WORKTREE_PS_FULL_LIMIT, afterSnapshotId: null }) }) @@ -140,8 +158,25 @@ describe('WorktreeCatalogSnapshotClient', () => { await snapshots.fetch(secondClient, 'host-2') expect(secondClient.sendRequest).toHaveBeenCalledWith('worktree.ps', { - limit: 10_000, + limit: WORKTREE_PS_FULL_LIMIT, afterSnapshotId: null }) }) + + it('drops a superseded host response without invalidating the current token', async () => { + const firstClient = clientWithResults({ worktrees: [], snapshotId: 'snapshot-1' }) + const secondClient = clientWithResults({ worktrees: [], snapshotId: 'snapshot-2' }) + const snapshots = new WorktreeCatalogSnapshotClient() + + // Host A's response is still in flight when the screen switches to host B. + const stale = await snapshots.fetch(firstClient, 'host-1') + snapshots.admit(await snapshots.fetch(secondClient, 'host-2')) + expect(snapshots.admit(stale)).toBeNull() + + await snapshots.fetch(secondClient, 'host-2') + expect(secondClient.sendRequest).toHaveBeenNthCalledWith(2, 'worktree.ps', { + limit: WORKTREE_PS_FULL_LIMIT, + afterSnapshotId: 'snapshot-2' + }) + }) }) diff --git a/mobile/src/worktree/worktree-catalog-snapshot-client.ts b/mobile/src/worktree/worktree-catalog-snapshot-client.ts index 547205b1944..8b66ceddddd 100644 --- a/mobile/src/worktree/worktree-catalog-snapshot-client.ts +++ b/mobile/src/worktree/worktree-catalog-snapshot-client.ts @@ -2,6 +2,9 @@ import type { RpcClient } from '../transport/rpc-client' import type { RpcSuccess } from '../transport/types' import type { Worktree } from './workspace-list-sections' +// Why: worktree.ps silently truncates at 200; use a high cap so large hosts don't drop workspaces. +export const WORKTREE_PS_FULL_LIMIT = 10_000 + export type WorktreeCatalogAdmission = | { kind: 'full'; snapshotId: string | null; worktrees: T[] } | { kind: 'unchanged'; snapshotId: string } @@ -13,11 +16,6 @@ type PendingWorktreeCatalog = { hostId: string } -export type AdmittedWorktreeCatalog = { - changed: boolean - worktrees: Worktree[] -} - function validSnapshotId(value: unknown): string | null { return typeof value === 'string' && value.length > 0 && value.length <= 128 ? value : null } @@ -34,22 +32,22 @@ export function admitWorktreeCatalogResponse( unchanged?: unknown worktrees?: unknown } - if ('unchanged' in response) { - const snapshotId = validSnapshotId(response.snapshotId) - if (response.unchanged === true && snapshotId && snapshotId === requestedSnapshotId) { - return { kind: 'unchanged', snapshotId } + // Why: a full response is defined by carrying rows, so discriminate on that rather + // than on the presence of `unchanged` — the latter could collide with a future + // catalog field and silently reclassify a full response. + if (Array.isArray(response.worktrees)) { + return { + kind: 'full', + snapshotId: validSnapshotId(response.snapshotId), + worktrees: response.worktrees as T[] } - return { kind: 'invalid' } } - if (!Array.isArray(response.worktrees)) { - return { kind: 'invalid' } - } - return { - kind: 'full', - snapshotId: validSnapshotId(response.snapshotId), - worktrees: response.worktrees as T[] + const snapshotId = validSnapshotId(response.snapshotId) + if (response.unchanged === true && snapshotId && snapshotId === requestedSnapshotId) { + return { kind: 'unchanged', snapshotId } } + return { kind: 'invalid' } } export class WorktreeCatalogSnapshotClient { @@ -67,7 +65,7 @@ export class WorktreeCatalogSnapshotClient { } const requestedSnapshotId = this.snapshotId const response = await client.sendRequest('worktree.ps', { - limit: 10_000, + limit: WORKTREE_PS_FULL_LIMIT, afterSnapshotId: requestedSnapshotId }) if (!response.ok) { @@ -83,26 +81,27 @@ export class WorktreeCatalogSnapshotClient { } } - admit(pending: PendingWorktreeCatalog | null): AdmittedWorktreeCatalog | null { + /** The confirmed host catalog to apply, or null when there is nothing to apply. */ + admit(pending: PendingWorktreeCatalog | null): Worktree[] | null { if (!pending) { return null } - if ( - pending.client !== this.client || - pending.hostId !== this.hostId || - pending.admission.kind === 'invalid' - ) { + // Why: a response from a superseded client/host is stale, not wrong — dropping it + // must not invalidate the token the current client/host just established. + if (pending.client !== this.client || pending.hostId !== this.hostId) { + return null + } + if (pending.admission.kind === 'invalid') { this.snapshotId = null return null } - const admission = pending.admission - this.snapshotId = admission.snapshotId - if (admission.kind === 'full') { - this.confirmedWorktrees = admission.worktrees + this.snapshotId = pending.admission.snapshotId + if (pending.admission.kind === 'full') { + this.confirmedWorktrees = pending.admission.worktrees } + // Why: unchanged responses still return the confirmed rows so every poll reasserts + // host truth over optimistic local edits, exactly as the full-payload path did. return this.confirmedWorktrees - ? { changed: admission.kind === 'full', worktrees: this.confirmedWorktrees } - : null } } diff --git a/src/main/runtime/rpc/methods/worktree.ts b/src/main/runtime/rpc/methods/worktree.ts index e3e8bd7ffa0..dfa81dde1f5 100644 --- a/src/main/runtime/rpc/methods/worktree.ts +++ b/src/main/runtime/rpc/methods/worktree.ts @@ -30,10 +30,11 @@ export const WORKTREE_METHODS: RpcMethod[] = [ params: WorktreePsParams, handler: async (params, { runtime }) => { const result = await runtime.getWorktreePs(params.limit) + // Why: callers that never send the field get the byte-exact legacy response. if (params.afterSnapshotId === undefined) { return result } - return resolveWorktreeCatalogSnapshot(runtime, params.limit, result, params.afterSnapshotId) + return resolveWorktreeCatalogSnapshot(result, params.afterSnapshotId) } }), defineMethod({ diff --git a/src/main/runtime/rpc/worktree-catalog-snapshot.test.ts b/src/main/runtime/rpc/worktree-catalog-snapshot.test.ts index 77248d4597e..70178aa48db 100644 --- a/src/main/runtime/rpc/worktree-catalog-snapshot.test.ts +++ b/src/main/runtime/rpc/worktree-catalog-snapshot.test.ts @@ -1,95 +1,71 @@ import { describe, expect, it } from 'vitest' import type { RuntimeWorktreePsResult } from '../../../shared/runtime-types' -import { WorktreeCatalogSnapshotCache } from './worktree-catalog-snapshot' +import { resolveWorktreeCatalogSnapshot } from './worktree-catalog-snapshot' function result(totalCount: number): RuntimeWorktreePsResult { return { worktrees: [], totalCount, truncated: false } } -describe('WorktreeCatalogSnapshotCache', () => { - it('returns unchanged only when the caller owns the exact completed snapshot', () => { - const snapshots = new WorktreeCatalogSnapshotCache() - const first = snapshots.resolve(10_000, result(1), null) +describe('resolveWorktreeCatalogSnapshot', () => { + it('returns unchanged only when the caller owns the exact current snapshot', () => { + const first = resolveWorktreeCatalogSnapshot(result(1), null) expect(first).toMatchObject({ totalCount: 1 }) - expect('snapshotId' in first).toBe(true) const snapshotId = first.snapshotId - expect(snapshots.resolve(10_000, result(1), snapshotId)).toEqual({ + expect(resolveWorktreeCatalogSnapshot(result(1), snapshotId)).toEqual({ unchanged: true, snapshotId }) - expect(snapshots.resolve(10_000, result(1), 'unknown')).toEqual({ + expect(resolveWorktreeCatalogSnapshot(result(1), 'unknown')).toEqual({ ...result(1), snapshotId }) }) - it('issues a new snapshot for changed content or a different limit', () => { - const snapshots = new WorktreeCatalogSnapshotCache() - const first = snapshots.resolve(10_000, result(1), null) - const changed = snapshots.resolve(10_000, result(2), first.snapshotId) - const differentLimit = snapshots.resolve(200, result(2), changed.snapshotId) + it('issues a new snapshot id for changed content', () => { + const first = resolveWorktreeCatalogSnapshot(result(1), null) + const changed = resolveWorktreeCatalogSnapshot(result(2), first.snapshotId) expect(changed.snapshotId).not.toBe(first.snapshotId) - expect(differentLimit.snapshotId).not.toBe(changed.snapshotId) + expect(changed).not.toHaveProperty('unchanged') }) - it('keeps alternating conditional limits independent', () => { - const snapshots = new WorktreeCatalogSnapshotCache() - const wide = snapshots.resolve(10_000, result(2), null) - const narrow = snapshots.resolve(200, result(1), null) + it('keeps concurrent callers independent without server-held state', () => { + // Two clients on different catalogs interleave; neither can displace the other. + const wide = resolveWorktreeCatalogSnapshot(result(2), null) + const narrow = resolveWorktreeCatalogSnapshot(result(1), null) - expect(snapshots.resolve(10_000, result(2), wide.snapshotId)).toEqual({ + expect(resolveWorktreeCatalogSnapshot(result(2), wide.snapshotId)).toEqual({ unchanged: true, snapshotId: wide.snapshotId }) - expect(snapshots.resolve(200, result(1), narrow.snapshotId)).toEqual({ + expect(resolveWorktreeCatalogSnapshot(result(1), narrow.snapshotId)).toEqual({ unchanged: true, snapshotId: narrow.snapshotId }) }) - it('bounds snapshots retained for distinct limits', () => { - const snapshots = new WorktreeCatalogSnapshotCache() - const first = snapshots.resolve(1, result(1), null) - for (let limit = 2; limit <= 9; limit += 1) { - snapshots.resolve(limit, result(limit), null) - } + it('repairs a caller holding a superseded snapshot with a full response', () => { + const older = resolveWorktreeCatalogSnapshot(result(1), null) - const replaced = snapshots.resolve(1, result(1), first.snapshotId) - expect(replaced).not.toHaveProperty('unchanged') - expect(replaced.snapshotId).not.toBe(first.snapshotId) - }) - - it('retains recently used limits when the cache is full', () => { - const snapshots = new WorktreeCatalogSnapshotCache() - const ids = Array.from({ length: 8 }, (_, index) => { - const limit = index + 1 - return snapshots.resolve(limit, result(limit), null).snapshotId - }) - - snapshots.resolve(1, result(1), ids[0]) - snapshots.resolve(9, result(9), null) - - expect(snapshots.resolve(1, result(1), ids[0])).toHaveProperty('unchanged', true) - expect(snapshots.resolve(2, result(2), ids[1])).not.toHaveProperty('unchanged') - }) - - it('repairs completion-order cache replacement with a full response', () => { - const snapshots = new WorktreeCatalogSnapshotCache() - const older = snapshots.resolve(10_000, result(1), null) - const newer = snapshots.resolve(10_000, result(2), older.snapshotId) - snapshots.resolve(10_000, result(1), older.snapshotId) - - const repaired = snapshots.resolve(10_000, result(2), newer.snapshotId) + const repaired = resolveWorktreeCatalogSnapshot(result(2), older.snapshotId) expect(repaired).toMatchObject({ totalCount: 2 }) expect(repaired).not.toHaveProperty('unchanged') + expect(repaired.snapshotId).not.toBe(older.snapshotId) }) - it('scopes snapshot ids to one runtime cache', () => { - const left = new WorktreeCatalogSnapshotCache().resolve(10_000, result(1), null) - const right = new WorktreeCatalogSnapshotCache().resolve(10_000, result(1), null) + it('derives ids from content alone, so memo state cannot change the answer', () => { + // Identical catalogs must yield identical ids whether the memo is warm or was just + // displaced — that is what makes the memo droppable and restarts safe. + const warm = resolveWorktreeCatalogSnapshot(result(1), null).snapshotId + resolveWorktreeCatalogSnapshot(result(99), null) - expect(left.snapshotId).not.toBe(right.snapshotId) + expect(resolveWorktreeCatalogSnapshot(result(1), null).snapshotId).toBe(warm) + }) + + it('produces ids within the request schema bound', () => { + const { snapshotId } = resolveWorktreeCatalogSnapshot(result(1), null) + expect(snapshotId.length).toBeGreaterThan(0) + expect(snapshotId.length).toBeLessThanOrEqual(128) }) }) diff --git a/src/main/runtime/rpc/worktree-catalog-snapshot.ts b/src/main/runtime/rpc/worktree-catalog-snapshot.ts index 63057a12c33..9514266a963 100644 --- a/src/main/runtime/rpc/worktree-catalog-snapshot.ts +++ b/src/main/runtime/rpc/worktree-catalog-snapshot.ts @@ -1,59 +1,48 @@ -import { randomUUID } from 'node:crypto' +import { createHash } from 'node:crypto' import { isDeepStrictEqual } from 'node:util' import type { RuntimeWorktreePsConditionalResult, RuntimeWorktreePsResult } from '../../../shared/runtime-types' -type CachedWorktreeCatalog = { +// Why: 192 bits of a sha256 over the serialized catalog. Accidental collision is +// impossible at any realistic poll rate, and this is not an auth boundary. +const SNAPSHOT_ID_LENGTH = 32 + +type MemoizedCatalogId = { result: RuntimeWorktreePsResult snapshotId: string } -const MAX_CACHED_LIMITS = 8 +// Why: a pure memo over the most recent catalog. Because ids are derived from content +// rather than from cache state, dropping or thrashing this slot costs CPU and nothing +// else — it can never produce a wrong answer, so it needs no scoping, eviction policy, +// or per-caller isolation. Serializing every poll instead would cost ~3x the compare. +let memoizedId: MemoizedCatalogId | null = null -export class WorktreeCatalogSnapshotCache { - private readonly runtimeScope = randomUUID() - private sequence = 0 - private readonly cachedByLimit = new Map() - - resolve( - limit: number | undefined, - result: RuntimeWorktreePsResult, - afterSnapshotId: string | null - ): RuntimeWorktreePsConditionalResult { - const cached = this.cachedByLimit.get(limit) - if (cached && isDeepStrictEqual(cached.result, result)) { - this.cachedByLimit.delete(limit) - this.cachedByLimit.set(limit, cached) - if (afterSnapshotId === cached.snapshotId) { - return { unchanged: true, snapshotId: cached.snapshotId } - } - return { ...result, snapshotId: cached.snapshotId } - } - - const snapshotId = `${this.runtimeScope}:${++this.sequence}` - this.cachedByLimit.delete(limit) - this.cachedByLimit.set(limit, { result, snapshotId }) - if (this.cachedByLimit.size > MAX_CACHED_LIMITS) { - this.cachedByLimit.delete(this.cachedByLimit.keys().next().value) - } - return { ...result, snapshotId } +function worktreeCatalogSnapshotId(result: RuntimeWorktreePsResult): string { + if (memoizedId && isDeepStrictEqual(memoizedId.result, result)) { + return memoizedId.snapshotId } + const snapshotId = createHash('sha256') + .update(JSON.stringify(result)) + .digest('base64url') + .slice(0, SNAPSHOT_ID_LENGTH) + memoizedId = { result, snapshotId } + return snapshotId } -const snapshotsByRuntime = new WeakMap() - +// Why: content-addressed like an HTTP ETag, so snapshot ownership lives entirely in the +// id. Any number of concurrent clients, any `limit`, and any runtime restart stay correct +// by construction: ids match only when the catalogs are byte-identical, which is exactly +// when `unchanged` is the right answer. export function resolveWorktreeCatalogSnapshot( - runtime: object, - limit: number | undefined, result: RuntimeWorktreePsResult, afterSnapshotId: string | null ): RuntimeWorktreePsConditionalResult { - let snapshots = snapshotsByRuntime.get(runtime) - if (!snapshots) { - snapshots = new WorktreeCatalogSnapshotCache() - snapshotsByRuntime.set(runtime, snapshots) + const snapshotId = worktreeCatalogSnapshotId(result) + if (afterSnapshotId === snapshotId) { + return { unchanged: true, snapshotId } } - return snapshots.resolve(limit, result, afterSnapshotId) + return { ...result, snapshotId } }