Fail loudly on corrupt Durofut envelopes (follow-up to #331)

[Copilot]

Summary

Narrow follow-up to #331 (which fixed #327). #331 fixed deep graph composition structurally, but a review flagged two remaining silent-corruption arms; this PR closes them.

• Durofut::ensure() now fails loudly when a JSON object carrying a node_type fails to deserialize (e.g. the new deserialize_raw_object rejection of a non-object child) instead of silently wrapping the raw envelope as a SQL node that only errors at execution time. Detection keys off the parse Err plus a generic-JSON node_type probe, so it is robust to serialized field order (avoids the field-order-coupled prefix sniff from this PR's original approach).
• collect_nodes() in df.explain() no longer panics via .expect() on an undeserializable child; it raises a clean PostgreSQL error via pgrx::error!, consistent with insert_nodes() in df.start().

Tests

• Loud failure on a corrupt Durofut envelope routed through df.seq.
• Envelope detection independent of serialized field order.
• Deep JOIN chain, locking in that JOIN/RACE share the corrected opaque-child path.

Validation

• cargo build --features pg17
• cargo clippy --features pg17 -- -D warnings
• cargo fmt -p pg_durable -- --check
• ./scripts/test-unit.sh (267 passed, 16 ignored)

[waldemort-auto]

Root cause confirmed: the serde_json 128-level deserialization recursion limit, and the node counts in #327 pin it exactly.

cc @pinodeca

Analysis pinned to main @ 75febcf, pg_durable 0.2.6, serde_json 1.0.151 (preserve_order only — unbounded_depth is not enabled, so the recursion check is compiled active).

Mechanism

Durofut embeds its children as nested objects rather than id references (src/types.rs:1163-1174), so the TEXT JSON envelope that carries a handle between SQL calls gains one nesting level per composer call. Every composer re-parses its operands through a single shared helper, Durofut::ensure (src/types.rs:1237-1251):

match serde_json::from_str::<Durofut>(s) {
    Ok(d) if VALID_NODE_TYPES.contains(&d.node_type.as_str()) => d,
    _ => Durofut {
        node_type: "SQL".to_string(),
        query: Some(s.to_string()),
        ..Default::default()
    },
}

The _ arm binds nothing, so the serde error is discarded with no log, no WARNING, no NOTICE. serde_json v1.0.151 (src/de.rs:63, remaining_depth: 128; check_recursion! at src/de.rs:1372-1387) decrements-then-tests-zero, so the deepest container that can be successfully entered is level 127 and entering the 128th errors with recursion limit exceeded — classified as Category::Syntax. That error is swallowed and the entire serialized tree becomes the query text of one node, which the worker later hands to PostgreSQL: syntax error at or near "{".

The counts are the proof

The chain does not stop at the collapse. After a collapse the handle is THEN{left:SQL, right:SQL}, i.e. depth 2, so nesting rebuilds and collapses again — period 127. A chain of N steps uses N-1 df.seq calls, the first collapse is call 128, and with c = greatest collapse op ≤ N-1 and j = (N-1) - c, the surviving tree is 2+j SQL nodes and 1+j THEN nodes:

Requested N	Predicted SQL / THEN	Reported in #327
100	no collapse (99 < 128) — completes	completes
200	73 / 72	73 / 72
1000	111 / 110	111 / 110

Both failing data points land to the unit with no fudge factor. No length cap, varlena limit, or truncation produces a 127-periodic pattern — which is also why the counts looked like "not a clean function of the requested depth" in the original report.

Other candidate boundaries were checked and ruled out: MAX_GRAPH_NODES = 10_000 is never approached and raises loudly; df.nodes.query is unbounded TEXT against an ~8 KB worst-case envelope; NAMEDATALEN and the varlena cap are not in play.

Two things the issue understates

This is not df.seq-specific. It is a Durofut::ensure defect. All eight composers funnel through the same helper — df.seq (src/dsl.rs:233-234), df.as (254), df.loop (326, 329), df.if (383-385), df.if_rows (406-407), df.join (428-429), df.join3 (444-446), df.race (466-467) — and corrupt identically once an operand exceeds the limit. df.seq is simply the one users chain deeply enough to hit it. df.start uses ensure_strict, whose Err branch tries to recover node_type via serde_json::Value — but Value is subject to the same 128 limit, so at depth ≥128 it also fails and falls through to an equivalent silent SQL wrap. ensure_strict is therefore not a safety net at depth.

MAX_GRAPH_DEPTH = 256 is unreachable. Durofut::validate_recursive (src/types.rs:1303-1341) enforces it, but any tree deep enough to trip it must first survive a serde parse of a ≥257-level envelope, which fails at 128. Since every entry point into the engine is a TEXT JSON envelope, the intended depth guard is effectively dead code for DSL-built and df.start-submitted graphs — reachable only by constructing Durofut structs directly in Rust, which is exactly what its tests do.

On this PR

The guard is placed correctly. Putting it inside Durofut::ensure — the single shared parse — means it covers all eight composers rather than just df.seq, and the parallel guard in ensure_strict covers the df.start path. It also fires at the right boundary: construction time, at the first offending df.seq call, instead of after df.start() has handed back an instance id. That is what the issue asks for.

Three things worth weighing:

1. It refuses rather than fixes. The ~127-call ceiling is unchanged; a user who legitimately wants a 200-step sequential chain still cannot build one, they just find out earlier. If that is the intended shipped behaviour it should be documented, and MAX_GRAPH_DEPTH should probably be lowered below the serde ceiling (e.g. 120) so users get the well-worded depth error instead of a serde recursion error. The alternative — enabling serde_json's unbounded_depth and calling Deserializer::disable_recursion_limit(), or moving to an id-reference/flattened envelope — would make long chains actually work and make MAX_GRAPH_DEPTH the real enforced limit. If that route is taken, note that insert_nodes (src/dsl.rs:1057-1240) recurses on left_node/right_node with no depth guard, so the failure mode would move to backend stack exhaustion unless the walk is bounded or made iterative.

2. looks_like_serialized_durofut() is coupled to serde field-declaration order. It requires "node_type" to be the literal first key. That holds today only because node_type is the first declared struct field — an implicit, unasserted invariant. Reorder the struct, or feed an externally authored envelope, and the sniff silently reverts to wrapping as SQL. An order-independent check (a bounded scan for a top-level node_type key, or keying off serde_err.classify() == Category::Syntax together with a leading {) would remove that coupling. Leading whitespace is handled; a UTF-8 BOM is not, though that is negligible. I found no meaningful false-positive risk — user SQL beginning with a JSON literal starts with SELECT, and the arm only runs after from_str already failed.

3. The new test does not pin the boundary. It asserts that N=200 raises, but not that a 127-call chain still succeeds and a 128-call chain fails, and it does not cover the other seven composers, the ensure_strict/df.start path, or that shorter chains still work. An off-by-one shift in the ceiling or a regression to silent-wrap on df.join would pass. Worth noting why the suite never caught this: the four existing depth tests (src/types.rs:2125-2176, src/lib.rs:3032-3090) build trees in memory at depth 255-257 and never call to_json()/ensure(), so they never touch serde_json::from_str. No test anywhere exercises the envelope path above depth ~3.

Sibling fallbacks of the same class, untouched here

• for_each_config_child (src/types.rs:1358-1361) and transform_config_children (src/types.rs:1411) swallow Err(_) and silently skip embedded condition_node/extra_nodes children.
• Durofut::is_durofut (src/types.rs:1231) unwrap_or(false), so a valid but >127-deep envelope is silently reported as "not a durofut".
• The PL/pgSQL df.ensure_durofut (src/lib.rs:790-818) has an EXCEPTION WHEN OTHERS → treat-as-SQL handler — the same silent-wrap anti-pattern on the ?>/!> path.
• df.explain on an already-corrupted instance renders the giant JSON as a SQL node body; flagging a SQL node whose query parses as a Durofut envelope would make triage obvious.

Happy to send a PR for whichever direction the maintainers prefer — the documented-ceiling variant or the unbounded-depth variant.