fix(SM-14): close sequential-path memory regression (Codex adversarial review)

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)
This commit is contained in:
Gergo Magyar 2026-04-09 18:39:37 +01:00
parent 803631fef7
commit d3c25d2093
5 changed files with 196 additions and 9 deletions

View file

@ -60,6 +60,7 @@ export class BindingAccumulator {
private readonly _fileScopeByFile = new Map<string, [string, string][]>();
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);

View file

@ -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,

View file

@ -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);

View file

@ -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);
});
});
});

View file

@ -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', () => {