From f7120f8547c0325e176a7daeccf47b72fe04e301 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Wed, 20 May 2026 09:10:51 +0100 Subject: [PATCH] refactor(parse-impl): move chunk-byte-budget env read to function scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves PR #1693 review F7 / U14: pre-U14, `CHUNK_BYTE_BUDGET` was a module-load IIFE constant that captured `GITNEXUS_CHUNK_BYTE_BUDGET` once and froze the value for the module's lifetime. That defeated per-call option threading (a future `PipelineOptions.chunkByteBudget` was silently no-op'd because the function body read the frozen module-level constant) AND forced tests to use `vi.resetModules` to vary chunk layout. The U7 deferred-extraction test and the U6 multi-chunk integration test both used the workaround. After this change: - `DEFAULT_CHUNK_BYTE_BUDGET = 2 * 1024 * 1024` stays as a module-level constant — purely a default, no env access. - `resolveChunkByteBudget(options)` runs per call: option wins, then env, then default. Same options-first/env-fallback/default pattern as resolveAutoPoolSize and the U1 parseChunkConcurrency resolver — keeps the ingestion code's configuration model uniform. - `PipelineOptions.chunkByteBudget?` added with documentation that threading through options lets long-running hosts (eval-server, MCP daemon) size per-call without leaking process.env state across analyze invocations. New test (parse-impl-env-reads.test.ts) pins all four behaviors: 1. option-first: option present + env present -> option wins 2. env-fallback: option absent + env present -> env wins 3. default-fallback: both absent -> 2 MB default 4. per-call: two back-to-back runs in the same vitest worker with different chunkByteBudget option values observe their OWN values, proving the module-load freeze is gone (no vi.resetModules in this test — that's the invariant being verified). All four assertions use exact `.toBe(N)` per DoD §2.7. The chunk count is observed by parsing the `Parsing chunk X/Y` progress message stream — a stable proxy that doesn't require exposing internal parse-impl counter state. Note: U7 and U6 tests still use `vi.resetModules` because they were written before this change. A follow-up cleanup could simplify those tests (drop the resetModules dance, pass chunkByteBudget via options), but they pass as-is so this commit doesn't touch them. --- .../ingestion/pipeline-phases/parse-impl.ts | 32 +++- gitnexus/src/core/ingestion/pipeline.ts | 13 ++ .../test/unit/parse-impl-env-reads.test.ts | 147 ++++++++++++++++++ 3 files changed, 186 insertions(+), 6 deletions(-) create mode 100644 gitnexus/test/unit/parse-impl-env-reads.test.ts diff --git a/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts b/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts index 99ef89ec1..a1ada9975 100644 --- a/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts +++ b/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts @@ -87,11 +87,24 @@ import { logger } from '../../logger.js'; * gives a useful invalidation floor (~1/N chunks on a multi-MB repo) * while keeping worker dispatch overhead under 5% on cold runs. */ -const CHUNK_BYTE_BUDGET = (() => { +/** + * Built-in chunk byte budget when neither `PipelineOptions.chunkByteBudget` + * nor `GITNEXUS_CHUNK_BYTE_BUDGET` is set. Tuned to give a useful + * cache-invalidation floor (~1/N chunks on a multi-MB repo) while keeping + * worker dispatch overhead under 5% on cold runs. Resolution happens at + * call time inside `runChunkedParseAndResolve` (U14 from PR #1693 review) + * — previously this was a module-load IIFE, which froze the env value at + * import time and meant per-call option threading silently no-op'd. + */ +const DEFAULT_CHUNK_BYTE_BUDGET = 2 * 1024 * 1024; + +function resolveChunkByteBudget(options?: PipelineOptions): number { + const opt = options?.chunkByteBudget; + if (typeof opt === 'number' && Number.isFinite(opt) && opt > 0) return opt; const env = Number(process.env.GITNEXUS_CHUNK_BYTE_BUDGET); if (Number.isFinite(env) && env > 0) return env; - return 2 * 1024 * 1024; -})(); + return DEFAULT_CHUNK_BYTE_BUDGET; +} // ── Main parse + resolve function ────────────────────────────────────────── @@ -188,12 +201,19 @@ export async function runChunkedParseAndResolve( }); } - // Build byte-budget chunks + // Build byte-budget chunks. The budget is resolved per-call (U14): options + // first, then env, then the built-in default. Pre-U14 this was a + // module-load IIFE constant, which froze the env value at import time + // and made `PipelineOptions.chunkByteBudget` silently no-op on warm test + // runs. Resolving in the function body restores per-call configurability + // and matches the pattern used by resolveAutoPoolSize and the U1 + // parseChunkConcurrency resolver. + const chunkByteBudget = resolveChunkByteBudget(options); const chunks: string[][] = []; let currentChunk: string[] = []; let currentBytes = 0; for (const file of parseableScanned) { - if (currentChunk.length > 0 && currentBytes + file.size > CHUNK_BYTE_BUDGET) { + if (currentChunk.length > 0 && currentBytes + file.size > chunkByteBudget) { chunks.push(currentChunk); currentChunk = []; currentBytes = 0; @@ -208,7 +228,7 @@ export async function runChunkedParseAndResolve( if (isDev) { const totalMB = parseableScanned.reduce((s, f) => s + f.size, 0) / (1024 * 1024); logger.info( - `📂 Scan: ${totalFiles} paths, ${totalParseable} parseable (${totalMB.toFixed(0)}MB), ${numChunks} chunks @ ${CHUNK_BYTE_BUDGET / (1024 * 1024)}MB budget`, + `📂 Scan: ${totalFiles} paths, ${totalParseable} parseable (${totalMB.toFixed(0)}MB), ${numChunks} chunks @ ${chunkByteBudget / (1024 * 1024)}MB budget`, ); } diff --git a/gitnexus/src/core/ingestion/pipeline.ts b/gitnexus/src/core/ingestion/pipeline.ts index eefbd87db..415388815 100644 --- a/gitnexus/src/core/ingestion/pipeline.ts +++ b/gitnexus/src/core/ingestion/pipeline.ts @@ -95,6 +95,19 @@ export interface PipelineOptions { * env var when undefined; defaults to 2 when neither is set. */ parseChunkConcurrency?: number; + /** + * Byte budget per parse chunk (in bytes). When set, parse-impl uses + * this instead of the `GITNEXUS_CHUNK_BYTE_BUDGET` env var or the + * built-in 2 MB default. Smaller values produce more chunks (finer + * cache-hit granularity, more worker dispatches); larger values + * batch more files per dispatch. + * + * Threading the value through options instead of the env var lets + * tests vary the chunk layout per-call without `vi.resetModules` and + * lets long-running hosts (eval-server, MCP daemon) size per-call + * without leaking `process.env` state across invocations. + */ + chunkByteBudget?: number; } // ── Phase registry ───────────────────────────────────────────────────────── diff --git a/gitnexus/test/unit/parse-impl-env-reads.test.ts b/gitnexus/test/unit/parse-impl-env-reads.test.ts new file mode 100644 index 000000000..9294f94a6 --- /dev/null +++ b/gitnexus/test/unit/parse-impl-env-reads.test.ts @@ -0,0 +1,147 @@ +/** + * U14 (F7 architectural from PR #1693 review) — Function-scope env reads + * in parse-impl. + * + * Pre-U14, `CHUNK_BYTE_BUDGET` was a module-load IIFE constant that + * captured `GITNEXUS_CHUNK_BYTE_BUDGET` once and froze the value for + * the module's lifetime. That defeated `PipelineOptions.chunkByteBudget` + * (silently no-op'd because the body read the frozen constant) AND + * forced tests to use `vi.resetModules` to vary the chunk layout (see + * the U7 deferred-extraction test and the U6 multi-chunk integration + * test for examples of the workaround). + * + * After U14: + * - Option present -> option wins (per-call, no env / no vi.resetModules) + * - Option absent -> env wins (back-compat) + * - Both absent -> built-in 2 MB default + * + * This file pins all three resolution branches, plus the behavioral + * invariant the workaround was masking: two back-to-back runs in the + * same vitest worker process can use DIFFERENT `chunkByteBudget` values + * and observe DIFFERENT chunking on the same fixture WITHOUT needing + * `vi.resetModules` between them. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { runChunkedParseAndResolve } from '../../src/core/ingestion/pipeline-phases/parse-impl.js'; +import { createKnowledgeGraph } from '../../src/core/graph/graph.js'; + +const ORIGINAL_BUDGET = process.env.GITNEXUS_CHUNK_BYTE_BUDGET; + +type Fixture = Record; + +function makeRepo(fixture: Fixture): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'parse-impl-env-reads-')); + for (const [name, content] of Object.entries(fixture)) { + fs.writeFileSync(path.join(dir, name), content); + } + return dir; +} + +function scanned(repo: string, files: string[]) { + return files.map((rel) => ({ + path: rel, + size: fs.statSync(path.join(repo, rel)).size, + })); +} + +/** + * Capture every per-chunk progress message emitted during a run. + * parse-impl emits one per chunk in the "Parsing chunk X/Y" form, so + * counting unique chunk indices in the captured stream is a stable + * proxy for the number of chunks the loop actually produced. Avoids + * exposing internal counter state from parse-impl. + */ +async function countChunksFromProgress( + repoPath: string, + files: string[], + options?: { chunkByteBudget?: number }, +): Promise { + const scan = scanned(repoPath, files); + const graph = createKnowledgeGraph(); + const chunkIndices = new Set(); + await runChunkedParseAndResolve( + graph, + scan, + files, + files.length, + repoPath, + Date.now(), + (p) => { + if (typeof p.message !== 'string') return; + const m = /Parsing chunk (\d+)\/(\d+)/.exec(p.message); + if (m !== null) chunkIndices.add(`${m[1]}/${m[2]}`); + }, + { skipWorkers: true, ...options }, + ); + return chunkIndices.size; +} + +describe('parse-impl chunkByteBudget resolution (U14 / F7)', () => { + let repoPath = ''; + + beforeEach(() => { + repoPath = makeRepo({ + 'a.ts': 'export const A = 1;\n', + 'b.ts': 'export const B = 2;\n', + 'c.ts': 'export const C = 3;\n', + }); + }); + + afterEach(() => { + if (repoPath && fs.existsSync(repoPath)) { + fs.rmSync(repoPath, { recursive: true, force: true }); + } + if (ORIGINAL_BUDGET === undefined) { + delete process.env.GITNEXUS_CHUNK_BYTE_BUDGET; + } else { + process.env.GITNEXUS_CHUNK_BYTE_BUDGET = ORIGINAL_BUDGET; + } + }); + + it('option-first: PipelineOptions.chunkByteBudget overrides the env var', async () => { + // Force the env to a HUGE value that would normally collapse the + // fixture to a single chunk; pass a SMALL option that produces 3 + // chunks. If the option wins, we observe 3 chunks; if env wins, 1. + process.env.GITNEXUS_CHUNK_BYTE_BUDGET = String(10 * 1024 * 1024); + const chunks = await countChunksFromProgress(repoPath, ['a.ts', 'b.ts', 'c.ts'], { + chunkByteBudget: 8, + }); + expect(chunks).toBe(3); + }); + + it('env-fallback: GITNEXUS_CHUNK_BYTE_BUDGET is honored when the option is absent', async () => { + process.env.GITNEXUS_CHUNK_BYTE_BUDGET = '8'; + const chunks = await countChunksFromProgress(repoPath, ['a.ts', 'b.ts', 'c.ts']); + expect(chunks).toBe(3); + }); + + it('default-fallback: large built-in budget keeps the fixture in a single chunk', async () => { + // Both option and env unset → falls through to DEFAULT_CHUNK_BYTE_BUDGET + // (2 MB). The fixture totals well under that, so exactly one chunk. + delete process.env.GITNEXUS_CHUNK_BYTE_BUDGET; + const chunks = await countChunksFromProgress(repoPath, ['a.ts', 'b.ts', 'c.ts']); + expect(chunks).toBe(1); + }); + + it('per-call: two back-to-back runs with different option values observe their own values, not the previous call', async () => { + // The behavioral invariant U14 restores: a long-running host + // (eval-server, MCP daemon) calling runChunkedParseAndResolve twice + // with different chunkByteBudget values gets the value it passed, + // not whatever the first call set (pre-U14, the module-load IIFE + // froze the value at import — the option was a silent no-op). + const files = ['a.ts', 'b.ts', 'c.ts']; + delete process.env.GITNEXUS_CHUNK_BYTE_BUDGET; + const small = await countChunksFromProgress(repoPath, files, { + chunkByteBudget: 8, + }); + const large = await countChunksFromProgress(repoPath, files, { + chunkByteBudget: 10 * 1024 * 1024, + }); + expect(small).toBe(3); + expect(large).toBe(1); + }); +});