refactor: migrate index metadata to on-disk JSON with fallible access
Migrate index state tracking from filesystem sentinel files to an on-disk JSON schema within `index.meta`. The `IndexMeta` struct is now wrapped in an `Arc` with internal locking, exposing only fallible methods for genome and state access. In-memory mutation capabilities have been removed, requiring callers to handle I/O errors explicitly and pass immutable references to downstream components like `PartitionRouter`. Public sentinel constants have been removed from exports.
This commit is contained in:
@@ -1224,6 +1224,127 @@ correctly inherent, not construction-only by the semantic criterion) —
|
||||
`create`/`open`/`exists` (identity, can't be trait methods needing `Self`
|
||||
before one exists) round that out.
|
||||
|
||||
## (11) done (2026-08-21): `KmerIndex`/`IndexMeta` made fully stateless, `IndexState` moved off sentinel files
|
||||
|
||||
Triggered mid-discussion of `obikalgorithm::Algorithm` (still not started —
|
||||
see "Still not done" below): user asked why `PartitionRouter::new` still
|
||||
took `&mut KmerIndex` at all, and questioned whether `mark_scattered`
|
||||
belonged in the algorithm or in `cmd/index`. Investigation found `&mut`
|
||||
had become *newly* necessary since (9) — `mark_scattered` was mutating
|
||||
`self.meta.genomes` in memory so `Counter`'s later `write_spectrum` call
|
||||
(same `idx` instance) would see the derived label. User's resolution: the
|
||||
"disk is truth, stateless" principle already agreed for `KmerPartition`/
|
||||
`Layer` in (5) (still unimplemented for those two) should extend to
|
||||
`KmerIndex` itself — move `IndexState` (`Empty`/`Scattered`/`Counted`/
|
||||
`Indexed`) off the three sentinel files (`scatter.done`/`count.done`/
|
||||
`index.done`, detected by existence) and into a field of `index.meta`'s
|
||||
own JSON, so the `mark_*` calls become plain disk writes an algorithm can
|
||||
legitimately make on `&self` — no in-memory mutation left to protect.
|
||||
|
||||
**Shape of `IndexMeta`, per the user's explicit spec**: one JSON file per
|
||||
index (`index.meta`), one `IndexMeta` instance per index, held and
|
||||
returned as `Arc<IndexMeta>` (not `&IndexMeta`) by `KmerIndex::meta()`.
|
||||
`config` (`kmer_size`/`minimizer_size`/`n_bits`/`with_counts`/`evidence`/
|
||||
`block_bits`) is fixed at construction, cached as a `pub` field (getter
|
||||
kept alongside, for symmetry) — "les champs constants restent des champs
|
||||
de la structure", read once, never re-read from disk. `genomes` and
|
||||
`state` are the opposite: no in-memory cache at all, every accessor
|
||||
(`genomes()`, `state()`) re-reads `index.meta` from disk, every mutator
|
||||
(`push_genome`/`rename_genome`/`set_genomes`/`set_state`/`mark_scattered`/
|
||||
`mark_counted`/`mark_indexed`) does a full read-modify-write of the same
|
||||
file. An internal `std::sync::RwLock<()>` is held across each
|
||||
read-modify-write sequence (not just the write) so two callers sharing
|
||||
the same `Arc<IndexMeta>` can't lose an update to each other — this is
|
||||
*not* a cross-process lock (that's `obisys::DirLock`, already held by
|
||||
`cmd/index` for the whole build); it only serialises access through one
|
||||
shared in-process instance.
|
||||
|
||||
**Construction, chain-of-responsibility style, matching (5)'s pattern**:
|
||||
`IndexMeta::create(&KmerIndex, config, genomes)` / `IndexMeta::open(&KmerIndex)`
|
||||
ask the index for its own root path rather than taking one directly. Since
|
||||
`KmerIndex::create` doesn't have a complete `KmerIndex` yet to hand in
|
||||
(it's what's being built), added lower-level `pub(crate)` path-based
|
||||
primitives `create_at(&Path, ...)` / `open_at(&Path)` that `KmerIndex::create`/
|
||||
`open` and `builder.rs`'s `create_skeleton` call directly, bypassing the
|
||||
convenience wrappers for that one bootstrap case.
|
||||
|
||||
**The one deliberate exception**: `select_in_place` and `reindex`
|
||||
genuinely rewrite `config` after an index already exists (output
|
||||
type/evidence mode changes in place) — contradicting "config never
|
||||
changes" for the general case. Resolved with a separate, explicitly
|
||||
rare-labelled `IndexMeta::rewrite_config(config, genomes)` (preserves
|
||||
`state`, overwrites everything else); callers refresh their own cached
|
||||
`Arc<IndexMeta>` afterward (`self.meta = Arc::new(IndexMeta::open(self)?)`)
|
||||
since `IndexMeta` has no way to reach back into whichever `KmerIndex`
|
||||
holds it.
|
||||
|
||||
**Consequence confirmed, not just hoped for**: with `mark_scattered` no
|
||||
longer touching anything in memory, `PartitionRouter` genuinely never
|
||||
needs `&mut KmerIndex` — `PartitionRouter<'a> { index: &'a KmerIndex }`,
|
||||
`new(&'a KmerIndex)`. This is effectively the `PartitionRouter` half of
|
||||
(5)'s "order of remaining work" item done as a side effect; `KmerPartition`/
|
||||
`Layer` themselves are still unimplemented for (5).
|
||||
|
||||
**Blast radius — much larger than (9)/(10), touched nearly every crate**:
|
||||
every `.meta().genomes`/`.meta.genomes` field access became a fallible
|
||||
`.genomes()?` method call (`genomes` reads `io::Result<Vec<GenomeInfo>>`
|
||||
now, not a field), and `.meta_mut()` was removed outright (no more direct
|
||||
field mutation from outside `IndexMeta`). Fixed across:
|
||||
- `obikindex` internals: `meta.rs`/`state.rs`/`kmer_index.rs`/`builder.rs`
|
||||
(full rewrites), `reindex.rs`/`select.rs` (switched to `rewrite_config`),
|
||||
`merge.rs` (heaviest single file — genome counts precomputed once per
|
||||
source into a `Vec<Vec<GenomeInfo>>` up front rather than re-reading
|
||||
`index.meta` from disk repeatedly through the function, sentinel write
|
||||
replaced with `dst2.meta.mark_indexed()`), `stats.rs`, `distance.rs`,
|
||||
`dump.rs`, `predicate.rs` (its `IndexMeta`-inherent `matching_genome_indices`/
|
||||
`build_group_filter` now read genomes fresh internally), `mod.rs`/`lib.rs`
|
||||
(sentinel constant re-exports removed — `IndexState` no longer has
|
||||
`SENTINEL_*`/`detect()` at all).
|
||||
- `obikindexer::extensions::PrivateBuilder`: `mark_scattered` signature
|
||||
dropped `&mut self` → `&self`; the four `mark_*`/`write_spectrum` bodies
|
||||
became one-line delegations to `self.meta().mark_*()`.
|
||||
- `obikphylo::siblings`: `alignment.rs`/`cardinality.rs`/`distance.rs`/
|
||||
`entropy.rs`/`sankoff_bundle.rs`/`stats.rs`/`tests.rs` — all had
|
||||
`self.meta().genomes.len()`-shaped reads, mechanically fixed to
|
||||
`.genomes().map_err(OKIError::Io)?.len()` (tests: `.unwrap()`).
|
||||
- `obikmer::cmd::*`: `annotate` (rewrote its rename path to load genomes
|
||||
once, mutate the in-memory `Vec`, then `idx.meta().set_genomes(...)`
|
||||
instead of `meta_mut()`), `filter`/`pack`/`dump`/`unitig`/`merge`/`select`/
|
||||
`phylo` (fetch-once-then-use pattern for genome counts/labels),
|
||||
`utils/maintenance.rs` (`run_rename` now calls the pre-existing
|
||||
`IndexMeta::rename_genome`, dropping its own hand-rolled field mutation
|
||||
entirely), `index/mod.rs` (three `idx.state() < IndexState::X`
|
||||
resumability checks needed a fallible read — factored into a small
|
||||
`current_state(&KmerIndex) -> IndexState` helper rather than repeating
|
||||
the same `unwrap_or_else` three times), `query/*` (`emit_batch`'s
|
||||
signature changed from `&IndexMeta` to `&[GenomeInfo]`, and `genomes` is
|
||||
now fetched once in `run()` and threaded down through `process_chunk`
|
||||
as `Arc<Vec<GenomeInfo>>` rather than re-reading `index.meta` from disk
|
||||
on every chunk — a deliberate deviation from the "always re-read"
|
||||
default, justified because this is a genuine per-chunk hot path, unlike
|
||||
every other call site touched in this pass).
|
||||
- One `&IndexMeta`-vs-`Arc<IndexMeta>` argument-type mismatch pattern
|
||||
recurred at several CLI call sites (`build_filters`/`build_specs`/
|
||||
`emit_batch`'s original signature) — resolved via `Arc`'s deref
|
||||
coercion (`&idx.meta()` coerces to `&IndexMeta`) rather than changing
|
||||
every downstream signature to accept `Arc<IndexMeta>`.
|
||||
|
||||
**Verification**: `cargo check --workspace --all-targets` and
|
||||
`cargo test --workspace` both green (0 failures) after the full
|
||||
propagation, `scripts/smoke_test_index.sh` green (870 kmers, same as every
|
||||
prior round), plus a manual CLI run of `index` (×2) → `merge` → `select`
|
||||
→ `reindex` → `utils --new-label` (rename) → `utils --stats`, all exit 0,
|
||||
confirming the four most-affected commands (the ones (10)'s verification
|
||||
already flagged as under-covered by the automated test suite) still work
|
||||
end to end against the new `Arc<IndexMeta>`/on-disk-`IndexState` shape.
|
||||
|
||||
Still not done: (5)'s `KmerPartition`/`Layer` self-naming redesign itself
|
||||
(only the `PartitionRouter`-`&mut`-removal piece landed, as a side
|
||||
effect); (8)'s `obikalgorithm::Algorithm` trait (paused, not abandoned,
|
||||
for this detour — points 2–4 from that discussion are still open); the
|
||||
`distance.rs` → `obikphylo` relocation ((9), explicitly deferred); the
|
||||
future cache-manager crate.
|
||||
|
||||
## The problem
|
||||
|
||||
Reading a layer's data (MPHF + matrix) is not free: `MphfLayer::open` mmaps
|
||||
|
||||
Reference in New Issue
Block a user