mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-10 03:27:59 +00:00
refactor(parse-impl): move chunk-byte-budget env read to function scope
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.
This commit is contained in:
parent
25ef6704a4
commit
f7120f8547
3 changed files with 186 additions and 6 deletions
|
|
@ -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`,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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 ─────────────────────────────────────────────────────────
|
||||
|
|
|
|||
147
gitnexus/test/unit/parse-impl-env-reads.test.ts
Normal file
147
gitnexus/test/unit/parse-impl-env-reads.test.ts
Normal file
|
|
@ -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<string, string>;
|
||||
|
||||
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<number> {
|
||||
const scan = scanned(repoPath, files);
|
||||
const graph = createKnowledgeGraph();
|
||||
const chunkIndices = new Set<string>();
|
||||
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);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue