fix(sql): propagate expression eval errors through remaining scan and validation paths

Widens the division-by-zero / undefined-function fix to the executor
and CRDT paths that still folded a failed expression to a silent
non-match instead of surfacing it: join residual ON predicates
(grace-hash and hash-probe), aggregate/window streaming accumulators,
GROUP BY key construction, msgpack scan filtering, and trigger WHEN
filters now thread `Result<_, EvalError>` up to the caller instead of
using `unwrap_or`/folding to false.

CRDT CHECK-constraint validation gains a first-class
`ValidationOutcome::EvalError` distinct from an ordinary `Rejected`
violation, so an unevaluable predicate is escalated straight to the
dead-letter queue rather than being handed to a declarative conflict
policy that could "resolve" it.

Adds composite pgwire coverage for division-by-zero in aggregate
arguments, GROUP BY keys, window ORDER BY, and join residual
predicates, and drops stale issue-number references from doc comments
now that the fix is broad rather than a single patch.

Author: farhan-syah

nodedb-crdt/src/constraint_checks.rs

@@ -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),
         }
     }
@@ -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());
@@ -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 {
@@ -81,41 +90,27 @@ impl Validator {
                             constraint.name
                         ),
                     },
-                });
+                }));
             }
         };
 
-        // A division/modulo-by-zero inside the predicate (nodedb issue #216)
-        // is neither a PASS nor an ordinary FAIL — it's an evaluation
-        // error. `Validator::validate`/`ValidationOutcome` (this crate's
-        // public API, consumed cross-crate by `nodedb`) has no error
-        // channel distinct from `Rejected(Vec<Violation>)`, so — exactly
-        // like the unparseable-expression case above — it fails closed as
-        // a `Violation` with a reason that names the real cause, rather
-        // than silently treating it as a pass. Giving eval errors a first-
-        // class outcome distinct from an ordinary CHECK failure would mean
-        // widening `ValidationOutcome` and its ~5 cross-crate call sites; a
-        // deliberate follow-up, not a mechanical part of this fix.
+        // 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) {
-            Ok(nodedb_types::Value::Bool(true)) => None,
-            Ok(nodedb_types::Value::Null) => None,
-            Ok(_) => 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) => Some(Violation {
-                constraint_name: constraint.name.clone(),
-                reason: format!(
-                    "CHECK `{}` failed to evaluate: {expr}: {e}",
-                    constraint.name
-                ),
-                hint: CompensationHint::ManualIntervention {
-                    reason: format!("CHECK predicate `{expr}` raised an evaluation error: {e}"),
-                },
-            }),
+            })),
+            Err(e) => Err(e),
         }
     }
 

nodedb-crdt/src/pre_validate.rs

@@ -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}"),
+        },
     }
 }
 

nodedb-crdt/src/validator/policy_dispatch.rs

@@ -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.
@@ -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,
+                    }],
+                })
+            }
         }
     }
 }

nodedb-crdt/src/validator/tests.rs

@@ -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:?}"),
+    }
+}

nodedb-crdt/src/validator/types.rs

@@ -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.

nodedb-crdt/src/validator/validate.rs

@@ -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,
+                    };
+                }
             }
         }
 

nodedb-query/src/expr/binary.rs

@@ -30,8 +30,8 @@ fn value_to_decimal(v: &Value) -> Option<Decimal> {
 }
 
 /// Apply a Decimal arithmetic operation, returning `Value::Null` on overflow
-/// and `Err(EvalError::DivisionByZero)` when `/` or `%` see a zero divisor
-/// (nodedb issue #216). The zero check happens before `checked_div`/
+/// and `Err(EvalError::DivisionByZero)` when `/` or `%` see a zero divisor.
+/// The zero check happens before `checked_div`/
 /// `checked_rem` are called because those methods return `None` for both
 /// div-by-zero *and* overflow, and only the former is a `22012` error —
 /// overflow keeps folding to `Value::Null` exactly as before.

nodedb-query/src/expr/eval.rs

@@ -20,9 +20,9 @@ use super::types::SqlExpr;
 /// `thiserror`-derived enum living next to the evaluator it describes.
 /// Everything that historically folded to `Value::Null` (bad casts, wrong
 /// arg counts, unknown-value coercions) keeps doing so — this type exists
-/// solely for division/modulo by a zero divisor (nodedb issue #216), which
-/// must surface as SQLSTATE `22012` (`division_by_zero`) instead of
-/// silently evaluating to `NULL`.
+/// solely for division/modulo by a zero divisor, which must surface as
+/// SQLSTATE `22012` (`division_by_zero`) instead of silently evaluating to
+/// `NULL`.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)]
 pub enum EvalError {
     #[error("division by zero")]
@@ -73,7 +73,7 @@ impl SqlExpr {
     /// TRANSITION CHECK path).
     ///
     /// Returns `Err(EvalError::DivisionByZero)` if evaluation reaches a
-    /// `/` or `%` with a zero divisor (nodedb issue #216) — every other
+    /// `/` or `%` with a zero divisor — every other
     /// historically-`NULL`-folding case (bad casts, wrong arg counts,
     /// unknown-value coercions) is unaffected and still evaluates to
     /// `Ok(Value::Null)`.
@@ -304,7 +304,7 @@ mod tests {
         assert_eq!(expr.eval(&doc()).unwrap(), Value::Bool(true));
     }
 
-    // ── Division/modulo by zero (nodedb issue #216) ─────────────────────────
+    // ── Division/modulo by zero ──────────────────────────────────────────────
 
     #[test]
     fn integer_division_by_zero_errors() {

nodedb-query/src/functions/math.rs

@@ -12,7 +12,7 @@ use nodedb_types::Value;
 /// Every function here returns `Ok(Value::Null)` on invalid/missing
 /// arguments, matching the crate-wide NULL-propagation convention — except
 /// `mod`'s zero-modulus arm, which returns `Err(EvalError::DivisionByZero)`
-/// (nodedb issue #216) instead of folding to `NULL`. This is the one
+/// instead of folding to `NULL`. This is the one
 /// function in the scalar-function table that can fail, so `try_eval`
 /// carries a `Result` inside the `Option` rather than making every sibling
 /// module in `functions/` fallible for a single arm.
@@ -58,8 +58,8 @@ pub(super) fn try_eval(name: &str, args: &[Value]) -> Option<Result<Value, EvalE
             };
             let b = num_arg(args, 1).unwrap_or(1.0);
             if b == 0.0 {
-                // nodedb issue #216: zero modulus raises SQLSTATE 22012
-                // instead of folding to NULL.
+                // Zero modulus raises SQLSTATE 22012 instead of folding to
+                // NULL.
                 return Some(Err(EvalError::DivisionByZero));
             }
             to_value_number(a % b)

nodedb-query/src/functions/mod.rs

@@ -26,7 +26,7 @@ use crate::expr::EvalError;
 /// Every function returns `Ok(Value::Null)` on invalid/missing arguments
 /// (SQL NULL propagation semantics) — the sole exception is `mod`'s
 /// zero-modulus arm (`math::try_eval`), which returns
-/// `Err(EvalError::DivisionByZero)` (nodedb issue #216). Every other
+/// `Err(EvalError::DivisionByZero)`. Every other
 /// sibling module here stays `Option<Value>`-shaped internally; only the
 /// `math` arm is threaded as `Option<Result<Value, EvalError>>` and the
 /// rest are wrapped in `Ok` at this dispatch boundary, so a single