fix(core): make FlakeMeta's Ord agree with its Eq

[aaj3f]

FlakeMeta derives PartialEq/Eq/Hash over both of its fields but hand-writes Ord, and the two had quietly drifted apart. cmp compared lang only when neither side carried a list index, so {lang: "en", i: 0} and {lang: "fr", i: 0} came back Equal while == said they were different values. Ord's contract says that can't happen, and the places that trusted it are exactly the places that broke.

The whole change is a tiebreak, replacing the match in fluree-db-core/src/flake.rs:

self.i.cmp(&other.i).then_with(|| self.lang.cmp(&other.lang))

Option's derived Ord reproduces the old explicit arms (None < Some) on both fields, so this is the old comparator plus a tiebreak — nothing gets reordered.

What it actually broke

Three sites, all of which express identity through this ordering rather than through Eq:

• fluree-db-novelty/src/fact_state.rs:45 — FactKey is an imbl::OrdMap key, so {en, 0} and {fr, 0} were literally the same map slot. is_asserted answered true for a fact the map had never seen, and the per-commit dedup at fluree-db-novelty/src/lib.rs:886 dropped it. Worth noting the key carries no t, so this bit across any two commits, not only same-t ones.
• fluree-db-novelty/src/lib.rs:1561 — same_identity tests cmp_meta(a, b) == Ordering::Equal, which drives the bulk_apply_commits dedup walk on the cold-load / commit-replay path.
• Segment::range's exclusive lower bound (fluree-db-novelty/src/lib.rs:237-268, the partition_point at :252). With an ordering that calls the twins Equal, seeking past the @en flake also skips the distinct @fr flake. This one I'd rather understate than oversell: tracing every in-repo caller, the two that pass leftmost: false (fluree-db-query/src/fast_path_common.rs:2077 and fluree-db-query/src/binary_scan.rs:1575) both build a synthetic bound with m: None and t: i64::MIN, and t is compared before the metadata tiebreak — so nothing in the tree reaches it today. It's latent, not live, unlike the first two. It still decides the shape of the fix, though, for a reason that doesn't lean on today's callers: Novelty::range_flakes (:1352) is pub and takes an arbitrary first, and "strictly after this flake" is inherently an ordering predicate — no identity helper and no amount of caller discipline can express it. It's pinned by a test now rather than left to a hand-check.

The user-visible shape is a novelty↔index divergence, not a lost write

Worth being precise about, because earlier descriptions of this bug (mine included) said the write was silently discarded, and that overstates it.

The persisted index contains no Flake and no FlakeMeta at all. It's built from RunRecordV2 (fluree-db-binary-index/src/format/run_record_v2.rs:66), a 32-byte #[repr(C)] POD of integer columns where m.lang is folded into o_type (tag 0b11 = rdf:langString, payload = lang_id) and m.i is its own o_i column — so the index always kept the two apart. The flake is durable in the commit blob, and the index build recovers it.

So what a user saw was the same query against the same ledger returning one entry before indexing and two after, with no write in between — an answer that flips on a background maintenance event. That's worse than a plain missing write in one respect (it's nondeterministic, and read-your-writes is violated until the indexer runs) and better in one that matters for landing this: nothing is durably lost, so there's no data repair and no migration here. The facts simply become visible consistently.

Why it's safe to touch a comparator every crate reads

cmp_meta is the last tiebreak in all four index comparators (fluree-db-core/src/comparator.rs:167,179,191,206), after s, p, o, dt, t, op — two flakes only reach it if they're identical in every one of those. And lang: Some(_) implies dt == rdf:langString on every parser path, so the metadata tiebreak is only reachable between two language-tagged flakes of the same lexical form. Within that, exactly one branch changes: both sides Some(i) with equal i and differing lang.

Rather than just assert that, new_ord_refines_old_ord holds the old comparator verbatim beside the new one and quantifies both over the (lang, i) domain. Every pair the old comparator decided is decided identically, and the entire change is pairs it called Equal that Eq already called distinct. That's the blast radius, and it's checkable from the test rather than taken on trust.

