mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-01 02:01:24 +00:00
* fix(parse-cache): retire a chunk whose durable generation could not be reset #3200 skipped the parse-cache write when `prepareDurableParsedFileChunk` failed, but the chunk hash was already in `usedKeys` from the lookup. When a previous generation existed on disk — reachable because the coherence gate re-dispatches a chunk whose `.v8` shard is live but whose durable shards are unreadable — `saveParseCache` copied that old shard forward, and the durable prune, which keeps exactly the saved keys, retained the mixed directory. The next run then served a warm hit out of a directory the previous run had already decided it could not account for. Retire the hash instead of only skipping the write: - `ParseCache.staleKeys` is a transient set that `saveParseCache` filters out of its key list. Filtering at save is what makes it survive the post-parse key merges in run-analyze (#2106 sibling fold, unreadable-meta retention), and it reaches both stores at once because the durable prune keeps exactly the keys `saveParseCache` returns. - The hash is retired at the reset-failure site, which runs unconditionally. The parse-cache write branch sits behind `rawResults.length > 0`, so a chunk whose worker round returns nothing would never have been retired there. - Worker-quarantined chunks get the same treatment for the same reason: they also reach the save with no in-memory entry, which is what triggers the copy-forward. That branch was previously unreachable when the worker died on the chunk and returned no results. - Guard the durable prune's non-survivor `fs.rm`. The causes that break the reset break that delete too, and it sat outside the validation try — one undeletable directory aborted the loop and cost every remaining chunk its index entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(review): apply review findings Retire a chunk only when a generation nobody cleared is still on disk. `prepareDurableParsedFileChunk` is rm-then-mkdir, and the catch could not tell the two apart: an rm that succeeded before a failing mkdir leaves NO directory, so the workers recreate it and write a clean generation. Retiring there discarded a good `.v8` for no safety gain — and under a correlated failure (an empty durable index turns every chunk into a re-dispatched miss, then a descriptor burst rejects the resets en masse) it would have wiped both shared stores for every branch, where the pre-#3204 posture cost only the writes. `durableChunkHasStaleShards` is the discriminator. Finish the delete guard on the path that runs before it. The staged→live overlay in `mergeStagedDurableParsedFileStore` awaited `replaceDurableChunkDir` unguarded, so on the cold-rebuild path one undeletable directory threw out of the merge before the prune ever ran — the durable index was never rewritten and a retired chunk kept its directory. Same log-and-continue treatment, plus best-effort handling of the two `.replacing` backup removals. Aggregate the prune's delete-failure warning: a store-wide cause hits every non-survivor, and one line per directory buries the message that matters. Tests: - Guard the chmod-based prune test with the repo's `skipIf` for root/Windows and assert the directory survived, so it cannot pass vacuously where the delete succeeds. - Add a two-chunk control: one chunk's reset fails, and the sibling must stay warm through the next run. One chunk plus a global spawn marker could not tell "retires the failing chunk" from "retires everything". - Add the rm-succeeded/mkdir-failed case, which must NOT retire. - Model both post-parse merges in the R4 test (the sibling fold re-adds the key, the unreadable-meta fallback unions `entries`), and move the in-memory `entries` assertion to a direct helper test — the sharded path never populates `entries`, so the old assertion proved nothing. - Type the cache factory as `ParseCache`; the `staleKeys` assertions were TS2339 and `?? false` read as a pass regardless. - Register the store test in the cross-platform filesystem list. Correct two comments that still described a quarantined chunk by the premise this fix disproves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(review): stop swallowing the backup removal in replaceDurableChunkDir Swallowing that `fs.rm` manufactured the very hazard this PR removes. When a non-empty `${to}.replacing` survives, the following `fs.rename(to, backup)` cannot overwrite it and is suppressed as "dest was missing", so `backedUp` stays false and the `fs.cp(from, to)` fallback merges the staged generation INTO the live directory — old shards alongside new, which the prune then indexes as one valid survivor. Let it throw; the per-entry guard added to `mergeStagedDurableParsedFileStore` already stops one such chunk from costing the others their prune. The post-publish backup cleanup stays best-effort, where an undeletable leftover really is litter. Also: - Make the sibling-isolation test perform the run its title claims. It asserted index membership and stopped; an index entry does not exercise the warm-hit path, so it would have passed even if the sibling re-dispatched. Each chunk now runs alone so the single spawn marker names which one re-parsed. - Correct two comments that outran the implementation: retirement is gated on shards actually surviving, and an undeletable directory is dropped from the index rather than removed from disk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
422 lines
15 KiB
TypeScript
422 lines
15 KiB
TypeScript
/**
|
|
* U20 — Integration regression test for chunk-cache corruption on
|
|
* worker quarantine.
|
|
*
|
|
* Pins the fix for the Codex adversarial review finding on PR #1693
|
|
* (`docs/plans/2026-05-20-002-fix-chunk-cache-corruption-on-worker-quarantine-plan.md`):
|
|
*
|
|
* The chunk hash is computed from every file in the chunk, but the
|
|
* worker pool's Layer 3 quarantine filters quarantined files out of
|
|
* dispatch. Before the fix, the chunk-loop would cache the partial
|
|
* worker results under the full-coverage chunk hash, locking in
|
|
* silent corruption that the next analyze would replay.
|
|
*
|
|
* Runs with REAL `worker_threads` + `createWorkerPool`. Injects a
|
|
* custom worker script via `workerUrlForTest` that:
|
|
* 1. Implements the U17/U19 IPC protocol (decode Buffer or hybrid
|
|
* envelope/contents shape, decode header + JSON payload).
|
|
* 2. Emits a `{type:'ready'}` handshake so the pool's
|
|
* `waitForWorkerReady` resolves promptly.
|
|
* 3. On a sub-batch containing `poison.ts`, emits a starting-file
|
|
* and exits with code 134 — deterministic worker death the pool
|
|
* attributes to `poison.ts` via the in-flight signal, then
|
|
* adds to its session-scoped quarantine.
|
|
* 4. On a sub-batch without poison, synthesizes a minimal valid
|
|
* ParseWorkerResult with a Function node per file (no
|
|
* tree-sitter dependency in the test worker — the synthesized
|
|
* nodes give the merge step deterministic content to add to the
|
|
* graph).
|
|
*
|
|
* U20 design pivot — no sequential fallback. The U1 sequential
|
|
* reparse for quarantined chunk files was removed: relying on the
|
|
* worker pool's resilience layers (respawn budget, circuit breaker,
|
|
* quarantine, slot-attribution, cumulative timeout) as the SOLE
|
|
* contract avoids re-triggering tree-sitter native crashes on the
|
|
* main thread and gives operators a clear hard signal when workers
|
|
* exhaust. Quarantined files are missing from this run's graph;
|
|
* they're surfaced in the per-chunk warn log; U2's cache-skip keeps
|
|
* the chunk uncached so the next analyze with a fresh pool retries.
|
|
*
|
|
* Assertions exercised here:
|
|
* - Worker-path runs and produces results for surviving files
|
|
* (good_a, good_c) via the synthesized worker output.
|
|
* - The quarantined file (poison.ts) is NOT in the graph — no
|
|
* sequential reparse fired.
|
|
* - U2 (cache-write suppression): `parseCache.entries` does NOT
|
|
* contain the chunk hash after the run. `parseCache.usedKeys`
|
|
* DOES contain it (chunk was processed; the cache write was
|
|
* specifically skipped). A cross-run scenario verifies that a
|
|
* subsequent dispatch with a fresh pool re-attempts the chunk
|
|
* (cache miss) and the cache stays empty for that chunk.
|
|
*
|
|
* Why integration over unit:
|
|
* - The fix lives at the boundary between processParsing
|
|
* (`parsing-processor.ts`) and the chunk-loop
|
|
* (`pipeline-phases/parse-impl.ts`) under a real
|
|
* workerPool. Unit-mocking the worker-pool import bypasses the
|
|
* structured-clone boundary, the dispatch lifecycle, and the
|
|
* actual quarantine flow — it verifies the test setup rather
|
|
* than the contract. The real worker thread executing the test
|
|
* script through the U17/U19 IPC protocol IS the load-bearing
|
|
* surface; this test exercises it end-to-end.
|
|
* - The `writeReadyWorker` pattern from `worker-pool.test.ts` is
|
|
* reused inline here (the READY_PREAMBLE + test-worker script
|
|
* composition).
|
|
*
|
|
* Wall-clock budget: well under 5 s under normal CI conditions.
|
|
*/
|
|
import { describe, it, expect, afterEach, beforeEach } from 'vitest';
|
|
import { tmpdir } from 'node:os';
|
|
import { mkdtempSync, writeFileSync, mkdirSync, rmSync, statSync } from 'node:fs';
|
|
import path from 'node:path';
|
|
import { pathToFileURL } from 'node:url';
|
|
|
|
import { createKnowledgeGraph } from '../../src/core/graph/graph.js';
|
|
import { runChunkedParseAndResolve } from '../../src/core/ingestion/pipeline-phases/parse-impl.js';
|
|
import {
|
|
computeChunkHash,
|
|
fileContentHash,
|
|
packParseCacheChunks,
|
|
} from '../../src/storage/parse-cache.js';
|
|
import type { ParseCache } from '../../src/storage/parse-cache.js';
|
|
import type { ParseWorkerResult } from '../../src/core/ingestion/workers/parse-worker.js';
|
|
|
|
/**
|
|
* Inline READY preamble + IPC decode wrapper (mirrors
|
|
* `test/integration/worker-pool.test.ts`'s READY_PREAMBLE). Lets the
|
|
* test worker script below speak the production U17/U19 IPC protocol
|
|
* without importing dist/protocol.js (the script runs as a standalone
|
|
* CJS file at a temp path, so it can't resolve dist/ via relative
|
|
* paths reliably).
|
|
*/
|
|
const READY_PREAMBLE = `
|
|
const { parentPort: __pp } = require('node:worker_threads');
|
|
const __decoder = new TextDecoder('utf-8');
|
|
const __decodeFrame = (raw) => {
|
|
if (
|
|
raw && typeof raw === 'object' &&
|
|
raw.type === 'sub-batch' &&
|
|
Array.isArray(raw.files)
|
|
) {
|
|
return {
|
|
type: 'sub-batch',
|
|
files: raw.files.map((f) => ({
|
|
path: f.path,
|
|
content: typeof f.content === 'string' ? f.content : __decoder.decode(f.content),
|
|
})),
|
|
};
|
|
}
|
|
return raw;
|
|
};
|
|
const __origOn = __pp.on.bind(__pp);
|
|
__pp.on = (event, handler) => {
|
|
if (event !== 'message') return __origOn(event, handler);
|
|
return __origOn(event, (raw) => handler(__decodeFrame(raw)));
|
|
};
|
|
__pp.postMessage({ type: 'ready' });
|
|
`;
|
|
|
|
/**
|
|
* Test worker script. Synthesizes minimal ParseWorkerResult entries for
|
|
* non-poison files; deterministically crashes on poison.ts via
|
|
* `process.exit(134)`. Accumulates across sub-batches; emits the
|
|
* accumulated result on `flush`.
|
|
*/
|
|
const TEST_WORKER_SCRIPT = `
|
|
const { parentPort } = require('node:worker_threads');
|
|
const accumulated = {
|
|
nodes: [],
|
|
relationships: [],
|
|
symbols: [],
|
|
imports: [],
|
|
calls: [],
|
|
assignments: [],
|
|
heritage: [],
|
|
routes: [],
|
|
fetchCalls: [],
|
|
fetchWrapperDefs: [],
|
|
decoratorRoutes: [],
|
|
routerIncludes: [],
|
|
routerImports: [],
|
|
toolDefs: [],
|
|
ormQueries: [],
|
|
constructorBindings: [],
|
|
fileScopeBindings: [],
|
|
parsedFiles: [],
|
|
skippedLanguages: {},
|
|
fileCount: 0,
|
|
};
|
|
parentPort.on('message', (msg) => {
|
|
if (msg && msg.type === 'sub-batch') {
|
|
const poison = msg.files.find((f) => f.path.endsWith('poison.ts'));
|
|
if (poison) {
|
|
parentPort.postMessage({ type: 'starting-file', path: poison.path });
|
|
process.exit(134);
|
|
}
|
|
for (const file of msg.files) {
|
|
const baseName = file.path.split('/').pop().replace(/\\.ts$/, '');
|
|
accumulated.nodes.push({
|
|
id: 'func:' + file.path,
|
|
label: 'Function',
|
|
properties: {
|
|
name: baseName,
|
|
filePath: file.path,
|
|
startLine: 1,
|
|
endLine: 1,
|
|
language: 'typescript',
|
|
isExported: true,
|
|
},
|
|
});
|
|
accumulated.fileCount++;
|
|
}
|
|
parentPort.postMessage({ type: 'progress', filesProcessed: accumulated.fileCount });
|
|
parentPort.postMessage({ type: 'sub-batch-done' });
|
|
return;
|
|
}
|
|
if (msg && msg.type === 'flush') {
|
|
parentPort.postMessage({ type: 'result', data: accumulated });
|
|
}
|
|
});
|
|
`;
|
|
|
|
const FIXTURE_FILES = {
|
|
'src/good_a.ts': 'export function good_a() { return 1; }\n',
|
|
'src/poison.ts': 'export function poison() { return 2; }\n',
|
|
'src/good_c.ts': 'export function good_c() { return 3; }\n',
|
|
};
|
|
|
|
const POISON_PATH = 'src/poison.ts';
|
|
const DEFAULT_TEST_CHUNK_BUDGET = 2 * 1024 * 1024;
|
|
|
|
const resolveTestChunkByteBudget = (): number => {
|
|
const env = Number(process.env.GITNEXUS_CHUNK_BYTE_BUDGET);
|
|
if (Number.isFinite(env) && env > 0) return env;
|
|
return DEFAULT_TEST_CHUNK_BUDGET;
|
|
};
|
|
|
|
const hashPacks = (
|
|
scanned: { path: string; size: number }[],
|
|
): { poison: string; others: string[] } => {
|
|
const packs = packParseCacheChunks(
|
|
scanned.map((file) => ({
|
|
path: file.path,
|
|
size: file.size,
|
|
language: 'typescript',
|
|
})),
|
|
resolveTestChunkByteBudget(),
|
|
);
|
|
const hashOf = (pack: string[]) =>
|
|
computeChunkHash(
|
|
pack.map((p) => ({
|
|
filePath: p,
|
|
contentHash: fileContentHash(FIXTURE_FILES[p as keyof typeof FIXTURE_FILES]),
|
|
})),
|
|
);
|
|
const poisonPack = packs.find((paths) => paths.includes(POISON_PATH));
|
|
if (!poisonPack) throw new Error('poison.ts was not packed');
|
|
return {
|
|
poison: hashOf(poisonPack),
|
|
others: packs.filter((paths) => !paths.includes(POISON_PATH)).map(hashOf),
|
|
};
|
|
};
|
|
|
|
describe('U20: parse-impl quarantine + chunk-cache integration (PR #1693 Codex finding)', () => {
|
|
let tempDir: string;
|
|
let repoDir: string;
|
|
let workerPath: string;
|
|
|
|
beforeEach(() => {
|
|
tempDir = mkdtempSync(path.join(tmpdir(), 'parse-impl-quarantine-cache-skip-'));
|
|
repoDir = path.join(tempDir, 'repo');
|
|
mkdirSync(repoDir, { recursive: true });
|
|
|
|
// Write the fixture files to repoDir so filesystem-walker / chunk
|
|
// loop pick them up by relative path.
|
|
for (const [rel, content] of Object.entries(FIXTURE_FILES)) {
|
|
const full = path.join(repoDir, rel);
|
|
mkdirSync(path.dirname(full), { recursive: true });
|
|
writeFileSync(full, content);
|
|
}
|
|
|
|
// Write the test worker script to the same tempDir so it doesn't
|
|
// collide with anything else. The READY preamble + test script
|
|
// share one .js file the pool spawns via `new Worker(URL)`.
|
|
workerPath = path.join(tempDir, 'test-quarantine-worker.js');
|
|
writeFileSync(workerPath, READY_PREAMBLE + TEST_WORKER_SCRIPT);
|
|
});
|
|
|
|
afterEach(() => {
|
|
rmSync(tempDir, { recursive: true, force: true });
|
|
});
|
|
|
|
it('worker quarantine leaves poison.ts out of the graph AND suppresses chunk-cache write', async () => {
|
|
const filePaths = Object.keys(FIXTURE_FILES);
|
|
const scanned = filePaths.map((rel) => ({
|
|
path: rel,
|
|
size: statSync(path.join(repoDir, rel)).size,
|
|
}));
|
|
|
|
const expectedChunkHash = hashPacks(scanned).poison;
|
|
|
|
const parseCache: ParseCache = {
|
|
version: 'test',
|
|
entries: new Map<string, ParseWorkerResult[]>(),
|
|
usedKeys: new Set<string>(),
|
|
};
|
|
|
|
const graph = createKnowledgeGraph();
|
|
await runChunkedParseAndResolve(
|
|
graph,
|
|
scanned,
|
|
filePaths,
|
|
filePaths.length,
|
|
repoDir,
|
|
Date.now(),
|
|
() => {},
|
|
{
|
|
skipWorkers: false,
|
|
// Force the worker-pool gate to open on the 3-file fixture.
|
|
// Inject the custom worker script — the pool will spawn it
|
|
// instead of the production parse-worker.js.
|
|
workerUrlForTest: pathToFileURL(workerPath) as URL,
|
|
// Test-only worker pool size — keep at 1 so the poison-file
|
|
// sub-batch deterministically lands on the only slot (no
|
|
// chance of poison + good landing in different slots).
|
|
workerPoolSize: 1,
|
|
parseCache,
|
|
},
|
|
);
|
|
|
|
const nodes = Array.from(graph.nodes.values());
|
|
|
|
// Quarantine contract: poison.ts is genuinely missing from the
|
|
// graph for this run. The custom worker crashed on it; no
|
|
// sequential reparse rescued it; the operator sees the per-chunk
|
|
// quarantine warn log. A future analyze with a fresh pool gets
|
|
// another chance via U2's cache-skip below.
|
|
expect(
|
|
nodes.some(
|
|
(n) => n.label === 'Function' && (n.properties as { name?: string }).name === 'poison',
|
|
),
|
|
).toBe(false);
|
|
|
|
// Surviving files' symbols come from the custom worker's
|
|
// synthesized output via the normal worker-path merge. Pinning
|
|
// them here catches a regression that would drop worker results
|
|
// entirely when quarantine fires.
|
|
expect(
|
|
nodes.some(
|
|
(n) => n.label === 'Function' && (n.properties as { name?: string }).name === 'good_a',
|
|
),
|
|
).toBe(true);
|
|
expect(
|
|
nodes.some(
|
|
(n) => n.label === 'Function' && (n.properties as { name?: string }).name === 'good_c',
|
|
),
|
|
).toBe(true);
|
|
|
|
// U2 assertion: chunk-cache write was suppressed. The chunk hash
|
|
// is in usedKeys (chunk WAS processed) but absent from entries
|
|
// (cache write skipped because of the quarantine intersection).
|
|
// This is the load-bearing cross-run protection: a future analyze
|
|
// with unchanged content will re-derive the same chunkHash, miss
|
|
// the cache, and re-dispatch — giving the file another chance
|
|
// against a fresh-quarantine pool.
|
|
expect(parseCache.entries.has(expectedChunkHash)).toBe(false);
|
|
expect(parseCache.usedKeys.has(expectedChunkHash)).toBe(true);
|
|
// #3204: `usedKeys` alone would let `saveParseCache` copy a pre-existing
|
|
// shard forward, so the skipped chunk is also retired from the save.
|
|
expect(parseCache.staleKeys?.has(expectedChunkHash)).toBe(true);
|
|
for (const hash of hashPacks(scanned).others) {
|
|
expect(parseCache.entries.has(hash)).toBe(true);
|
|
}
|
|
});
|
|
|
|
it('cross-run: unchanged fixture re-dispatches the poison pack because that pack was not cached', async () => {
|
|
// First pass: same setup as the previous test. The poison pack is not
|
|
// cached; other packs may be.
|
|
const filePaths = Object.keys(FIXTURE_FILES);
|
|
const scanned = filePaths.map((rel) => ({
|
|
path: rel,
|
|
size: statSync(path.join(repoDir, rel)).size,
|
|
}));
|
|
const expectedChunkHash = hashPacks(scanned).poison;
|
|
|
|
const parseCache: ParseCache = {
|
|
version: 'test',
|
|
entries: new Map<string, ParseWorkerResult[]>(),
|
|
usedKeys: new Set<string>(),
|
|
};
|
|
|
|
// FIRST PASS.
|
|
{
|
|
const graph = createKnowledgeGraph();
|
|
await runChunkedParseAndResolve(
|
|
graph,
|
|
scanned,
|
|
filePaths,
|
|
filePaths.length,
|
|
repoDir,
|
|
Date.now(),
|
|
() => {},
|
|
{
|
|
skipWorkers: false,
|
|
workerUrlForTest: pathToFileURL(workerPath) as URL,
|
|
workerPoolSize: 1,
|
|
parseCache,
|
|
},
|
|
);
|
|
// Confirm the precondition for the second-pass test: cache is
|
|
// empty for this chunk hash.
|
|
expect(parseCache.entries.has(expectedChunkHash)).toBe(false);
|
|
}
|
|
|
|
// SECOND PASS — same content, same parseCache, fresh worker pool
|
|
// (createWorkerPool is called per `runChunkedParseAndResolve`, so
|
|
// every invocation gets a clean quarantine slate). With the cache
|
|
// empty for this chunk, the second pass MUST dispatch the chunk
|
|
// again rather than replaying a cache entry. The custom worker
|
|
// crashes again on poison.ts → quarantine again → cache still
|
|
// skipped. Symptom: cache state unchanged, graph still complete.
|
|
{
|
|
const graph2 = createKnowledgeGraph();
|
|
await runChunkedParseAndResolve(
|
|
graph2,
|
|
scanned,
|
|
filePaths,
|
|
filePaths.length,
|
|
repoDir,
|
|
Date.now(),
|
|
() => {},
|
|
{
|
|
skipWorkers: false,
|
|
workerUrlForTest: pathToFileURL(workerPath) as URL,
|
|
workerPoolSize: 1,
|
|
parseCache,
|
|
},
|
|
);
|
|
|
|
// Cache stayed empty (still no entry for this chunk hash) — the
|
|
// load-bearing cross-run protection.
|
|
expect(parseCache.entries.has(expectedChunkHash)).toBe(false);
|
|
expect(parseCache.usedKeys.has(expectedChunkHash)).toBe(true);
|
|
for (const hash of hashPacks(scanned).others) {
|
|
expect(parseCache.entries.has(hash)).toBe(true);
|
|
}
|
|
// Worker path ran again; surviving files in the graph; poison
|
|
// still absent per the U20 contract (workers are the sole
|
|
// resilience layer, no sequential reparse).
|
|
const nodes2 = Array.from(graph2.nodes.values());
|
|
expect(
|
|
nodes2.some(
|
|
(n) => n.label === 'Function' && (n.properties as { name?: string }).name === 'good_a',
|
|
),
|
|
).toBe(true);
|
|
expect(
|
|
nodes2.some(
|
|
(n) => n.label === 'Function' && (n.properties as { name?: string }).name === 'poison',
|
|
),
|
|
).toBe(false);
|
|
}
|
|
});
|
|
});
|