From 434cc1ef802703daa52060f865435e09e3dff905 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:21:09 +0200 Subject: [PATCH] test(server): count containment derivations instead of timing the scan The previous budget compared wall-clock scan time against a readdir walk of the same tree. packages/server/AGENTS.md requires tests to be deterministic, and a ratio between an I/O-bound walk and a CPU-bound scan moves with filesystem caching and runner load. isPathInsideRoot now counts its calls behind start/stopPathContainmentMetrics, matching the existing git command metrics. Scanning a symlink-free tree of 320 entries derived containment 1,760 times before this branch and derives it none now, which is the defect stated exactly rather than inferred from a stopwatch. A second test covers the other direction: a symlink that leaves the root is still checked, and still excluded from results. --- .../src/utils/directory-suggestions.test.ts | 107 +++++++----------- packages/server/src/utils/path.ts | 15 +++ 2 files changed, 56 insertions(+), 66 deletions(-) diff --git a/packages/server/src/utils/directory-suggestions.test.ts b/packages/server/src/utils/directory-suggestions.test.ts index cab15318cf..57b06602e2 100644 --- a/packages/server/src/utils/directory-suggestions.test.ts +++ b/packages/server/src/utils/directory-suggestions.test.ts @@ -8,11 +8,11 @@ import { unlinkSync, writeFileSync, } from "node:fs"; -import { readdir } from "node:fs/promises"; 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, @@ -955,81 +955,56 @@ describe("relative typed-entry configuration", () => { }); }); -// A home-directory scan is dominated by per-entry bookkeeping, not by reading the tree. These -// budgets are expressed against a plain readdir walk of the same tree, so they mean the same -// thing on a slow CI runner as on a developer machine. -// -// Skipped on Windows: the tree this needs is deeper than MAX_PATH allows, and the directory -// listing cache this measures is disabled there because directory metadata cannot validate it. -describe.skipIf(isWindows)("home-tree scan cost", () => { - const ENTRIES = 5_000; +// 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; - // Long names cost more to normalize, and per-entry path work is spent normalizing them. - const SEGMENT_NAME_LENGTH = 48; + const CHAINS = 40; let scanRoot: string; - function buildDeepTree(root: string): void { - const chains = Math.ceil(ENTRIES / DEPTH); - for (let chain = 0; chain < chains; chain += 1) { - const segments: string[] = []; - for (let level = 0; level < DEPTH; level += 1) { - segments.push(`d${chain * DEPTH + level}`.padEnd(SEGMENT_NAME_LENGTH, "x")); - } - mkdirSync(path.join(root, ...segments), { recursive: true }); - } - } - - async function readWholeTree(root: string): Promise { - let level = [root]; - while (level.length) { - const next: string[] = []; - for (const directory of level) { - const children = await readdir(directory, { withFileTypes: true }).catch(() => []); - for (const child of children) { - if (child.isDirectory()) next.push(path.join(directory, child.name)); - } - } - level = next; - } - } - - async function measure(run: () => Promise): Promise { - const startedAt = process.hrtime.bigint(); - await run(); - return Number(process.hrtime.bigint() - startedAt) / 1e6; - } - - // Nothing matches, so every scan spends its whole budget and the timings are comparable. - const scanDeepTree = () => - searchAbsoluteDirectoryPaths({ - homeDir: scanRoot, - query: "nomatchanywhereinthistree", - limit: 30, - maxDepth: DEPTH + 4, - maxDirectoriesScanned: ENTRIES, - }); - beforeEach(() => { scanRoot = realpathSync.native(mkdtempSync(path.join(tmpdir(), "directory-scan-cost-"))); - buildDeepTree(scanRoot); + 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 }); }); - it("scans a deep tree within a bounded multiple of reading it", async () => { - const readMs = await measure(() => readWholeTree(scanRoot)); - await scanDeepTree(); - const scanMs = await measure(scanDeepTree); + // Nothing matches, so the scan walks the whole tree instead of stopping at a confident result. + const scanWholeTree = () => + searchAbsoluteDirectoryPaths({ homeDir: scanRoot, query: "nomatch", maxDepth: DEPTH + 4 }); - // Re-deriving containment and Git-ignore state for every ancestor of every entry, and - // sweeping the directory cache on every miss, put this past 12x. That is what made real - // home directories exceed the client request timeout. - const ratio = scanMs / readMs; - expect({ ratio: Math.round(ratio), withinBudget: ratio < 6 }).toEqual({ - ratio: expect.any(Number), - withinBudget: true, - }); - }, 60_000); + 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/path.ts b/packages/server/src/utils/path.ts index d5f0d30c8c..3c225f04b2 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; }