add_index: definition committed before reindex — a crash mid-backfill yields silently incomplete index lookups

[fabracht]

Surfaced (confirmed real, and confirmed PRE-EXISTING) while quorum-reviewing PR #116 (#112). Not introduced by that PR — main had the same ordering.

The gap

Database::add_index (crates/mqdb-agent/src/database/schema_ops.rs) persists+commits the index definition first, then backfills existing rows' idx/{entity}/{field}/… entries in a separate per-1000-row loop:

let merged = manager.merged_definition(&entity, fields);
manager.persist_index(&mut batch, &merged)?;
batch.commit()?;              // <- definition committed here
manager.add_index(merged);
// ... reindex loop backfills entries in separate committed batches ...

If the process crashes (or errors) after the definition commit but before the reindex loop completes, on restart load_indexes advertises the field as indexed while pre-existing rows have no entries for it. A subsequent index-backed lookup_by_field / lookup_by_range then returns silently incomplete results (a wrong-answer class bug, worse than an error). This exists even for a first single-field index.

Confirmed by a counter-test: persisting a merged [email,username] definition with pre-existing rows that only ever received email entries, then reloading and doing lookup_by_field(username=…) returns EMPTY.

Proposed fix

Defer the disk persist+commit of the definition until AFTER the reindex loop completes. The in-memory add_index(merged) must still precede the loop (the reindex reads the live field set via get_indexed_fields to know what to backfill). Then a crash mid-reindex leaves the field un-advertised on restart → correct full-scan fallback (and orphaned entries that a re-declare cleans) instead of silent-incomplete results.

Note this trades the current failure mode (def present, entries missing → wrong answers) for a strictly safer one (def absent, entries partial → correct but slow). Verify it does not reintroduce the commit-before-mutate inconsistency that PR #116's persist-before-mutate order fixed (the final def-commit failing would leave in-memory advertising the field with entries fully present, so lookups stay correct; restart just loses the advertisement).

Related

• Migration cleanup: pre-add_index replaces instead of merging, de-registering an entity's earlier indexed fields #112 databases can also carry orphaned entries for de-registered fields; if such a field's value was updated while de-registered, a stale entry can produce a false positive. Fully cleaning these needs an index-rebuild operation (drop all entries for the entity + reindex), which does not exist yet. Consider adding rebuild index/reindex as part of this work.

Acceptance

• A crash simulated between definition-commit and reindex completion leaves the field unadvertised (full-scan fallback), not silently-incomplete index results.
• Existing add_index merge/commit-consistency tests still pass.
• Optional: a reindex/rebuild-index operation that drops and rebuilds an entity's index entries (also fixes the migration stale-entry case).