[DecisionNerd]

Summary

Part of #1388. Implements steps O1, O2, and O3 of the open-path redesign (design doc reviewed at session start; O3 explicitly approved by the maintainer mid-implementation as an in-scope extension — see reasoning below). Does not implement O4 (XXH3 swap in ordinal_identity_v4.rs) or anything past O3 — those are owned by other agents / require separate sign-off.

ResolvedProjectGeneration::graph_files_inventory() decoded the manifest and, for the V2/mapped-root participant, ran a full streamed SHA-256 over every declared graph payload object. This function was called at least four times inside a single GraphForge::open, so a V2/mapped-root open paid for four-plus full-byte-range SHA-256 passes over the same graph payload.

• O1 — memoize graph_files_inventory() on ResolvedProjectGeneration via Arc<OnceLock<Result<Option<Arc<GraphFilesInventory>>, GfError>>>. A resolved generation is immutable by construction (CURRENT is never re-consulted after resolution), so every call site, and every clone of a ResolvedProjectGeneration for the same generation, now shares one computation. Zero behavior change to what gets verified.
• O3 — delete the payload verify_graph_object call inside graph_files_inventory()'s V2 loop; keep the existing presence-and-declared-length check. This is the step that removes the O(E) term. Every real consumer of a graph object's bytes already re-authenticates lazily on first touch via open_graph_object_by_digest/read_graph_object_by_digest, so the eager sweep was provably redundant for objects a session reads, and simply undetected-until-next-read for objects it does not. Mirrors the presence-only pattern ShardedCsrIndex::open already uses for adjacency shards (fix(exec): plateau S18→S19 recount/query RSS so host ladder admits S20 #1094).
• O2 — replace materialize_from_cas's post-hardlink re-verify (graph_object_store/materialization.rs) with an identity, length, and link-count check. link_child_into already confirms the newly linked directory entry resolves to the same inode as the CAS source; nothing between the link syscall and the check can have altered those bytes within the same open, so re-reading and re-hashing the whole object was pure duplicate work.

The tradeoff O3 makes (maintainer-approved)

Before: GraphForge::open proved every declared graph object was byte-for-byte intact before returning — including objects no query in the session ever reads. After: open proves every declared object is present at its declared length; content authentication is deferred to first real read (which already happens unconditionally) or to the explicit administrative verify command (#1384 decision 4). A corrupted object that a session never reads is no longer caught at open — it surfaces on the next read of that object, or on an explicit verify run. Corruption in an object a query does read is still caught, exactly as before, because the lazy per-object authentication path is untouched. The maintainer's framing, recorded here per the coordinator's request: the verify command covers whole-store integrity, this path maximizes usable speed, and this is an analyst bench rather than a forensic system.

One correction to the design doc

The design cited GraphObjectReadLease::open_for_construction (graph_object_store.rs:959-1054) as an existing precedent for "trust identity + length without re-hashing." Reading it directly: it still streams the whole file through SHA-256 and compares the digest — it does not skip hashing. The real precedent for O2's metadata-only check is StableDirectory::link_child_into itself (graphforge-filesystem/src/lib.rs), which already asserts file_identity(installed) == file_identity(source) as part of completing the hard link, plus the identity/link-count revalidation pattern already used elsewhere in graph_object_store.rs (e.g. revalidate_lifecycle, replace_child). O2 mirrors that actual pattern, not open_for_construction.

Verified

All runs below compare this branch against origin/main at the commit this branch is based on, back to back, on the same host.

• cargo fmt --all -- --check — clean.
• cargo clippy --workspace -- -D warnings — clean, zero warnings, full workspace.
• openCypher TCK corpus (cargo test -p graphforge-api --test bdd --release): 3,897/3,897 scenarios passing, identical before and after (verified via the harness's structured report.json, not just the summary line).
• API BDD suite (same binary, 118 scenarios): 115 passed / 3 failed, identical failing scenario set before and after. All 3 failures are pre-existing and environment-caused (UnsupportedFilesystem/filesystem_class_unproven — the CI/dev sandbox's /tmp is tmpfs, which filesystem_admission.rs correctly refuses to classify as durable storage), not caused by this change.
• graphforge-storage --lib (1,128 tests, clean rerun on ext4-backed worktrees after tmpfs was found to be the confound above): 850 passed / 278 failed, identical failing test set before and after. Every sampled failure traces to the same filesystem_class_unproven cause in filesystem_admission.rs, a module this PR does not touch.
• graphforge-api --lib (742 tests — includes durability_certification_tests, composite_recovery_tests, mutation_transaction_fault_tests, stream_cancellation_isolation_tests, workspace_hydration tests, corruption/crash-recovery/cancellation coverage), run with TMPDIR pointed at an ext4-backed directory to remove the tmpfs confound entirely: 742 passed / 0 failed, both before and after this change.

Measured: open cost per edge, before vs. after

Two graph sizes (8x range), each opened repeatedly (10-15 iterations) from a durably-published generation, wall-clock measured around GraphForge::new(Some(path)), on the same ext4-backed host, back to back:

Edges	Before: avg open	Before: µs/edge	After: avg open	After: µs/edge
50,000	108.2 ms	2.16	110.3 ms	2.21
400,000	1515.1 ms	3.79	785.9 ms	1.96

At 400,000 edges: 1.93x wall-clock speedup, per-edge cost drops 48% (3.79 → 1.96 µs/edge). The qualitative shape matches the design's prediction: before, per-edge cost grows with graph size (3.79 vs 2.16 µs/edge, 8x range) — the O(bytes) signature; after, per-edge cost is flat-to-decreasing (2.21 vs 1.96 µs/edge) — the O(files) signature. At 50,000 edges the two are within noise of each other because fixed per-open overhead (DataFusion context, ordinal-identity open, etc.) dominates at that size; the linear term only becomes visible once it outweighs the fixed cost, exactly as in the epic's own ladder data.

Peak resident memory (VmHWM after the open phase) at 400,000 edges: before 187.0 MB, after 161.6 MB — no regression; if anything a modest reduction, consistent with fewer full-file read buffers allocated during the removed verification passes. No new O(E) resident-memory term is introduced by this change; the bounded-memory property from #1094 is untouched (adjacency shard opens are unaffected).

I could not reproduce the exact multi-million-edge, sixteen-fold ladder rungs from #1388's own table within this session (that ladder is a separate, long-running, dedicated fixture); the numbers above are from an independently constructed fixture at a smaller but still meaningfully O(E)-dominated scale, intended to demonstrate the mechanism and its direction/magnitude, not to replace #1388's own ladder re-run (tracked as O6 in the design doc, owned separately).

Out of scope (unchanged in this PR)

• O4 (XXH3 swap in ordinal_identity_v4.rs) — different agent, file untouched here.
• The administrative verify command (S7) — different agent; this PR's O3 tradeoff is exactly the work that command is meant to cover on demand.
• supersession.rs, shaping_merge.rs, installation.rs, benches/, per-phase I/O attribution, the CLI verify command — untouched, per the brief's file-ownership boundaries. Confirmed no open PR currently touches materialization.rs or project_generation.rs before starting.

Part of #1388

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[coderabbitai]

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2af3c84c-92c4-4ccf-a6ef-b2a905c3736e

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[DecisionNerd]

Blocking: this silently accepts corrupted Topology data

Converted to draft. An independent review with a working mutation test found that a same-inode, same-length one-byte corruption of topology/nodes.parquet is accepted — the open succeeds, and an ordinary query over the corrupted data succeeds and returns a count.

Reproduced against two worktrees at cd964b69 and its parent 13632d4b:

commit	fresh GraphForge::new after flipping one byte in a hardlinked Topology payload
13632d4b (before this PR)	refused — validation error: graph object digest does not match its address
cd964b69 (this PR)	succeeded, and MATCH (n:Person) RETURN count(n) then returned a row

The refusal that disappears is graph_files_inventory()'s verify_graph_object call in the V2 loop, which O3 deletes.

Why the identity check cannot cover this

verify_hardlinked_materialization checks file_identity (device + inode), declared length, and link count >= 2. Rewriting a file's bytes in place changes none of them — those are directory-entry properties, not content properties.

The cited precedent, StableDirectory::link_child_into, does genuinely compare file_identity() before and after linking; that half of the PR's claim is accurate. But identity proves "this entry points at the inode I just read", not "the bytes at that inode match the digest". Those coincide only if something hashed the bytes at a point you are relying on, and post-O3 nothing hashes Topology objects at all.

Why lazy re-authentication on first read does not hold here

The commit message's central justification is that every real consumer re-authenticates lazily via open_graph_object_by_digest / read_graph_object_by_digest. Neither symbol appears anywhere in crates/graphforge-exec/src — verified by grep. Their callers are construction, catalog, GC, manifest-tree, ontology composition and export.

After hydrate_graph_workspace hardlinks the compact graph into a private workspace with route-encoded paths, the query engine opens those files by path. There is no digest lookup and therefore no "first real read" that re-authenticates. The justification is false for exactly the data class the test corrupts.

Properties survive only by accident, and that is the #1392 pattern again

property_overlay/inventory.rs re-hashes every property route file against entry.content_sha256 whenever AuthenticatedPropertyInventory is built, which happens on every ordinary open. It is untouched by this PR and wired only to Properties-role objects. So Properties stay protected by a wholly separate subsystem while Topology loses its only check.

That asymmetry also explains why compact_graph_root_reopens_through_ordinary_api_and_rematerializes still passes: its victim selection is inventory.files.iter().find(|entry| entry.byte_length > 0), which lands on a Properties/Catalog entry in file order. It never exercises the O2 hardlink path. That test is not evidence this PR is safe — it covers a path this PR does not touch.

The ShardedCsrIndex::open analogy does not transfer

ShardedCsrIndex::open is genuinely presence-only, but its row-read path validates an embedded per-shard checksum on decode — flip a byte and row() fails. Topology Parquet has no embedded self-check and no digest re-check on read.

What is sound

O1 is fine. The OnceLock memoization is correctly per-instance and does not leak across opens. The measured win is real but lands on Topology's share of the bytes only, which is consistent with the ~2x rather than ~4x wall clock and the null result at 50K.

To unblock

Either give Topology objects a real content check on first materialization — cheap is fine, but it must be a function of the bytes, not of the inode — or state explicitly and honestly that Topology data is unverified until graphforge verify runs. O3's comment already says something like that for the inventory sweep; O2 does not, and as written both are false in practice.

Whichever route, ship the mutation test with it. This PR currently has no test that would have caught this.

Note for #1414: its description documents that corrupting a referenced object is refused at ordinary open. That holds today and would remain true for Properties, but this PR makes it false for Topology.

[blacksmith-sh]

Found 1 test failure on Blacksmith runners:

Failure

Test	View Logs
//crates/graphforge-api:scale_g500_ladder (Exit 101)	View Logs

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[DecisionNerd]

Superseded by #1435.

#1435 carries both commits — the O1 memoization from this PR (4a92035b, rebased) and the restoration of the content check this PR removed (3e624070) — so the optimization is not lost, only the data-integrity regression.

Why this one is not landing

An independent review with a working mutation test showed that after this PR, a one-byte corruption of topology/nodes.parquet — same inode, same declared length — is silently accepted at open, and an ordinary MATCH (n) RETURN count(n) then returns a count over the corrupted data. Reproduced against cd964b69 and its parent: the parent refuses with graph object digest does not match its address; this PR does not.

The justification in the commit message — that every real consumer re-authenticates lazily on first touch via open_graph_object_by_digest / read_graph_object_by_digest — is false for Topology-role objects. Neither symbol appears anywhere in crates/graphforge-exec/src. After hydrate_graph_workspace hardlinks the compact graph into a private workspace, the query engine opens those files by path, so there is no first read that re-authenticates.

Properties-role objects stayed covered only by accident, via property_overlay::inventory's independent re-hash of every property route file — a mechanism this PR never touched and that does not reach Topology.

What #1435 does differently

It restores verify_graph_object in graph_files_inventory()'s V2 loop, and because O1's memoization is kept, that check now runs once per resolved generation instead of the 3-4 times per open it ran before this PR. The redundancy that motivated this work is still removed; the guarantee is back. No durable-format change.

entry.content_sha256 stays a full cryptographic digest because it is the content-addressed CAS lookup key, not merely a corruption checksum. A fast-checksum alternative belongs to #1417, which needs an inventory format change.

Ships with hardlinked_topology_payload_corruption_is_refused, which fails against this PR's head and passes with the restoration.

Part of #1388.