Conversation
- geniml link was missing // after https: - gtars dependency snippets pointed at github.com/databio/gtars/gtars (duplicate path segment); the copy-pasteable [dependencies] blocks resolved to a nonexistent repo URL
Rename total_disk_size() -> logical_sequence_bytes() (cheap, no-I/O, sequence-content-only) and cache the value in StoreMetadata at index-write time so consumers can read a store's size from the manifest without loading the sequence index. Carries through the alias-refresh path untouched; surfaced via store_metadata() and StoreStats/stats(). Propagated to Node/Python/R bindings.
Add `FastaImportOptions::collection_alias(namespace, alias)` so a caller can
name a collection at import time, e.g. `.collection_alias("ucsc", "hg38")`.
A store keeps two independent alias indexes, and the FASTA import path only
ever populated one of them. `namespaces` harvests `ns:value` tokens out of
FASTA headers into the SEQUENCE alias index; nothing ever wrote the COLLECTION
alias index. That is defensible on its face -- a FASTA says what each sequence
is called but never what the assembly is called; the string `hg38` appears
nowhere in the file -- but it makes a silent trap. A consumer asking for
`ucsc:hg38` searches the collection alias index, finds nothing, and (in
heliux's case) falls through returning the literal string as though it were a
digest, which fails later with a misdirecting error. Every consumer had to
remember to call `add_collection_alias` themselves after import, and forgetting
produced no warning at import time.
The option is opt-in, and deliberately so. Naming a collection is an assertion
the caller makes, not something derivable from the file. Nothing is inferred
from the filename: deriving `ucsc:GRCh38_no_alt_analysis_set` from
`GRCh38_no_alt_analysis_set.fna.gz` would resolve, and would hide the fact that
nobody ever asserted that name -- the same silent-guessing failure mode this
change removes. The pair is also kept separate from `namespaces` rather than
reusing it: fanning one alias across `["ucsc", "ensembl"]` would manufacture
`ensembl:hg38`, and a wrong alias is worse than a missing one because it
resolves.
Omitting the option preserves current behavior exactly -- no alias registered,
no TSV written, no manifest change. Every existing `FastaImportOptions::new()`
call site compiles and behaves identically; none were touched.
Details:
- Registration covers ALL paths a collection can take, not just
`finalize_collection`: the build-side `InsertMsg::Skip` arm (which never
reaches finalize) and finalize's own early return both register the alias.
Missing either would mean re-importing an already-imported FASTA silently
drops the requested name -- reintroducing the very bug being fixed.
- Collisions error instead of overwriting. Re-registering the same digest is a
no-op (idempotent re-import); a different digest errors unless `force`, since
silently remapping `ucsc:hg38` from one assembly to another is precisely the
misdirection being eliminated.
- Multi-file import with an alias is a hard error raised before any thread
spawns or any file is opened, so the store is left untouched. One alias
cannot name N collections, and finalize order across builder threads is
non-deterministic, so applying it to every collection would let an arbitrary
one win. The error message shows the per-file post-import loop instead
(results come back in input order).
- `gtars refget build` gains `--collection-alias NAMESPACE:ALIAS`.
- Persistence is inherited: the helper routes through `add_collection_alias`,
so the TSV write and `rgstore.json` manifest refresh come for free. No
on-disk format change.
Not addressed here (pre-existing, orthogonal): the CLI still exposes no
`--namespaces` flag, so sequence aliases remain unreachable from
`gtars refget build`.
Tests: 13 new store tests (including the required guard asserting an import
WITHOUT the option registers no collection alias, and coverage of the skip
paths, conflict/force semantics, persistence round-trip, and the multi-file
rejection) plus 3 CLI subprocess tests. Full gtars-refget suite passes (297).
`remove_collection(digest, remove_orphan_sequences=true)` and `remove_orphan_seq_files` both ran `md5_lookup.retain(...)` once per orphan. `md5_lookup` holds one entry per sequence in the entire store, so this was O(n_orphans x n_sequences_in_store). Observed on the production `plantref` store (15,207,554 sequences, 149 collections): removing a collection with 735,945 orphan sequences ran 78 minutes pegged at ~99% CPU with 16 GB RSS and deleted zero files before being cancelled — the `fs::remove_file` loop only runs after the retain loop finishes, so the process shows no progress at all. Larger collections (2.7M and 10.3M orphans) were projected at days to a week. Hoist the orphans into a HashSet and scan `md5_lookup` exactly once at both sites. `md5_lookup` maps md5 key -> sha512 key, so the retain predicate filters on the value, matching the previous semantics exactly. Per-orphan `sequence_store.remove` is unchanged (already O(1)). For the 735,945-orphan case this is ~1.5e7 operations instead of ~1.1e13. Also amends the `remove_orphan_seq_files` doc comment — its "O(sequences + collections)" claim was false before this change and is true now — and adds a matching complexity note to `remove_collection` so a future reader does not reintroduce a per-orphan scan. Adds two regression tests asserting `md5_lookup` is reclaimed for orphaned sequences and retained for shared/still-referenced ones. Cross-reference (NOT fixed here, fixed downstream in the caller): on a freshly reopened on-disk store, collections are stubs and orphan GC silently no-ops, because `name_lookup` is empty for unloaded collections. Callers must force-load all collections before requesting orphan removal. Whether gtars should defend against this itself is follow-up work.
A conflicting `--collection-alias` used to be rejected only AFTER `finalize_collection` had already committed the collection: inserted it into `self.collections`, written `collections/<digest>.rgsi` to disk, and installed name_lookup and sequence aliases. The `?` then propagated the error and the caller saw a failed import, but the collection was still in the store. On disk it was worse -- the top-level index (`collections.rgci`) is only written by `write_index_files()` at the very end of a successful import, so the error path left an orphaned per-collection `.rgsi` that nothing ever referenced. The comment claiming the check "aborts without claiming the import succeeded" was false. This is reachable from ordinary use: a typo'd or reused alias value with otherwise valid input. Split the check out of `register_import_collection_alias` into a new read-only `check_import_collection_alias`, and run it at the very top of `finalize_collection`, before any mutation. Registration now routes through the same helper, so there is one source of truth for the error. The alias write stays where it was, after the collection is committed, leaving the happy path, idempotent re-import, `force`, and the build-side `Skip` path unchanged. Adds two regression tests: one asserting `store.collections` does not retain the rejected collection (the existing conflict test only checked that the alias was not remapped), and a disk-backed one asserting no orphaned `collections/<digest>.rgsi` survives and the reopened store is still coherent. Both fail without the fix.
The on-disk orphan deletion loops in remove_collection and remove_orphan_seq_files attempted fs::remove_dir on the parent shard directory once per orphan file. Sequence files are sharded across ~4096 two-char prefix dirs, so removing N orphans issued N rmdir syscalls to successfully remove at most ~4096 dirs -- the rest failing with ENOTEMPTY. That roughly doubled the syscall count in a loop already bound by filesystem metadata latency (measured ~750 unlinks/sec on GPFS). Now each loop only unlinks, collecting parent dirs into a HashSet, and a single rmdir pass runs after all unlinks complete. Running the pass after (not during) the unlinks is required so dirs that only become empty later are still cleaned up. Best-effort semantics are unchanged: all errors are still ignored and non-empty dirs are left alone.
`StoreStats` is a RAM-residency gauge, but its field names read as per-run ingest counters. A production build that ingested 74 new collections reported `n_sequences_loaded: 0`, which looked like silent data loss even though the store grew 378 GB -> 430 GB on disk. `n_sequences_loaded` is structurally always 0 on a disk-backed store: importing hands the bytes to the writer pool and downgrades the record to a Stub. `n_collections_loaded` is worse — it happened to equal the newly-added count in that report, so it looks like an ingest counter while actually counting whatever is resident in RAM when stats() is called. It resets on process start and also counts collections merely touched by a read. Rename both to say what they measure: n_sequences_loaded -> n_sequences_in_memory n_collections_loaded -> n_collections_in_memory BREAKING: the old keys are removed outright, with no aliases, across the Rust API, the Python/Node/R bindings, and the `refget store stats` JSON. Callers reading them now get a KeyError/undefined instead of a silently wrong number — a silent 0 is what caused the false bug report. In JS the fields are nSequencesInMemory / nCollectionsInMemory. No genuine per-run ingest counter existed anywhere; the information was computed and discarded. Add `ImportReport`, returned by add_sequence_collections_from_fastas, carrying the per-file results plus n_sequences_written, n_sequences_deduped, and n_collections_new. `written + deduped` equals the records seen across all processed files; files short-circuited by the .rgsi pre-decode skip contribute to neither, since their FASTA is never opened. BREAKING: add_sequence_collections_from_fastas returns ImportReport instead of Vec<(SequenceCollectionMetadata, bool)>; per-file results move to `report.collections`. The singular add_sequence_collection_from_fasta is deliberately unchanged, so its ~150 call sites are untouched. Counting adds no measurable cost to the import hot path. The counters are plain locals (the inserter loop runs on the owner thread, so no atomics are needed), and classification on the disk-backed path is derived from `force` plus whether a write was dispatched — no extra hash lookup. Only in-memory stores, where a `None` return is ambiguous between a dedup hit and a fresh insert, pay for a lookup, and the short-circuit keeps it off the disk path. Measured on a 200 MB / 2000-sequence build: 1.014s vs 1.021s baseline, within run-to-run noise. `gtars refget build` now prints an "Ingested this run" summary — the line that should have answered the original question. Also fixes doc drift: the pyo3 stats docstring omitted one of the five keys entirely, and now states the always-0-on-disk invariant.
The store was a one-way street: `refget build` imports FASTA, but nothing exposed the library's export_fasta() on the command line. Add an `export` subcommand: gtars refget export -s <store> -o out.fa [-c <digest>] [--names ...] [-w N] `-c` may be omitted when the store holds exactly one collection; otherwise the available digests are listed. An output path ending in `.gz` is gzip-compressed. `--names` restricts the export to specific sequences. Also add unwrapped output. FASTA conventionally wraps sequence bodies at 80 bases, but GGCAT, SSHash and similar k-mer tooling expect one sequence per line. `write_fasta_record` used chunks(line_width), and chunks(0) panics, so a zero-width branch is required rather than merely an optimization. `-w 0` (library: `Some(0)`) now emits each sequence on a single unbroken line; `None` still defaults to 80. The handler loads only what the export needs. An earlier draft called load_all_sequences(), which pulls EVERY sequence in the store into RAM regardless of the collection requested — fatal on a large store (the vgp store holds ~384k sequences across hundreds of GB) and wasteful even when it fits. Collection metadata is loaded first, the digest is validated against it, and only then are that collection's sequence bytes loaded — narrowed further to the requested names when --names is given. Validating before loading also means a typo'd digest fails immediately instead of after a full-store load. Two tests cover the wrapping behavior: a 120-base sequence splits 80 + 40 when wrapped and stays on one line when unwrapped.
…etStore RefgetStore took no lock and rewrote collections.rgci, sequences.rgsi and rgstore.json wholesale from the state loaded when the store was opened, so the read-modify-write window was the entire lifetime of the handle. Two concurrent writers were last-writer-wins, silently: on 2026-07-23 four concurrent genome inits ran and athaliana's collection was written to disk but dropped from both indexes, its alias stopped resolving, and the nightly failed with 'Genome not found'. Neither init reported an error. Four layers: - atomic.rs: one write-temp, sync, rename, fsync-parent helper, applied to all index, manifest and alias writers. The existing .reftx path omitted the directory fsync; it now shares this helper. - lock.rs: an exclusive O_EXCL lockfile at <store>/.rgstore.lock. Not flock: on Lustre without the flock mount option, flock degrades to node-local silently, which is worse than no lock. Stale locks carry a heartbeat and are broken by steal-by-rename, never unlink-then-create. - Merge at commit instead of overwriting from the open-time snapshot. Both indexes are digest-keyed sets of content-derived rows, so the merge is a set union minus explicit tombstones. This shrinks the exclusive section from a multi-hour import to a short index rewrite, which is what makes concurrent writers possible rather than merely safe. - Orphan GC derives its live set by streaming the per-collection .rgsi files from disk and fails closed on any unreadable one. It could previously read a partial name_lookup as 'nothing references this' and delete live sequences. No on-disk format change: the merge parses the existing rgsi/rgci line forms, so existing stores and their S3 copies stay readable. Known tradeoff: the name/description columns in sequences.rgsi are not content-determined, so a digest-keyed union cannot resolve them non-arbitrarily. First committer wins and that column is advisory; the authoritative per-collection names live in each collections/<digest>.rgsi. 330 tests pass. tests/concurrent_store_writers.rs covers the three properties that matter: concurrent processes all land in the index, an abandoned lock from a dead process is broken, and a reader never observes a torn store.
A commit used to write back the store's ENTIRE in-memory state, not just what the handle had changed. Opening a store loads a stub for every collection in the index (persistence.rs load_collection_stubs_from_rgci) purely so they can be read, and the commit then unioned that whole pile onto whatever was on disk. Because a union can only add, deleting anything needed a second mechanism to say "not this row" -- that was Tombstones, added in 65a8caf. It was per-process and in-memory, so it could only suppress rows the COMMITTING handle had removed. It did nothing about a removal performed by anyone else, which meant a handle opened before someone else's deletion put that deletion straight back at commit: 1. Writer W opens; its snapshot still contains collection C. 2. Remover R removes C and its orphan sequences, commits. Disk correct. 3. W adds something unrelated and commits. Its union re-adds C from the stale snapshot; W has no tombstone for it. The result is corruption, not merely a missed prune: the index rows come back after R already unlinked the .seq files, leaving rows that point at nothing. get_substring raises, get_collection raises, and the alias TSV resolves to a collection that cannot be opened. This was never a lock race. R's commit completed before W's began; they never overlapped, and the reverse order was always fine. Serializing commits cannot fix it because nothing checked whether a snapshot had gone stale. It was reachable in production: stores/build.py opens the store and holds one handle across an ingest that runs for hours on plantref, and `build.py --sync --delete` would have pushed the corrupt state to S3. The fix is to stop writing back rows the handle never touched. Commit is now: take the lock, read the index fresh from disk, apply only this handle's own additions and removals, write, release. Nothing stale is written, so there is nothing to subtract, so tombstones are deleted outright rather than persisted. This also subsumes what the union was reaching for -- a long-lived handle must not clobber concurrent writes -- because the re-read happens at commit time. Requires distinguishing rows this handle CHANGED from rows it merely loaded to read. Those were one indistinguishable pile, which is precisely why the commit path could not tell them apart. PendingChanges now records only real mutations; the index loaders deliberately do not populate it. remove_collection takes the lock BEFORE the orphan scan and holds it through the scan, the index commit and the unlinks. Orphan-ness is only true as of a moment: computing it outside the lock lets another writer add a collection referencing a planned orphan, and the unlink would then delete live data. On plantref that scan is ~a minute rather than seconds, which is an accepted exception to short lock holds -- removals are rare and hand-run. plan_orphan_removal stays lock-free and is documented as advisory, since the authoritative scan reruns under the lock. The index is now written before the .seq unlinks. If a crash lands mid-removal, rows-gone-files-present is recoverable garbage; rows-present-files-gone is the dangling-reference corruption above. Regression test another_processs_removal_is_not_resurrected reproduces the sequence exactly -- the writer opens before the removal and asserts the victim is in its snapshot, the remover is a fully sequenced separate process, and a precondition asserts the removal landed before the writer commits. Verified to FAIL on 65a8caf ("the other process's removal was undone") and pass here. concurrent_processes_all_land_in_the_index still passes, so the lost-update case the union protected has not regressed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #271 +/- ##
==========================================
+ Coverage 82.67% 83.21% +0.54%
==========================================
Files 95 97 +2
Lines 27022 28471 +1449
==========================================
+ Hits 22340 23693 +1353
- Misses 4682 4778 +96 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several lock, orphan-cleanup, and publication races can still permit concurrent corruption or dangling store references.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Release 0.10.0 strengthens refget persistence, adds collection-aware FASTA export, and updates bindings and package versions.
Changes:
- Adds lock-based delta commits and atomic file publication.
- Adds collection aliases, FASTA export, and import statistics.
- Updates APIs, bindings, tests, documentation, and release versions.
File summaries
| File | Description |
|---|---|
README.md |
Fixes repository links. |
gtars/Cargo.toml |
Bumps package and dependencies. |
gtars-wasm/js/remote-refget-store.js |
Corrects WASM return documentation. |
gtars-wasm/js/remote-refget-store.d.ts |
Corrects return type. |
gtars-wasm/Cargo.toml |
Bumps WASM version. |
gtars-vrs/Cargo.toml |
Bumps package and refget dependency. |
gtars-refget/tests/concurrent_store_writers.rs |
Adds multi-process safety tests. |
gtars-refget/src/transcripts/builder.rs |
Uses shared atomic writes. |
gtars-refget/src/store/readonly.rs |
Adds pending deltas, locking, GC, and stats changes. |
gtars-refget/src/store/persistence.rs |
Implements delta commits and atomic indexes. |
gtars-refget/src/store/mod.rs |
Exposes new locking and reporting APIs. |
gtars-refget/src/store/lock.rs |
Implements writer lockfiles and heartbeats. |
gtars-refget/src/store/import.rs |
Adds import reports and collection aliases. |
gtars-refget/src/store/fhr_metadata.rs |
Atomically writes FHR sidecars. |
gtars-refget/src/store/export.rs |
Adds unwrapped, collection-aware export. |
gtars-refget/src/store/core.rs |
Exposes new store operations. |
gtars-refget/src/store/atomic.rs |
Adds atomic publication utilities. |
gtars-refget/src/store/alias.rs |
Adds delta-based alias persistence. |
gtars-refget/src/lib.rs |
Re-exports report and stats types. |
gtars-refget/src/collection.rs |
Atomically publishes collection indexes. |
gtars-refget/Cargo.toml |
Bumps version and adds dependencies/test target. |
gtars-r/src/rust/src/refget.rs |
Updates R binding APIs and stats. |
gtars-r/src/rust/Cargo.toml |
Bumps R crate version. |
gtars-r/DESCRIPTION |
Bumps R package version. |
gtars-python/tests/test_refget.py |
Tests import reports. |
gtars-python/src/refget/mod.rs |
Adds Python report, locking, and stats APIs. |
gtars-python/py_src/gtars/refget/__init__.pyi |
Updates Python type declarations. |
gtars-python/Cargo.toml |
Bumps Python package version. |
gtars-overlaprs/src/multi_chrom_overlapper.rs |
Removes unused error type. |
gtars-overlaprs/Cargo.toml |
Bumps version and removes dependency. |
gtars-node/src/lib.rs |
Updates Node statistics. |
gtars-node/package.json |
Bumps Node package versions. |
gtars-node/npm/linux-x64-gnu/package.json |
Bumps Linux artifact version. |
gtars-node/npm/darwin-arm64/package.json |
Bumps macOS artifact version. |
gtars-node/Cargo.toml |
Bumps Node crate version. |
gtars-cli/tests/refget_export_collection_alias.rs |
Tests alias-based export. |
gtars-cli/tests/refget_build_collection_alias.rs |
Tests import-time aliases. |
gtars-cli/src/refget/handlers.rs |
Implements export and lock commands. |
gtars-cli/src/refget/cli.rs |
Defines new CLI options and commands. |
gtars-cli/Cargo.toml |
Bumps CLI and refget versions. |
Changes.md |
Documents release 0.10.0. |
Cargo.toml |
Reduces development debug information. |
Review details
- Files reviewed: 42/43 changed files
- Comments generated: 14
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release 0.10.0. This is mostly a
gtars-refgetstore-safety release: concurrent writers now take a lock and commit only their own changes, so two processes writing one store can no longer lose or resurrect collections. It also adds thegtars refget exportcommand, collection aliases at import and export time, and fixes the quadratic orphan cleanup.See the 0.10.0 section of
Changes.mdfor the full list.Component versions in this release:
StoreStatsfield renames,ImportReportreturn type)Verified locally: Rust workspace 1024 tests pass, Python bindings 235 tests pass.
After merging, cut releases in this order so crates.io dependencies resolve:
gtars-overlaprs-v0.6.1andgtars-refget-v0.10.0, thengtars-vrs-v0.7.1, thengtars-v0.10.0, thengtars-cli-v0.10.0. Binding releases (gtars-python-v0.10.0,gtars-r-v0.10.0,gtars-node-v0.8.0,gtars-wasm-v0.9.2) are independent.