diff --git a/packages/server/src/utils/directory-suggestions.test.ts b/packages/server/src/utils/directory-suggestions.test.ts index 2b24ca2174..2611fee842 100644 --- a/packages/server/src/utils/directory-suggestions.test.ts +++ b/packages/server/src/utils/directory-suggestions.test.ts @@ -12,6 +12,7 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { isPlatform } from "../test-utils/platform.js"; +import { startPathContainmentMetrics, stopPathContainmentMetrics } from "./path.js"; import { startGitCommandMetrics, stopGitCommandMetrics } from "./run-git-command.js"; import { searchDirectoryEntries, @@ -169,6 +170,26 @@ describe("searchDirectoryEntries", () => { ); }); + it.skipIf(isWindows)( + "prunes a symlink whose target is inside a Git-ignored directory", + async () => { + initGitRepo(searchRoot, "generated/\n"); + mkdirSync(path.join(searchRoot, "generated", "output"), { recursive: true }); + symlinkSync(path.join(searchRoot, "generated", "output"), path.join(searchRoot, "linked")); + + await expect( + searchDirectoryEntries({ + root: searchRoot, + query: "linked", + pathFormat: "relative", + includeFiles: false, + includeDirectories: true, + respectGitIgnore: true, + }), + ).resolves.toEqual([]); + }, + ); + it("configures raw blank queries independently from explicit root aliases", async () => { const rootEntries = [ { path: "projects", kind: "directory" as const }, @@ -953,3 +974,62 @@ describe("relative typed-entry configuration", () => { expect(results).toEqual([{ path: "blankpage/editor", kind: "directory" }]); }); }); + +// Reading a tree is cheap; re-deriving each entry's path state is not. A scan that asks whether +// every ancestor of every entry is inside the root, or is Git-ignored, costs multiples of the +// read and is what made large home directories exceed the client request timeout. +describe("home-tree scan cost", () => { + const DEPTH = 8; + const CHAINS = 40; + let scanRoot: string; + + beforeEach(() => { + scanRoot = realpathSync.native(mkdtempSync(path.join(tmpdir(), "directory-scan-cost-"))); + for (let chain = 0; chain < CHAINS; chain += 1) { + const segments: string[] = []; + for (let level = 0; level < DEPTH; level += 1) segments.push(`c${chain}l${level}`); + mkdirSync(path.join(scanRoot, ...segments), { recursive: true }); + } + }); + + afterEach(() => { + rmSync(scanRoot, { recursive: true, force: true }); + }); + + // Nothing matches, so the scan walks the whole tree instead of stopping at a confident result. + function scanWholeTree() { + return searchAbsoluteDirectoryPaths({ + homeDir: scanRoot, + query: "nomatch", + maxDepth: DEPTH + 4, + }); + } + + async function countContainmentChecks(run: () => Promise): Promise { + startPathContainmentMetrics(); + await run(); + return stopPathContainmentMetrics(); + } + + it("derives no containment for a tree that cannot leave the root", async () => { + const checks = await countContainmentChecks(scanWholeTree); + + expect({ checks, scanned: CHAINS * DEPTH }).toEqual({ checks: 0, scanned: 320 }); + }); + + it.skipIf(isWindows)( + "derives containment for symlinked entries, which can leave the root", + async () => { + const outside = realpathSync.native(mkdtempSync(path.join(tmpdir(), "directory-scan-away-"))); + symlinkSync(outside, path.join(scanRoot, "escape"), "dir"); + + const checks = await countContainmentChecks(scanWholeTree); + + expect({ checked: checks > 0, escaped: (await scanWholeTree()).includes(outside) }).toEqual({ + checked: true, + escaped: false, + }); + rmSync(outside, { recursive: true, force: true }); + }, + ); +}); diff --git a/packages/server/src/utils/directory-suggestions.ts b/packages/server/src/utils/directory-suggestions.ts index aa24c4a6b9..35e8f5154b 100644 --- a/packages/server/src/utils/directory-suggestions.ts +++ b/packages/server/src/utils/directory-suggestions.ts @@ -46,6 +46,7 @@ interface ChildEntry { name: string; resolvedPath: string; kind: DirectorySuggestionKind; + viaSymlink: boolean; } interface RawChildEntry { @@ -249,7 +250,7 @@ async function searchChildren(input: SearchInput): Promise { async function searchTree(input: SearchInput): Promise { if (!(input.maxEntriesScanned > 0)) return []; const roots = (await readChildren(input.root)).filter((entry) => - isPathInsideRoot(input.root, entry.resolvedPath), + staysInsideRoot(entry, input.root), ); const visited = new Set([input.root]); const branches = roots.flatMap((entry) => @@ -294,7 +295,7 @@ async function* walkBranch( return; visited.add(entry.resolvedPath); const children = (await readChildren(entry.resolvedPath)).filter((child) => - isPathInsideRoot(input.root, child.resolvedPath), + staysInsideRoot(child, input.root), ); const branches = children.flatMap((child) => shouldDiscover(child, input) @@ -329,8 +330,14 @@ async function* roundRobin(branches: Array>): AsyncGenerato } } +// Only a symlink can resolve outside the tree being walked. Every other child is its parent's +// path plus a name, and the parent was already proved inside the root. +function staysInsideRoot(entry: ChildEntry, root: string): boolean { + return !entry.viaSymlink || isPathInsideRoot(root, entry.resolvedPath); +} + function shouldDiscover(entry: ChildEntry, input: SearchInput): boolean { - if (isGitIgnoredPath(entry.resolvedPath, input)) return false; + if (isNewlyGitIgnored(entry, input)) return false; if (entry.kind === "file") { return input.includeFiles && !entry.name.startsWith("."); } @@ -339,6 +346,14 @@ function shouldDiscover(entry: ChildEntry, input: SearchInput): boolean { return input.hiddenDirectoryNames.has(entry.name); } +// Traversal only descends through entries that already passed this check, so every ancestor of +// a named child is known discoverable and only the entry itself can be newly ignored. A symlink +// resolves to a path with different ancestors, so it still needs the full walk. +function isNewlyGitIgnored(entry: ChildEntry, input: SearchInput): boolean { + if (entry.viaSymlink) return isGitIgnoredPath(entry.resolvedPath, input); + return input.gitIgnoredPaths.has(entry.resolvedPath); +} + function isGitIgnoredPath(absolutePath: string, input: SearchInput): boolean { let candidate = absolutePath; while (candidate !== input.root && isPathInsideRoot(input.root, candidate)) { @@ -712,14 +727,14 @@ function toRawChildEntry(dirent: Dirent): RawChildEntry | null { async function resolveChild(directory: string, entry: RawChildEntry): Promise { const visiblePath = path.join(directory, entry.name); if (entry.kind !== "symlink") { - return { name: entry.name, resolvedPath: visiblePath, kind: entry.kind }; + return { name: entry.name, resolvedPath: visiblePath, kind: entry.kind, viaSymlink: false }; } const resolvedPath = await realpath(visiblePath).catch(() => null); if (!resolvedPath) return null; const info = await stat(resolvedPath).catch(() => null); const kind = getEntryKind(info); - return kind ? { name: entry.name, resolvedPath, kind } : null; + return kind ? { name: entry.name, resolvedPath, kind, viaSymlink: true } : null; } function getEntryKind(info: Stats | null): DirectorySuggestionKind | null { @@ -728,10 +743,9 @@ function getEntryKind(info: Stats | null): DirectorySuggestionKind | null { return null; } +// Reads already reject stale entries by expiry and by directory metadata, so this only has to +// bound the map. Sweeping it for expired keys would cost a full pass on every cache miss. function pruneCache(): void { - if (directoryListCache.size <= DIRECTORY_LIST_CACHE_MAX_ENTRIES) return; - for (const [key, entry] of directoryListCache) - if (entry.expiresAt <= Date.now()) directoryListCache.delete(key); while (directoryListCache.size > DIRECTORY_LIST_CACHE_MAX_ENTRIES) { const key = directoryListCache.keys().next().value; if (!key) return; diff --git a/packages/server/src/utils/path.ts b/packages/server/src/utils/path.ts index a63ab52c1a..577e48efb4 100644 --- a/packages/server/src/utils/path.ts +++ b/packages/server/src/utils/path.ts @@ -58,7 +58,22 @@ export function createRealpathAwarePathMatcher(target: string): (candidate: stri }; } +let pathContainmentChecks: number | null = null; + +// Containment is re-derived from scratch on every call. A tree walk that asks it per entry, or +// per ancestor of every entry, costs more than reading the tree does, so tests count the calls. +export function startPathContainmentMetrics(): void { + pathContainmentChecks = 0; +} + +export function stopPathContainmentMetrics(): number { + const checks = pathContainmentChecks ?? 0; + pathContainmentChecks = null; + return checks; +} + export function isPathInsideRoot(root: string, candidate: string): boolean { + if (pathContainmentChecks !== null) pathContainmentChecks += 1; return getRelativePathInsideRoot(root, candidate) !== null; }