diff --git a/gitnexus/src/server/analyze-worker-core.ts b/gitnexus/src/server/analyze-worker-core.ts new file mode 100644 index 000000000..09b8b0c11 --- /dev/null +++ b/gitnexus/src/server/analyze-worker-core.ts @@ -0,0 +1,64 @@ +/** + * Side-effect-free core of the analyze worker's message handler. + * + * Extracted from `analyze-worker.ts` — a `fork()` entry module whose top-level + * `process.on(...)` handlers and `ready` handshake make it unsafe to import in a + * unit test. This module has no top-level side effects and takes its collaborators + * by dependency injection, so the worker's run → finalize → report contract is + * unit-testable without spawning a process. The entry module wires the real deps + * and owns the `process.exit` lifecycle. + * + * The `import type ... typeof import(...)` forms below are erased at runtime, so + * importing this module does NOT load `run-analyze`, `repo-manager`, or the entry + * worker — only the lightweight `analyze-worker-ipc` projection helper. + */ +import type { AnalyzeOptions } from '../core/run-analyze.js'; +import type { WorkerMessage } from './analyze-worker.js'; +import { projectAnalyzeResultForIpc } from './analyze-worker-ipc.js'; + +export interface WorkerAnalysisDeps { + runFullAnalysis: typeof import('../core/run-analyze.js').runFullAnalysis; + assertAnalysisFinalized: typeof import('../storage/repo-manager.js').assertAnalysisFinalized; + send: (msg: WorkerMessage) => void; +} + +/** + * Run the analysis and report the outcome to the parent over IPC. Always reports + * exactly one terminal message (`complete` or `error`) and never throws — the + * caller schedules `process.exit` after this resolves. + */ +export async function runWorkerAnalysis( + repoPath: string, + options: AnalyzeOptions, + deps: WorkerAnalysisDeps, +): Promise { + try { + const result = await deps.runFullAnalysis( + repoPath, + // This worker force-exits right after reporting, so skip the native close + // (it can double-free in LadybugDB's ClientContext destructor after --pdg + // writes); flushWAL still persists the index, process.exit reclaims handles. + { ...options, skipNativeCloseOnExit: true }, + { + onProgress: (phase, percent, message) => + deps.send({ type: 'progress', phase, percent, message }), + onLog: (message) => deps.send({ type: 'progress', phase: 'log', percent: -1, message }), + }, + ); + // P2 (#2264): a half-finalized repo — meta.json written but the global + // registry entry missing (e.g. a prior collision-aborted run, or a wiped + // registry) — must NOT be reported as a successful analysis. Mirror the CLI's + // assertAnalysisFinalized guard so the worker surfaces it as an error instead + // of a false `complete` that leaves the repo invisible to list_repos. + await deps.assertAnalysisFinalized(repoPath); + + // Send a JSON-safe projection, NOT the raw result: the IPC channel is + // default-JSON serialization and `result.pipelineResult` carries the live + // KnowledgeGraph. See analyze-worker-ipc.ts. + deps.send({ type: 'complete', result: projectAnalyzeResultForIpc(result) }); + } catch (err: unknown) { + // Report the failure to the parent over IPC (the parent surfaces the message). + const message = err instanceof Error ? err.message : 'Analysis failed'; + deps.send({ type: 'error', message }); + } +} diff --git a/gitnexus/src/server/analyze-worker.ts b/gitnexus/src/server/analyze-worker.ts index 5054682c0..da44b3409 100644 --- a/gitnexus/src/server/analyze-worker.ts +++ b/gitnexus/src/server/analyze-worker.ts @@ -12,7 +12,9 @@ */ import { runFullAnalysis, type AnalyzeOptions } from '../core/run-analyze.js'; -import { projectAnalyzeResultForIpc, type AnalyzeResultIpc } from './analyze-worker-ipc.js'; +import { type AnalyzeResultIpc } from './analyze-worker-ipc.js'; +import { runWorkerAnalysis } from './analyze-worker-core.js'; +import { assertAnalysisFinalized } from '../storage/repo-manager.js'; import { closeLbug } from '../core/lbug/lbug-adapter.js'; interface StartMessage { @@ -102,38 +104,19 @@ process.on('message', async (msg: StartMessage) => { started = true; try { - const result = await runFullAnalysis( - msg.repoPath, - // This worker force-exits (process.exit(0) below) right after sending its - // result, so skip the native close: it can double-free in LadybugDB's - // ClientContext destructor after --pdg writes and abort the worker BEFORE it - // can send 'complete' (#2264). flushWAL still persists the index; the - // subsequent process.exit reclaims the native handles. - { ...msg.options, skipNativeCloseOnExit: true }, - { - onProgress: (phase, percent, message) => { - send({ type: 'progress', phase, percent, message }); - }, - onLog: (message) => { - send({ type: 'progress', phase: 'log', percent: -1, message }); - }, - }, - ); - - // Send a JSON-safe projection, NOT the raw result: the IPC channel is - // default-JSON serialization and `result.pipelineResult` carries the live - // KnowledgeGraph (wasteful to materialize, silently corrupted by JSON, and - // a BigInt/circular value would throw and mis-report this success as a - // failure). See analyze-worker-ipc.ts. - send({ type: 'complete', result: projectAnalyzeResultForIpc(result) }); - } catch (err: unknown) { - // Report the failure to the parent over IPC (the parent surfaces the message). - const message = err instanceof Error ? err.message : 'Analysis failed'; - send({ type: 'error', message }); + // The run → finalize → report contract lives in the side-effect-free + // analyze-worker-core seam (unit-testable without this entry module's + // process.on side effects). It reports exactly one terminal message and + // never throws. + await runWorkerAnalysis(msg.repoPath, msg.options, { + runFullAnalysis, + assertAnalysisFinalized, + send, + }); } finally { // LadybugDB's native module prevents clean exit — force it (same reason the // CLI uses process.exit(0)). In `finally` so the exit still fires even if the - // error report above throws on a closed IPC channel (#2264 review P3). + // report above throws on a closed IPC channel (#2264 review P3). setTimeout(() => process.exit(0), 500); } }); diff --git a/gitnexus/test/unit/analyze-worker-core.test.ts b/gitnexus/test/unit/analyze-worker-core.test.ts new file mode 100644 index 000000000..d2961f650 --- /dev/null +++ b/gitnexus/test/unit/analyze-worker-core.test.ts @@ -0,0 +1,75 @@ +/** + * Unit tests for the analyze-worker core seam (#2264 P2). The worker must NOT + * report `complete` for a half-finalized repo (meta.json written but the global + * registry entry missing) — it must surface that as an error, mirroring the CLI's + * assertAnalysisFinalized guard. Driven via the side-effect-free + * `runWorkerAnalysis` seam with injected fakes, so no fork()/process.on side + * effects of the entry module are touched. + */ +import { describe, it, expect, vi } from 'vitest'; +import { + runWorkerAnalysis, + type WorkerAnalysisDeps, +} from '../../src/server/analyze-worker-core.js'; +import type { AnalyzeResult } from '../../src/core/run-analyze.js'; +import type { WorkerMessage } from '../../src/server/analyze-worker.js'; + +describe('runWorkerAnalysis — worker finalize guard (#2264 P2)', () => { + const baseResult: AnalyzeResult = { + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: false, + ftsRepairedOnly: false, + }; + + const okRun: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => baseResult); + + it('reports error (not complete) when finalization fails for an unregistered repo', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const assertAnalysisFinalized: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn( + async () => { + throw new Error('registry entry for /repo was not added'); + }, + ); + + await runWorkerAnalysis('/repo', {}, { runFullAnalysis: okRun, assertAnalysisFinalized, send }); + + expect(send).toHaveBeenCalledWith({ + type: 'error', + message: 'registry entry for /repo was not added', + }); + expect(send).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'complete' })); + }); + + it('reports complete exactly once when finalization succeeds', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const assertAnalysisFinalized: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn( + async () => undefined, + ); + + await runWorkerAnalysis('/repo', {}, { runFullAnalysis: okRun, assertAnalysisFinalized, send }); + + const completes = send.mock.calls.filter((c) => c[0].type === 'complete'); + expect(completes).toHaveLength(1); + }); + + it('reports error when finalization passes but the analysis itself throws', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const failingRun: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => { + throw new Error('boom'); + }); + const assertAnalysisFinalized: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn( + async () => undefined, + ); + + await runWorkerAnalysis( + '/repo', + {}, + { runFullAnalysis: failingRun, assertAnalysisFinalized, send }, + ); + + expect(send).toHaveBeenCalledWith({ type: 'error', message: 'boom' }); + expect(assertAnalysisFinalized).not.toHaveBeenCalled(); + }); +});