fix(indexer): merge novelty into emptied-by-retract leaflets without failing

[bplatz]

Problem

On churn-heavy workloads (identity purge/recluster cycles, materialize delete-then-rewrite), roughly every third incremental index build fails with:

incremental indexing failed, falling back to full rebuild
error: Storage write error: missing column block for SId

Each fallback replays the entire commit history (~130–150s on a ledger with ~1.2M asserts and growing), instead of the ~5–8s the incremental would have cost. Observed live on testoco-5: 6 forced full rebuilds of _identity in one day, all with this error.

Root cause

When an incremental build retracts every row in a leaflet, update_leaf deliberately keeps the leaflet — preserving its key range for branch routing and its history sidecar for time-travel — but writes it via empty_encoded_leaflet_with_keys with row_count: 0 and column_refs: Vec::new() (no column blocks at all). That is valid V3.

A later incremental that routes novelty back into that leaflet's key range calls merge_and_encode_leaflet, which loaded the existing columns with ColumnProjection::all() and no zero-row guard. find_ref(ColumnId::SId) finds nothing → missing column block for SId → the whole incremental aborts and the indexer falls back to a full rebuild.

The full rebuild only writes live rows, so the empty leaflets vanish — which is why the next couple of incrementals succeed before the cycle repeats. Existing tests covered writing the emptied leaflet but never fed it back through a second update_leaf.

Fix

Guard the zero-row case in merge_and_encode_leaflet: an emptied leaflet contributes ColumnBatch::empty() instead of demanding column blocks that were never written. Merge then proceeds normally (novelty vs. zero base rows), and the leaflet is re-encoded with the new rows.

Read-path-only change — no index format bump; existing indexes containing empty leaflets become mergeable as-is.

Testing

• New two-generation regression test test_update_after_retract_all_reinserts_into_empty_leaflet (build → retract-all → re-insert into the emptied leaflet). Reproduces the exact production error without the fix; passes with it.
• Full fluree-db-indexer suite green (335 lib + integration tests), clippy and fmt clean.

[zonotope]

lgtm except for one test improvement, and as long as that improvement doesn't reveal anything.

[zonotope]

This test doesn't verify that gen1's history doesn't survive into gen2's sidecar. It also doesn't verify that the merged row's content round-trips. There should at least be a time travel read at t == 1 at the end.

[bplatz]

Addressed in d68b7f9

[aaj3f]

This is a nice fix, @bplatz

The two things the test doesn't assert are the two I'd have worried about — whether the generation-1 retraction history survives the generation-2 merge, and whether ordering holds when the emptied leaflet is in the middle of a leaf rather than alone — so I wrote both and mutation-checked them; both pass at HEAD and both go red on the reverted guard. Ordering is fine for a structural reason worth recording: novelty is sliced by the next sibling's first_key at both the leaflet (incremental_leaf.rs:228-286) and leaf (incremental_branch.rs:651-678) levels, so when a repopulated leaflet's key range collapses from the preserved span to the new rows' span, the preceding sibling's half-open interval just widens and nothing is orphaned.

Only note I'd genuinely like your read on is the optional one about whether that guard eventually belongs inside load_leaflet_columns — six copies of it now, and this PR exists because one was missing — but I can argue the call-site convention just as well, and it's not a reason to hold this.

Adherence to repo commitments:

• Patterns/abstractions: ✔ Reuses the existing ColumnBatch::empty() mechanism and matches the read path's guard verbatim; invents nothing orthogonal.
• Performance (speed first, memory second): ✔ Hot path touched, but the common case gains one predictable u32 compare ahead of a zstd decode, and ColumnBatch::empty() allocates nothing. Strongly net-positive: removes a ~130–150s full rebuild on ~1-in-3 incrementals and reduces garbage, since a rebuild retires the whole prior index tree while an incremental retires only touched leaves. No performance-degradation risk.
• Testing: ✔ Real two-generation regression test that goes red on revert with the exact production error; full indexer suite green. ⚠️ only in that its assertions stop at row counts — see the optional note.

Approving so you can merge whenever you're ready — none of the notes above are blocking, though the test-strengthening one is cheap enough that it might be worth folding in while you're here.

[aaj3f]

Optional, more of a question than a suggestion. With this change the row_count == 0 → ColumnBatch::empty() guard now lives at six call sites — fluree-db-query/src/join.rs:1831, :2723, :3313, :3601, fluree-db-binary-index/src/read/binary_cursor.rs:382, and now here — plus a continue-shaped cousin at fluree-db-indexer/src/build/incremental.rs:4042. This PR exists precisely because one caller forgot it, so I wonder if the guard wants to move into load_leaflet_columns as an early if entry.row_count == 0 { return Ok(ColumnBatch::empty()); }, which would make the whole class of bug unrepeatable for the next caller. I can argue the other side too: the call-site guard also skips the projection setup and, in the cached variants, a cache probe, and call-site guarding is clearly the established convention rather than an accident — so matching it is defensible and arguably the right call for a fix that wants to be surgical. Definitely not something to hold this PR for; happy to punt it to a follow-up issue if you think the shared-reader version is worth doing at all.

[aaj3f]

Optional. The test's teeth are real (I reverted the guard and it goes red with the exact missing column block for SId production error, and it's the only failure in the suite), but its assertions stop at leaves.len() and total_rows, which leaves the two consequences I'd actually worry about unpinned: that generation-1's retraction history survives into generation 2, and that ordering holds when the emptied leaflet is a middle leaflet rather than the only one. I wrote both and they pass — so this is a "lock in what already works" suggestion, not a bug report. Sketching them in case they're useful:

// (a) history survival — every gen-1 sidecar entry must still be findable after gen 2
let h1 = all_history(&g1.leaf_bytes, g1.sidecar_bytes.as_deref());
assert!(!h1.is_empty(), "gen1 must record retraction history");
// ... run gen 2 ...
let h2 = all_history(&g2.leaf_bytes, g2.sidecar_bytes.as_deref());
for e in &h1 {
    assert!(h2.iter().any(|x| x.s_id == e.s_id && x.t == e.t && x.op == e.op));
}

// (b) ordering across the boundary — 6 rows at leaflet_target_rows: 2,
//     empty the MIDDLE leaflet (s=30,40), then re-insert s=35
assert_eq!(all_s_ids(&g2.leaf_bytes), vec![10, 20, 35, 50, 60]);

Both go red on the reverted guard with the same error, so they'd be regression-proofing rather than duplicate coverage. Entirely reasonable to skip (b) if the single-leaflet case feels like enough.

[bplatz]

Addressed in d68b7f9

[aaj3f]

Nit on the PR body's wording, purely for the record and not worth an edit. "The full rebuild only writes live rows, so the empty leaflets vanish" reads, on a first pass, like the fallback might have been costing us time-travel fidelity as well as time. It isn't — I checked, because if it were, this would be a much more urgent merge. index_build.rs:236-262 resolves each identity's whole event log and files the non-final transitions into the sidecar, and build_skips_never_asserted_retract_lifecycles (index_build.rs:1085) pins it with the fixture comment s=10: asserted@1, retracted@2 -> no row; history keeps both events. So the emptied leaflet structures vanish on rebuild but the history does not, and the pre-PR bug was cost-only. Might be worth a clause in the description so nobody later reads a data-loss story into it.

Commenting here because a PR-description note has no diff line of its own, and this is the guard it's about.