Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions nodedb-crdt/src/constraint_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,29 @@ fn render_value(value: &LoroValue) -> String {
}

impl Validator {
///
/// Returns `Err(EvalError)` when a CHECK predicate cannot be *evaluated*
/// (division/modulo by zero). This is distinct from an
/// ordinary constraint failure (`Ok(Some(Violation))`): the caller lifts it
/// to [`ValidationOutcome::EvalError`], a hard statement failure that
/// declarative conflict policies must never "resolve".
pub(crate) fn check_constraint(
&self,
state: &impl RowLookup,
change: &ProposedChange,
constraint: &Constraint,
) -> Option<Violation> {
) -> Result<Option<Violation>, nodedb_query::EvalError> {
match &constraint.kind {
ConstraintKind::Unique => self.check_unique(state, change, constraint),
ConstraintKind::Unique => Ok(self.check_unique(state, change, constraint)),
ConstraintKind::ForeignKey {
ref_collection,
ref_key,
}
| ConstraintKind::BiTemporalFK {
ref_collection,
ref_key,
} => self.check_foreign_key(state, change, constraint, ref_collection, ref_key),
ConstraintKind::NotNull => self.check_not_null(change, constraint),
} => Ok(self.check_foreign_key(state, change, constraint, ref_collection, ref_key)),
ConstraintKind::NotNull => Ok(self.check_not_null(change, constraint)),
ConstraintKind::Check { expr, .. } => self.check_expr(change, constraint, expr),
}
}
Expand All @@ -60,7 +66,7 @@ impl Validator {
change: &ProposedChange,
constraint: &Constraint,
expr: &str,
) -> Option<Violation> {
) -> Result<Option<Violation>, nodedb_query::EvalError> {
// Build a row Value::Object from the proposed change's fields so the
// expression evaluator can resolve column references by name.
let mut row = std::collections::HashMap::with_capacity(change.fields.len());
Expand All @@ -72,7 +78,10 @@ impl Validator {
let parsed = match nodedb_query::expr_parse::parse_generated_expr(expr) {
Ok((expr, _deps)) => expr,
Err(e) => {
return Some(Violation {
// A malformed *stored* predicate is a catalog-integrity failure,
// not an evaluation error — it stays an ordinary Violation
// (fails closed) so a corrupt entry can't bypass its invariant.
return Ok(Some(Violation {
constraint_name: constraint.name.clone(),
reason: format!("invalid CHECK expression `{expr}`: {e}"),
hint: CompensationHint::ManualIntervention {
Expand All @@ -81,20 +90,27 @@ impl Validator {
constraint.name
),
},
});
}));
}
};

// A division/modulo-by-zero inside the predicate
// is neither a PASS nor an ordinary FAIL — it's an evaluation error,
// propagated as `Err(EvalError)` and lifted by `validate` to
// `ValidationOutcome::EvalError`. That keeps it out of the conflict-
// policy machinery: an unevaluable predicate is a hard SQLSTATE-22012
// failure, never something LastWriterWins et al. may "resolve".
match parsed.eval(&row_value) {
nodedb_types::Value::Bool(true) => None,
nodedb_types::Value::Null => None,
_ => Some(Violation {
Ok(nodedb_types::Value::Bool(true)) => Ok(None),
Ok(nodedb_types::Value::Null) => Ok(None),
Ok(_) => Ok(Some(Violation {
constraint_name: constraint.name.clone(),
reason: format!("CHECK `{}` failed: {expr}", constraint.name),
hint: CompensationHint::ManualIntervention {
reason: format!("row violates CHECK predicate `{expr}`"),
},
}),
})),
Err(e) => Err(e),
}
}

Expand Down
10 changes: 10 additions & 0 deletions nodedb-crdt/src/pre_validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ pub fn pre_validate(
reason: v.reason.clone(),
}
}
// An unevaluable predicate (division/modulo by zero)
// is rejected before it ever reaches Raft — same terminal outcome as a
// violation, so the delta never commits.
ValidationOutcome::EvalError {
constraint_name,
error,
} => PreValidationResult::FastReject {
constraint: constraint_name,
reason: format!("CHECK predicate failed to evaluate: {error}"),
},
}
}

Expand Down
53 changes: 52 additions & 1 deletion nodedb-crdt/src/validator/policy_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@

use crate::CrdtAuthContext;
use crate::constraint::{Constraint, ConstraintKind};
use crate::dead_letter::CompensationHint;
use crate::error::Result;
use crate::policy::{ConflictPolicy, PolicyResolution, ResolvedAction};
use crate::row_lookup::RowLookup;

use super::core::Validator;
use super::types::{ProposedChange, ValidationOutcome};
use super::types::{ProposedChange, ValidationOutcome, Violation};

