Increase custom datatype limit and make it explicit - #1966

🟠 CRITICAL-1 — the read path now panics where it used to degrade

(the assert! in customer_datatype)

The debug_assert! → assert! promotion here is the right instinct in the wrong direction, and I think it needs to be split: hard-fail at the write boundary, keep degrading at the read boundary.

The write side is already covered — checked_dt_id in the resolver, check_commit in build_commit, the push and import checks — so nothing legitimate can mint a payload past MAX_PAYLOAD any more. But customer_datatype is also called from the decode path, and there the input is bytes off disk rather than an id the engine just allocated.

Concretely: OTypeRegistry::builtin_only() is new(&[]), so dt_otypes.len() is exactly RESERVED_COUNT (o_type_registry.rs:31-55). That means in binary_index_store.rs:2630 (decode_value_from_kind, the late-materialization route for Binding::EncodedLit) and :2613 (o_type_from_kind), every dt_id >= 15 falls into the else arm at o_type_registry.rs:127:

} else {
  // dt value beyond what we know — treat as customer datatype.
  OType::customer_datatype(dt.as_u16())
}

That comment is the point — that arm is the documented graceful handling for a dt the registry doesn't recognize, and it is now a process panic for any dt_id > 16383. The dt_id it receives is unvalidated: DatatypeDictId is pub struct DatatypeDictId(pub u16) with a plain from_u16, and FactKey::from_decoded_row builds one straight from Region 2 decode output (run_record.rs:418, DatatypeDictId::from_u16(dt_raw as u16)).

