Skip to content

Commit fb48b31

Browse files
committed
fix(crdt): reject deltas that violate one-document-per-delta contract
A CRDT delta may coalesce writes to multiple rows (or arrive tagged with a synthetic frame id matching no written row), but cross-engine identity only assigns one surrogate per delta. Materializing just the frame-declared row silently dropped the rest. `apply_committed_delta_validated` now returns the full write-set on a clean apply, and both the non-sync and sync apply paths reject the delta loudly when it names any row outside its declared target instead of partially materializing it. Split the apply handler out of crdt.rs into crdt_apply.rs to keep it within the file-size limit.
1 parent bb60351 commit fb48b31

5 files changed

Lines changed: 455 additions & 231 deletions

File tree

nodedb/src/data/executor/dispatch/crdt.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ impl CoreLoop {
2727
constraint_version_required,
2828
} => self.execute_crdt_apply(
2929
task,
30-
crate::data::executor::handlers::control::crdt::CrdtApplyParams {
30+
crate::data::executor::handlers::control::crdt_apply::CrdtApplyParams {
3131
collection,
3232
document_id,
3333
delta,

nodedb/src/data/executor/handlers/control/crdt.rs

Lines changed: 2 additions & 222 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,16 @@
11
// SPDX-License-Identifier: BUSL-1.1
22

33
//! CRDT operation handlers: read, versioned read, version vector, delta export,
4-
//! restore, compact, list insert/delete/move, and apply.
4+
//! restore, compact, and import snapshot. The delta-apply handler lives in the
5+
//! sibling `crdt_apply` module.
56
67
use tracing::{debug, warn};
78

8-
use nodedb_types::Surrogate;
9-
use nodedb_types::sync::violation::ViolationType;
10-
use nodedb_types::sync::wire::{AckStatus, SyncProvenance};
11-
129
use crate::bridge::envelope::{ErrorCode, Response};
13-
use crate::data::executor::sync_gate::SyncAdmit;
14-
use crate::engine::crdt::tenant_state::ValidatedApplyOutcome;
1510

1611
use crate::data::executor::core_loop::CoreLoop;
1712
use crate::data::executor::task::ExecutionTask;
1813

19-
/// Parameters for [`CoreLoop::execute_crdt_apply`].
20-
pub(in crate::data::executor) struct CrdtApplyParams<'a> {
21-
pub collection: &'a str,
22-
pub document_id: &'a str,
23-
pub delta: &'a [u8],
24-
pub surrogate: Surrogate,
25-
pub peer_id: u64,
26-
pub provenance: Option<&'a SyncProvenance>,
27-
pub constraint_version_required: u64,
28-
}
29-
3014
impl CoreLoop {
3115
pub(in crate::data::executor) fn execute_crdt_read(
3216
&mut self,
@@ -218,210 +202,6 @@ impl CoreLoop {
218202
}
219203
}
220204

221-
pub(in crate::data::executor) fn execute_crdt_apply(
222-
&mut self,
223-
task: &ExecutionTask,
224-
params: CrdtApplyParams<'_>,
225-
) -> Response {
226-
let CrdtApplyParams {
227-
collection,
228-
document_id,
229-
delta,
230-
surrogate,
231-
peer_id,
232-
provenance,
233-
constraint_version_required,
234-
} = params;
235-
let tenant_id = task.request.tenant_id;
236-
237-
let Some(prov) = provenance else {
238-
// Non-sync path (SQL / native client): validate + apply, no gate.
239-
// There is no client to reject here, so the validated outcome is
240-
// only observed for its DLQ side effect and logged.
241-
// Borrow the engine in a nested block so the &mut borrow is dropped
242-
// before the sparse write below takes &self. On a Clean apply we
243-
// read the merged row back and encode it while the borrow is live,
244-
// carrying the materialized bytes out.
245-
let materialized = {
246-
let engine = match self.get_crdt_engine(tenant_id) {
247-
Ok(e) => e,
248-
Err(e) => {
249-
warn!(core = self.core_id, error = %e, "failed to create CRDT engine");
250-
return self.response_error(
251-
task,
252-
ErrorCode::Internal {
253-
detail: e.to_string(),
254-
},
255-
);
256-
}
257-
};
258-
let outcome = engine.apply_committed_delta_validated(
259-
collection,
260-
delta,
261-
surrogate,
262-
document_id,
263-
peer_id,
264-
);
265-
match outcome {
266-
ValidatedApplyOutcome::Clean => {
267-
if surrogate != Surrogate::ZERO {
268-
Self::encode_crdt_row(engine, collection, document_id)
269-
} else {
270-
None
271-
}
272-
}
273-
ValidatedApplyOutcome::Rejected(vt) => {
274-
debug!(core = self.core_id, %collection, reason = %vt, "crdt apply violated constraint (DLQ)");
275-
None
276-
}
277-
ValidatedApplyOutcome::Malformed => {
278-
warn!(core = self.core_id, %collection, "crdt apply skipped malformed delta");
279-
None
280-
}
281-
}
282-
};
283-
// engine borrow dropped here; materialize into the sparse document
284-
// store so DocumentScan / ShapeSnapshot see the synced document.
285-
if let Some(bytes) = materialized {
286-
self.materialize_synced_document(
287-
task,
288-
tenant_id.as_u64(),
289-
collection,
290-
surrogate,
291-
&bytes,
292-
);
293-
}
294-
self.checkpoint_coordinator.mark_dirty("crdt", 1);
295-
return self.response_ok(task);
296-
};
297-
298-
// Sync path: run the idempotency gate before touching the engine.
299-
// Call sync_admit first (exclusive &mut self borrow, no engine borrow).
300-
let admit = self.sync_admit(prov);
301-
302-
// Snapshot the current HWM for Duplicate / Fenced / Gap responses
303-
// before any engine borrow.
304-
let current_hwm = self.sync_hwm_value(prov.producer_id, prov.stream_id);
305-
306-
let (status, applied_seq, reject) = match admit {
307-
SyncAdmit::Apply => {
308-
// Borrow the engine in a nested block so the &mut borrow is
309-
// dropped before sync_commit takes &mut self for sync_hwm.
310-
// The validated apply never fails: a violation is DLQ'd and a
311-
// corrupt blob is a no-op, so the HWM always advances and the
312-
// stream cannot wedge.
313-
//
314-
// Before validating, fence the delta against the constraint
315-
// version it was admitted against. `SetConstraints` rides the
316-
// same per-vshard data Raft log as this `CrdtApply`, so at
317-
// this log index every replica has applied the identical log
318-
// prefix and therefore has the identical installed
319-
// `constraint_versions[collection]` — the gate decision is
320-
// deterministic across replicas, no divergence.
321-
enum GateOutcome {
322-
Pending { installed: u64 },
323-
Applied(ValidatedApplyOutcome),
324-
}
325-
let (outcome, materialized) = {
326-
let engine = match self.get_crdt_engine(tenant_id) {
327-
Ok(e) => e,
328-
Err(e) => {
329-
warn!(core = self.core_id, error = %e, "failed to create CRDT engine");
330-
return self.response_error(
331-
task,
332-
ErrorCode::Internal {
333-
detail: e.to_string(),
334-
},
335-
);
336-
}
337-
};
338-
let installed = engine.installed_constraint_version(collection);
339-
if constraint_version_required > installed {
340-
(GateOutcome::Pending { installed }, None)
341-
} else {
342-
let applied = engine.apply_committed_delta_validated(
343-
collection,
344-
delta,
345-
surrogate,
346-
document_id,
347-
peer_id,
348-
);
349-
// On a Clean apply, read the merged row back and encode
350-
// it while the engine borrow is still live so the bytes
351-
// can be materialized into the sparse store below.
352-
let mat = if matches!(applied, ValidatedApplyOutcome::Clean)
353-
&& surrogate != Surrogate::ZERO
354-
{
355-
Self::encode_crdt_row(engine, collection, document_id)
356-
} else {
357-
None
358-
};
359-
(GateOutcome::Applied(applied), mat)
360-
}
361-
};
362-
// engine borrow is dropped here; mark_dirty / sync_commit take
363-
// &mut self, and the sparse materialize takes &self.
364-
let reject = match outcome {
365-
GateOutcome::Pending { installed } => {
366-
// Create-race: the constraints this delta was admitted
367-
// against are not yet installed on THIS replica (the
368-
// reconcile loop delivers SetConstraints
369-
// asynchronously). Do NOT import an unvalidated delta
370-
// — that is exactly the hole this fence closes.
371-
// Carry a retryable reject; the client re-pushes once
372-
// the install catches up. This is NOT a dead letter,
373-
// so it is not DLQ'd.
374-
debug!(
375-
core = self.core_id,
376-
%collection,
377-
required = constraint_version_required,
378-
installed,
379-
"crdt apply fenced: constraint version pending (retryable)"
380-
);
381-
Some(ViolationType::ConstraintVersionPending {
382-
collection: collection.to_string(),
383-
required: constraint_version_required,
384-
installed,
385-
})
386-
}
387-
GateOutcome::Applied(ValidatedApplyOutcome::Clean) => {
388-
self.checkpoint_coordinator.mark_dirty("crdt", 1);
389-
None
390-
}
391-
GateOutcome::Applied(ValidatedApplyOutcome::Rejected(vt)) => {
392-
self.checkpoint_coordinator.mark_dirty("crdt", 1);
393-
Some(vt)
394-
}
395-
GateOutcome::Applied(ValidatedApplyOutcome::Malformed) => {
396-
warn!(core = self.core_id, %collection, "crdt apply skipped malformed delta");
397-
None
398-
}
399-
};
400-
// Materialize the merged document into the sparse store so
401-
// DocumentScan / ShapeSnapshot see the synced write. `materialized`
402-
// is Some only on a Clean apply with an assigned surrogate.
403-
if let Some(bytes) = materialized {
404-
self.materialize_synced_document(
405-
task,
406-
tenant_id.as_u64(),
407-
collection,
408-
surrogate,
409-
&bytes,
410-
);
411-
}
412-
// Advance the HWM unconditionally after apply — a rejected,
413-
// fenced, or malformed delta must not wedge the sync stream.
414-
self.sync_commit(prov);
415-
(AckStatus::Applied, prov.seq, reject)
416-
}
417-
SyncAdmit::Duplicate => (AckStatus::Duplicate, current_hwm, None),
418-
SyncAdmit::Fenced => (AckStatus::Fenced, current_hwm, None),
419-
SyncAdmit::Gap { expected } => (AckStatus::Gap { expected }, current_hwm, None),
420-
};
421-
422-
self.sync_ack_response_ext(task, status, applied_seq, reject)
423-
}
424-
425205
/// Import a per-collection Loro snapshot into the tenant CRDT engine.
426206
///
427207
/// The durable RESTORE re-issue path replicates this through Raft so every

0 commit comments

Comments
 (0)