2 commits

Author	SHA1	Message	Date
Gergő Magyar	b895a20415	perf(lbug): overlap node COPY with relationship emit (#2203) (#2226) ... * test(lbug): lock PARALLEL=false as a tested correctness invariant (#2203) The parallel CSV reader (Kuzu-derived, default PARALLEL=true) cannot parse quoted fields with embedded newlines (kuzudb/kuzu#5778); our content/text columns hold source code, so PARALLEL=false is mandatory for correctness. Add a live-DB multiline-quoted round-trip that fails if it is ever flipped, plus a static guard on the generated COPY queries. Export COPY_CSV_OPTS / getCopyQuery for the static assertion; document the invariant at the source. * feat(lbug): expose node/rel phase boundary via onNodePhaseComplete hook (#2203) streamAllCSVsToDisk now fires an optional onNodePhaseComplete(nodeFiles) callback right after node CSVs are flushed and before the relationship pass writes any rel_*.csv — the boundary the COPY-overlap leg needs. The node-file manifest construction is hoisted above the rel pass and reused in the return, so output is byte-for-byte identical when no callback is supplied (verified by the emit bench fingerprint and the splitRelCsvByLabelPair differential oracle). The callback is not awaited, so the rel pass runs concurrently with the caller's node COPY. * perf(lbug): overlap node COPY with relationship emit (#2203) The deferred parallelism leg of #2203. LadybugDB is single-writer and its parallel CSV reader is unsafe for our multiline content (kuzudb/kuzu#5778), so the only safe parallelism is pipeline-overlap: node COPY (uses conn, never the rel files) runs concurrently with the relationship emit pass (writes rel_*.csv, never conn). Node COPY now starts at streamAllCSVsToDisk's onNodePhaseComplete boundary while the rel pass keeps writing; the relationship COPY still waits for node COPY (FK precondition), so DB load order and content are unchanged. - Extract copyNodeCSVs; start it in the hook (overlap) or after emit (serial). - GITNEXUS_SERIAL_LBUG_LOAD=1 forces the legacy strictly-sequential path (operator escape hatch + differential-test oracle). - Settle the in-flight node-COPY promise on emit failure (no unhandled rejection); rethrow node-COPY errors at the FK barrier. - Preserve the PDG manifest merge + collision guards (node merge at the hook, rel merge before rel COPY) and all retry/fallback/cleanup behavior. - PROF_LBUG_LOAD gains mode=overlap|serial; copy-nodes becomes the residual node-COPY time after emit (trends to 0 as overlap hides it). * test(lbug): differential gate — overlap load === serial load (#2203) Loads one fixture (multiple node tables, multiple edge pairs, multiline File content + BasicBlock text) into two fresh DBs — once via the default node-COPY ‖ rel-emit overlap, once via GITNEXUS_SERIAL_LBUG_LOAD=1 — and asserts the two databases are content-equivalent: identical per-table node counts, per-type edge counts, byte-for-byte multiline content/text, and identical insertedRels/skippedRels/warnings. This is the issue's byte-identical-content acceptance gate for the parallelism leg. * fix(review): apply autofix feedback - csv-generator: onNodePhaseComplete doc-contract now matches reality (a sync throw is allowed and is how loadGraphToLbug surfaces the manifest collision guard) — drops the inaccurate 'must not throw synchronously' line. - lbug-adapter: copyNodeCSVs totalSteps is the node-table count (drop the +1 rel-step holdover; the rel COPY has its own progress line). - lbug-adapter: on emit+node-COPY double-failure, log the swallowed node-COPY error before rethrowing the emit error (diagnosability). - lbug-load-prof test: assert mode=overlap on the default path. * fix(test): use mkdtemp for secure temp dirs (CodeQL js/insecure-temporary-file) CodeQL flagged lbug-load-overlap.test.ts writing a file into a predictable os.tmpdir() path. Create the base temp dir with fs.mkdtemp (atomic, random suffix) in both new live-DB tests, and switch to the gitnexus-lbug- prefix that TEST_FIXTURE_PREFIXES recognizes so the Windows stale-sidecar sweep covers these fixtures. * fix(lbug): check PDG manifest rel-pair collision before node COPY (#2203) Found by Codex in tri-review. The manifest rel-pair collision guard ran after the FK barrier (after node COPY committed), so on that should-never-happen error branch the overlap path left orphan node rows AND the GITNEXUS_SERIAL_LBUG_LOAD escape hatch diverged from the legacy 'validate manifest before any COPY' behavior. Move the rel merge + collision check ahead of beginNodeCopy/the barrier: the serial path now detects a collision before committing any node rows (legacy parity restored — the escape hatch is a faithful oracle again), and the overlap path detects it as early as csvResult is available. The node-collision guard already ran before node COPY (in the hook). * test(lbug): cover rel-emit failure with node COPY in flight (#2203) Resolves a P1 review gap on PR #2226: the overlap's catch(emitErr) branch (settle the in-flight node-COPY promise, then rethrow the emit error) was untested. Fault-injects via a vi.mock of streamAllCSVsToDisk that fires onNodePhaseComplete (starting a real node COPY on a live DB) then throws, asserting loadGraphToLbug rejects with the emit error and no unhandled rejection leaks. Also covers the both-fail case (node COPY error is logged, emit error still wins). Listener removed in finally; macrotask queue flushed before the assertion so it can't pass vacuously. * test(lbug): cover node-COPY hard-failure rethrow at the FK barrier (#2203) Resolves the second P1 review gap on PR #2226. Mocks emit to fire onNodePhaseComplete with a nodeFiles entry pointing at a missing CSV (a bind-time COPY error that IGNORE_ERRORS does not suppress) and otherwise succeed, so copyNodeCSVs throws, the error is captured in nodeCopyError, and loadGraphToLbug rethrows it at the FK barrier — asserted via rejects /COPY failed for File/. * test(lbug): cover PDG manifest rel-pair collision in overlap + serial (#2203) Resolves the P2 gap behind the Codex tri-review finding: the manifest rel-pair collision guard (moved ahead of node COPY in ad195582) had no test. A leaky graph with a structural BasicBlock->BasicBlock edge (routed by id-prefix, no BasicBlock nodes — isolating the rel-pair clash from the node-CSV one) plus a PdgEmitSink manifest declaring the same pair makes loadGraphToLbug reject with the rel-pair collision error, asserted on both the overlap (default) and serial (GITNEXUS_SERIAL_LBUG_LOAD=1) paths. * perf(lbug): yield the event loop periodically during relationship emit (#2203) Resolves a P2 review finding on PR #2226: the relationship-emit loop ran long synchronous stretches between write-stream drain awaits, which could starve the overlapped node-COPY callbacks on fast I/O and erode the node-COPY-||-rel-emit overlap. Yield via setImmediate every REL_YIELD_EVERY (5000) edges so the node COPY and drains get scheduling time. Scheduling-only — emit bench fingerprint unchanged (byte-identical), csv-pipeline determinism + overlap differential green. * refactor(lbug): extract shared copyCsvWithRetry helper (#2203) Resolves a P2 maintainability finding on PR #2226: the COPY + IGNORE_ERRORS retry block was duplicated in copyNodeCSVs and the inline relationship-COPY loop. Extract copyCsvWithRetry(conn, query, onError); the callback receives the RAW retry error so each site keeps its own message shape + slice length (node throws, slices 200; relationship warns + records the failed pair, slices 80). Behavior-preserving — guarded by the live-DB round-trips plus the new node-COPY-failure and overlap error-path tests. * docs(lbug): document loadGraphToLbug non-transactionality (#2203) Resolves the advisory review finding on PR #2226: loadGraphToLbug runs independent COPYs with no surrounding transaction, so a mid-load failure leaves a partial DB and recovery is a --force re-analyze. Make that contract explicit on the function so callers don't assume atomicity.	2026-06-16 10:57:26 +01:00
Gergő Magyar	df08ecc397	perf(lbug): cut graph-DB emit/persistence wall time (#2203) (#2215) ... * perf(lbug): add PROF_LBUG_LOAD persistence-path timing breakdown (#2203 U1) loadGraphToLbug is un-timed today; the analyze 'emit' number is the scope-resolution emit bucket, not the CSV->COPY persistence path. Add a zero-cost-when-off per-stage breakdown (csv-emit/copy-nodes/rel-split/ copy-rels/fallback/total + node/rel counts) gated by PROF_LBUG_LOAD=1, mirroring the PROF_SCOPE_RESOLUTION pattern. Document the flag in README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(lbug): route relationships to per-pair CSVs in the emit pass (#2203 U2) Relationships were written once to a monolithic relations.csv, then re-read line-by-line (regex per edge) and re-split into per-FROM->TO-label-pair files before COPY — writing and reading the entire ~1M-edge set twice. Route each edge to its pair file directly during the single emit pass via a shared RelPairRouter, eliminating the monolithic write + re-read + per-edge regex. The router applies the SAME getNodeLabel + validTables filter as the legacy splitRelCsvByLabelPair, which is retained as a differential oracle. A new differential test asserts the direct-emit per-pair files are byte-for-byte identical to the oracle's, with identical skip/total accounting. The prof line (U1) drops its rel-split stage (routing now folds into csv-emit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(lbug): skip per-row microtask tick in BufferedCSVWriter (#2203 U3) addRow awaited an already-resolved promise on every buffered row, scheduling a microtask per node even when nothing flushed (millions at scale). It now returns a promise ONLY when it flushes; the node-emit loop awaits once per iteration after the switch. Flush/drain semantics are unchanged, so backpressure on the rows that actually write is preserved and the emitted CSV bytes are byte-identical (covered by the determinism + differential tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * bench(lbug): emit throughput + byte-identity gate for the persistence path (#2203 U4) Build-free bench (bench/emit-persistence/measure.mjs) times streamAllCSVsToDisk on a synthetic graph at two scales and gates: (1) an order-independent sha256 fingerprint over every emitted CSV line — the byte-identity guard for the U2/U3 emit optimisations — and (2) a scaling-ratio budget catching an O(n^2) emit re-regression. Wired into ci-tests.yml alongside the cfg/scope-capture benches. The LadybugDB COPY half needs a real DB, so its timing stays in PROF_LBUG_LOAD + the integration round-trip tests (documented in the bench README, with the deferred COPY-parallelism follow-up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(review): apply autofix feedback (#2203) - P1: router backpressure drain-await rejected with a generic AbortError, masking the real EMFILE/disk-full error. Expose RelPairRouter.lastError and rethrow it in the emit catch — mirrors the oracle's throw streamError ?? err. - P1: cover RelPairRouter error + backpressure + teardown paths with a new unit test (test/unit/rel-pair-routing.test.ts) using an injected mock stream. - P2: wrap streamAllCSVsToDisk body in try/finally so the setMaxListeners bump is always restored (the U2 rel-routing throw path could leak it). - P2: dedup WriteStreamFactory — re-export the canonical type from rel-pair-routing instead of a second identical declaration. - P2: annotate splitRelCsvByLabelPair @internal as the retained differential oracle so a future dead-code sweep doesn't delete the byte-identity guard. - P3: differential test now covers the proc_ prefix + clears GITNEXUS_SORT_GRAPH_OUTPUT to prevent env-leak desync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(lbug): scope byte-identity to quote-free ids + lock the quote-in-id divergence (#2215 review) The 'byte-identical' claim was unconditional, but the router derives labels from the raw id while the retained splitRelCsvByLabelPair oracle re-derives them via a regex over the escaped row — so for an id containing a double-quote they diverge (the router is the more-correct path). Soften the wording in rel-pair-routing.ts, the bench README, and the differential-test comment to document the exception, and add a differential test asserting the intended divergence (router routes the quote-in-id edge; oracle drops it) so a future change can't silently revert to the buggy regex semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * bench(lbug): per-file fingerprint so the gate catches pair-file mis-routing (#2215 review) fingerprintEmit flattened every line of every per-pair file into one array, sorted globally, and hashed — losing file boundaries, so a row routed to the WRONG pair file produced an identical fingerprint. Hash a per-file digest (filename + sha256(file bytes)) and combine the sorted entry list, so mis-routing (and within-file row reordering) now changes the fingerprint. Baseline regenerated; the new scheme yields a different hash on byte-identical emit, confirming it is sensitive to file structure the old flatten ignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * bench(lbug): add absolute large-scale wall-time backstop to the emit gate (#2215 review) The scaling-ratio gate only compares large/small, so a uniform Nx slowdown at both scales passes with ratio ~1.0. Add an opt-in max_ms_large ceiling (1000ms vs observed ~200ms — generous, host-noise-tolerant) that --check enforces alongside the ratio, catching a gross absolute regression the ratio misses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): cover the sorted-output path in the byte-identity differential (#2215 review) The differential test only exercised the default insertion-order emit path. Add a case under GITNEXUS_SORT_GRAPH_OUTPUT=1 that feeds the oracle the same id-sorted order orderedRelationships() uses and asserts per-pair byte-identity, so within-pair row reordering on the sorted path can't slip past the gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): cover the invalid-TO-label skip branch (#2215 review) Only an invalid-FROM label was exercised; the validTables skip is an OR over both endpoints, so the invalid-TO branch was untested (an inverted && would have slipped through). Add a valid-FROM/invalid-TO edge to the differential test and the router unit test, asserting it's skipped identically by router and oracle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): exercise the BufferedCSVWriter FLUSH_EVERY boundary in vitest (#2215 review) The U3 addRow change (returns a flush promise only on flush; undefined when buffered) and the loop's `if (pending) await pending` were only crossed by the bench, never vitest (all fixtures are <500 nodes). Add a 600-node graph through streamAllCSVsToDisk asserting all rows land exactly once across the 500-row flush boundary — no drops, dups, or corruption. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(lbug): drop redundant step cast in buildRelRow (#2215 review) GraphRelationship.step is already typed number?, so (rel as { step?: number }).step was a no-op structural cast that obscured the shared-type coupling. Use rel.step directly. Byte-identical — bench fingerprint unchanged, differential test green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(lbug): make the unknown-label node drop explicit (#2215 review) With the U3 `let pending` switch idiom, a node whose label matches neither codeWriterMap nor multiLangWriters left `pending` undefined and was silently dropped — a footgun for a future node type. Add an explicit else with a comment documenting that unknown labels are intentionally not persisted and that a new type must be wired into a writer map. No behavior change (byte-identity + tests unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(lbug): drop the unused WriteStreamFactory re-export (#2215 review) The type was re-exported from lbug-adapter 'to preserve this module's surface,' but no external code imports it by name from here (the only test reference is a comment). Keep the import from rel-pair-routing.ts (its canonical home, still used by splitRelCsvByLabelPair's signature) and drop the dead re-export. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-15 19:40:59 +01:00