test(go-scope-capture): address code-review findings

Self-review (ce-code-review) polish on the #1848 fix + benchmark:

- benchmark: tighten the scaling guard from timeRatio/fileRatio < 3 to < 1.5.
  At the 2.5x/2x scale steps, a quadratic regression yields ratio == fileRatio
  (2.5, 2.0), which < 3 waved through — the guard could not detect the O(n^2)
  it exists for. Measured O(n) ratios are 0.45/0.59, so < 1.5 has headroom.
- benchmark: add a non-gated O(n^2) regression tripwire that calls
  emitGoScopeCaptures on a 400-struct source directly (no worker, no
  GITNEXUS_BENCH gate) so the regression is actually guarded in CI.
- benchmark: clearTimeout the Promise.race timer in finally (no lingering
  rejection); set the worker-suite env vars inside the try so finally always
  restores them.
- captures.ts: clarify the isRawMultiAssignTypeBinding comment to name both
  var-form cases (assertion + call-return). Comment-only.

Left as-is: resolveImportNode's defensive range-equality branch — deleting it
as dead code would remove the self-documentation of the grammar invariant the
threaded-node logic depends on (reviewer tension; a wash).

Verified: tsc clean; 165/165 Go resolver + scope tests; new tripwire passes
(237ms); scaling suite passes at <1.5; #1848 worker suite still green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Gergo Magyar 2026-05-30 07:33:54 +00:00
parent eaf0a3052a
commit ed33489e38
2 changed files with 79 additions and 12 deletions

View file

@ -203,10 +203,13 @@ function isRawMultiAssignTypeBinding(nodeMap: Record<string, SyntaxNode>): boole
nodeMap['@type-binding.assertion'];
if (anchor === undefined) return false;
// Original looked up a short_var_declaration at the anchor's range; these
// tags are captured directly on the short_var_declaration (except the
// var_declaration form of @type-binding.assertion, which the old range+type
// lookup would have missed -> null -> false).
// These tags are captured directly ON the short_var_declaration, so the
// captured node IS what the original findNodeAtRange(root, range,
// 'short_var_declaration') re-derived. The var_declaration (var-form)
// variants — @type-binding.assertion (`var x = e.(T)`) and
// @type-binding.call-return (`var x = Func()`) — anchor on a var_declaration
// instead; the old range+type lookup found no short_var_declaration at that
// range and returned null -> false, which this type guard reproduces exactly.
if (anchor.type !== 'short_var_declaration') return false;
const node = anchor;
const lhs = node.childForFieldName('left');

View file

