From cf82ee8b4ccdf568bff28f6c54525ffed115d0cd Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:40:13 +0000 Subject: [PATCH] review-collect: keep the consumer scan deadline under the 180 s plan timeout (review fix) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-544-consumer-scan-timeout.md | 4 ++-- docs/review-policy.md | 5 +++-- lib/review-collect.ts | 9 ++++++--- test/review-collect.test.ts | 14 +++++++++++--- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/changelog.d/gst-544-consumer-scan-timeout.md b/changelog.d/gst-544-consumer-scan-timeout.md index cb1d242e7..71262c329 100644 --- a/changelog.d/gst-544-consumer-scan-timeout.md +++ b/changelog.d/gst-544-consumer-scan-timeout.md @@ -2,7 +2,7 @@ ### 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. +- **One deadline for the whole scan:** 120 seconds by default, a minute under the 180 seconds /ship gives the whole planner, 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. +- `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 default deadline stays a minute under the planner timeout in `gstack-review-dispatch` and `gstack-ship-preflight`. The new behavior tests failed before the fix. diff --git a/docs/review-policy.md b/docs/review-policy.md index 3c27e6e53..7c0542b56 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -180,8 +180,9 @@ consumer each keep the path code, and YAML, TOML and SVG are always code. More t 2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed` 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 +costs one cheap pass. The whole scan gets one 120-second deadline, a minute under the +180 seconds /ship gives `gstack-review-plan`, 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 diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 7807143d5..ca052d958 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -104,9 +104,12 @@ function readDiff(root: string, range: string[], errors: DepthError[]): Pick[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\)$/); + expect(scanConsumers(r.dir, ['data/rows.json'], [r.head], false, r.dir, failed, CONSUMER_SCAN_TIMEOUT_MS)).toBe('all'); + expect(failed[0]!.detail).toMatch(new RegExp(`^git grep exited \\d+: .*\\(deadline ${CONSUMER_SCAN_TIMEOUT_MS / 1000} s\\)$`)); + }); + // Value: protects=a slow scan ends in its own fail-closed consumer_scan_failed, not in the parent killing gstack-review-plan; + // fails_when=the scan deadline reaches the timeout the dispatch and preflight callers give the whole plan; seam=caller source + test('the default scan deadline leaves a minute of the plan timeout its callers set', () => { + const callers = ['bin/gstack-review-dispatch', 'lib/ship-preflight.ts'].map(f => fs.readFileSync(path.join(import.meta.dir, '..', f), 'utf8')); + const limits = callers.flatMap(src => [...src.matchAll(/gstack-review-plan'[^\n]*?(?:timeout: |\], )(\d[\d_]+)/g)].map(m => Number(m[1]!.replace(/_/g, '')))); + expect(limits.length).toBeGreaterThanOrEqual(2); + expect(CONSUMER_SCAN_TIMEOUT_MS).toBeLessThanOrEqual(Math.min(...limits) - 60_000); }); });