impl Validator {
/// Validate with declarative policy resolution.
Expand Down Expand Up @@ -195,6 +196,56 @@ impl Validator {
}
}
}
ValidationOutcome::EvalError {
constraint_name,
error,
} => {
// An unevaluable predicate (division/modulo by zero)
// is NOT a resolvable conflict — it must never be
// handed to a declarative policy, which would "resolve" a delta
// the server cannot actually evaluate. Escalate straight to the
// DLQ (fails closed) regardless of the collection's configured
// policy, mirroring `EscalateToDlq` but for an eval error.
let constraint = self
.constraints
.all()
.iter()
.find(|c| c.name == constraint_name)
.cloned()
.unwrap_or_else(|| Constraint {
name: constraint_name.clone(),
collection: change.collection.clone(),
field: String::new(),
kind: ConstraintKind::NotNull,
});
let reason = format!("CHECK `{constraint_name}` failed to evaluate: {error}");
let hint = CompensationHint::ManualIntervention {
reason: reason.clone(),
};
self.dlq
.enqueue(crate::dead_letter::EnqueueDeadLetterArgs {
peer_id,
user_id: auth.user_id,
tenant_id: auth.tenant_id,
delta: delta_bytes,
constraint: &constraint,
reason: reason.clone(),
hint: hint.clone(),
})?;
tracing::warn!(
constraint = %constraint_name,
collection = %change.collection,
%error,
"CRDT CHECK predicate raised an evaluation error; escalated to DLQ"
);
Ok(PolicyResolution::Escalate {
violations: vec![Violation {
constraint_name,
reason,
hint,
}],
})
}
}
}
}
38 changes: 38 additions & 0 deletions nodedb-crdt/src/validator/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,3 +415,41 @@ fn check_constraint_rejects_malformed_expr() {
_ => panic!("expected rejection for malformed CHECK expression"),
}
}

/// A CHECK predicate that divides by zero is an *evaluation* error, not an
/// ordinary violation: `validate` returns `ValidationOutcome::EvalError` so the
/// downstream conflict-policy machinery never "resolves" an unevaluable
/// predicate. Distinct from both `Rejected` (a genuine violation) and
/// `Accepted`.
#[test]
fn check_constraint_division_by_zero_surfaces_eval_error() {
let state = CrdtState::new(1).unwrap();
let mut cs = ConstraintSet::new();
// `100 / age` is unevaluable when age = 0.
cs.add_check(
"people_ratio_check",
"people",
"age",
"100 / age > 0",
"100 / age > 0",
);
let validator = Validator::new(cs, 100);

let change = ProposedChange {
collection: "people".into(),
row_id: "p1".into(),
surrogate: nodedb_types::Surrogate::ZERO,
fields: vec![("age".into(), LoroValue::I64(0))],
};

match validator.validate(&state, &change) {
ValidationOutcome::EvalError {
constraint_name,
error,
} => {
assert_eq!(constraint_name, "people_ratio_check");
assert_eq!(error, nodedb_query::EvalError::DivisionByZero);
}
other => panic!("expected ValidationOutcome::EvalError, got {other:?}"),
}
}
13 changes: 13 additions & 0 deletions nodedb-crdt/src/validator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@ pub enum ValidationOutcome {
Accepted,
/// One or more constraints violated — delta rejected.
Rejected(Vec<Violation>),
/// A CHECK predicate could not be *evaluated* — division/modulo by zero.
/// Kept distinct from [`ValidationOutcome::Rejected`]
/// because an eval error is a hard statement failure (SQLSTATE `22012`),
/// NOT a resolvable conflict: declarative conflict policies
/// (`LastWriterWins`, `RenameSuffix`, …) must never "resolve" an
/// unevaluable predicate the way they resolve a genuine violation. It fails
/// closed — escalated straight to the dead-letter queue.
EvalError {
/// The CHECK constraint whose predicate failed to evaluate.
constraint_name: String,
/// The underlying evaluation error (currently only `DivisionByZero`).
error: nodedb_query::EvalError,
},
}

/// A single constraint violation.
Expand Down
15 changes: 13 additions & 2 deletions nodedb-crdt/src/validator/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,19 @@ impl Validator {
let mut violations = Vec::new();

for constraint in constraints {
if let Some(violation) = self.check_constraint(state, change, constraint) {
violations.push(violation);
match self.check_constraint(state, change, constraint) {
Ok(Some(violation)) => violations.push(violation),
Ok(None) => {}
// A predicate that could not be evaluated (division/modulo by
// zero) is a hard failure, not a violation to
// be batched with the others — surface it immediately so it
// bypasses conflict-policy resolution downstream.
Err(error) => {
return ValidationOutcome::EvalError {
constraint_name: constraint.name.clone(),
error,
};
}
}
}

Expand Down
Loading