@ -33,6 +33,7 @@ import os from 'node:os';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import { runPipelineFromRepo } from '../../src/core/ingestion/pipeline.js';
import { emitGoScopeCaptures } from '../../src/core/ingestion/languages/go/index.js';
const BENCH_ENABLED = process.env.GITNEXUS_BENCH === '1';
@ -182,16 +183,20 @@ async function runBenchmark(
if (heap > peakHeapMB) peakHeapMB = heap;
}, 50);
let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
try {
const start = Date.now();
const result = await Promise.race([
runPipelineFromRepo(dir, () => {}, { skipGraphPhases: true }),
new Promise<never>((_, reject) =>
setTimeout(
new Promise<never>((_, reject) => {
// Hold the handle so the winning (pipeline) path can cancel it in the
// finally — otherwise the timer lingers for up to budgetMs and its
// late rejection surfaces as an unhandled promise rejection.
timeoutHandle = setTimeout(
() => reject(new Error(`Pipeline exceeded ${budgetMs}ms at ${fileCount} files`)),
budgetMs,
),
),
);
}),
]);
const elapsedMs = Date.now() - start;
@ -205,6 +210,7 @@ async function runBenchmark(
edgeCount: result.graph.relationshipCount,
};
} finally {
if (timeoutHandle !== undefined) clearTimeout(timeoutHandle);
clearInterval(heapSampler);
fs.rmSync(dir, { recursive: true, force: true });
}
@ -307,7 +313,13 @@ describe.skipIf(!BENCH_ENABLED)('Go pipeline benchmark', () => {
for (let i = 1; i < results.length; i++) {
const fileRatio = results[i].fileCount / results[i - 1].fileCount;
const timeRatio = results[i].elapsedMs / results[i - 1].elapsedMs;
expect(timeRatio / fileRatio).toBeLessThan(3);
// The scale steps are 2.5x (100->250) and 2x (250->500). A quadratic
// regression makes timeRatio ~= fileRatio^2, i.e. timeRatio/fileRatio ~=
// fileRatio (2.5 and 2.0) — which a `< 3` bound would wave through. The
// O(n) path keeps this ratio ~1 (measured 0.44 and 0.75 post-fix), so a
// `< 1.5` bound (the printResults "linear" boundary) actually fails on a
// re-regression to O(n^2) while leaving comfortable headroom for linear.
expect(timeRatio / fileRatio).toBeLessThan(1.5);
}
}, 300_000);
});
@ -334,11 +346,11 @@ describe.skipIf(!BENCH_ENABLED || !DIST_WORKER_AVAILABLE)(
const { dir, bigFileBytes, fileCount } = generateGoQuarantineFixture(entityCount, 14);
// Sub-batch knobs that force fine chunking onto the worker (env-only —
// there is no PipelineOptions field for these two).
// there is no PipelineOptions field for these two). Capture the prior
// values before the try; set them as the first statements INSIDE it so
// the finally's restore covers every path that mutated the process env.
const prevMaxBytes = process.env.GITNEXUS_WORKER_SUB_BATCH_MAX_BYTES;
const prevTimeout = process.env.GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS;
process.env.GITNEXUS_WORKER_SUB_BATCH_MAX_BYTES = '262144';
process.env.GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS = String(subBatchTimeoutMs);
let peakHeapMB = 0;
const heapSampler = setInterval(() => {
@ -347,6 +359,8 @@ describe.skipIf(!BENCH_ENABLED || !DIST_WORKER_AVAILABLE)(
}, 50);
try {
process.env.GITNEXUS_WORKER_SUB_BATCH_MAX_BYTES = '262144';
process.env.GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS = String(subBatchTimeoutMs);
const start = Date.now();
const result = await runPipelineFromRepo(dir, () => {}, {
skipGraphPhases: true,
@ -390,3 +404,53 @@ describe.skipIf(!BENCH_ENABLED || !DIST_WORKER_AVAILABLE)(
}, 360_000);
},
);
/**
* Unlike the two suites above, this one is NOT gated behind GITNEXUS_BENCH and
* needs no compiled worker — so it runs in normal CI and is the actual guard
* against an O(n^2) re-regression of emitGoScopeCaptures (issue #1848). It calls
* the hotpath directly on a ~400-struct generated source. The O(n) path does
* this in a few hundred ms; the old findNodeAtRange-from-root behaviour took
* ~25s+ at this size. The budget is a coarse tripwire (huge margin over the
* fixed path, far below a quadratic regression), not a microbenchmark — keep it
* generous so it never flakes on a loaded CI runner.
*/
describe('Go scope-capture O(n^2) regression tripwire', () => {
function generateGoStructSource(structCount: number): string {
const lines = ['package generated', ''];
for (let i = 0; i < structCount; i++) {
const n = String(i).padStart(4, '0');
lines.push(
`type Item${n} struct {`,
'\tid int64',
'\tname string',
'}',
'',
`func (d *Item${n}) GetID() int64 { return d.id }`,
`func (d *Item${n}) SetID(id int64) { d.id = id }`,
`func (d *Item${n}) GetName() string { return d.name }`,
`func (d *Item${n}) Validate() error { return nil }`,
'',
);
}
return lines.join('\n');
}
it('parses a 400-struct file in well under the O(n^2) tripwire budget', () => {
const STRUCT_COUNT = 400;
const BUDGET_MS = 10_000; // coarse: ~40x the fixed path, ~3x under a quadratic regression
const src = generateGoStructSource(STRUCT_COUNT);
emitGoScopeCaptures(src, 'tripwire-warmup.go'); // warm up the parser/query JIT
const start = Date.now();
const matches = emitGoScopeCaptures(src, 'tripwire.go');
const elapsedMs = Date.now() - start;
// Sanity: the captures are actually produced (each struct + 4 methods emits
// far more than 10 capture groups), so a fast-but-empty result can't pass.
expect(matches.length).toBeGreaterThan(STRUCT_COUNT * 10);
// The actual regression guard: a re-regression to O(n^2) blows this budget.
expect(elapsedMs).toBeLessThan(BUDGET_MS);
}, 30_000);
});