Nothing on disk is ordered by this relation either. Across fluree-db-binary-index/src and fluree-db-indexer/src the only reference to fluree_db_core::comparator is an IndexType enum import in stats/class_property.rs — no comparator function from core is called on any index write or read path, so FlakeMeta::cmp can't participate in branch routing, a leaflet seek, a range bound, or a leaflet-skip predicate. I checked it the other way round as well, by building a real persisted indexed ledger under the old comparator and then reading the identical bytes back under the new one (subject crawl, predicate-value lookup, an ordered range with orderBy/limit, a full scan, a bound-object lookup, and select * crawls of the lang/list-heavy subjects) — results identical. So: no format bump, no reindex, no dual-read, no migration.

The one behavioral edge

FlakeMeta::max() is {lang: None, i: Some(i32::MAX)}. Under the old relation {lang: Some(_), i: i32::MAX} compared Equal to it, so an inclusive upper-bound test admitted it; now it compares Greater and is excluded — the inclusive bound narrows by exactly that pathological case.

It's unreachable through all six bound builders in the tree: the four Flake::max_* here, plus predicate_walk_bounds and overlay_walk_bounds in fluree-db-query. What guards them is t, not the object — all six pin t to i64::MAX and op to true, both compared before the metadata tiebreak in every one of the four comparators, and no real flake carries t == i64::MAX. The o/dt maxima look like they're doing the work and mostly aren't: overlay_walk_bounds pins o to the pattern's bound object and dt to Sid::max() whenever the pattern carries one, so at that site the object half of the argument is simply false. Since "max() dominates every meta" stops being true here, the doc at FlakeMeta::max names t as the load-bearing guard and says what a new bound builder would have to do to reach the narrowing — and the guard is now enforced rather than described: every_bound_builder_pins_the_sentinel_guard in fluree-db-query/src/binary_scan.rs quantifies over all six builders (the four Flake::max_* — with the max_psot alias asserted separately, so de-aliasing it can't silently drop the pin — plus predicate_walk_bounds and overlay_walk_bounds, the latter across both of its object arms) and asserts each pins t == i64::MAX and op == true. The doc points at the test by name, so the paragraph is a pointer to a gate rather than the gate itself. predicate_walk_bounds goes pub(crate) solely so the test can reach it; its one production caller is unchanged. The narrowing itself is pinned in new_ord_refines_old_ord too. (min() is unaffected: {lang: None, i: None} sorted below it under both relations.)

EdgeKey is deliberately left alone — and now says so itself

fluree-db-core/src/edge.rs has its own lang/list_i fields whose derived Ord nests them in the opposite order from FlakeMeta::cmp's, and that must not be "harmonized": the field order is a persisted on-disk key order for the edge-annotation arenas, and reordering it would invalidate every arena already written. The full argument — including why the mismatch only becomes observable when list-occurrence annotations land, which is exactly when someone might be tempted to fix it — now lives where the next reader will actually be, as a doc comment on EdgeKey itself, rather than only in this description.

remove_stale_flakes is not a workaround to unwind

Two comments in fluree-db-core/src/range.rs described #1711 in the present tense, and after this they'd read as false, so they move to the past tense. Their argument is untouched and still worth keeping: remove_stale_flakes hashes the full fact identity, which is robust precisely because it never consults Ord at all.

Which is also why it was already correct, and I've said so in the comment rather than leaving it implicit. "The Ord bug is fixed, so this is no longer necessary" is the wrong thing for the next reader to conclude — dropping m from that key is what silently collapses language variants on insert (#1273), a separate hazard this change does nothing about.

Tests

• fluree-db-core/src/flake.rs — the three laws: new_ord_refines_old_ord, ord_agrees_with_eq, ord_is_a_total_order.
• fluree-db-novelty — both fact_state::meta_is_part_of_identity and the local_identity_helpers_match_core_spot_comparator_semantics drift guard read as though they already covered this and didn't: every meta they build has i: None, which is the one branch the old ordering compared lang on. Both grow the missing case, and the fact_state one also pins that retracting one tag doesn't tombstone the other.
• fluree-db-novelty/src/lib.rs — exclusive_seek_past_a_tagged_bound_keeps_its_sibling_tag pins the seek directly, since it's the one of the three sites nothing else in the suite would notice regressing. It builds a one-segment novelty holding both tags at list position 0 and seeks strictly past @en; pre-fix that comes back []. It goes through a single apply_commit on purpose, so fact_state's dedup — updated only after the accept loop — never sees either flake. That keeps the assertion independent of the FactKey/same_identity route every other test here takes, and it's why the pre-fix failure lands on the seek rather than on the segment-population guard above it.
• fluree-db-api/tests/it_filtered_delete_list_meta.rs — the note in there reserved an end-to-end leg for whenever this shape became constructible. It is now, so the novelty-only sibling test builds it and retracts the tag that sorts first, and a new test asserts the novelty view and the post-index view are equal across three legs: two commits landing at the same position, one commit with two sibling @lists, and a cold reload replaying the commit chain. Equality is the sharper assertion here — the divergence was the defect, so agreement fails whichever side regresses.
• fluree-db-query/src/binary_scan.rs — every_bound_builder_pins_the_sentinel_guard, described above: the six-builder t/op pin as a gate instead of prose. It isn't a comparator regression test (it passes under either comparator, by design), so its non-vacuity check is its own: dropping the t pin from Flake::max_for_predicate, and separately from overlay_walk_bounds, each fails the test naming exactly that builder; restored, the suite is green.

Every one of the new and extended comparator regression tests fails on the pre-fix comparator, which I verified by reverting the one-line change and re-running before restoring it: the two api tests with the symptom ["hello@en"] where both tags are expected, the seek with [] where ["fr"] is.

The number worth recording, though, is the one that didn't move. Under the pre-fix comparator 887 of the 892 tests in fluree-db-core + fluree-db-novelty still pass, and 7 of the 9 in it_filtered_delete_list_meta — the five and the two being exactly the ones written for this bug. So nothing else in either suite depended on the old fold, which is a considerably better argument that the ordering change is inert than "the tests pass" is.

Unblocked, not done here

fluree-db-novelty/src/fact_state.rs:26-31 argues for moving its OrdMap to imbl::HashMap, and correctly flags that switching key equality from Ord to Hash/Eq is a semantic change. Now that FlakeMeta's two relations agree, that switch is a pure performance change as far as metadata goes — and it deserves more weight than a footnote: is_asserted/record run per accepted flake on the per-commit path, and the switch takes them from O(log novelty_facts) to amortized O(1). That's a larger effect than anything this PR's tiebreak costs, which is two Option compares over a short tag at the very tail of the key, reached only when everything before it already ties. What's left before making the switch is FlakeValue's cross-representation numerics (Long(3) == Double(3.0) == BigInt(3)), which still wants its own cross-type dedup tests. Deliberately not doing it in this PR, but it's a much better place to make that change from than it was, and I didn't want the option to get lost.

Fixes #1711

[bplatz]

Approving. The mutation evidence here is the strongest of the recent batch — I reverted the one-line comparator and ran both suites with --no-fail-fast (the api test is its own binary, so a --test grp_query filter silently runs nothing):

suite	baseline	legacy comparator
core + novelty	847 pass	842 pass, 5 fail
it_filtered_delete_list_meta	9 pass	7 pass, 2 fail

Exactly the five and two you cite, and exactly the tests written for this bug. Nothing else in either suite depended on the old fold — that's a far better inertness argument than a green run, and it holds up.

The contract violation is real: the old match consulted lang only in the (None, None) arm of i, so the precondition is both sides carrying i: Some(x) with equal x — two language-tagged values at one @list position. Narrow, and the writeup scopes it accurately rather than overselling.

Perf: nil, as far as I can measure or reason. cmp_meta is the last tiebreak in all four comparators and FlakeMeta is the last component of FactKey, so then_with short-circuits before the added Option<String> compare in almost every call. The one site that reaches it often is bulk_apply_commits' par_sort_unstable_by at lib.rs:1032 (which orders m before t, so every pair sharing (s,p,o) hits it) — still two Option compares on a 2-5 byte tag, on the cold-load path. The structural costs (a few more OrdMap slots, a few more novelty bytes, the seek returning the sibling) are all just the bug not happening.

Worth saying the forward-looking win is underweighted in the description: unblocking the fact_state OrdMap → imbl::HashMap switch takes is_asserted/record from O(log novelty_facts) to amortized O(1) on the per-commit path. That's larger than anything this costs.

One thing to clear before merge: CI's test job is red. it_ledger_lifecycle::ledger_exists_on_file_storage failed (11412/11413 passed). It passes locally on this branch and asserts soft-drop nameservice record state, so there's no plausible path from a metadata comparator to it — almost certainly an unrelated flake. But the gate list in the description says green, and a check that failed isn't a pass. Re-run, or name it as known-flaky.

Two inline notes below.

[bplatz]

The EdgeKey reasoning in the PR description is right and worth keeping — reordering those fields would invalidate every persisted annotation arena — but it's in the wrong place. edge.rs is untouched, so the warning lives only in a PR body. The next person to spot the mismatch will be reading edge.rs, not #1727.

It also sharpens over time: EdgeKey::list_i is documented "v1 always None", so the opposite nesting (lang before list_i) is latent today and goes live when list-occurrence annotations land — which is exactly when someone might "fix" it.

That paragraph belongs as a doc comment on EdgeKey.

[bplatz]

"That doc is the enforcement mechanism" is an honest thing to write, and it's the part I'd want to firm up. The narrowing behaviour is pinned in new_ord_refines_old_ord, but the property a future bound builder has to preserve — pin t to i64::MAX — is enforced only by this prose.

A test quantifying over all six builders (the four Flake::max_* plus predicate_walk_bounds / overlay_walk_bounds) and asserting each pins t == i64::MAX would turn the doc into a gate for roughly ten lines.

Flagging it as a pattern rather than a nit specific to this PR: #1718 (Fluree::ledger() must stay manager-free or the commit path deadlocks) and #1719 ("every 503 here is retryable", keyed on status alone) both landed the same shape recently — a load-bearing invariant whose only guard is a comment.

[aaj3f]

Thanks @bplatz — both items landed, and both were the right call.

The EdgeKey warning now lives on EdgeKey (6549f4f02): a doc section stating that field order is a persisted on-disk key order — arena leaf rows sort (EdgeKey, ann_sid, t, op) and the branch first_edge/last_edge bounds are seeked in EdgeKey order — that it intentionally differs from FlakeMeta::cmp's nesting, and that reordering means an ARENA_VERSION bump plus a rebuild. I re-verified the anchors rather than copying them across (annotation_arena/format.rs:20-23, :93/:95, reader.rs:151-155, and the partition_point leaf seeks at :422/:522). Your sharpening about list_i being "v1 always None" — latent until list-occurrence annotations land, which is exactly when someone would "fix" it — is in the comment verbatim in spirit.

The sentinel invariant is now a gate, not prose (bdf4f121d): every_bound_builder_pins_the_sentinel_guard in binary_scan.rs asserts t == i64::MAX and op == true across all six builders — I first re-verified by grep that six is exhaustive (the four Flake::max_* plus the two query-side walk-bounds; max_psot is a delegating alias, asserted separately so a de-aliasing can't drop the pin). It lives in the query crate because overlay_walk_bounds is private to BinaryScanOperator; the one visibility change is predicate_walk_bounds → pub(crate), marked test-only. Mutating the t pin out of either a core builder or overlay_walk_bounds fails the test naming that builder. The FlakeMeta::max() doc now points at the test by name.

Also took your re-weighting on the forward-looking win — the body now states the OrdMap → imbl::HashMap unblock takes is_asserted/record from O(log novelty_facts) to amortized O(1) on the per-commit path, and that it's larger than anything the tiebreak costs.

On the red test job: that was the main-side it_ledger_lifecycle failure from 43d758610, which your 6b4c211d4 has since fixed on main (I closed #1739 as superseded by it) — the fresh run against green main should clear it. And the #1718/#1719 pattern flag is fair; it's being handled as its own pass rather than smuggled in here.