Repository navigation
feat: Enrich Cypher transaction commit result for write parity - #1527
Merged
Merged
Conversation
commit_cypher_transaction returned only the committed t, and both write staging helpers ran Tracker::disabled(), so multi-clause Cypher writes driven through the CypherTransaction API could not be billed or trigger background reindexing. - begin_cypher_transaction takes TrackingOptions and pins a shared Tracker on the transaction; because the tracker accumulates atomically, fuel sums automatically across every write statement and every sequential clause. - Replace both Tracker::disabled() sites with the transaction's tracker. - commit_cypher_transaction returns TransactResultRef (receipt with the final commit id, t, and flake count summed across staged statements; indexing signals; fuel tally) — parity with the autocommit execute() path so callers map one result shape. - Gate the format_binding_with_result re-export to the iceberg feature, its only consumer, clearing an unused-import warning under default features.
zonotope
approved these changes
Jul 21, 2026
zonotope
left a comment
zonotope
left a comment
Contributor
There was a problem hiding this comment.
One suggestion, but lgm
Comment on lines
+423
to
+433
| // Head unchanged at the pinned base. `begin` requires a resolvable | ||
| // ledger, so a prior commit — hence a base head id — always exists. | ||
| let commit_id = txn | ||
| .base_head | ||
| .as_ref() | ||
| .and_then(|r| r.id.clone()) | ||
| .ok_or_else(|| { | ||
| ApiError::internal( | ||
| "cannot commit an all-reads Cypher transaction on an uncommitted ledger", | ||
| ) | ||
| })?; |
Contributor
There was a problem hiding this comment.
This is inconsistent with the rest of the system, and a regression. create_ledger just registers a new ledger with the nameservice but doesn't write a genesis commit and its commit_head_id is None. Users can then add data after the fact with no error. Plus, before this pr an all reads commit here just returned Ok(0) instead of erroring. I think it would be more consistent to go back to that behavior and allow commits on such a ledger after only reads to just return what the autocommit noop paths return with t == 0 instead of erroring.
Contributor
Author
There was a problem hiding this comment.
Thanks for the catch, addressed in 0e3cd74
Erroring when the transaction had no base head id was a regression: create_ledger registers a nameservice record without writing a genesis commit, so head_commit_id is legitimately None and data can still be added after the fact. An all-reads COMMIT on such a ledger previously succeeded, and must keep doing so. Return the same no-op receipt the autocommit paths use — the empty-content commit id with the base t (0 on such a ledger) and no reindex signal — instead of erroring. Adds a regression test covering begin/commit on a create_ledger ledger and a write after it.
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.
Problem
commit_cypher_transactionreturned only the committedt, and both write staging helpers ranTracker::disabled(). As of v4.1.3,WritePlangained aSequentialvariant for multi-clause statements that can only be driven through theCypherTransactionAPI — but that API couldn't report the signals a transactor reads offstage().execute():commit_id, fuel (billing), and indexing state (needed,novelty_size). Without fuel, multi-clause writes can't be billed; without the indexing signals, novelty accumulates and never triggers background reindexing.Changes
begin_cypher_transactionnow takesTrackingOptionsand pins a sharedTrackeron the transaction. SinceTrackeraccumulates atomically and fuel is charged at staging time, fuel sums automatically across every write statement and every clause of a sequential write — no per-clause bookkeeping. BothTracker::disabled()sites now use the transaction's tracker.commit_cypher_transactionreturnsTransactResultRef— the same struct the autocommitexecute()path yields — so callers map one result shape. Carries the finalcommit_id+t, the flake count summed across all staged statements, the indexing signals (needed,novelty_size), and the accumulated fuel tally. Per-statementRETURNrows continue to flow viaCypherTxnWriteOutcome.format_binding_with_resultre-export to theicebergfeature (its only consumer), clearing an unused-import warning under default features.Callers updated:
bolt.rs(begin + commit) and the bolt transaction test. Added tests covering the fuel tally + indexing signals on the enriched result, and that opting out of tracking leaves the tally empty.Testing
it_cypher_seq_writes— 20/20 pass (incl. 2 new).cargo clippy -p fluree-db-api -p fluree-db-server --all-features --all-targetsclean; warning fix verified under both default andicebergfeatures.