From d3c25d20931ac0eac535550cdd42a0fcc72c958c Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Thu, 9 Apr 2026 18:39:37 +0100 Subject: [PATCH] fix(SM-14): close sequential-path memory regression (Codex adversarial review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the medium-severity finding from Codex's adversarial review of commit 803631fe: the sequential path's `typeEnv.flush()` was still writing every scope (file + function) into the BindingAccumulator, and the accumulator stayed alive through Phase 14 and runGraphAnalysisPhases with no reader. On fallback runs (workers disabled or unavailable), large repos accumulated heap for nothing. Applies BOTH Codex remediations — narrowing AND disposal: R1 — Narrow typeEnv.flush() to file-scope only - The sequential path now mirrors the worker-path narrowing from commit 803631fe. `flush()` iterates only `env.get(FILE_SCOPE)` instead of the nested `for (scope, scopeMap) of env` loop, writing entries with `scope: ''` hardcoded. Function-scope bindings never reach the accumulator from either execution path until a Phase 9 consumer lands. - The earlier rationale for keeping sequential-path full-scope data ("preserve a Phase 9 prototyping sample") did not survive the Codex challenge — Phase 9 authors use synthetic fixtures, and a live-repo sample from the sequential-only path isn't representative of production worker-dominant runs. - Phase 9 reversion path documented inline at the flush() seam. R2 — Add BindingAccumulator.dispose() - New public method clears `_allByFile`, `_fileScopeByFile`, and explicitly resets `_totalBindings = 0` (feasibility review caught that clearing the maps alone would leave the `totalBindings` getter reporting stale counts). Idempotent and orthogonal to finalize() — calling dispose() doesn't change the finalized state. - Post-dispose contract: all read methods return empty/undefined state matching a never-appended accumulator. Documented in class JSDoc with the lifecycle sequence. - Used before finalize(): accumulator behaves like a fresh one, appends still succeed. - Used after finalize(): reads return empty but appends still throw the existing "finalized" error. R3 — Wire dispose() into the pipeline - Inserted immediately after the dev telemetry log at pipeline.ts line ~1723, before `runCrossFileBindingPropagation` (Phase 14) and `runGraphAnalysisPhases`. Sequence is: enrichment loop → finalize → telemetry → dispose → Phase 14 The telemetry log captures peak state before disposal, then the heap footprint is released for the long tail of graph analysis. - Verified both runCrossFileBindingPropagation and runGraphAnalysisPhases signatures do NOT take a bindingAccumulator parameter — grep confirmed the last usage is at line 1720. Tests (+6 scenarios) - test/unit/type-env.test.ts: existing "flushes function-scoped bindings into accumulator" test was rewritten as a negative assertion ("does NOT flush function-scoped bindings, narrowed per PR #743 Codex review"). Plus a new "narrows mixed file-scope and function-scope env to file-scope only" test that builds a realistic TypeScript file with both scopes and asserts only the file-scope entry lands in the accumulator. This is the R1 red/ green signal — both tests were written test-first and failed against the pre-narrowing flush() body. - test/unit/binding-accumulator.test.ts: new `describe('dispose', ...)` block with 5 scenarios — empty all read methods, idempotency, pre- finalize behavior, post-finalize behavior, and `estimateMemoryBytes() === 0` guard. Verification - `tsc --noEmit` clean - 3109 unit tests pass (+6 net: +3 narrowing tests — 2 new + 1 rewritten — plus +5 dispose scenarios − 1 pre-existing test replaced = net +6) - 1766 resolver integration tests pass - Zero regressions Plan: docs/plans/2026-04-09-005-fix-sm14-sequential-path-memory-regression-plan.md Codex review: branch diff against main, verdict needs-attention Previous commit: 803631fe (worker-path narrowing) --- .../src/core/ingestion/binding-accumulator.ts | 35 +++++++ gitnexus/src/core/ingestion/pipeline.ts | 11 +++ gitnexus/src/core/ingestion/type-env.ts | 26 +++++- .../test/unit/binding-accumulator.test.ts | 91 +++++++++++++++++++ gitnexus/test/unit/type-env.test.ts | 42 ++++++++- 5 files changed, 196 insertions(+), 9 deletions(-) diff --git a/gitnexus/src/core/ingestion/binding-accumulator.ts b/gitnexus/src/core/ingestion/binding-accumulator.ts index 306ef9d06..3d0c3708f 100644 --- a/gitnexus/src/core/ingestion/binding-accumulator.ts +++ b/gitnexus/src/core/ingestion/binding-accumulator.ts @@ -60,6 +60,7 @@ export class BindingAccumulator { private readonly _fileScopeByFile = new Map(); private _totalBindings = 0; private _finalized = false; + private _disposed = false; /** * Append bindings for a file. Safe to call multiple times for the same file. @@ -100,6 +101,40 @@ export class BindingAccumulator { this._finalized = true; } + /** + * Release the accumulator's heap footprint. Clears both internal storage + * maps and resets `_totalBindings` to zero. Idempotent and orthogonal to + * `finalize()` — calling `dispose()` does not change the finalized state. + * + * Post-dispose contract: all read methods return empty/undefined state + * matching a never-appended-to accumulator. Specifically: + * - `fileCount === 0` + * - `totalBindings === 0` + * - `files()` yields an empty iterator + * - `getFile(x)` returns `undefined` for all `x` + * - `fileScopeEntries(x)` returns `[]` for all `x` + * - `estimateMemoryBytes()` returns `0` + * + * If `dispose()` is called **before** `finalize()`, subsequent `appendFile` + * calls succeed — the accumulator behaves like a fresh one. If called + * **after** `finalize()`, subsequent `appendFile` calls throw the existing + * "finalized" error. + * + * Added in response to PR #743 Codex adversarial review (plan + * 2026-04-09-005): the pipeline disposes the accumulator after the + * ExportedTypeMap enrichment loop consumes its file-scope entries, so + * the heap is released before Phase 14 (`runCrossFileBindingPropagation`) + * and `runGraphAnalysisPhases` begin their long-running work. When Phase 9 + * wires a consumer into that stage, the dispose call should move later in + * the pipeline or be removed entirely. + */ + dispose(): void { + this._allByFile.clear(); + this._fileScopeByFile.clear(); + this._totalBindings = 0; + this._disposed = true; + } + /** Get all bindings for a file, or undefined if the file is unknown. */ getFile(filePath: string): readonly BindingEntry[] | undefined { return this._allByFile.get(filePath); diff --git a/gitnexus/src/core/ingestion/pipeline.ts b/gitnexus/src/core/ingestion/pipeline.ts index 5f6e55c0a..9f3c0c8e8 100644 --- a/gitnexus/src/core/ingestion/pipeline.ts +++ b/gitnexus/src/core/ingestion/pipeline.ts @@ -1721,6 +1721,17 @@ export const runPipelineFromRepo = async ( ); } + // PR #743 Codex adversarial review follow-up (plan 2026-04-09-005): + // Release the accumulator's heap footprint now. The ExportedTypeMap + // enrichment loop above is the only current consumer, and the dev + // telemetry log just captured peak state. Phase 14 and + // runGraphAnalysisPhases do not read the accumulator today — keeping + // it alive through those long-running phases pins heap for no reason. + // When Phase 9 wires a consumer into runCrossFileBindingPropagation, + // move this dispose() call to after that consumer completes or delete + // it entirely if the consumer takes lifecycle ownership. + bindingAccumulator.dispose(); + // ── Phase 14: Cross-file binding propagation (topological level sort) ── await runCrossFileBindingPropagation( graph, diff --git a/gitnexus/src/core/ingestion/type-env.ts b/gitnexus/src/core/ingestion/type-env.ts index 3e621ce58..211deddbd 100644 --- a/gitnexus/src/core/ingestion/type-env.ts +++ b/gitnexus/src/core/ingestion/type-env.ts @@ -1256,11 +1256,29 @@ export const buildTypeEnv = ( throw new Error(`TypeEnv.flush called twice for ${filePath} — flush is single-use`); } flushed = true; + // PR #743 Codex adversarial review follow-up (plan 2026-04-09-005): + // Narrow flush() to iterate only the FILE_SCOPE entry, mirroring the + // worker-path narrowing in parse-worker.ts (commit 803631fe). Before + // this change, both execution paths had the same asymmetry bug: the + // worker path was fixed but the sequential path (this code) still + // wrote function-scope entries into long-lived accumulator storage + // that no consumer reads until Phase 9 lands. + // + // Phase 9 reversion: when a downstream consumer of function-scope + // bindings exists, restore the nested iteration: + // + // for (const [scope, scopeMap] of env) { + // for (const [varName, typeName] of scopeMap) { + // entries.push({ scope, varName, typeName }); + // } + // } + // + // See BindingAccumulator class JSDoc and FileAllScopeBindings JSDoc in + // parse-worker.ts for the full reversion checklist. + const fileScope = env.get(FILE_SCOPE) ?? EMPTY_FILE_SCOPE; const entries: BindingEntry[] = []; - for (const [scope, scopeMap] of env) { - for (const [varName, typeName] of scopeMap) { - entries.push({ scope, varName, typeName }); - } + for (const [varName, typeName] of fileScope) { + entries.push({ scope: '', varName, typeName }); } if (entries.length > 0) { accumulator.appendFile(filePath, entries); diff --git a/gitnexus/test/unit/binding-accumulator.test.ts b/gitnexus/test/unit/binding-accumulator.test.ts index 93cf7e208..2df30aa82 100644 --- a/gitnexus/test/unit/binding-accumulator.test.ts +++ b/gitnexus/test/unit/binding-accumulator.test.ts @@ -498,4 +498,95 @@ describe('BindingAccumulator', () => { expect(exportedTypeMap.has('src/missing.ts')).toBe(false); }); }); + + // ------------------------------------------------------------------------- + // PR #743 Codex adversarial review follow-up (plan 2026-04-09-005): + // BindingAccumulator.dispose() releases the accumulator's heap footprint + // after the enrichment loop has consumed everything it needs. Post-dispose + // reads return empty/undefined without throwing, matching "never-appended" + // state. Idempotent and orthogonal to finalize(). + // ------------------------------------------------------------------------- + + describe('dispose', () => { + it('empties all read methods after dispose', () => { + const acc = new BindingAccumulator(); + acc.appendFile('src/a.ts', [ + { scope: '', varName: 'x', typeName: 'X' }, + { scope: 'fn@10', varName: 'y', typeName: 'Y' }, + ]); + acc.appendFile('src/b.ts', [{ scope: '', varName: 'z', typeName: 'Z' }]); + + // Sanity: pre-dispose state is populated. + expect(acc.fileCount).toBe(2); + expect(acc.totalBindings).toBe(3); + + acc.dispose(); + + // Post-dispose state: all read methods return empty/undefined. + expect(acc.fileCount).toBe(0); + expect(acc.totalBindings).toBe(0); + expect([...acc.files()]).toEqual([]); + expect(acc.getFile('src/a.ts')).toBeUndefined(); + expect(acc.getFile('src/b.ts')).toBeUndefined(); + expect(acc.fileScopeEntries('src/a.ts')).toEqual([]); + expect(acc.fileScopeEntries('src/b.ts')).toEqual([]); + }); + + it('is idempotent — calling twice is a no-op', () => { + const acc = new BindingAccumulator(); + acc.appendFile('src/a.ts', [{ scope: '', varName: 'x', typeName: 'X' }]); + acc.dispose(); + expect(() => acc.dispose()).not.toThrow(); + expect(acc.fileCount).toBe(0); + expect(acc.totalBindings).toBe(0); + }); + + it('works before finalize() — accumulator behaves like a fresh one after dispose', () => { + const acc = new BindingAccumulator(); + acc.appendFile('src/a.ts', [{ scope: '', varName: 'x', typeName: 'X' }]); + acc.dispose(); + // Not finalized, so appends still work post-dispose. + expect(() => + acc.appendFile('src/b.ts', [{ scope: '', varName: 'y', typeName: 'Y' }]), + ).not.toThrow(); + expect(acc.fileCount).toBe(1); + expect(acc.totalBindings).toBe(1); + expect(acc.getFile('src/b.ts')).toHaveLength(1); + expect(acc.getFile('src/a.ts')).toBeUndefined(); + }); + + it('works after finalize() — append still throws, reads return empty', () => { + const acc = new BindingAccumulator(); + acc.appendFile('src/a.ts', [{ scope: '', varName: 'x', typeName: 'X' }]); + acc.finalize(); + acc.dispose(); + // Finalized, so appends throw even post-dispose. + expect(() => + acc.appendFile('src/b.ts', [{ scope: '', varName: 'y', typeName: 'Y' }]), + ).toThrow(/finalized/); + // But reads return empty. + expect(acc.fileCount).toBe(0); + expect(acc.totalBindings).toBe(0); + expect(acc.getFile('src/a.ts')).toBeUndefined(); + }); + + it('estimateMemoryBytes drops to zero after dispose', () => { + const acc = new BindingAccumulator(); + // Populate a large batch to give the estimate a non-trivial baseline. + for (let i = 0; i < 100; i++) { + acc.appendFile(`src/file${i}.ts`, [ + { scope: '', varName: `var${i}a`, typeName: 'string' }, + { scope: '', varName: `var${i}b`, typeName: 'number' }, + ]); + } + const preDisposeBytes = acc.estimateMemoryBytes(); + expect(preDisposeBytes).toBeGreaterThan(0); + + acc.dispose(); + + // After dispose, the iteration over `_allByFile` in estimateMemoryBytes + // has zero files to walk, so the returned value is exactly 0. + expect(acc.estimateMemoryBytes()).toBe(0); + }); + }); }); diff --git a/gitnexus/test/unit/type-env.test.ts b/gitnexus/test/unit/type-env.test.ts index c8691f79e..5ec3cebe2 100644 --- a/gitnexus/test/unit/type-env.test.ts +++ b/gitnexus/test/unit/type-env.test.ts @@ -5878,7 +5878,14 @@ function process() { expect(userEntry!.scope).toBe(''); }); - it('flushes function-scoped bindings into accumulator', () => { + // PR #743 Codex adversarial review follow-up (plan 2026-04-09-005): + // flush() was narrowed to file-scope-only to match the worker-path + // narrowing in commit 803631fe. Function-scope entries are dropped at + // the flush seam and never reach the accumulator until a Phase 9 + // consumer lands. This test was previously the positive assertion that + // function-scope entries DID land in the accumulator; it is now a + // negative assertion guarding the narrowing. + it('does NOT flush function-scoped bindings into accumulator (narrowed per PR #743 Codex review)', () => { const code = `function process() {\n const result: Response = fetch();\n}`; const tree = parse(code, TypeScript.typescript); const typeEnv = buildTypeEnv(tree, 'typescript'); @@ -5886,12 +5893,37 @@ function process() { typeEnv.flush('/src/test.ts', acc); + // With only a function-scope binding (`result` inside `process()`) and + // no file-scope bindings, the accumulator should have nothing for this + // file — the function-scope entry is dropped at the flush boundary. const entries = acc.getFile('/src/test.ts'); + expect(entries).toBeUndefined(); + expect(acc.fileCount).toBe(0); + expect(acc.totalBindings).toBe(0); + }); + + it('narrows mixed file-scope and function-scope env to file-scope only', () => { + // Core narrowing assertion: a realistic file with BOTH file-scope and + // function-scope bindings flushes only the file-scope subset. This is + // the R1 red/green signal for plan 2026-04-09-005. + const code = `const dbClient: Database = connectDb();\nfunction handleRequest() {\n const localRequest: Request = parseRequest();\n const localUser: User = loadUser();\n}`; + const tree = parse(code, TypeScript.typescript); + const typeEnv = buildTypeEnv(tree, 'typescript'); + const acc = new BindingAccumulator(); + + typeEnv.flush('/src/service.ts', acc); + + const entries = acc.getFile('/src/service.ts'); expect(entries).toBeDefined(); - const resultEntry = entries!.find((e) => e.varName === 'result'); - expect(resultEntry).toBeDefined(); - expect(resultEntry!.typeName).toBe('Response'); - expect(resultEntry!.scope).not.toBe(''); + // Exactly one entry: the file-scope `dbClient`. The two function-scope + // entries (`localRequest`, `localUser`) are dropped. + expect(entries).toHaveLength(1); + expect(entries![0].scope).toBe(''); + expect(entries![0].varName).toBe('dbClient'); + expect(entries![0].typeName).toBe('Database'); + // Function-scope entries are absent from the accumulator. + expect(entries!.find((e) => e.varName === 'localRequest')).toBeUndefined(); + expect(entries!.find((e) => e.varName === 'localUser')).toBeUndefined(); }); it('flushes nothing for an empty TypeEnv', () => {