Size the spill buffers from the machine, and count records honestly

CLAUDE.md

@@ -100,4 +100,4 @@ Shared crates (`du-domain`, `du-atproto`, `du-bio`) live in the sibling repo `..
 
 ### Useful Environment Variables
 
-`NAVIGATOR_ANALYSIS_THREADS`, `NAVIGATOR_BGZF_THREADS`, `NAVIGATOR_IO_SYNC_MB` (how much a multi-GB writer may leave dirty in the page cache; `0` disables the pacing), `NAVIGATOR_Y_TREE_PROVIDER` (`decodingus`/`ftdna`), `NAVIGATOR_TREE_TTL_DAYS`, `NAVIGATOR_REFGENOME_DIR`, `NAVIGATOR_TREE_DIR`, `NAVIGATOR_ANCESTRY_PANEL` / `NAVIGATOR_ANCESTRY_PCA`, `DECODINGUS_APPVIEW_URL`.
+`NAVIGATOR_ANALYSIS_THREADS`, `NAVIGATOR_BGZF_THREADS`, `NAVIGATOR_IO_SYNC_MB` (how much a multi-GB writer may leave dirty in the page cache; `0` disables the pacing), `NAVIGATOR_SORT_MB` / `NAVIGATOR_REVERT_SORT_MB` (spill-buffer size in MB; both are sized from installed RAM when unset), `NAVIGATOR_Y_TREE_PROVIDER` (`decodingus`/`ftdna`), `NAVIGATOR_TREE_TTL_DAYS`, `NAVIGATOR_REFGENOME_DIR`, `NAVIGATOR_TREE_DIR`, `NAVIGATOR_ANCESTRY_PANEL` / `NAVIGATOR_ANCESTRY_PCA`, `DECODINGUS_APPVIEW_URL`.

crates/navigator-analysis/src/postprocess/sort.rs

@@ -1,8 +1,13 @@
 //! Coordinate-sort a BAM, on disk.
 //!
 //! Same shape as [`crate::revert::collate`] and for the same reason: a WGS alignment does not fit
-//! in memory, so this fills a fixed budget, sorts it, spills a run, and k-way merges the runs at
-//! the end. Peak memory is the budget plus one buffered block per run, independent of input size.
+//! in memory, so this fills a budget, sorts it, spills a run, and k-way merges the runs at the end.
+//! Peak memory is the budget plus one buffered block per run, independent of input size.
+//!
+//! The budget comes from the machine ([`navigator_resource::spill_budget`]), not from a constant.
+//! It was 512 MB for everyone, which on a 30x WGS spilled **688 runs** that the merge then opened
+//! at once — bounded memory by design, and a great deal of fan-in to buy on a machine with 128 GB
+//! sitting idle.
 //!
 //! ## Runs are ordinary BAM files
 //!
@@ -35,9 +40,6 @@ use crate::error::AnalysisError;
 /// How often the record loop asks whether it has been cancelled — same cadence as the walkers.
 const CANCEL_CHECK_INTERVAL: u64 = 4096;
 
-/// Default in-memory budget before a run is spilled.
-const DEFAULT_SORT_BUFFER_MB: usize = 512;
-
 /// Tuning for [`sort_alignment`].
 #[derive(Debug, Clone)]
 pub struct SortParams {
@@ -46,14 +48,12 @@ pub struct SortParams {
 }
 
 impl Default for SortParams {
+    /// Sized from the machine, not from a constant — see [`navigator_resource::spill_budget`], which
+    /// also documents `NAVIGATOR_SORT_MB`. The constant this replaced was 512 MB, which spilled 688
+    /// runs on a 30x WGS regardless of whether the machine had 8 GB or 128 GB to work with.
     fn default() -> Self {
-        let mb = std::env::var("NAVIGATOR_SORT_MB")
-            .ok()
-            .and_then(|s| s.parse::<usize>().ok())
-            .unwrap_or(DEFAULT_SORT_BUFFER_MB)
-            .max(1);
         Self {
-            buffer_bytes: mb * 1024 * 1024,
+            buffer_bytes: navigator_resource::spill_budget("NAVIGATOR_SORT_MB") as usize,
         }
     }
 }
@@ -322,17 +322,38 @@ fn sort_key(record: &RecordBuf) -> (u32, u64) {
     }
 }
 
-/// Rough heap footprint, for the memory budget. The variable-length parts dominate; the constant
-/// covers the record's fixed fields and allocator overhead closely enough to size a buffer by.
-fn heap_bytes(record: &RecordBuf) -> usize {
+/// What one record costs the buffer.
+///
+/// This used to be the variable-length parts plus a flat 256, which read as a reasonable stand-in
+/// for "fixed fields and allocator overhead" and was not one. It left out the tag dictionary
+/// entirely, and a mapped record carries a dozen tags — `NM`, `MD`, `AS`, `ms`, `nn`, `tp`, `cm`,
+/// `s1`, `s2`, `de`, `rl` from minimap2 alone — each an entry in a `Vec<(Tag, Value)>`. The buffer
+/// therefore held well over its stated budget, which mattered little against a constant picked with
+/// an unwritten margin and matters a great deal now that the budget is a fraction of the machine
+/// (see [`navigator_resource::spill_budget`]).
+///
+/// Still an estimate: it does not chase a `Value`'s own heap (a string tag's bytes) or a `Vec`'s
+/// spare capacity. It is close enough to size a buffer by, and it no longer omits a whole field.
+pub(super) fn heap_bytes(record: &RecordBuf) -> usize {
     use noodles::sam::alignment::record::Cigar as _;
-    record.name().map(|n| n.len()).unwrap_or(0)
+    // The record itself sits inline in the buffer's `Vec`, so its size is part of what a record
+    // costs — not something to approximate around.
+    std::mem::size_of::<RecordBuf>()
+        + record.name().map(|n| n.len()).unwrap_or(0)
         + record.sequence().len()
         + record.quality_scores().len()
         + record.cigar().len() * 4
-        + 256
+        + record.data().len() * TAG_ENTRY_BYTES
+        // Name, sequence, qualities, CIGAR, tags: five vectors, five allocations.
+        + 5 * navigator_resource::ALLOCATION_OVERHEAD
 }
 
+/// Bytes one tag occupies in a record's `Vec<(Tag, Value)>`.
+///
+/// Pinned by `a_tag_entry_is_not_larger_than_the_estimate_assumes`, so a noodles upgrade that grows
+/// `Value` fails a test here rather than quietly halving the buffer's honesty.
+pub(super) const TAG_ENTRY_BYTES: usize = 48;
+
 /// Stamp `@HD SO:coordinate` on the header.
 ///
 /// Not cosmetic: an index is only valid for a coordinate-sorted file, and readers decide whether

crates/navigator-analysis/src/postprocess/tests.rs

@@ -116,6 +116,51 @@ fn unsorted_fixture(dir: &Path) -> (PathBuf, sam::Header, usize) {
     (input, hdr, total)
 }
 
+// ---- the buffer estimate ---------------------------------------------------
+
+/// The buffer is now sized as a fraction of the machine rather than a hand-picked constant, so what
+/// a record is charged against it has to be roughly true. It was not: the tag dictionary was free,
+/// and a mapped record carries a dozen tags.
+#[test]
+fn the_buffer_estimate_counts_the_tag_dictionary() {
+    use noodles::sam::alignment::record::data::field::Tag;
+    use noodles::sam::alignment::record_buf::data::field::Value;
+
+    let bare = record("r0", Some(0), 1);
+    let mut tagged = bare.clone();
+    for (tag, value) in [
+        (Tag::ALIGNMENT_HIT_COUNT, Value::from(1i32)),
+        (Tag::MISMATCHED_POSITIONS, Value::from("10")),
+        (Tag::ALIGNMENT_SCORE, Value::from(60i32)),
+    ] {
+        tagged.data_mut().insert(tag, value);
+    }
+
+    let charged = heap_bytes(&tagged) - heap_bytes(&bare);
+    assert_eq!(charged, 3 * TAG_ENTRY_BYTES, "three tags should cost three entries");
+}
+
+/// A noodles upgrade that grows `Value` should fail here, rather than quietly making every buffer
+/// hold more than its budget says.
+#[test]
+fn a_tag_entry_is_not_larger_than_the_estimate_assumes() {
+    use noodles::sam::alignment::record::data::field::Tag;
+    use noodles::sam::alignment::record_buf::data::field::Value;
+
+    assert!(
+        std::mem::size_of::<(Tag, Value)>() <= TAG_ENTRY_BYTES,
+        "a tag entry is {} bytes, which the estimate does not cover",
+        std::mem::size_of::<(Tag, Value)>()
+    );
+}
+
+/// The record's own size is part of what it costs — it lives inline in the buffer's `Vec`.
+#[test]
+fn the_buffer_estimate_covers_the_record_itself() {
+    let empty = RecordBuf::default();
+    assert!(heap_bytes(&empty) >= std::mem::size_of::<RecordBuf>());
+}
+
 // ---- the properties that matter -------------------------------------------
 
 /// Coordinate order, with unplaced reads at the end where SAM puts them.

crates/navigator-analysis/src/revert/collate.rs

@@ -8,7 +8,8 @@
 //!
 //! The property that matters is that peak memory is the budget plus one buffered block per run,
 //! *independent of input size*. That is what lets the same code path revert a 5 GB exome and a
-//! 200 GB WGS on the same laptop, and it is why this is disk-backed rather than clever.
+//! 200 GB WGS on the same laptop, and it is why this is disk-backed rather than clever. The budget
+//! itself is sized from the machine — see [`navigator_resource::spill_budget`].
 //!
 //! Runs use a plain length-prefixed binary encoding rather than a serialization framework: the
 //! format is written and read in this one file, it is a hot path measured in billions of records,
@@ -25,7 +26,8 @@ use super::transform::{Mate, RevertedRead};
 use crate::error::AnalysisError;
 
 /// Buffer size for run spill/read-back. Large enough that the merge's per-run reads stay
-/// sequential, small enough that a few dozen concurrent runs don't add up to real memory.
+/// sequential, small enough that the runs a WGS produces don't add up to real memory when the merge
+/// holds all of them open at once.
 const RUN_IO_BUFFER: usize = 256 * 1024;
 
 /// Accumulates reverted reads, spilling sorted runs to scratch when the budget is reached.

crates/navigator-analysis/src/revert/mod.rs

@@ -56,11 +56,6 @@ pub use transform::{Mate, RevertedRead};
 /// reasoning as the other walkers (see [`crate::cancel`]).
 const CANCEL_CHECK_INTERVAL: u64 = 4096;
 
-/// Default memory budget for the sort buffer before a run is spilled to scratch. 256 MiB holds
-/// roughly a million 150 bp reads with names, which keeps the run count (and so the merge's open
-/// file handles) low for a WGS input without being greedy on an 8 GB machine.
-const DEFAULT_SORT_BUFFER_MB: usize = 256;
-
 /// What to do with a **primary** record whose CIGAR contains a hard clip.
 ///
 /// Hard clipping means the aligner discarded sequence from the record, so the read cannot be fully
@@ -90,14 +85,14 @@ pub struct RevertParams {
 }
 
 impl Default for RevertParams {
+    /// The collator is sized from the machine by the same rule as the coordinate sort — see
+    /// [`navigator_resource::spill_budget`], which also documents `NAVIGATOR_REVERT_SORT_MB`. The
+    /// constant this replaced was 256 MB, described as keeping the run count low for a WGS; at
+    /// ~340 bytes a reverted read that is a run every million reads, so a 30x WGS spilled several
+    /// hundred of them and the merge opened every one.
     fn default() -> Self {
-        let mb = std::env::var("NAVIGATOR_REVERT_SORT_MB")
-            .ok()
-            .and_then(|s| s.parse::<usize>().ok())
-            .unwrap_or(DEFAULT_SORT_BUFFER_MB)
-            .max(1);
         Self {
-            sort_buffer_bytes: mb * 1024 * 1024,
+            sort_buffer_bytes: navigator_resource::spill_budget("NAVIGATOR_REVERT_SORT_MB") as usize,
             hard_clipped: HardClipPolicy::default(),
             prefer_original_qualities: true,
         }

crates/navigator-analysis/src/revert/transform.rs

@@ -64,7 +64,15 @@ impl RevertedRead {
     /// Rough heap footprint, for the sort's memory budget. The two `Vec`s dominate; the constant
     /// covers the struct itself and allocator overhead closely enough to size a buffer by.
     pub fn heap_bytes(&self) -> usize {
-        self.name.len() + self.sequence.len() + self.qualities.len() + 64
+        // The read sits inline in the collator's `Vec`, so its own size counts; the three vectors
+        // each cost an allocation on top of their contents. The flat 64 this replaced was smaller
+        // than the struct alone, which was harmless against a fixed 256 MB budget and is not
+        // against a budget sized from the machine (see `navigator_resource::spill_budget`).
+        std::mem::size_of::<Self>()
+            + self.name.len()
+            + self.sequence.len()
+            + self.qualities.len()
+            + 3 * navigator_resource::ALLOCATION_OVERHEAD
     }
 }
 

crates/navigator-app/src/realign_job.rs

@@ -356,8 +356,10 @@ impl App {
             let dir = scratch.path().join("revert");
             clear_stage_dir(&dir);
             let token = cancel.clone();
+            let params = RevertParams::default();
+            log_buffer("collating", params.sort_buffer_bytes);
             let stats = tokio::task::spawn_blocking(move || {
-                revert::revert_alignment(&bam, reference.as_deref(), &dir, &RevertParams::default(), &token)
+                revert::revert_alignment(&bam, reference.as_deref(), &dir, &params, &token)
             })
             .await
             .map_err(|e| AppError::Join(e.to_string()))??;
@@ -466,9 +468,11 @@ impl App {
             // weight — and at WGS scale they are tens of GB the sort is about to want back.
             clear_stage_dir(&dir);
             let token = cancel.clone();
+            let params = SortParams::default();
+            log_buffer("sorting", params.buffer_bytes);
             stage(&sorted, async {
                 tokio::task::spawn_blocking(move || {
-                    postprocess::sort_alignment(&input, &out, &dir, &SortParams::default(), &token, &mut |_| {})
+                    postprocess::sort_alignment(&input, &out, &dir, &params, &token, &mut |_| {})
                 })
                 .await
                 .map_err(|e| AppError::Join(e.to_string()))?
@@ -673,6 +677,15 @@ fn gb(bytes: u64) -> u64 {
     bytes / 1_000_000_000
 }
 
+/// Record the spill budget a stage is about to run with.
+///
+/// Both budgets now come from the machine rather than a constant, so the run count they produce is
+/// no longer inferable from the version number. Without this line, "it spilled 400 runs" in a bug
+/// report is a fact with nothing to attach it to.
+fn log_buffer(stage: &str, bytes: usize) {
+    eprintln!("realign: {stage} with a {} MB buffer", bytes / (1024 * 1024));
+}
+
 /// Whether a job needing `needed` bytes may start given `free` bytes available.
 ///
 /// Split from the syscall so the *decision* is testable on any machine; the probe itself is not.

crates/navigator-resource/src/lib.rs

@@ -150,6 +150,75 @@ impl Write for PacedFile {
     }
 }
 
+/// The budget an external-sort stage holds in memory before spilling a run to disk.
+///
+/// Both of the pipeline's spill-to-disk stages — the revert's collator and the coordinate sort —
+/// were sized by a constant, 256 MB and 512 MB, chosen when nobody had run a WGS through them.
+/// The measured cost of that: a 30x WGS coordinate sort spilled **688 runs**, all of which the
+/// merge then opens at once. It is bounded memory by design and it does work, but on a machine with
+/// 128 GB of RAM it is a lot of fan-in bought for no reason, and the constant that produced it was
+/// the same on a laptop that genuinely needed it.
+///
+/// So the number comes from the machine. `var` still wins when it is set — an explicit MB count is
+/// the escape hatch for a run that has to be reproduced or squeezed — and the sizing is otherwise:
+///
+/// - **A quarter of installed RAM.** Total rather than free, because free fluctuates with whatever
+///   the user happens to have open, and a stage whose run count depends on the browser is a stage
+///   whose behaviour cannot be reproduced from a bug report.
+/// - **Never below 512 MB.** That is the sort's existing default, so no machine sorts with less
+///   than it does today.
+/// - **Never above 8 GB.** Past that the returns are gone — 88 runs against 44 is nothing next to
+///   688 against 88 — while the costs are not: the stable sort allocates half the buffer again as
+///   scratch, and growing the record vector doubles its allocation while still holding the old one.
+/// - **Never more than half of what is free right now.** The stable part of the rule assumes a
+///   machine that is otherwise idle. When it is not, spilling an extra run is cheap and swapping
+///   the buffer is not.
+///
+/// Memory the platform will not report reads as zero, and zero means *unknown*, which must not be
+/// read as "no memory" — the same rule [`classify`] follows. An unknown machine gets the floor.
+pub fn spill_budget(var: &str) -> u64 {
+    if let Some(mb) = std::env::var(var).ok().and_then(|s| s.parse::<u64>().ok()) {
+        return mb.max(1) * 1024 * 1024;
+    }
+    let mut system = sysinfo::System::new();
+    system.refresh_memory();
+    budget(system.total_memory(), system.available_memory())
+}
+
+/// What one heap allocation costs beyond the bytes asked for: the allocator's own bookkeeping plus
+/// rounding up to a size class.
+///
+/// It lives beside [`spill_budget`] because the two are one contract. A budget is only as honest as
+/// the tally that fills it, and a stage that counts only payload bytes will hold well over its
+/// budget in real memory — which is fine against a hand-picked 512 MB constant chosen with a margin
+/// nobody wrote down, and not fine against a fraction of the machine.
+///
+/// Sixteen bytes is the conventional figure for the allocators on the three desktop targets. This
+/// is a budget estimate rather than an audit; the point is that a record with four small vectors
+/// costs meaningfully more than the sum of their lengths.
+pub const ALLOCATION_OVERHEAD: usize = 16;
+
+/// The floor, and the answer for a machine that will not say how much memory it has.
+const MIN_SPILL_BUDGET: u64 = 512 << 20;
+/// The ceiling. See [`spill_budget`] for why bigger stops paying.
+const MAX_SPILL_BUDGET: u64 = 8 << 30;
+
+/// The sizing decision, split from the probe so it is testable on any machine — the same split
+/// [`classify`] makes, and for the same reason.
+fn budget(total_memory: u64, available_memory: u64) -> u64 {
+    if total_memory == 0 {
+        return MIN_SPILL_BUDGET;
+    }
+    let budget = (total_memory / 4).clamp(MIN_SPILL_BUDGET, MAX_SPILL_BUDGET);
+    if available_memory == 0 {
+        return budget;
+    }
+    // The floor holds even here: a machine this short of memory would have taken 512 MB under the
+    // old constant anyway, so honouring the busy-machine guard past that point would be a
+    // regression dressed as caution.
+    budget.min((available_memory / 2).max(MIN_SPILL_BUDGET))
+}
+
 /// How hard the machine is being leaned on.
 ///
 /// Bands, not a single threshold, because the interesting reading is the trend: a stage that
@@ -361,6 +430,58 @@ mod tests {
         assert_eq!(classify(0, 0, 0), Pressure::Normal);
     }
 
+    /// The case the autosizing exists for: a big machine should stop spilling hundreds of runs.
+    #[test]
+    fn a_large_machine_gets_the_ceiling() {
+        assert_eq!(budget(128 << 30, 100 << 30), MAX_SPILL_BUDGET);
+    }
+
+    #[test]
+    fn an_ordinary_machine_gets_a_quarter_of_it() {
+        assert_eq!(budget(16 << 30, 12 << 30), 4 << 30);
+    }
+
+    /// A machine whose memory is already spoken for gets a smaller buffer, because an extra spilled
+    /// run costs a file and swapping the buffer costs the run.
+    #[test]
+    fn a_busy_machine_is_held_to_half_of_what_is_free() {
+        assert_eq!(budget(64 << 30, 6 << 30), 3 << 30);
+    }
+
+    /// Never below what the sort used before any of this existed.
+    #[test]
+    fn a_small_or_busy_machine_never_goes_under_the_old_default() {
+        assert_eq!(budget(2 << 30, 2 << 30), MIN_SPILL_BUDGET);
+        assert_eq!(budget(64 << 30, 100 << 20), MIN_SPILL_BUDGET);
+    }
+
+    /// Unknown is not zero. A platform that will not report memory must not be sized as if it had
+    /// none — and must not be sized as if it had plenty either.
+    #[test]
+    fn unknown_memory_gets_the_floor() {
+        assert_eq!(budget(0, 0), MIN_SPILL_BUDGET);
+        assert_eq!(budget(64 << 30, 0), MAX_SPILL_BUDGET.min(16 << 30));
+    }
+
+    /// The escape hatch has to win, or a run cannot be reproduced on a different machine.
+    #[test]
+    fn an_explicit_override_beats_the_machine() {
+        let var = "NAVIGATOR_TEST_SPILL_MB_OVERRIDE";
+        std::env::set_var(var, "7");
+        assert_eq!(spill_budget(var), 7 * 1024 * 1024);
+        std::env::remove_var(var);
+    }
+
+    /// A nonsense override must not produce a zero-byte budget, which would spill one run per
+    /// record.
+    #[test]
+    fn a_zero_override_is_floored_at_one_megabyte() {
+        let var = "NAVIGATOR_TEST_SPILL_MB_ZERO";
+        std::env::set_var(var, "0");
+        assert_eq!(spill_budget(var), 1024 * 1024);
+        std::env::remove_var(var);
+    }
+
     #[test]
     fn written_bytes_accumulate() {
         let before = bytes_written();