fix(server): worker asserts finalization before reporting complete (#2264 P2)

The forked analyze worker reported {type:'complete'} straight after runFullAnalysis,
so a server/web analyze of a half-finalized repo (meta.json written but the global
registry entry missing — a prior collision-aborted run, or a wiped registry) was
reported successful while the repo stayed unregistered/invisible to list_repos. The
CLI already guards this with assertAnalysisFinalized; the worker did not.

Extract the run -> finalize -> report contract into a side-effect-free
analyze-worker-core seam (the entry module's top-level process.on handlers make it
untestable directly) and call assertAnalysisFinalized before sending complete — a
failure is reported as {type:'error'} instead of a false success. The seam is
dependency-injected and unit-tested with fakes; the entry module wires the real deps
and keeps owning process.exit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm

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<void> {
+  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 });
+  }
+}

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

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();
+  });
+});