Skip to content

perf(storage): publish the adjacency CSR with the generation instead of rebuilding it per process - #1453

Merged
DecisionNerd merged 15 commits into
mainfrom
fix/adjacency-csr-persist
Sep 19, 2026
Merged

DecisionNerd merged 15 commits into
mainfrom
fix/adjacency-csr-persist

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Part of epic #1388.

Every query process rebuilt the entire adjacency CSR into a per-process graphforge-adjacency-cache-* temp dir and deleted it at exit: ~16 s CPU at S20, paid four times per ladder rung, and the reason the adjacency storage category reported 0 bytes. This makes the generation that publishes canonical topology also publish its derived CSR, so a query process opens it (ShardedCsrIndex::open, presence-only per #1094) instead of rebuilding it.

Design (ADR 0037)

  • Construction. graph_construction_encoding::encode builds indexes/adjacency/ from the exact edge tables it just encoded, stamps the generation it is about to bind, and records every file as a SHA-256-declared ConstructionEncodedArtifact. The existing publisher installs them into the CAS like every other file; hydration hardlinks and digest-verifies them at open; the persistent provider finds a fresh manifest and serves it. No read-path change and no workspace_hydration.rs edit.
  • Portable import. A complete package of a construction-published generation already carries the index and imports it unchanged. Packages without one (subset exports, pre-epic(query): bounded queries should cost what their results cost, not what the graph costs #1388 packages) get it built into the verified stage tree before the compact CAS append. The v1 inventory contract keeps the lazy rebuild (it is verified file-for-file against the package tree).
  • Not a durable-format change. The CSR format, its manifest, and Index-role inventory entries all pre-date this; explicit index("adjacency") already published them. What changes is when the artifact is produced. Rebuild-on-absence is untouched: an old project still opens (measured below).
  • Determinism. CSR bytes derive from topology/ alone, the shard directory is named by its content digest, and the manifest build time is the session's recorded clock, so the encoded inventory authority stays reproducible; the determinism suite compares every encoded artifact and now covers these.
  • Authentication. Every published CSR object is digest-verified by the open-time sweep like Topology; shard payloads are additionally authenticated on first row touch. *.csr.json and index_manifest.parquet are sweep-only; a disagreeing index is treated as stale and rebuilt privately, never served.
  • Scope limit, stated plainly: only initial constructions (parent_topology_generation == 0). An append carries the parent's files forward without re-reading them; its carried-forward manifest reads as stale and keeps today's lazy rebuild. Follow-up.

Measurement (S20 = 2^20 nodes, 16.8M edges; contended host, so user CPU and disk bytes are the comparable figures, wall is indicative)

Per hop query (query and reopen_proof phases, ladder's exact ONE_HOP/TWO_HOP):

before (frozen 1955f17d) after after binary on old project
one-hop user CPU 21.3 s / 21.6 s 2.6 s / 2.7 s 19.7 s / 19.1 s
two-hop user CPU 22.1 s / 22.4 s 2.8 s / 2.8 s 18.9 s / 18.3 s
wall 30.5–37.5 s 6.9–7.6 s 26.9–27.6 s
peak RSS 153–156 MB 106–138 MB 163–167 MB

Cost moved to publish (per-step ingest, both binaries): validate (where encoding runs) 134.9 s → 168.1 s user (+33.2 s, +3.5 GB rchar, peak RSS 102 → 191 MB); commit 3.9 s → 6.4 s user (+4.3 GB rchar: CAS install hashes the new objects). Net per rung: −4 × ~18.5 s = −74 s of query CPU for +36 s at publish.

adjacency storage category: 0 → 428 MB logical / 214 MB allocated (the CAS deduplicates the _all pair against the single relation's identical shards). Project 2.1 GB → 2.7 GB on disk.

Verification

  • cargo test -p graphforge-api --test bdd: 3897 passing of 3897 (0 regressed, 0 xpass); API BDD 118/118.
  • GDC suites (cargo test --manifest-path benchmarks/Cargo.toml): green.
  • graph_construction::tests::determinism (incl. same_input_twice_produces_identical_digests, resume), lifecycle_budget, partition::tests, recovery::tests, adjacency*, project_portable_v2_import: green. New: initial_construction_publishes_a_current_adjacency_index, complete_import_publishes_the_derived_adjacency_index, import_builds_the_index_a_package_lacks_and_never_twice, and API published_construction_serves_its_adjacency_index_and_refuses_corruption (flips a byte in a published shard object; reopen is refused with a digest mismatch).
  • graphforge-api lib + lifecycle_io_attribution + fixed_hop_limit, graphforge-exec lib + persistent_adjacency: 1702 passed, 0 failed.
  • cargo clippy --workspace -- -D warnings clean; scoped --tests clippy clean on every line this PR touches.
  • make pre-push-fast green; source-size policy required splitting graph_construction_encoding and project_portable_v2_import adjacency code into child modules.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Closes #1446

@coderabbitai

coderabbitai Bot commented Sep 17, 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: c6b31c4a-2ce8-44bf-9388-df713a4fa90b

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

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 documentation Improvements or additions to documentation labels Sep 17, 2026
@DecisionNerd

Copy link
Copy Markdown
Contributor Author

The CSR is built twice for a single-relation graph — worth −12 to −16 s at S20, and it halves two other costs

Measured while pricing S26 levers on project-after (this PR's own output).

build_adjacency_index_for_edge_files sorts and writes a per-relation group and then an _all group. For a single relation the two shard directories are byte-identical — confirmed by digest: …in.csr.shards-befd32da… and …out.csr.shards-72a8b7c4… appear under both stems.

So roughly half the +33.2 s publish-side build measured in this PR is a duplicate sort and write of bytes that already exist. Aliasing the _all manifest rows at the shared shard digest directory removes it, and it is deterministic because the directories are already content-named.

It also halves two costs this PR introduces elsewhere:

  • The open-time sweep. Hydration hashes every declared CAS object, and the inventory declares both copies (428 MB logical, deduplicated to 214 MB allocated — but the sweep hashes what is declared). At S26 the CSR is ~25.5 B/edge → ~27 GB logical; per open that is roughly 19 s of SHA-256 at the measured 1.46 GB/s plus ~11 s cold read, so ~30 s x 6 opens ≈ 3 min ≈ 0.05 h. It erodes this PR's 0.66 h rather than erasing it — and halving the declared bytes halves that erosion.
  • Package growth. Export +4.4 s, verify +0.4 s, clean import +4.7 s at S20, package 678 MB → 1,106 MB (+63 %). Roughly half of that is the duplicate.

Cheapest experiment: time encode_adjacency with the _all group skipped when groups.len() == 1, at S20, comparing validate user CPU. No build of anything else required.

Not asking for it in this PR unless it is easy — the PR is measured, green and worth landing as it stands. Filing it here so it is not rediscovered later, and because it changes the S26 arithmetic for the better.

Query side must still resolve _all; the manifest can point at the shared digest directory rather than a copy.

@blacksmith-sh

This comment has been minimized.

@DecisionNerd

Copy link
Copy Markdown
Contributor Author

permanent_storage_budgets is failing, and it is right to

Bazel Bootstrap fails two targets on this branch:

FAIL: //crates/graphforge-api:permanent_storage_budgets
FAIL: //crates/graphforge-storage:graphforge_storage_test

The first is the gate working. This PR publishes the adjacency CSR as declared inventory, and its own measurements record the cost: the adjacency storage category goes 0 -> 428 MB logical / 214 MB allocated, and the project goes 2.1 -> 2.7 GB (+28%) at S20. A budget test over permanent storage is supposed to refuse that arriving unannounced.

I have disarmed auto-merge on this PR so it cannot land on a re-run while that gate is red.

This is a decision, not a re-run

The trade is real and good: +28% project size for 8x on hop queries (one-hop user CPU 21.3 s -> 2.6 s, two-hop 22.1 -> 2.8) and roughly −0.58 h at S26. But the budget should be raised with that justification recorded next to the number, not adjusted until the test passes.

Ask of the row what we have asked of every other gate today: cheaper, or not running? Here it is neither — it is more, deliberately, in exchange for something measured. That is a third answer and it deserves to be written down rather than absorbed.

Do the duplication fix first

Roughly half of those bytes are a duplicate. For a single-relation graph build_adjacency_index_for_edge_files writes an _all group whose shard directories are byte-identical to the per-relation group — confirmed by digest, …in.csr.shards-befd32da… and …out.csr.shards-72a8b7c4… under both stems.

Aliasing the _all manifest rows at the shared digest directory would:

  • roughly halve the 428 MB that trips this gate, so the budget rise is smaller and easier to justify;
  • remove ~half of the +33.2 s publish-side build;
  • halve the open-time sweep cost, which at S26 is ~27 GB of declared CSR hashed on every open (~30 s x 6 opens ≈ 0.05 h);
  • halve the package growth (export +4.4 s, verify +0.4 s, import +4.7 s; package 678 MB -> 1,106 MB).

Fixing it before setting the budget means setting the budget once, against the real number.

The other failure

graphforge_storage_test also fails and I have not diagnosed it. Worth checking whether it shares this cause (a size or inventory-count expectation) or is independent.

@DecisionNerd

Copy link
Copy Markdown
Contributor Author

The profile says PR #1453 is the fix for this, and that it also unblocks PR #1466

Following the call path posted above to its cause, and ruling out the obvious suspect first.

It is not a missing page index. permanent_parquet::writer_properties() already sets EnabledStatistics::Page, set_offset_index_disabled(false) and a column index truncate length, and the reader asks for PageIndexPolicy::Optional. The index is written and it is read.

It is the access pattern against the page granularity. Pages are PAGE_ROWS = 20_000 rows / PAGE_BYTES = 1 MiB. At S18's 4.19M edges that is roughly 210 pages per column. A one-hop query asks for a scattered set of edge ids, and a scattered selection lands at least one wanted row in nearly every page. A page index can only skip pages that contain nothing wanted, so it skips almost nothing here, and skip_records walks and zstd-decompresses essentially the whole column either way.

That is why 43.6% of the thread is in skip_records and ArrowReaderBuilder::build, both decompressing.

The consequence

This is a layout-versus-access-pattern mismatch, not a coding defect. Filtered parquet scanning is the wrong mechanism for a scattered adjacency lookup, at any page size — shrinking pages trades decompression for index size and per-page overhead.

Which is exactly what this issue proposes to remove. PR #1453 publishes the adjacency CSR with the generation instead of rebuilding it per process, so a hop query opens a CSR rather than scanning filtered parquet. If the hop path stops going through read_edges_filtered_projected_from_inventory, this cost disappears rather than being tuned.

The dependency worth recording

PR #1453 plausibly unblocks PR #1466. #1466 (unpin the default resource policy) currently hangs the S18 rung — 5 of 5 attempts — and the profile puts the hang in this query path, amplified by target_partitions going from 2 to 8. If #1453 takes the hop query off filtered parquet, the amplified cost has nothing to amplify.

That is a hypothesis, not a result: I have not built #1453 and re-run the rung. It is a cheap test — --rung S18 is about a minute on a quiet host when it passes, and hangs inside two minutes when it does not.

Supporting measurement

#1449 records that a one-hop at S20 costs 14 s user CPU and 6.4 GB of disk reads while read_path_scan attributes 579 KB. The 6.4 GB is consistent with decompressing whole columns to serve a scattered selection, which is what this profile shows happening.

Reproduction for anyone picking this up: --rung S18 under benchexec with gf built from 1287a2f8, frame-pointer release build, perf record --call-graph fp on the thread in R state. Hangs within 110 s, profiles cleanly.

@DecisionNerd

Copy link
Copy Markdown
Contributor Author

Tested: #1453 removes the hang that blocks #1466

I posted this as a hypothesis. It now has a measurement.

build S18 rung under benchexec
#1466 alone hung, 5 of 5 attempts — >400 s, one thread spinning in expand_single_hop_chunk
#1466 + #1453 work completes in 37.6 s

37.6 s is ordinary S18 territory; the hang was unbounded. Taking the hop query off filtered parquet removes the cost that #1466's target_partitions increase was multiplying, exactly as the profile predicted.

Suggested merge order: #1453 before #1466.

Two honest caveats

The combined run does not pass. It completes the work and then fails evidence_invalid at ingest. The receipt key sets are identical to a passing run — no missing or extra fields — so it is a value-level rejection rather than a shape change. I have not chased it.

And it may be my mess rather than #1453's. This was a local merge across three worktrees carrying my own in-flight fixes, and I tripped over that twice while testing: first a certify/gf binary mismatch, then finding that perf/1464-chain-split forked from observe/1462-wire-ingest-phases before I committed the certify validator fix on it. Neither was a defect in #1453.

The timing result is robust to that — 37.6 s versus >400 s is not a subtle difference and does not depend on which certify binary validated it. The evidence_invalid is not robust to it, and should be reproduced from clean branches before anyone treats it as a property of #1453.

Reproduction

# hangs, 5/5
--rung S18, gf built from 1287a2f8

# completes in 37.6s
--rung S18, gf built from 1287a2f8 merged with origin/fix/adjacency-csr-persist

Both on a quiet host, benchexec, same generator and data.

@DecisionNerd
DecisionNerd force-pushed the fix/adjacency-csr-persist branch from f9b0c77 to 284eee9 Compare September 18, 2026 20:08
Main introduced the title/adr/status/date/superseded_by frontmatter policy
(ADR 0038, #1390) after this branch's ADR was written, so the rebase left
0037 as the only record without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@blacksmith-sh

This comment has been minimized.

DecisionNerd and others added 3 commits September 18, 2026 14:43
…adjacency CSR

`detail_codec_current_format_resumes_cross_multiple_merge_levels` failed under
Bazel on

    assertion failed: allocation.snapshot().unwrap().matches_file_inventory(&raw, references)

`matches_file_inventory` admits no untracked file: the tracker's active set must
equal the on-disk inventory, owner for owner and byte for byte. Every other
artifact writer registers through `replace_file_at`; `encode_adjacency` did
not, so the CSR files it publishes existed on disk while belonging to no owner.

`encode_adjacency` now registers each published file in the loop that already
opens and authenticates it, before authentication consumes the handle. No new
parameter was needed: `StableDirectory` is this module's alias for
`ConstructionDirectory`, so the handle was already reachable as
`output.allocation()`.

This deliberately does not attribute the bounded builder's individual write and
fsync calls; that exclusion is documented on `AdjacencyEvidence::write_bytes`
and stays. What is accounted for is the set of files the builder leaves behind,
which is a different question and the one the inventory asks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DecisionNerd
DecisionNerd force-pushed the fix/adjacency-csr-persist branch from c36f46f to ae1de7f Compare September 18, 2026 21:35
@blacksmith-sh

This comment has been minimized.

@DecisionNerd

Copy link
Copy Markdown
Contributor Author

Three invariants collide with ADR 0037, not one

Bazel Bootstrap has been red on this branch across two runs with the same two targets failing (104 tests pass and 2 fail locally). Fixing the first failure surfaced the others rather than adding them — all three are the same root cause, and two need a decision rather than a patch.

Root cause. ADR 0037 moves the derived adjacency CSR from built lazily, per process to published with the generation at construction time. Three existing invariants still encode the old lifecycle.

1. Allocation ownership — fixed in ae1de7f9

detail_codec_current_format_resumes_cross_multiple_merge_levels failed on matches_file_inventory, which admits no untracked file. Every other artifact writer registers through replace_file_at; encode_adjacency did not, so the published CSR existed on disk owned by nobody. It now registers in the loop that already opens and authenticates each file. No new parameter was needed — StableDirectory is that module's alias for ConstructionDirectory, so output.allocation() was already in scope. This test now passes.

2. permanent_storage_budgets asserts adjacency is empty after construction — needs a decision

permanent_storage_budgets.rs:959:

let unindexed = graph.storage_attribution().unwrap();
assert_eq!(unindexed.categories[&ArtifactCategory::Adjacency].allocated_bytes, 0);

Four fixtures fail with 1,347,584 and 1,441,792 bytes against an expected 0. Those bytes are real and correct under ADR 0037 — the variable is named unindexed because, before this PR, adjacency did not exist until someone indexed. That is precisely what the PR changes.

So the expectation is stale, not the behaviour. But "stale" is a judgement about intended semantics: is a freshly constructed generation now indexed by definition? If yes, the assertion and the unindexed framing should move to the new lifecycle. I have not changed it, because it decides what the category means.

3. Supersession requires encoded artifacts to be identity-stable across resume — needs a decision

canonical_encoder_outputs_feed_ordinary_readers_index_and_adjacency fails with supersession encoded artifact identity changed. At supersession.rs:513, an encoded artifact must be present in storage_active_identity_allocated_bytes with matching allocated bytes, link count 1, and logical bytes equal to expected — and is_none_or means an absent key fails too.

The CSR is pushed into artifacts, so supersession checks it. But encode_adjacency opens with:

// A crashed earlier attempt may have left a torn index or spill behind;
// the artifact set must be exactly this build's.
if adjacency.exists() { std::fs::remove_dir_all(&adjacency)?; }

Unconditional rebuild means new inodes on every encode, so on resume the native identity differs from the recorded evidence. An artifact cannot be both unconditionally rebuilt and reusable under supersession. Either it is excluded from the supersession set as derived-and-regenerated, or the rebuild becomes conditional on the existing CSR being intact — which is also the performance argument this PR makes, since rebuilding per process is the cost #1466 measured.

Why this matters beyond this PR

#1466 is green and held behind this one: on #1466 alone S18 hangs 5/5, and with #1453 applied the same work completes in 37.6 s.

Also worth noting the shape of (1): a new published artifact reached main review without the attribution layer being told it exists, and nothing forced the writer to declare itself — it was caught only by whichever test happened to assert completeness. #1415 hit the same class of defect independently in the same batch.

🤖 Generated with Claude Code

Both failures under Bazel were the same root cause, and neither was a defect
in the code under test: they assert the lifecycle that ADR 0037 replaces,
where derived adjacency did not exist until some reader built it.

`canonical_encoder_outputs_feed_ordinary_readers_index_and_adjacency` called
`build_adjacency_index_from_inventory` -- the legacy reader-side builder --
on the directory construction had just published into. That replaced the
inodes `record_encoded_active_artifacts` had pinned, so the resume below
refused them with `supersession encoded artifact identity changed`. That is
the identity ledger doing its job. The test now reads the published manifest,
which is what an ordinary reader does once the CSR ships with the generation;
`build_adjacency_index_for_edge_files` documents this as the point of the
publish-side entry.

`permanent_storage_budgets` asserted the Adjacency category was empty after
construction, and that its presence equalled `f.adjacency` -- "unbuilt and
built adjacency are distinct measured capabilities". ADR 0037 deletes that
distinction: a constructed project always carries adjacency. The remaining
property is that an explicit rebuild neither removes it nor exceeds the
category budget, which is what it now asserts. `unindexed` is renamed to
`constructed` for the same reason, including in the emitted evidence keys.

`authenticated_lookups_checked` moves 352 -> 373 because `indexes/adjacency/**`
are graph files, so the manifest Merkle tree covers them and the authenticated
walk visits their leaves and the interior nodes above. That constant is a
pinned observation of the tree's shape rather than an invariant; the reason is
recorded at the site so the next reader does not have to re-derive it.

Verified: 1,186 storage lib tests pass, 59 permanent_storage_budgets tests
pass, fmt and clippy clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(query): persist the adjacency CSR at publish instead of rebuilding it in every process

1 participant