Repository navigation
fix(txn,search): route predicate writes outside a block, refuse two writes of one sum balance, refuse the doc_id score alias - #426
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
Request changes. The transaction path still permits silent omissions and rejection after a durable commit verdict.
The per-write prediction mapping and owner-supplied identities address real defects. Five correctness findings block merge.
- Transaction staging must capture its own matched rows atomically.
- Batched statements must observe preceding writes.
- Calvin must enforce transitions before voting commit.
- Identity reads must cover temporal storage.
- Identity metadata must remain separate from user-named score fields.
Rebuild this 26-commit series after correcting the implementation. Combine dependent changes with their regression tests into coherent commits. Keep the sound changes. Rewriting history alone resolves none of these findings.
Static review only, against f63110. No builds, tests, or formatters ran. Contributor test claims remain unchecked. Existing CI reports Static gates passed and Test Suite skipped.
| ) | ||
| .await | ||
| .map_err(StagingGateError::Dispatch)?; | ||
| let staged = stage_write(state, sessions, session_id, task, &dispatch).await?; |
There was a problem hiding this comment.
[P1] Capture the prediction from the staging scan
statement_time_prediction completes before stage_write dispatches. Both bulk staging handlers independently rescan the live predicate.
- A matching row inserted between those dispatches enters the staging scan.
RETURNINGand the transaction overlay include that row.- The saved prediction excludes it, so Calvin omits it at
COMMIT.
The static commit route performs no OLLP membership check that repairs this mismatch.
Return the matched surrogates from the staging handler and save that exact set. Add a deterministic interleaving test between prediction and staging.
| ); | ||
| if !matches!(leader, RouteDecision::Local) | ||
| || crate::control::planner::calvin::writes_edges(&buffered) | ||
| || crate::control::planner::calvin::routes_through_calvin(&buffered) |
There was a problem hiding this comment.
[P1] Enforce recomputed transitions before Calvin votes commit
This routes local predicate transactions through Calvin, which recomputes updates against current bodies. The new stage_bulk_update transition check only judges statement-time images. stage_calvin_bulk_update calls stage_apply_update without repeating transition enforcement.
- A collection allows
A→BandA→C, but forbidsC→B. - One transaction stages
A→B. - Another session commits
A→Cbefore the first transaction commits. - Calvin recomputes
C→Band votes commit. - Committed redo enforcement rejects the transition, and the scheduler halts with
RedoInstallRefused.
Run transition enforcement against the current and recomputed bodies during Calvin staging, before voting. Add coverage for this interleaving.
| let mut scans = Vec::with_capacity(bulks.len()); | ||
| for bulk in bulks { | ||
| scans.push( | ||
| reconnoitre( |
There was a problem hiding this comment.
[P1] Predict later statements against preceding writes
Every recon scan reads committed state with txn_id: None, before any batch statement applies. Sequentially awaiting these scans does not expose preceding writes.
For an edge-bearing collection containing n=1, submit this HTTP SQL batch without joined triggers:
UPDATE t SET n = 2 WHERE n = 1;
UPDATE t SET n = 3 WHERE n = 2;The implicit-transaction gate does not fire for this pair of ordinary bulk updates. Recon predicts [row] for the first statement and [] for the second. Calvin applies those fixed sets and reports success with n=2. The second update silently disappears.
Compute each prediction against the preceding statements' staged writes. Cover dependent predicates in one collection, alongside the existing independent two-collection case.
| let storage_key = StorageKey::for_surrogate(surrogate); | ||
| let Some(raw) = self | ||
| .sparse | ||
| .get(database_id, tid, collection, &storage_key)? |
There was a problem hiding this comment.
[P2] Read temporal identities from the versioned table
sparse.get reads the ordinary document table. Bitemporal inserts write their bodies into DOCUMENTS_VERSIONED, so this helper returns None for existing temporal rows.
On a nonminting coordinator without the catalog binding, an unlinked fusion hit still reports its surrogate decimal. Hybrid and vector searches inherit the missing owner identity through this helper.
Reuse text_base_body, which already selects versioned_get_current for temporal collections. Add RF1 coverage with WITH (bitemporal=true) and query every cluster node.
| let JsonValue::Object(map) = row else { | ||
| continue; | ||
| }; | ||
| let owner_key = match map.remove("_key") { |
There was a problem hiding this comment.
[P2] Separate owner identity from user-named score fields
The planner accepts _key as a hybrid score alias. For rrf_score(...) AS _key, this removal discards the user's score, including numeric values when no owner identity exists.
HybridSearchHit::write also emits duplicate _key fields when the score alias and owner identity share that name. flatten_hybrid_hits_to_relational_rows repeats the unconditional removal on gathered results.
Carry owner identity separately from fields whose names the query controls. Add direct and gathered hybrid coverage using _key as the score alias.
|
Thanks for the review. All five findings hold. The fixes are pushed to this branch as ten commits on top of
1. On the interleaving test: I did not write one, and I think none can exist. The race needed two reads of the predicate with a gap between them, the prediction scan and the staging scan. There is now one read, the staging scan. The rows it writes to the overlay, the rows it returns under
There is no fake-dispatch test of 2. 3. A request of two or more predicate writes runs as one implicit transaction, so each statement reads the rows the one before it staged. The recon coordinator keeps one predicate write per submission and returns an internal error for a second. The guard for two writes that move one sum balance now runs ahead of the route, because recon no longer sees those requests. Tests: 4. 5. The hybrid hit carries the owner key as Run on the new head: wire filters for edge, predicate, materialized sum, transactions, triggers, procedures and graph RAG (469 pass), in-process |
6f6194e to
c688c41
Compare
farhan-syah
left a comment
There was a problem hiding this comment.
Request changes. Four previous findings are resolved. Sequential batch visibility remains incomplete.
| Previous finding | Status | Checked implementation |
|---|---|---|
| Capture the prediction from the staging scan | Resolved | Staging reports its matched set, including empty sets. The session saves that response. |
| Enforce recomputed transitions before Calvin votes commit | Resolved | Calvin checks current-body transitions before staging the recomputed image. |
| Predict later statements against preceding writes | Partial | Multiple predicates use implicit staging. One predicate beside preceding point writes still bypasses it. |
| Read temporal identities from the versioned table | Resolved | Owner identity reads reuse text_base_body. |
| Separate owner identity from user-named score fields | Resolved | Reserved metadata and planner alias checks preserve _key scores. |
I accept your reasoning about the removed race. Capturing matches inside staging removes the second predicate read. A forced interleaving test between two reads is no longer required. I inspected the replacement tests without executing them.
The rebuilt series contains 11 commits. Their grouping addresses the history request, but two publication cleanups remain:
- Remove
Claude-Sessiontrailers from all 11 commits. Session URLs do not belong in the public commit history. - Update the PR description for this revision. It still reports 26 commits, superseded SHAs, the removed prediction scan, and the old
_keymetadata field.
Static review only, against c688c41ecb3e8d65a37f2b93eddedd33e61eb736. No builds, tests, or formatters ran. git diff --check passes. Existing CI reports Static gates passed and Test Suite skipped. Contributor runtime claims remain unchecked.
| fn holds_several_predicate_writes(tasks: &[PhysicalTask]) -> bool { | ||
| tasks | ||
| .iter() | ||
| .filter(|task| is_dependent_predicate(&task.plan)) |
There was a problem hiding this comment.
[P1] Predict later statements against preceding writes
The original two-predicate example now takes implicit staging. This classifier still excludes batches containing one predicate and preceding point writes.
Use plain document collections t and u on different vShards, with declared id primary keys. Seed t.x and u.y with n=0, without triggers or materialized sums. Submit this HTTP SQL batch:
UPDATE t SET n = 1 WHERE id = 'x';
UPDATE t SET n = 2 WHERE n = 1;
UPDATE u SET n = 9 WHERE id = 'y';- The task set contains
PointUpdate,BulkUpdate, andPointUpdate, so this predicate count is one. - Implicit staging does not fire. The strict multi-vShard route instead invokes
try_multi_shard_predicate_recon. - Recon scans
n=1before the first point update, producing an empty set. - The point writes keep the submission nonempty, so Calvin stages
t.x=1, an empty bulk update, thenu.y=9.
The stored result is t.x.n=1, instead of 2. All three commands use UPDATE, avoiding the separate mixed-command response-fold limitation.
Preserve statement order for batches containing a predicate and preceding writes, regardless of those writes' plan variants. Add mixed point/predicate coverage across vShards.
c688c41 to
ea23abe
Compare
|
Thanks for the second pass. The series is now at Remaining finding (predicate beside preceding point writes). Reproduced as written: with your batch the stored result is Publication cleanups. The On this head I ran the wire filters for edge, predicate, materialized sum, transactions, triggers, procedures and graph RAG (471 pass), in-process |
farhan-syah
left a comment
There was a problem hiding this comment.
Static re-review passes at ea23abef0d5d6316817ff2eed99243f2b10d0d4a. All five original findings are resolved in the reviewed code.
| Previous finding | Status |
|---|---|
| Capture the prediction from the staging scan | Resolved |
| Enforce recomputed transitions before Calvin votes commit | Resolved |
| Predict later statements against preceding writes | Resolved |
| Read temporal identities from the versioned table | Resolved |
| Separate owner identity from user-named score fields | Resolved |
The classifier now detects a predicate after preceding writes, including point updates. That routes the reported mixed batch through implicit staging. The new regression test uses both vShards and checks t.x.n=2 and u.y.n=9. I inspected the test without executing it.
The other four repairs remain unchanged since the previous review. The 11-commit series contains no Claude-Session trailers. The updated PR description records the current implementation and outstanding checks.
No new static findings. git diff --check passes. Existing CI reports Static gates passed and Test Suite skipped.
Full-suite and three-node results remain pending in your update. This records a clean static review, without merge approval or independent confirmation of runtime claims. No builds, tests, or formatters ran during this review.
…chosen Two writes of one request that move one materialized-sum balance settle against the same committed rows. The request is refused whole. The guard runs ahead of the route on HTTP, NDJSON, WebSocket and native requests.
ea23abe to
fdbe1c4
Compare
|
This branch is rebased on #429 closes #424 and #425, and it replaces most of what this PR did. I ran the tests of the old series on Ten wire tests still fail on
The PR description lists the red and green runs. The title and description changed: this PR no longer closes anything. If you prefer a new PR with a smaller history and this one closed, say so and I will open it from the same branch. I run the full local mirror of the |
|
Results of the local mirror of the
The timed-out test is Not run here: the three extra The binary of this head also runs on a NodeDB node with a real data set: search and |
…ransaction A multi-shard predicate write over HTTP, NDJSON, WebSocket and native SQL took the static Calvin path with no matched rows and was refused. It takes the recon coordinator that pgwire uses, and the coordinator scans per write. A request in which a predicate write follows another write runs as an implicit transaction, so each statement reads the rows the ones before it staged. Recon keeps one predicate write per submission and refuses a second.
A hybrid hit names its row under doc_id. A score alias of that name replaced the field, so every row answered an empty id. The planner refuses the alias and names the field.
fdbe1c4 to
eb0c9b2
Compare
|
The head is now
The three |
farhan-syah
left a comment
There was a problem hiding this comment.
Request changes. The three-commit rebuild reintroduces a materialized-sum concurrency error on the new recon route.
| Previous review area | Current status |
|---|---|
| Staged matched sets and transition enforcement | Superseded by merged #429. These changes are absent from this PR's diff. |
| Temporal and search identity repairs | Superseded by merged #429. The remaining doc_id alias guard has coverage. |
| Mixed point/predicate batch visibility | Retained. The implicit-transaction classifier still covers the reported batch. |
| Publication cleanup | Three commits, updated description, no session trailers. |
| Settlement read protection | Regressed. Recon receives no settlement read versions and constructs an empty OCC read set. |
The inline finding blocks merge. Restore the settlement reads through every recon adapter and into the transaction builder. Add deterministic coverage for a concurrent source change between settlement and recon.
Current-head CI now supplies independent test evidence:
- Workspace: 22,779 passed, including one recovered flaky test. Three skipped.
- Cluster: 515 passed.
- Static gates, CodeQL and Gitleaks: passed.
- Lint & Check: failed during
cargo deny. It reportsRUSTSEC-2026-0378andRUSTSEC-2026-0379againstwasmtime 49.0.3.
I checked that the dependency manifests, lockfile and deny.toml match the base. The dependency gate error is inherited, but CI remains red.
Static source review against eb0c9b25b2e95b9bfb95c93c402c02a7ba79e40a. I read existing CI logs without running builds or tests. git diff --check passes.
| // coordinator reads them, so it takes the same route as on | ||
| // pgwire. | ||
| let (tasks, authorized) = | ||
| match try_multi_shard_predicate_recon(state, exec.identity, tasks, authorized) |
There was a problem hiding this comment.
[P1] Preserve settlement reads on the recon route
This route consumes the tasks without passing sum_target_reads, then returns before the normal Calvin dispatch uses those reads. dependent_recon.rs:405-410 builds the predicted transaction with &[] instead.
Cross-shard balances still carry deltas computed from source images during planning. Their protection requires the corresponding OCC read versions. The Data Plane's materialized_sum/divergence.rs:98-111 explicitly skips cross-shard bindings because it relies on those reads.
A concrete interleaving:
- One source row has amount 10, and its cross-shard target balance is 10.
- Planning a predicate
SET amount = 1produces a shipped delta of -9. - Another request commits source amount 20 and target balance 20 before recon.
- Recon sees the same row identity and join key, so its prediction does not detect the amount change.
- Calvin sets the source amount to 1 and adds the stale -9 delta, leaving balance 11.
This commits a balance unequal to the source sum. The added uncontended settlement tests do not exercise this window.
Thread settlement reads through recon into build_single_vshard_predicted_tx_class. Preserve those versions across retries, or refresh settlement and reads together. Add a deterministic interleaving test that rejects or correctly recomputes the stale settlement.
A predicate write of a cross-shard materialized-sum source settles its balance delta before dispatch, from the source images it reads, and stamps those images as OCC reads. Recon consumed the tasks without the reads and built its TxClass with an empty read set. The Data Plane skips cross-shard bindings in its prediction check because it relies on those reads. A source row that moved after settlement then kept its row identity, passed recon, and applied the stale delta. The balance ended unequal to the source sum. Thread the settlement reads from the SQL, native and pgwire adapters through try_edge_recon, try_multi_shard_predicate_recon, run_recon and dispatch_authorized_dependent_edge_recon into build_single_vshard_predicted_tx_class. Every attempt of the retry loop reuses the same reads. A fresh reconnaissance does not refresh them, so a moved image aborts the transaction. write_admission still passes none: it settles no sum. Add the recon::before_submit fail gate and two failpoints wire tests. Each parks a predicate write after settlement, commits a source change, then releases the write and checks the balance against the source sum.
A predicate write on an edge-bearing collection that also drives a cross-shard materialized sum reaches maybe_dispatch_implicit_edge_recon with its tasks already settled: resolve_materialized_sum_targets runs earlier in the same function, and its reads are in scope at the call. The call passed an empty read set under a comment that said it fires before settlement. A source row that moved after settlement kept its identity, passed the stage vote and applied the stale delta. Pass the reads through to dispatch_calvin_multishard and correct the comment. The recon adapters of the previous commit take the same reads.
Red before the fix: balance stays 15, source sum is 0. Covers an edge-bearing and an edge-free source.
dispatch_truncate dropped every task but the TRUNCATE and passed no reads. It now submits the other tasks and the OCC reads in the one Calvin transaction.
Add two failpoints wire tests. One runs the predicate UPDATE over the native SQL opcode. One runs it through OpCode::DocumentBulkUpdate, which reaches handle_direct_op. Each parks the write at recon::before_submit, commits a source change, releases, and checks balance == source sum.
|
@farhan-syah The settlement-reads finding is addressed on this branch. Five commits sit on top of
Two questions:
|
A balance is one column of a target row. The guard keyed on collection and document id, so one predicate write that moved two sum columns of one row was refused as two writes of one balance. It keys on the column too. The doc_id score alias refusal now names the remedy.
No caller passes several predicate writes to recon: a request of several runs as an implicit transaction, and write admission passes one. The per-write scan, prediction map and node-guard dedup had no second item to serve. Restore the single-write form and keep the settlement reads and the before_submit gate. A second predicate write is still refused.
…ge recon maybe_dispatch_implicit_edge_recon took eight arguments after the settlement reads joined it, one over clippy's limit. It builds a CalvinDispatchSession from four of them anyway, so the caller builds that session and passes it.
…onstant Five wire tests carried their own copy of the POST to /v1/query. TestServer now owns it as http_query and http_exec. The several-writes refusal text is a public constant. The wire test matches the constant and the 400 status, so a rewording cannot leave it stale.
One predicate write that moves two sum columns of a cross-shard row ships two deltas to that row. Two predicate writes of one request that each move a different column pass the guard and both columns end equal to their source sums. Both tests fail with the guard keyed on the row alone.
A predicate write and the shipped balance of a materialized sum commit as separate groups in this mode. A failure of one leaves the balance unequal to the source sum.
The recon submit gate now also parks a TRUNCATE between settlement and its Calvin submit. Two wire tests, edge-free and edge-bearing, commit a new source row while the TRUNCATE is parked, then release it. The balance must equal the source sum. Without the settlement reads in the TRUNCATE transaction the balance ends 7 against a source sum of 0.
…ction A request that deletes a CRDT document and writes the source of a cross-shard materialized sum took the CRDT delete route. That route built its Calvin transaction with an empty read set, so a source row that moved after settlement applied a stale balance delta. The route now passes the settlement reads to the transaction builder. The wire test parks the request at the recon submit gate, moves a source row, then releases it. With the reads the balance equals the source sum. With an empty read set it ends 16 against 6.
…ment reads The route submits one task. A write that ships a materialized-sum balance to another vShard is a multi-shard task set. Under Strict it takes the group route, which carries the reads. Only BestEffortNonAtomic splits it, and that mode performs no cross-shard OCC.
Summary
Rebased on
mainafter #429. #424 and #425 are closed by #429, so this PR closes nothing.#429 fixes a predicate
UPDATEorDELETEinsideBEGIN … COMMIT, a trigger body or a procedure body, and the fusion and search key naming. This PR covers what #429 leaves open. It has 17 commits. Each fix sits beside its tests.mainfix(txn)refuse several writes of one sum balance before a route is chosen.fix(txn)key the guard on the balance column.XX000. It is refused whole. One predicate write that moves two sum columns of one row stays legal.fix(txn)run a request in which a predicate write follows another write as one implicit transaction.refactor(txn)recon scans one predicate write.UPDATEorDELETEover HTTP, NDJSON, WebSocket or native SQL outside a block, on a collection that spans vShards, answersXX000(calvin bulk predicate write reached overlay staging without ollp_predicted_surrogates). It takes the recon coordinator that pgwire uses, with one scan. A predicate that follows another write runs as an implicit transaction and matches the rows the earlier statements wrote.fix(txn)carry settlement reads into the recon transaction and into the pgwire implicit edge recon.40001. Every retry reuses the same reads.fix(txn)TRUNCATE carries settled balance tasks and reads.dispatch_truncatekept only the TRUNCATE task. TheApplyBalanceDeltatasks were dropped and the balance kept its old total. This fails onmainfor an edge-bearing collection. Without the new recon route an edge-free TRUNCATE passed, so commit 2 would have added the regression.fix(search)refuse a hybrid score alias nameddoc_id.doc_id. An alias of that name replaced the field, and every row answered an empty id. The planner refuses the alias with a message that names the remedy.fix(txn)carry settlement reads into the CRDT document delete transaction.40001.Also in the PR:
refactor(pgwire):maybe_dispatch_implicit_edge_recontakes theCalvinDispatchSessionbundle. The added arguments pushed it to 8, which clippytoo_many_argumentsrefuses.test(txn): oneTestServer::http_queryandhttp_exechelper serves five wire files. The refusal tests match status 400 and the constantSEVERAL_WRITES_REFUSAL.docs(txn): a comment onroute_write_to_calvinstates why the write-admission route holds no settlement reads. It submits one task, a multi-shard set takes the group route underStrict, andBestEffortNonAtomicperforms no cross-shard OCC.docs(architecture): a predicate write and its shipped balance commit as separate groups underbest_effort_non_atomic. A failure there leaves the balance unequal to the source sum.Strictis the default.How the commits were chosen
The tests of the earlier series were run on
mainafter #429, with none of its source. Passing means #429 covers the fix, and the fix was dropped.main_keyscore alias, fusion over HTTP and native SQL (39 tests)doc_idaliasTest plan
Red means the test run on
mainwithout the commit.mainmaterialized_sum_cross_shard_http: predicateUPDATEandDELETEof a cross-shard sum source over HTTP, NDJSON and WebSocket RPCXX000predicate_write_two_statements_http: two predicate updates of two collections in one requestXX000predicate_write_sequence_http: a third statement reads the first across two vShards. A predicate reads the point write before it across two vShards.XX000materialized_sum_same_request: two predicate updates of one balance. A predicate beside a point update of it.XX000before the guard is reachedmaterialized_sum_two_columns: one predicate write moves two sum columns. Two predicate writes each move a different column of one row.400with the guard keyed on the document onlymaterialized_sum_recon_settlement_reads: a predicate write parks after settlement, a source change commits, the write resumes (SQL, pgwire, native SQL, native bulk update)40001, balance equals the source summaterialized_sum_truncate_edge_bearing: TRUNCATE of an edge-bearing cross-shard sum sourcematerialized_sum_truncate_settlement_reads: a TRUNCATE parks after settlement, a posting joins the source, the TRUNCATE resumes (edge-free and edge-bearing)7, source sum0materialized_sum_crdt_delete_settlement_reads: a request with a CRDT document delete and a write to a cross-shard sum source parks after settlement, a source row moves, the request resumes16, source sum6sql_hybrid_search::a_score_alias_named_doc_id_is_refusedChecks on head
ad3447267--features failpoints; filtersmaterialized_sum,predicate_write,sql_hybrid_search,graph_crdt_delete_edges,truncate)settling_writes,calvin,txn_route)--all-targets --all-features --profile ci -D warnings)cargo fmt --all -- --checknodedb-preflight.shin the pre-push hook, rustc 1.99.1Earlier heads, same tests, nextest with
RUST_MIN_STACK=33554432:cfe5152e4:dml_suite59 of 59,common_suite332 of 332.eb0c9b25b: workspace 22,783 of 22,784 (the one timeout isnodedb-vectorsearch_allocations_do_not_scale_with_distance_count, which times out onmaintoo),nodedb-cluster-tests515 of 515.The full CI mirror has not run on
603e755af. TheTest Suitejob is skipped on a draft. It runs when the PR is ready.Tradeoffs
UPDATEandDELETEanswers 500 (one statement reported two command verbs) onmaintoo. This PR does not change it.best_effort_non_atomic, a predicate write and its balance commit as separate groups. Seedocs/architecture.md.BadRequestclass reaches HTTP clients as{"error": msg}with status 400 and no code field. This PR does not change it.Open
Two questions to the maintainer are on the PR thread. They have no answer yet.
is_edge_recon_planincludesCrdtOp::DocDelete, anddispatch_crdt_doc_deletesignores the reads. No sum binding can name a CRDT collection today. Is that an invariant? If yes, adebug_assertgoes there.submit_calvin_routed, so therecon::before_submitgate is not on its path. The concurrent-change test for TRUNCATE needs atruncate::before_submitgate infail_gate.rs, or another hook.Known on
mainwith rustc 1.96.1, not changed by this PR: twononminimal_boollints and an unused import innodedb-sql(search_cells.rs:32). Clippy 1.99.1 is clean.Evidence: https://github.com/EnRaiha/nodedb-fusion-evidence