fix transaction safety for hypergraph store writes#567
Merged
CassOnMars merged 5 commits intoJun 15, 2026
Merged
Conversation
Step 1 — Failing regression test (TDD) Added get_vertex_data_not_visible_when_commit_txn_aborted to crates/quil-rpc/tests/vertex_data_end_to_end.rs. It drives the real lazy-commit path into a transaction, aborts it, and asserts GetVertexData returns nothing. Confirmed it failed on current code before any fix. Step 2 — save_vertex_underlying is now transaction-aware - crates/quil-types/src/store.rs: added txn: &dyn Transaction to the trait method. - crates/quil-store/src/hypergraph.rs: added inherent save_vertex_underlying_txn / save_tree_blob_txn using the existing with_rocks_batch pattern (stage into the batch, fall back to a direct write for unrecognized txns); the trait impl delegates to the txn variant. Kept the original inherent direct-write methods intact so the ~25 concrete-typed callers are untouched. Step 3 — Threaded txn through the lazy-tree commit crates/quil-tries/src/lazy_tree.rs: walk_leaves_persist now takes txn, passes it recursively and into save_vertex_underlying; the call site passes the txn that commit already receives. Leaf writes now join the same batch as the tree nodes and shard commit. Step 4 — Sync-apply path made atomic crates/quil-rpc/src/hypergraph_sync_probe.rs: extracted a persist_shard_refresh helper that wraps the tree-blob write and the per-vertex loop in a single transaction (new_transaction → writes → commit); on any error the txn is dropped, so partial syncs don't persist. Used in both the fresh and incremental paths. Step 5 — Implementors updated & verified Added the _txn param to the four other HypergraphStore impls (BTreeStore, MemStore, E2EHgStore, the quil-execution test store). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make LazyVectorCommitmentTree::commit retry-safe by deferring dirty-state
clearing until the surrounding transaction is durably committed.
Problem: commit drained its dirty-node map and pending_deletions and
cleared dirty_flag before returning — i.e. before the caller's
txn.commit(). If the txn was aborted/failed/never committed, the store
stayed unchanged while the in-memory tree (which is reused across frames)
believed it had persisted, so a retry silently skipped re-staging the
missing writes.
Fix:
- crates/quil-tries/src/lazy_tree.rs: commit now takes a non-draining
snapshot of the dirty set and pending_deletions to stage into the txn,
and no longer clears dirty_flag. Removed the dead latest map. Added
mark_persisted() — the sole place dirty bookkeeping is cleared; must be
called only after txn.commit() succeeds. Idempotent.
- crates/quil-hypergraph/src/crdt.rs: HypergraphCRDT::commit collects the
trees it staged and calls mark_persisted() on each only after
txn.commit()? succeeds. A failed commit returns early, leaving every
tree dirty for a safe retry.
- Side benefit: compute_shard_root (which commits under a NoopTxn) no
longer clears dirty state, so the subsequent real commit re-stages those
nodes transactionally.
Tests:
- crates/quil-tries/tests/golden_lazy_tree.rs — added
commit_defers_dirty_clear_until_mark_persisted: proves the tree stays
dirty after commit, that a retry after a discarded txn re-stages node
writes, and that a clean tree (post mark_persisted) stages nothing.
Added kv_len/clear_kv test helpers.
- crates/quil-rpc/tests/vertex_data_end_to_end.rs — folded the is_dirty()
lifecycle into the existing real-RocksDB commit-path tests:
- success path now asserts dirty after txn.commit(), clean after
mark_persisted();
- abort path now asserts the tree stays dirty (retry-safe).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
compute_shard_root claimed to read "without writing anything to the store" but computed its root via tree.commit(&NoopTxn, ...). Since NoopTxn isn't a RocksTxn, every write inside commit (tree nodes, deletions, and vertex underlying blobs) fell through to a direct db.put, persisting data outside any frame transaction for an uncommitted frame. This runs on the hot master-stream receive path (maybe_sync_before_global_frame), so a routine root comparison leaked writes to disk. - quil-tries: extract LazyVectorCommitmentTree::compute_root(prover), the read-only half of commit — recomputes commitments in memory, returns the root, writes nothing and touches no dirty/deletion/dirty-flag state. Refactor commit to delegate to it. - quil-hypergraph: compute_shard_root now calls tree.compute_root(); deleted the inline NoopTxn shim and the doubled doc comment. - Add quil-hypergraph test compute_shard_root_is_read_only (with MemStore node_count/per_vertex_count accessors and a dirty-state helper): asserts a real root is returned, nothing is persisted, the tree stays dirty, and the root matches what a real commit then produces. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With compute_shard_root no longer passing a NoopTxn, no caller ever hands a non-RocksTxn to a RocksHypergraphStore write, so the silent direct-write fallback only served to mask bugs like the one compute_shard_root hit. Remove the with_rocks_batch helper. Replace it with RocksTxn::from_dyn(&dyn Transaction) -> Result<&RocksTxn>, which downcasts once and errors loudly on an unrecognized txn instead of writing outside the transaction. Rewrite all writers (save_tree_blob_txn, save_vertex_underlying_txn, insert_node, save_root, delete_node, set_shard_commit, set_alt_shard_commit, track_change, untrack_change) to operate on the concrete RocksTxn batch directly. The HypergraphStore trait signatures stay &dyn Transaction for object-safety (five implementors, used as Arc<dyn HypergraphStore>). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dazthecorgi
commented
Jun 14, 2026
Comment on lines
+1063
to
+1065
| /// transaction. Either everything commits or (on error) nothing does — | ||
| /// the txn is dropped, which aborts the batch. Returns the number of | ||
| /// per-vertex blobs staged. |
Contributor
Author
There was a problem hiding this comment.
note that this is a change in behavior. previously partial failures were tolerated
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A vertex-data persistence test failure surfaced a broader problem where hypergraph writes could escape their surrounding transaction: persisting for uncommitted frames, leaking via a read path, or leaving the in-memory tree out of sync with the store on retry.
quil-rpctests #539, this made me look deeper into hypergraph persistence.save_vertex_underlyingso that vertex data isn't persisted if the tx reverts.LazyVectorCommitmentTree::commitretry-safe c6c6398:txn.commit()succeeds, so an aborted txn leaves the tree dirty for a safe retry (covers all staged writes, not just vertex data).compute_shard_rootread-only 61db774:compute_root()so root comparison on the hot master-stream path stops leaking writes via theNoopTxnfallback.RocksTxnfor store writes 4b61ab5:RocksTxnbatch and error loudly on an unrecognized txn.See individual commit messages for full detail.