fix(workspace-cleanup): address verifier findings

- Fall back to a full reflog read when the newest record exceeds the 8KB
  tail window, so an oversized subject cannot hide recent ref activity.
- Bound the single-removal snapshot prune batch id with a UUID; embedding
  the unbounded worktreeId silently failed main's 128-char validation and
  skipped the prune for long remote ids.
- Key the main-side broad-scan supersession by sender AND scan mode so
  legacy suggestion-only and full-workspace scans stay isolated, matching
  the renderer registry.
This commit is contained in:
Brennan Benson
2026-08-13 16:57:08 -07:00
parent 00784c58d0
commit d0fe1c73a0
5 changed files with 95 additions and 18 deletions
@@ -1,3 +1,5 @@
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'
import os from 'node:os'
import path from 'node:path'
import { describe, expect, it, vi } from 'vitest'
import type { Repo, Worktree } from '../../shared/types'
@@ -139,6 +141,35 @@ describe('resolveWorkspaceCleanupActivityWorktree', () => {
expect(worktree.lastActivityAt).toBe(1_700_000_900_000)
})
it('falls back to a full reflog read when the newest entry exceeds the tail window', async () => {
const dir = await mkdtemp(path.join(os.tmpdir(), 'cleanup-activity-'))
try {
const gitDir = path.join(dir, 'gitdir')
await mkdir(path.join(gitDir, 'logs'), { recursive: true })
const worktreePath = path.join(dir, 'wt')
await mkdir(worktreePath, { recursive: true })
await writeFile(path.join(worktreePath, '.git'), `gitdir: ${gitDir}\n`)
// Why: a single record longer than the 8192-byte tail window keeps its
// timestamp before the window; the reader must fall back to a full read.
await writeFile(
path.join(gitDir, 'logs', 'HEAD'),
'0000 1111 Dev <dev@example.com> 1700000000 -0700\tbranch: Created from HEAD\n' +
`1111 2222 Dev <dev@example.com> 1700000900 -0700\tcommit: ${'x'.repeat(9000)}\n`
)
const statPath = vi.fn(async () => ({ mtimeMs: 10_000 }))
const worktree = await resolveWorkspaceCleanupActivityWorktree(
REPO,
makeWorktree({ path: worktreePath }),
statPath
)
expect(worktree.lastActivityAt).toBe(1_700_000_900_000)
} finally {
await rm(dir, { recursive: true, force: true })
}
})
it('degrades to other probes when the reflog was expired to an empty file', async () => {
const gitDirPath = path.join('/repo', '.git', 'worktrees', 'repo-feature')
const statPath = vi.fn(async () => ({ mtimeMs: 10_000 }))
+19 -8
View File
@@ -134,20 +134,31 @@ async function readNewestReflogEntryAt(
try {
// A tail cut can start mid-line; scanning backward for the last parseable
// entry tolerates that.
const lines = (await readTextFile(reflogPath, { tailBytes: REFLOG_TAIL_BYTES })).split('\n')
for (let index = lines.length - 1; index >= 0; index -= 1) {
// The trailing timezone + tab anchors the capture, so no digit-count floor is needed.
const seconds = /\s(\d{1,11})\s[-+]\d{4}\t/.exec(lines[index] ?? '')?.[1]
if (seconds) {
return Number(seconds) * 1000
}
const tail = await readTextFile(reflogPath, { tailBytes: REFLOG_TAIL_BYTES })
const newestFromTail = parseNewestReflogEntryAt(tail)
if (newestFromTail !== 0 || tail.length === 0) {
return newestFromTail
}
return 0
// Why: a single record longer than the tail window keeps its timestamp
// before the window starts; fall back to the full read in that rare case.
return parseNewestReflogEntryAt(await readTextFile(reflogPath))
} catch {
return 0
}
}
function parseNewestReflogEntryAt(reflog: string): number {
const lines = reflog.split('\n')
for (let index = lines.length - 1; index >= 0; index -= 1) {
// The trailing timezone + tab anchors the capture, so no digit-count floor is needed.
const seconds = /\s(\d{1,11})\s[-+]\d{4}\t/.exec(lines[index] ?? '')?.[1]
if (seconds) {
return Number(seconds) * 1000
}
}
return 0
}
async function readLocalWorktreeGitDir(
worktreePath: string,
gitPath: string,
@@ -177,6 +177,31 @@ describe('workspace cleanup snapshot IPC', () => {
await expect(scan).rejects.toThrow('cancelled')
})
it('keeps legacy suggestion-only and full broad scans isolated across modes', async () => {
scanWorkspaceCleanupMock.mockImplementation((_store, _args, options) => {
return new Promise((resolve, reject) => {
options.signal?.addEventListener('abort', () => reject(new Error('cancelled')), {
once: true
})
setTimeout(() => resolve({ scannedAt: NOW, candidates: [], errors: [] }), 5)
})
})
registerWorkspaceCleanupHandlers(makeEmptyStore())
const handler = vi
.mocked(ipcMain.handle)
.mock.calls.find(([channel]) => channel === 'workspaceCleanup:scan')?.[1]
// Why: different includeAllWorkspaces modes are separate lanes; one must
// not abort the other even from the same renderer.
const [legacy, full] = await Promise.allSettled([
handler?.(makeScanEvent({ id: 7 }), { includeAllWorkspaces: false }) as Promise<unknown>,
handler?.(makeScanEvent({ id: 7 }), { includeAllWorkspaces: true }) as Promise<unknown>
])
expect(legacy?.status).toBe('fulfilled')
expect(full?.status).toBe('fulfilled')
})
it('supersedes a concurrent broad scan from the same renderer', async () => {
const settlements: Promise<unknown>[] = []
scanWorkspaceCleanupMock.mockImplementation((_store, _args, options) => {
+16 -8
View File
@@ -37,7 +37,14 @@ type WorkspaceCleanupHandlerDeps = {
// Why: module scope — handler re-registration on a new main window must not
// orphan the previous window's controllers in a discarded map.
const activeScans = new Map<string, AbortController>()
const broadScanControllersBySender = new Map<number, AbortController>()
// Keyed by sender AND scan mode: legacy suggestion-only and full-workspace
// broad scans are separate lanes and must not supersede each other, matching
// the renderer's broad-scan registry.
const broadScanControllersBySenderMode = new Map<string, AbortController>()
function getBroadScanModeKey(senderId: number, args: WorkspaceCleanupScanArgs): string {
return `${senderId}\0${args.includeAllWorkspaces === true}`
}
export function registerWorkspaceCleanupHandlers(
store: Store,
@@ -65,12 +72,13 @@ export function registerWorkspaceCleanupHandlers(
activeScans.set(scanKey, controller)
}
const targeted = hasTargetedWorkspaceCleanupScan(scanArgs)
const broadScanKey = getBroadScanModeKey(sender.id, scanArgs)
if (!targeted) {
// Why: two broad fleet scans from one renderer can only be a refresh
// race; running both doubles git subprocess and fs load for a result
// the renderer will discard.
broadScanControllersBySender.get(sender.id)?.abort()
broadScanControllersBySender.set(sender.id, controller)
// Why: two same-mode broad fleet scans from one renderer can only be a
// refresh race; running both doubles git subprocess and fs load for a
// result the renderer will discard.
broadScanControllersBySenderMode.get(broadScanKey)?.abort()
broadScanControllersBySenderMode.set(broadScanKey, controller)
}
// Why: a window close or reload must stop the fleet scan's git and fs
// work, not merely mute its progress events.
@@ -101,8 +109,8 @@ export function registerWorkspaceCleanupHandlers(
if (scanKey && activeScans.get(scanKey) === controller) {
activeScans.delete(scanKey)
}
if (!targeted && broadScanControllersBySender.get(sender.id) === controller) {
broadScanControllersBySender.delete(sender.id)
if (!targeted && broadScanControllersBySenderMode.get(broadScanKey) === controller) {
broadScanControllersBySenderMode.delete(broadScanKey)
}
}
}
+4 -2
View File
@@ -4329,8 +4329,10 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
await window.api.workspaceCleanup?.recordRemovalSnapshotPrune?.({
// Why: a single (unbatched) remote delete must still drop the row
// from the local persisted snapshots or it resurrects from cache;
// an unknown batch id degrades to an immediate one-off prune.
batchId: options?.snapshotPruneBatchId ?? `single-removal:${worktreeId}`,
// an unknown batch id degrades to an immediate one-off prune. The
// id must stay bounded — main rejects batch ids over 128 chars,
// so it cannot embed the unbounded worktreeId.
batchId: options?.snapshotPruneBatchId ?? `single-removal:${crypto.randomUUID()}`,
worktreeId,
...(hostId ? { executionHostId: hostId } : {})
})