bug(build): tryNativeOrchestrator's catch swallows an unreadable-build-state error from the Rust pipeline

[carlos-alm]

Summary

Found while fixing #2418 (native load_file_hashes collapsed a read failure into "no prior build state", silently wiping a healthy graph on a transient SQLITE_BUSY/schema fault). #2418 fixes the Rust loader itself to correctly return an error instead of None in that case, and that error now propagates through NativeDatabase::build_graph's #[napi] wrapper as a thrown napi::Error — up through session.runBuildGraph() and tryNativeOrchestrator(ctx) in src/domain/graph/builder/stages/native-orchestrator.ts, neither of which catch anything.

But pipeline.ts's own call site does:

try {
  const nativeResult = await tryNativeOrchestrator(ctx);
  if (nativeResult === 'early-exit') return;
  if (nativeResult) return nativeResult;
} catch (err) {
  warn(`Native build orchestrator failed, falling back to JS pipeline: ${toErrorMessage(err)}`);
  // ...falls through to the JS pipeline, does NOT re-throw...
}

This unconditionally swallows any error from the native orchestrator — including the one #2418 makes it throw for unreadable prior state — and falls back to the JS pipeline instead of surfacing it. Contrast with the earlier JS-side fast-skip pre-flight a few lines above in the same function, which explicitly checks isUnreadableBuildStateError(err) before deciding whether to re-throw or fall through, specifically because "falling through here would... produce exactly the wipe this error prevents."

Why this isn't just #2418 again

isUnreadableBuildStateError checks e instanceof DbError — a TS class. A Rust-thrown error surfaces at this boundary as a plain napi-generated Error, never a DbError instance, so the existing check can't recognize it even if added to this catch block unchanged. Correctly closing this gap needs either:

• matching on the error message content (fragile — ties TS logic to Rust's exact wording), or
• a structured signal from the Rust side (e.g. a distinguishable error code/prefix NativeDatabase::build_graph could set, that TS parses), or
• some other explicit contract between the two error-reporting layers.

That's a real design decision, not a one-line fix, which is why it's being filed separately rather than folded into #2418's precise, narrow ask (fix the Rust loader itself).

Impact

In practice, whether this matters depends on what the JS-pipeline fallback does next: if it re-runs its own detectChanges/loadFileHashes (already fixed by #2414) against the same corrupted file_hashes table, it may independently throw the correctly-tagged error anyway — meaning the user-visible outcome could still be correct via a different path. This hasn't been verified either way. Worth confirming with a real end-to-end reproduction (a corrupted/locked file_hashes table on a --engine native incremental build going through the full pipeline.ts buildGraph() flow, not just the Rust unit-test level) before scoping a fix.

Suggested approach

Not investigated/scoped here — options include the ones listed above, or restructuring so the native orchestrator's own error surfaces a stable, checkable signal (e.g. a specific Error subclass or .code property set at the napi boundary or immediately after catching nativeDb.buildGraph()'s throw) that both this catch site and the pre-flight's can share.

[carlos-alm]

Verified the "Impact" question empirically before moving on to other queued issues.

Reproduced end-to-end: a fresh native-engine build, then corrupting `file_hashes`' schema (drop + recreate missing the `hash`/`mtime`/`size` columns), then an incremental `buildGraph(dir, { incremental: true, engine: 'native' })` call.

Result: the call throws — `Could not read the file_hashes table: no such column: hash. ... Refusing to fall back to a from-scratch build...` — and the node count is unchanged (not wiped).

So the catch in `pipeline.ts` does swallow the Rust orchestrator's error as described, but the JS-pipeline fallback it falls through to independently re-detects the same corruption via its own `loadFileHashes` (fixed by #2414) and throws its own correctly-tagged error, which propagates uncaught. No data loss via the full buildGraph() flow today — the two fixes compose correctly by accident of both paths re-reading the same table, not by design.

This does NOT close the issue: the underlying design gap (the native-orchestrator catch has no way to distinguish "safe to fall back" from "must not fall back" for a native-thrown error) is still real, and matters for:

• any caller that invokes the native NativeDatabase.buildGraph() API directly, bypassing pipeline.ts's TS orchestration and its JS fallback entirely (the scenario fix(native): load_file_hashes collapses read failures into "no prior build state" #2418 itself calls out — no JS re-check ever runs for those callers);
• future changes to the JS fallback path that might stop re-reading file_hashes itself (e.g. an optimization that trusts the native orchestrator's result instead of redetecting from scratch), which would silently reopen this exact wipe.

Keeping this open as a real (if currently non-data-lossy via the common path) design gap worth closing properly.

[carlos-alm]

Fixed the severity-critical part of this in PR #2541 itself (Greptile caught it during review there, correctly pointing out it was worse than my initial verification found: a scoped build's JS fallback skips file_hashes re-validation entirely, so it would silently complete a partial rebuild instead of even wiping to a full one).

Fix: extended `isUnreadableBuildStateError` (detect-changes.ts) to also recognize the `DB_STATE_UNREADABLE` marker embedded in a native-thrown error's message (since a Rust error crossing the napi boundary is a plain `Error`, never a `DbError` instance), and added the same check to `tryNativeOrchestrator`'s own catch that the earlier JS-side pre-flight's catch already had. Verified via a real end-to-end scoped-build reproduction, plus a revert-verify pass.

This closes the practical severity gap for both scoped and non-scoped native builds. Keeping this issue open only for the residual, lower-priority generalization: the fix here is a message-substring contract between the two layers (acceptable since I control both message strings and documented the sync requirement in both places), not a fully general "any native error can be identified as X" mechanism. If a future native error needs the same treatment, it'll need its own marker, or this could be generalized into a shared error-code convention at that point. Not urgent enough to block on now.