Skip to content

perf(api): run chunk-API initial builds on the bulk builder (#1901) - #1924

Merged
DecisionNerd merged 1 commit into
mainfrom
perf/1901-chunk-api-bulk-builds
Oct 9, 2026
Merged

DecisionNerd merged 1 commit into
mainfrom
perf/1901-chunk-api-bulk-builds

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1901

Slice of #1881. Initial builds through the chunk API (GraphConstructionSession append_nodes/append_edges) now run on the bulk builder.

What changed

  • Durability contract (checked first): an accepted chunk is promised to survive a crash (accepted_chunks is "durably accepted", resume_graph_construction reopens, exact replay by chunk id is idempotent, the crash matrix kills the append at every boundary). So each accepted chunk is spooled once as one self-describing Arrow IPC file: temp name, synced, renamed, directory synced. The footer carries the chunk id, kind, sequence, row count and digests, so there is no receipt journal, chunk key or per-chunk checkpoint rewrite.
  • Seal: the spooled chunks are the bulk builder's sources (row counts from the receipts, in-place parallel decode in passes 1 and 2). Chunk-time refusals still fire at the same append call with the same messages; global refusals (duplicates, endpoints) fire at seal.
  • Over-budget builds use the merged scratch route (perf(storage): bound initial graph builds with adaptive scratch passes #1912, perf(storage): bound bulk property sorting and emission with scratch #1920) via BulkBuildPlan::route over the receipts. Only a build whose node tables alone exceed the budget replays the spool through the staged path, the same case an import session stages. The route is recorded before any work and read back on retry.
  • Authentication. Every decode of a spooled chunk (memory route, scratch route, staged replay) checks the footer descriptor, row count, schema digest and a typed logical digest against what was acknowledged at accept time. A same-size, still-valid Arrow IPC corruption fails with spooled chunk differs from its acknowledged digest.
  • Typed chunk digest. The chunk digest hashed Arrow display text for property values, so List<Utf8> values ["a, b", "c"] and ["a", "b, c"] digested alike. It is now a typed, recursive, length-prefixed encoding with validity markers (hash_column). This changes input_sha256 for property-bearing chunks on the staged path too (pre-v1, in place).
  • Replay keeps the accepted admission. Replaying a spooled chunk through the staged path no longer re-measures the IPC-decoded batch (which reports more memory than the original arrays); it keeps the size it was admitted at.
  • Spool files open through StableDirectory::open_child_file. Import sessions stage through the new begin_staged_graph_construction; the facade's begin_graph_construction spools on an empty project. Appends to a non-empty graph stage.
  • ADR 0058, the ADR 0038 amendment and resumable-import.md are updated; the Rust, Python and Node release-surface parity contracts record the new public method.

Review findings (#1924 at 8ea57a8)

  • Confirmed: digest collision. the_chunk_digest_separates_values_that_print_alike and the list property that prints alike corruption case (same-size edit turning ["a, b","c"] into ["a","b, c"]). With the list rows hashed by display text again, both fail ("corruptions not refused: list property that prints alike").
  • Confirmed: staged replay re-admission. a_chunk_admitted_at_its_exact_byte_window_replays_through_the_staged_path. Re-measuring on replay fails with "construction resource window exhausted"; reverting the fix reproduces it.
  • Rejected: recovery trusts footer descriptors. An acknowledged chunk is data-synced, renamed, then directory-synced before append returns, so no crash leaves an acknowledged chunk missing, truncated or half-written; only an unacknowledged temporary file can remain, and reopen removes it. The reviewer's scenario needs someone to edit or delete files inside the private session directory and rewrite the footers, which is outside the threat model as for the staged artifacts. A hole in the sequence is still refused ("not contiguous"). Documented in the spool.rs module docs; the kill-at-every-failpoint tests (a_process_killed_while_accepting_chunks_resumes_and_continues, a_kill_between_rename_and_directory_sync_loses_nothing_acknowledged) pass. A process kill cannot lose page-cache data, so no test can show the barrier order by killing; it is argued from the code.
  • Fixed: staged API docs, StableDirectory use. Added: chunk API equals registered sources (spooled_chunks_publish_the_bytes_of_registered_sources).
  • Mutation proofs. Each fails its tests when applied: replay re-measures admission; list values hashed by display text; node labels not hashed (only node label is missed); Utf8 property values not hashed (only property value and the list case are missed); the logical-digest check disabled for node chunks, then for edge chunks (the node identity and edge endpoint cases are missed respectively).

Acceptance criteria

  • Facade and binding bulk-load suites pass; initial builds take the bulk path (asserted): chunk_api_initial_builds_take_the_bulk_path, an_over_budget_chunk_api_build_runs_on_scratch_without_staging, storage an_over_budget_spooled_build_takes_the_scratch_route_and_publishes_the_staged_bytes.
  • Published artifacts byte-identical to the staged path and to registered sources for the same chunks, in any arrival order, chunk size and worker count, with properties and mixed schemas, in memory and on scratch.
  • Crash tests: kill during accept at every spool failpoint, between rename and directory sync, during the build (rerun to identical artifacts), and midway through staged replay.
  • S20 timing, contended (load 19 to 37 on 16 cores; not a quiet-host number). Two alternating base/candidate pairs, separate target dirs, cmp differs, binary sha256 base a2ec3ccf...c728a, candidate 19bc2514...90da4. Full table and commands on perf(api): run chunk-API initial builds on the bulk builder #1901.
    • pair 1: base 229,782 ms, candidate 50,101 ms, -78.2% (4.6x).
    • pair 2: candidate 56,509 ms, base 372,432 ms, -84.8% (6.6x).
    • Every column has the same sign. Pre-existing hazard seen while measuring: on unmodified main the bulk builder fails S20 initial builds intermittently with captured encoded source identity or length changed (unsynced artifact st_blocks changes under ext4 delayed allocation; 5 of 6 runs of an import session on main, 3 of 7 of the chunk-API candidate). Not caused or fixed here.

Verification (head df99dcc, rebased on main after #1910, #1923, #1926, #1927)

  • cargo nextest run -p graphforge-storage with GF_TEST_TMPDIR=/home/ubuntu/t1901: 1845 run, 1845 passed (includes the spool authentication, digest, replay-admission, scratch-route and chunk-versus-registered-source equivalence tests).
  • cargo nextest run -p graphforge-api -E 'not binary(bdd)' in a non-nested worktree: 1702 run, 1702 passed. cargo test -p graphforge-api --test bdd: 118 passed.
  • make check: exit 0.
  • Bindings were last rebuilt and run at the pre-rebase head (pytest 150 passed, Node 305/305, scripts pass); the rebase touched no binding code or contracts beyond what is above.

🤖 Generated with Claude Code

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: CurateLabs/graphforge/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5affe896-9e94-484d-b942-53ff326be556

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added core Core source code changes testing Test coverage and testing infrastructure documentation Improvements or additions to documentation tooling Developer tooling and automation labels Oct 8, 2026
@DecisionNerd
DecisionNerd force-pushed the perf/1901-chunk-api-bulk-builds branch from 8ea57a8 to 42127d7 Compare October 8, 2026 22:10
@github-actions github-actions Bot added the executor Changes to query executor label Oct 8, 2026
@DecisionNerd
DecisionNerd added this pull request to the merge queue Oct 8, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 8, 2026
…1901)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@DecisionNerd
DecisionNerd force-pushed the perf/1901-chunk-api-bulk-builds branch from 42127d7 to df99dcc Compare October 9, 2026 00:21
@DecisionNerd
DecisionNerd added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit b312c4c Oct 9, 2026
17 checks passed
@DecisionNerd
DecisionNerd deleted the perf/1901-chunk-api-bulk-builds branch October 9, 2026 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core source code changes documentation Improvements or additions to documentation executor Changes to query executor testing Test coverage and testing infrastructure tooling Developer tooling and automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(api): run chunk-API initial builds on the bulk builder

1 participant