Consequence: a truncated leaflet read, a bit flip in the dt column, or an index written by a future format that widens dt past 14 bits takes down the query — where before the PR it produced a wrong-but-contained OType and the query returned. That is the inverse of the fallback contract the engine holds elsewhere (docs/contributing aside, it's the same shape as the PSOT fast-path builders returning Ok(None) rather than propagating when overlay translation fails).

I could not construct a normal-operation input that reaches it — the write-side guards do look complete to me — so I'm not calling this a live bug. But the branch that now panics is precisely the unknown-dt branch, and its input is decoded bytes, so I don't think it should be the one that aborts.

Minimally, degrade in the registry and leave the assert! as the write-side guard it was meant to be:

  fn resolve_by_dt(&self, dt: DatatypeDictId) -> OType {
      let idx = dt.as_u16() as usize;
      if idx < self.dt_otypes.len() {
          self.dt_otypes[idx]
      } else if dt.as_u16() <= OType::MAX_PAYLOAD {
          OType::customer_datatype(dt.as_u16())
      } else {
          // A dt past the payload width can only come from a damaged or
          // future-format row; decode it as unknown rather than aborting.
          OType::RESERVED
      }
  }

Or, if you'd rather keep one bound in one place, give DatatypeDictId::from_u16 the same Option treatment try_from_dict_id has and make the decode sites say what they want on overflow. Either way the write path keeps its hard guard.

Commenting here because fluree-db-core/src/o_type_registry.rs:127 and fluree-db-binary-index/src/read/binary_index_store.rs:2630 are not in this diff.

Fixed in 8edff18

🟠 CI-1 — the test check is red, and the PR body accounts for a different test

(the line that wires the new file into this binary)

The test job on 7e3de3b7c fails: grp_index::it_indexing_workflow::file_based_indexing_then_new_connection_loads_and_queries hits nextest's terminal timeout at 360.005 s (run 36263447359). The "Known limitations" section names it_cached_handle_cow_cancel as the intermittent one, so this failure isn't covered by it.

I dug into whether this is a regression and I don't think it is, so I want to give you what I found rather than just "CI is red":

• That test took 0.278 s on feat(cli): fluree server run --memory for a throwaway in-memory server #1968's run, 0.406 s on feat(api): branch from a point in time with at: "time:<ISO>" #1969's, 0.496 s on Accept TriG on insert (API, server, CLI) #1971's — all within a day of this branch.
• On this branch it passes in 2.531 s in isolation locally, and the full grp_index binary passes 104/104.
• But in that full local run it took 26.696 s under contention, and the new it_custom_datatype_limit::insert_past_datatype_limit_is_rejected_from_index took 42.443 s.

So my read is that the new file lands several genuinely heavy tests into grp_index and the CI runner — which is fighting 13,429 tests at once on far fewer cores than my laptop — tipped a neighbour over the 360 s wall. It's a plausible story but it is still a story, and a 0.4 s test going to >360 s is a big jump to attribute entirely to scheduling.

What I'd want before merge: a re-run. If it comes back green, a line in the PR body saying so is enough. If it recurs, I think the answer is to move the heavy round-trip cases (the 300-datatype full-rebuild and incremental ones) out of grp_index into their own target, so they stop competing with the indexing suite they were filed next to.

This also happens on main and several other branches as well, so the flakiness predates this pr. I'll dig into it after this is merged.

🟡 PERF-1 — every commit pays a per-flake scan for a ceiling no ledger is near

(the check_commit call at step 4b)

check_commit walks every flake in the commit. For each one it compares the datatype Sid to the previous flake's, and whenever they differ it runs is_reserved_datatype() — a linear scan over the 15 RESERVED_IRIS doing strip_prefix plus a string compare — and then an FxHashMap lookup.

The last == Some(dt) run-skip in new_datatypes only pays off when flakes arrive grouped by datatype. In an ordinary commit they don't — a subject with a string, an int and a date alternates the datatype on essentially every row — so most flakes take the full reserved scan plus the lookup. That's a new linear pass over the commit's flakes on the write hot path, and build_commit is on every transaction, SPARQL update, merge, rebase, revert and Cypher write.

What makes it worth fixing rather than accepting is that the work is provably unnecessary almost always. adding can never exceed the number of flakes plus txn-meta entries, so if the remaining headroom already exceeds that count the check cannot fail. That's an exact O(1) guard, not a heuristic:

pub fn check_commit(base: &LedgerState, flakes: &[Flake], txn_meta: &[TxnMetaEntry]) -> Result<()> {
  let known = known_datatypes(base);
  // Each flake and each txn-meta entry contributes at most one new datatype,
  // so a commit smaller than the remaining headroom cannot cross the limit.
  let used = known.non_reserved_datatype_count();
  if used + flakes.len() + txn_meta.len() <= MAX_NON_RESERVED_DATATYPES {
      return Ok(());
  }
  let meta_datatypes: Vec<Sid> = txn_meta

With 16,369 non-reserved slots, that short-circuits every commit on every ledger that isn't already deep into custom datatypes — which is all of them today — and the full scan only starts running once a ledger is genuinely close to the ceiling, where you want it.

Worth noting known_datatypes() sits above this too: when persisted_datatype_count() != store.dt_sids().len() it rebuilds an entire RuntimeSmallDicts (cloning every datatype Sid, then re-assigning every runtime datatype through is_reserved_datatype) — per commit. The early-out above skips that as well, since the guard only needs non_reserved_datatype_count(). If the two counts routinely disagree on a freshly-loaded ledger, that rebuild is the more expensive half of the two.

Fixed in 60949e1. The early exit also skips the dictionary rebuild too, and the reserved check is faster for big commits that the early exit doesn't cover.

🔵 OPT-1 — the push path now holds all_flakes across the whole validation section

Moving the forged-flake screening from 4.5 up to 4.0.2 is fine on its own terms — base_state isn't mutated inside the loop and c is an immutable borrow, so the all_flakes clone can't go stale, and I confirmed the drop_forged_commit_flakes / stamp_graph_on_commit_flakes pair still runs on every commit before it reaches novelty, so #1846's defense is intact.

The side effect is that a full clone of each commit's flakes is now live across steps 4.1–4.4 (retraction invariant, policy, staging, SHACL) instead of being created and consumed at 4.5. For a large push that's a longer-lived allocation at a point where the staging work is also holding memory. It's per-iteration so it doesn't accumulate across commits, and I don't think it's a problem — but since the reason for the move is to get all_flakes early for the datatype check, and the check only needs f.dt, I wonder if it's worth collecting just the datatypes at 4.0.2 and leaving the flake clone where it was. More of a question than a suggestion.

Fixed in 2b7e0b4.

🔵 OPT-2 — the downgrade direction deserves a sentence in the docs

The docs say a ledger already past 241 datatypes "need no migration and index again once upgraded", which is the right and reassuring half. The other half isn't stated: once a ledger accepts its 242nd custom datatype under this release, rolling the binary back makes it un-indexable again, because the old resolver's u8 guard returns. That's inherent and I don't think it needs anything but a note — but it's the kind of thing an operator wants to read before the rollback, not during it.

This is minor and non-blocking, but if you agree it's right I'd rather see it folded in now than lost in the backlog.

Added in 3a7707a.

⚪ NIT-1 — u16::MAX saturation is safe by ordering, not by construction

The comment is careful and the reasoning holds — current_custom_datatype_iris errors before sort_remap_and_write_sorted_commit runs, so a saturated id can't reach an OType. I traced it and agree.

The thing that nags slightly is that u16::MAX is now a value that would panic (per CRITICAL-1) if the ordering ever changed, and nothing in the type system says so. If CRITICAL-1 lands as a saturating/Option read path this stops mattering entirely, so it may just fall out.

Fixed in 6392f01