diff --git a/changelog.d/gst-544-consumer-scan-timeout.md b/changelog.d/gst-544-consumer-scan-timeout.md new file mode 100644 index 000000000..cb1d242e7 --- /dev/null +++ b/changelog.d/gst-544-consumer-scan-timeout.md @@ -0,0 +1,8 @@ +**The review planner no longer fails closed on repositories with big data trees just because its consumer search ran past 60 seconds (GST-544).** `gstack-review-plan` looks for code that names each changed data file. It used to run one `git grep` over the whole tree with every data file name, and each call had a hard 60-second limit. In garrytan/harness-evals, with 8,000+ result files, that search took about 150 seconds, so the plan fell back to a full review every time. + +### Changed +- **The consumer scan searches only files that can match** (`lib/review-collect.ts`). A cheap first pass lists the files that contain a changed file's extension, such as `.json`; the full search then reads only those files, a batch of paths per `git grep` call, with docs files left out. The result is the same as before. +- **One deadline for the whole scan:** 300 seconds by default, set in milliseconds with `GSTACK_CONSUMER_SCAN_TIMEOUT_MS` (docs/review-policy.md#review-depth). Past it the depth still fails closed with `consumer_scan_failed`, and the detail now says whether git timed out or exited with an error, and names the deadline. Other git reads keep their 60-second limit. + +### Tests +- `test/review-collect.test.ts`: the narrowed search prints the same lines as one whole-tree `git grep` at the head, the merge base and the working tree; the same holds when every file is its own call; consumed sets on a mixed fixture (a chain through a data manifest, an untracked consumer, docs mentions ignored); a tiny deadline fails closed and says it timed out. The three new behavior tests failed before the fix. diff --git a/docs/review-policy.md b/docs/review-policy.md index 301d187e5..3c27e6e53 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -178,7 +178,11 @@ directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`, `manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named consumer each keep the path code, and YAML, TOML and SVG are always code. More than 2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed` -and fails the depth closed. Only `classifyDiff` applies the rule (`lib/review-collect.ts` +and fails the depth closed. The scan first lists the files that contain a candidate's +extension and then searches only those, docs files left out, so a large data tree +costs one cheap pass. The whole scan gets one 300-second deadline, set in milliseconds +with `GSTACK_CONSUMER_SCAN_TIMEOUT_MS`; a scan that runs past it is also +`consumer_scan_failed`, and its detail says it timed out and names the deadline. Only `classifyDiff` applies the rule (`lib/review-collect.ts` finds the consumers, `gstack-ship-preflight`'s `RUNTIME_LINES` reads the same function); every path-only caller (the receipt's gate-scan decision, the Step 16 post-review route, /autoplan's docs-only scope) keeps data files code. A numeric claim diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 7fbfdae12..7807143d5 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -34,6 +34,13 @@ function git(root: string, args: string[]): { ok: boolean; status: number | null const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); return { ok: r.status === 0, status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim() }; } +/** git with what is left of an overall deadline (epoch ms) as its timeout, for a read that may take minutes. */ +function gitUntil(root: string, args: string[], deadline: number): { status: number | null; out: string; err: string; timedOut: boolean } { + const left = deadline - Date.now(); + if (left <= 0) return { status: null, out: '', err: '', timedOut: true }; + const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, encoding: 'utf8', timeout: left, maxBuffer: 256 * 1024 * 1024 }); + return { status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim(), timedOut: r.status === null && Date.now() >= deadline }; +} const scratch = (p: string) => p.startsWith('.gstack/tmp/'); const oneLine = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').trim().slice(0, 200); const unavailable = (input: string, detail: string): RegistryInfo => ({ sha: null, outputs: [], error: { code: 'registry_unavailable', detail: oneLine(detail), input, repair: 'fetch the base branch (git fetch origin ) so its commit and registry resolve' } }); @@ -97,36 +104,81 @@ function readDiff(root: string, range: string[], errors: DepthError[]): Pick 0 ? v : CONSUMER_SCAN_TIMEOUT_MS; +} + +export type GrepFailure = { timedOut: boolean; status: number | null; err: string }; + +/** + * The lines `git grep -I -F -o --null` prints for `patterns` at `rev` ('' is the working tree with untracked files), for + * the files `keep` accepts, in some order. Every pattern holds its own extension, so a `-l` pass for the extensions alone + * lists every file that can match; the full pattern list then reads only those files, CONSUMER_SCAN_PATHSPEC_BYTES of + * literal pathspecs per call. Each call gets what is left of `deadline` (epoch ms). + */ +export function grepConsumers(root: string, patterns: readonly string[], rev: string, keep: (file: string) => boolean, deadline: number, chunkBytes = CONSUMER_SCAN_PATHSPEC_BYTES): string[] | GrepFailure { + const where = rev ? [rev, '--'] : ['--untracked', '--']; + const exts = [...new Set(patterns.map(b => b.slice(b.lastIndexOf('.'))))].flatMap(e => ['-e', e]); + const listed = gitUntil(root, ['grep', '-I', '-F', '-l', '-z', '--no-color', ...exts, ...where], deadline); + if (listed.status !== 0 && listed.status !== 1) return listed; + const files = listed.out.split('\0').filter(Boolean).map(f => (rev ? f.slice(rev.length + 1) : f)).filter(keep); + const flags = ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...where]; + const lines: string[] = []; + for (let i = 0; i < files.length;) { + const chunk: string[] = []; + for (let bytes = 0; i < files.length; i++) { + const spec = `:(literal)${files[i]}`; + if (chunk.length && bytes + spec.length > chunkBytes) break; + chunk.push(spec); + bytes += spec.length; + } + const r = gitUntil(root, [...flags, ...chunk], deadline); + if (r.status !== 0 && r.status !== 1) return r; + lines.push(...r.out.split('\n')); + } + return lines; +} /** * The data candidates (lib/review-depth.ts dataCandidate) among the changed and deleted paths that a non-docs file names * by basename, at each rev and, with `live`, in the working tree with untracked files. The merge base is one of the revs, * so a consumer the change removes still counts. A data file named only by another data file is consumed when that one - * is, followed CONSUMER_SCAN_DEPTH files deep; anything still open after that counts as consumed. A git grep failure is - * a collector error, which fails the depth closed. + * is, followed CONSUMER_SCAN_DEPTH files deep; anything still open after that counts as consumed. A git grep failure, or + * a scan that outlives `timeoutMs`, is a collector error, which fails the depth closed. */ -export function scanConsumers(root: string, paths: readonly string[], revs: readonly string[], live: boolean, classRoot: string | readonly string[], errors: DepthError[]): string[] | 'all' { +export function scanConsumers(root: string, paths: readonly string[], revs: readonly string[], live: boolean, classRoot: string | readonly string[], errors: DepthError[], timeoutMs = consumerScanTimeoutMs()): string[] | 'all' { const candidates = [...new Set(paths)].filter(p => dataCandidate(p, classRoot)).sort(); if (candidates.length > CONSUMER_SCAN_CAP) return 'all'; + const deadline = Date.now() + timeoutMs; const base = (p: string) => p.slice(p.lastIndexOf('/') + 1); + const read = (file: string) => !scratch(file) && classifyPath(file, classRoot) !== 'docs'; const named = new Map>(); const direct = new Set(); const probed = new Set(); let probe = candidates; for (let depth = 0; probe.length && depth < CONSUMER_SCAN_DEPTH; depth++) { - const patterns = [...new Set(probe.map(base))].flatMap(b => ['-e', b]); - const runs = [...revs.map(rev => ({ at: rev, args: [rev, '--'] })), ...(live ? [{ at: '', args: ['--untracked', '--'] }] : [])]; + const patterns = [...new Set(probe.map(base))]; const next = new Set(); - for (const run of runs) { - const r = git(root, ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns, ...run.args]); - if (r.status !== 0 && r.status !== 1) { errors.push({ code: 'consumer_scan_failed', detail: oneLine(r.err || `git grep exited ${r.status}`), input: `git grep -F ${run.at || '--untracked'}`, repair: 'run the git grep by hand and fix what git reports' }); return 'all'; } - for (const line of r.out.split('\n')) { + for (const at of [...revs, ...(live ? [''] : [])]) { + const r = grepConsumers(root, patterns, at, read, deadline); + if (!Array.isArray(r)) { + const detail = r.timedOut ? `timed out: the scan's ${timeoutMs / 1000} s deadline passed (GSTACK_CONSUMER_SCAN_TIMEOUT_MS)` : `git grep exited ${r.status}${r.err ? `: ${r.err}` : ''} (deadline ${timeoutMs / 1000} s)`; + errors.push({ code: 'consumer_scan_failed', detail: oneLine(detail), input: `git grep -F ${at || '--untracked'}`, repair: r.timedOut ? 'raise GSTACK_CONSUMER_SCAN_TIMEOUT_MS (milliseconds) or run the git grep by hand to see why it is slow' : 'run the git grep by hand and fix what git reports' }); + return 'all'; + } + for (const line of r) { const parts = line.split('\0'); const where = parts[0]; const match = parts.length > 1 ? parts[parts.length - 1] : undefined; if (!where || !match) continue; - const file = run.at ? where.slice(run.at.length + 1) : where; - if (scratch(file) || classifyPath(file, classRoot) === 'docs') continue; + const file = at ? where.slice(at.length + 1) : where; + if (!read(file)) continue; for (const p of probe) { if (file === p || !match.includes(base(p))) continue; if (!dataCandidate(file, classRoot)) { direct.add(p); continue; } diff --git a/test/review-collect.test.ts b/test/review-collect.test.ts index 1e3e180cc..7a7e478d4 100644 --- a/test/review-collect.test.ts +++ b/test/review-collect.test.ts @@ -3,7 +3,7 @@ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { collectCommit, collectLive, CONSUMER_SCAN_CAP, loadRegistryAt } from '../lib/review-collect'; +import { collectCommit, collectLive, CONSUMER_SCAN_CAP, grepConsumers, loadRegistryAt, scanConsumers } from '../lib/review-collect'; import { depthOfCommitInput } from '../lib/review-depth'; const dirs: string[] = []; @@ -177,3 +177,66 @@ describe('GSTA-167 part 4a: which data files code names (scanConsumers)', () => expect(depthOfCommitInput({ ...c, gateTagged: 0 }).class).toBe('code'); }, 60_000); }); + +describe('GST-544: the consumer scan reads only files that can match, under one overall deadline', () => { + const rows = Array.from({ length: 40 }, (_, i) => `{"i":${i}}`).join('\n') + '\n'; + const blob = Buffer.concat([Buffer.from([0, 1, 2]), Buffer.from(' rows.json post.json\n')]); + const mixed = () => { + const r = repo({ 'src/old.ts': "read('rules.json')\n", 'src/manifest.ts': "load('index.json')\n", 'docs/wiki.md': 'see orphan.json and rows.json\n', 'bench/blob.bin': blob, 'bench/notes.ts': 'no data names here, only .json and .csv\n' }); + const head = r.commit({ 'src/old.ts': null, 'src/load.ts': "require('./data/rows.json'); require('my rows.json')\n", 'data/rows.json': rows, 'data/my rows.json': rows, 'assets/rules.json': rows, 'assets/index.json': '["post.json"]\n', 'assets/post.json': rows, 'wiki/orphan.json': rows, 'wiki/live.csv': 'a\n', 'bench/overlap.ts': "'xrows.jsonl' 'ab.csv' 'b.csv'\n" }); + r.write({ 'src/untracked.ts': "read('live.csv')\n" }); + return { ...r, head }; + }; + const patterns = ['rows.json', 'my rows.json', 'rules.json', 'index.json', 'post.json', 'orphan.json', 'live.csv', 'b.csv']; + const wholeTree = (dir: string, at: string) => { + const r = spawnSync('git', ['-c', 'core.quotePath=false', 'grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...(at ? [at, '--'] : ['--untracked', '--'])], { cwd: dir, encoding: 'utf8', timeout: 10_000 }); + expect([0, 1]).toContain(r.status); + return r.stdout.split('\n').filter(Boolean).sort(); + }; + // Value: protects=the narrowed scan prints exactly the lines one whole-tree git grep prints, at a commit, at the merge base + // and in the working tree with untracked files; fails_when=the extension pre-pass or the pathspecs drop a consumer; + // seam=git grep output, compared with the single grep the scan ran before GST-544 + test('grepConsumers prints the same lines as one whole-tree git grep at each rev and in the working tree', () => { + const r = mixed(); + for (const at of [r.baseSha, r.head, '']) { + const got = grepConsumers(r.dir, patterns, at, () => true, Date.now() + 10_000); + expect(Array.isArray(got)).toBe(true); + expect((got as string[]).filter(Boolean).sort()).toEqual(wholeTree(r.dir, at)); + } + expect(wholeTree(r.dir, '').length).toBeGreaterThan(5); + }); + test('the pathspec chunks give the same lines when every file is its own git grep call', () => { + const r = mixed(); + const got = grepConsumers(r.dir, patterns, '', () => true, Date.now() + 10_000, 1); + expect((got as string[]).filter(Boolean).sort()).toEqual(wholeTree(r.dir, '')); + expect(new Set(wholeTree(r.dir, '').map(l => l.split('\0')[0])).size).toBeGreaterThan(3); + }); + test('consumed sets on the mixed fixture: chains through a data manifest, a live untracked consumer, docs ignored', () => { + const r = mixed(); + const commit = collectCommit(r.dir, r.baseSha, r.head); + expect([commit.consumed, commit.errors]).toEqual([['assets/index.json', 'assets/post.json', 'assets/rules.json', 'data/my rows.json', 'data/rows.json'], []]); + const live = collectLive(r.dir, 'main'); + expect([live.consumed, live.errors]).toEqual([['assets/index.json', 'assets/post.json', 'assets/rules.json', 'data/my rows.json', 'data/rows.json', 'wiki/live.csv'], []]); + }); + // Value: protects=a scan that runs out of time keeps every data file code and says it timed out, with the deadline; + // fails_when=a slow grep drops data to docs or reads as a git error; seam=the spawnSync timeout + test('a scan past its deadline fails closed with consumer_scan_failed naming the timeout and the deadline', () => { + const r = mixed(); + const errors: Parameters[5] = []; + expect(scanConsumers(r.dir, ['data/rows.json', 'wiki/orphan.json'], [r.head], true, r.dir, errors, 1)).toBe('all'); + expect(errors).toEqual([expect.objectContaining({ code: 'consumer_scan_failed', detail: expect.stringMatching(/timed out.*0\.001 s deadline.*GSTACK_CONSUMER_SCAN_TIMEOUT_MS/) })]); + const saved = process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS; + process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS = '1'; + try { + const c = collectCommit(r.dir, r.baseSha, r.head); + expect(c.consumed).toBe('all'); + expect(c.errors.map(e => [e.code, /timed out/.test(e.detail)])).toEqual([['consumer_scan_failed', true]]); + } finally { + if (saved === undefined) delete process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS; else process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS = saved; + } + r.git('config', 'grep.lineNumber', 'not-a-bool'); + const failed: Parameters[5] = []; + expect(scanConsumers(r.dir, ['data/rows.json'], [r.head], false, r.dir, failed)).toBe('all'); + expect(failed[0]!.detail).toMatch(/^git grep exited \d+: .*\(deadline 300 s\)$/); + }); +});