diff --git a/crates/uffs-client/src/protocol/aggregate_wire.rs b/crates/uffs-client/src/protocol/aggregate_wire.rs index f3274bd13..0464e1882 100644 --- a/crates/uffs-client/src/protocol/aggregate_wire.rs +++ b/crates/uffs-client/src/protocol/aggregate_wire.rs @@ -62,6 +62,13 @@ pub struct AggregateSpecWire { /// Byte count for `verify=first_bytes` mode (default: 4096). #[serde(default, skip_serializing_if = "Option::is_none")] pub verify_bytes: Option, + /// Drive letter scoping an `ancestor`/`drilldown` rollup (e.g. `"C"`). + /// + /// Required when `kind` is `"rollup"` with `field` = `"ancestor"` — + /// the rollup's record index is per-drive, so the drilldown must + /// name the drive it belongs to. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub drive: Option, } /// Wire format for an aggregate result. diff --git a/crates/uffs-core/src/aggregate/accumulators.rs b/crates/uffs-core/src/aggregate/accumulators.rs index 0c5298b31..1390cf856 100644 --- a/crates/uffs-core/src/aggregate/accumulators.rs +++ b/crates/uffs-core/src/aggregate/accumulators.rs @@ -201,9 +201,10 @@ pub enum AccumulatorKind { inner: super::rollup::RollupAccumulator, /// Requested metrics. metrics: Vec, - /// Per-group sub-accumulators for nested rollups. + /// Per-group sub-accumulators for nested rollups, keyed by the + /// drive-qualified rollup group key. /// `None` when no sub-aggregation is requested. - sub_accumulators: Option>, + sub_accumulators: Option>, /// The sub-aggregation spec (cloned from `AggregateKind::Rollup.sub`). sub_kind: Option, }, @@ -313,7 +314,7 @@ impl GroupAccumulator { } => { let sub_accumulators = sub .as_ref() - .map(|_| std::collections::HashMap::::new()); + .map(|_| std::collections::HashMap::::new()); let sub_kind = sub.as_deref().cloned(); ( AccumulatorKind::Rollup { @@ -369,7 +370,7 @@ impl GroupAccumulator { drive: &DriveCompactIndex, idx: usize, drive_ordinal: u8, - ext_map: Option<&super::ExtensionMap>, + ext_map: &super::ExtensionMap, ) { let field = self.field; match &mut self.kind { @@ -438,12 +439,16 @@ impl GroupAccumulator { .. } => { // Compute group key and feed the top-level stats. - inner.feed(record, drive, idx); + // `feed` returns false when the record is out of scope + // (ancestor rollups skip records on other drives) — such + // records must not reach the sub-accumulators either. + let bucketed = inner.feed(record, drive, idx, drive_ordinal); // If nested sub-aggregation is configured, feed the // per-group sub-accumulator. - if let (Some(sub_map), Some(sub_spec)) = - (sub_accumulators.as_mut(), sub_kind.as_ref()) + if bucketed + && let (Some(sub_map), Some(sub_spec)) = + (sub_accumulators.as_mut(), sub_kind.as_ref()) { let key = inner.last_key(); let sub_acc = sub_map @@ -603,22 +608,21 @@ const fn extract_timestamp(field: Option, record: &CompactRecord) -> i6 /// Extract a group key (encoded as u64) from a record. /// -/// For `Extension`, uses the `ExtensionMap` (when provided) to return a -/// canonical cross-drive extension ID. This ensures that `"exe"` on -/// drive C and `"exe"` on drive D share the same group key. +/// For `Extension`, uses the `ExtensionMap` to return a canonical +/// cross-drive extension ID. This ensures that `"exe"` on drive C and +/// `"exe"` on drive D share the same group key. The map is required: +/// raw per-drive `extension_id`s must never enter a cross-drive merge +/// key (the same id means different extensions on different drives). #[inline] fn extract_group_key( field: Option, record: &CompactRecord, drive: &DriveCompactIndex, drive_ordinal: u8, - ext_map: Option<&super::ExtensionMap>, + ext_map: &super::ExtensionMap, ) -> u64 { match field { - Some(FieldId::Extension) => ext_map.map_or_else( - || u64::from(record.extension_id), - |map| map.canonical_id(drive_ordinal, record.extension_id), - ), + Some(FieldId::Extension) => ext_map.canonical_id(drive_ordinal, record.extension_id), Some(FieldId::Drive) => u64::from(u32::from(drive.letter.as_byte())), Some(FieldId::Type) => { use crate::search::derived::{ diff --git a/crates/uffs-core/src/aggregate/duplicates.rs b/crates/uffs-core/src/aggregate/duplicates.rs index e157298c9..1a5ec69fe 100644 --- a/crates/uffs-core/src/aggregate/duplicates.rs +++ b/crates/uffs-core/src/aggregate/duplicates.rs @@ -43,7 +43,19 @@ impl CompositeKey { match field { FieldId::Size => components.push(record.size), FieldId::SizeOnDisk => components.push(record.allocated), - FieldId::Extension => components.push(u64::from(record.extension_id)), + FieldId::Extension => { + // `extension_id` is a per-drive intern id — the same + // extension gets different ids on different drives (and + // the same id can mean different extensions), so the id + // must never enter a cross-drive merge key. Hash the + // interned extension string instead (already lowercased + // by the interner, so equal across drives). + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + if let Some(ext) = drive.ext_names.get(usize::from(record.extension_id)) { + ext.hash(&mut hasher); + } + components.push(hasher.finish()); + } FieldId::Modified => components.push(uffs_mft::nonneg_to_u64(record.modified)), FieldId::Created => components.push(uffs_mft::nonneg_to_u64(record.created)), FieldId::Name => { @@ -285,501 +297,4 @@ pub struct DuplicateResult { } #[cfg(test)] -#[expect( - clippy::indexing_slicing, - reason = "tests assert against fixtures with known shape; indexing panic = test failure" -)] -mod tests { - use uffs_mft::index::{IndexNameRef, MftIndex, ROOT_FRS, SizeInfo}; - - use super::*; - use crate::compact::build_compact_index; - - #[test] - fn composite_key_equality() { - let key1 = CompositeKey { - components: vec![100, 42], - name_hash: 12345, - }; - let key2 = CompositeKey { - components: vec![100, 42], - name_hash: 12345, - }; - assert_eq!(key1, key2); - } - - #[test] - fn composite_key_inequality() { - let key1 = CompositeKey { - components: vec![100, 42], - name_hash: 12345, - }; - let key2 = CompositeKey { - components: vec![100, 43], - name_hash: 12345, - }; - assert_ne!(key1, key2); - } - - #[test] - fn duplicate_accumulator_new() { - let acc = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 2, - ); - assert!(acc.groups.is_empty()); - } - - /// Build a synthetic drive with known duplicate files. - /// - /// Layout: - /// - root (dir) - /// - "readme.txt" (FRS 100, 500 bytes) — unique - /// - "data.bin" (FRS 101, 1000 bytes) — duplicate (3 copies) - /// - "data.bin" (FRS 102, 1000 bytes) - /// - "data.bin" (FRS 103, 1000 bytes) - /// - "config.ini" (FRS 104, 200 bytes) — duplicate (2 copies) - /// - "config.ini" (FRS 105, 200 bytes) - fn build_dup_drive() -> DriveCompactIndex { - let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); - - // Root directory. - let root_off = idx.add_name("."); - let root = idx.get_or_create(ROOT_FRS.into()); - root.stdinfo.set_directory(true); - root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); - root.first_name.parent_frs = Into::into(ROOT_FRS); - - let add_file = |index: &mut MftIndex, frs: u64, name: &str, size: u64| { - let off = index.add_name(name); - let ext = index.intern_extension(name); - let rec = index.get_or_create(frs.into()); - rec.first_name.name = - IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); - rec.first_name.parent_frs = Into::into(ROOT_FRS); - rec.first_stream.size = SizeInfo { - length: size, - allocated: size, - }; - rec.stdinfo.flags = 0x20; // archive - }; - - add_file(&mut idx, 100, "readme.txt", 500); - add_file(&mut idx, 101, "data.bin", 1000); - add_file(&mut idx, 102, "data.bin", 1000); - add_file(&mut idx, 103, "data.bin", 1000); - add_file(&mut idx, 104, "config.ini", 200); - add_file(&mut idx, 105, "config.ini", 200); - - let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); - drive - } - - // ── S4E.2: synthetic duplicates — group count + reclaimable ─── - - #[test] - fn synthetic_duplicates_group_count_and_reclaimable() { - let drive = build_dup_drive(); - let mut acc = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 3, - ); - - for (idx, rec) in drive.records.iter().enumerate() { - acc.feed(rec, &drive, idx); - } - - let result = acc.finalize(50); - - // Should have exactly 2 duplicate groups: - // 1. data.bin (3 copies × 1000 bytes) - // 2. config.ini (2 copies × 200 bytes) - assert_eq!(result.candidate_groups, 2, "expected 2 duplicate groups"); - - // Total duplicate files: 3 + 2 = 5 - assert_eq!( - result.candidate_files, 5, - "expected 5 total files across duplicate groups" - ); - - // Reclaimable: - // data.bin: 3×1000 - 1000 = 2000 - // config.ini: 2×200 - 200 = 200 - // total: 2200 - assert_eq!( - result.total_reclaimable_bytes, 2200, - "expected 2200 reclaimable bytes" - ); - - // Groups sorted by reclaimable desc: data.bin first. - assert_eq!(result.groups.len(), 2); - assert_eq!( - result.groups[0].count, 3, - "first group: data.bin (3 copies)" - ); - assert_eq!(result.groups[0].file_size, 1000); - assert_eq!(result.groups[0].reclaimable_bytes, 2000); - assert_eq!( - result.groups[1].count, 2, - "second group: config.ini (2 copies)" - ); - assert_eq!(result.groups[1].file_size, 200); - assert_eq!(result.groups[1].reclaimable_bytes, 200); - - // Member indices captured (sample=3). - assert_eq!(result.groups[0].member_indices.len(), 3); - assert_eq!(result.groups[1].member_indices.len(), 2); - } - - // ── Cross-drive merge (parallel aggregation reducer path) ─── - - /// Regression guard for the v0.5.39 aggregate-parallelism fix. - /// - /// Before the fix, `GroupAccumulator::merge` silently no-op'd on - /// `(Duplicates, Duplicates)` pairs, so the rayon reducer used by - /// `run_aggregate{,_filtered,_with_filters}` lost every per-drive - /// group that didn't happen to survive the reduce tree, collapsing - /// real duplicate buckets to zero (see `LOG/Output` T140/T147/S4C.*). - /// - /// This test feeds two "drives" that share a `(size=1000, name="data.bin")` - /// group (1 copy on drive X, 2 copies on drive Y → 3 total) and a - /// drive-local singleton, then merges them via the new - /// `DuplicateAccumulator::merge` and asserts the final group count, - /// member count, and drive-ordinal provenance are all preserved. - #[test] - fn merge_sums_groups_across_drives() { - // Drive X — uses build_dup_drive but we'll only feed a subset - // into acc_x to simulate per-drive scans over a shared corpus. - let drive_x = build_dup_drive(); - let drive_y = build_dup_drive(); - - let mut acc_x = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 5, - ); - let mut acc_y = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 5, - ); - - // Drive X is scanned as drive ordinal 0. - acc_x.set_drive_ordinal(0); - for (idx, rec) in drive_x.records.iter().enumerate() { - acc_x.feed(rec, &drive_x, idx); - } - - // Drive Y is scanned as drive ordinal 1. - acc_y.set_drive_ordinal(1); - for (idx, rec) in drive_y.records.iter().enumerate() { - acc_y.feed(rec, &drive_y, idx); - } - - // Reduce: X absorbs Y. - acc_x.merge(&acc_y); - - let result = acc_x.finalize(50); - - // Both drives hold the same synthetic layout (1× readme.txt + - // 3× data.bin + 2× config.ini). After merge: - // * data.bin: 6 copies × 1000 B → reclaimable 5000 - // * config.ini: 4 copies × 200 B → reclaimable 600 - // * readme.txt: 2 copies × 500 B → reclaimable 500 (was a singleton on each - // drive individually — the fact that it becomes a duplicate *only after - // cross-drive merge* is exactly the property the old silent no-op arm was - // destroying.) - assert_eq!( - result.candidate_groups, 3, - "merge must keep all three cross-drive duplicate groups" - ); - assert_eq!( - result.candidate_files, 12, - "3+3 data.bin + 2+2 config.ini + 1+1 readme.txt = 12 duplicate members" - ); - - // Groups sorted desc by reclaimable bytes → data.bin first. - assert_eq!(result.groups[0].count, 6, "6 data.bin across both drives"); - assert_eq!(result.groups[0].total_bytes, 6000); - assert_eq!(result.groups[0].reclaimable_bytes, 5000); - assert_eq!(result.groups[1].count, 4, "4 config.ini across both drives"); - assert_eq!(result.groups[1].total_bytes, 800); - assert_eq!(result.groups[1].reclaimable_bytes, 600); - assert_eq!( - result.groups[2].count, 2, - "readme.txt becomes a duplicate *only* via cross-drive merge" - ); - assert_eq!(result.groups[2].total_bytes, 1000); - assert_eq!(result.groups[2].reclaimable_bytes, 500); - - // member_indices cap = 5; we merged 6 potential members for - // data.bin, so we should see exactly 5 entries, and the union - // must contain BOTH drive ordinals (otherwise the parallel - // reducer would be losing cross-drive provenance). - assert_eq!(result.groups[0].member_indices.len(), 5); - let ordinals: std::collections::HashSet = result.groups[0] - .member_indices - .iter() - .map(|(_, dist)| *dist) - .collect(); - assert!( - ordinals.contains(&0) && ordinals.contains(&1), - "member_indices must include members from BOTH drives after merge, got {ordinals:?}" - ); - } - - /// Guard: merge respects the `max_groups` OOM cap when absorbing - /// entirely-new groups from `other`. We build a self with the cap - /// already consumed, then merge another accumulator carrying a - /// brand-new group key — the new group must be dropped, not - /// silently pushed past the cap. - #[test] - fn merge_respects_max_groups_cap() { - let drive_x = build_dup_drive(); - let drive_y = build_dup_drive(); - - // Cap acc_x at exactly the number of groups it will build - // (data.bin + config.ini = 2) so any NEW keys from `other` get - // rejected. - let mut acc_x = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 2, - 5, - ); - let mut acc_y = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 5, - ); - - acc_x.set_drive_ordinal(0); - for (idx, rec) in drive_x.records.iter().enumerate() { - acc_x.feed(rec, &drive_x, idx); - } - - // Feed acc_y with a record whose key is NEW — fabricate by - // switching the size on the fly (still uses drive_y's name table). - acc_y.set_drive_ordinal(1); - // Inject the existing groups plus a synthetic new one. - for (idx, rec) in drive_y.records.iter().enumerate() { - acc_y.feed(rec, &drive_y, idx); - } - - let groups_before = acc_x.groups.len(); - acc_x.merge(&acc_y); - let groups_after = acc_x.groups.len(); - - assert_eq!( - groups_before, groups_after, - "merge must not push past max_groups cap when absorbing new keys" - ); - } - - // ── S4E.3: singleton elimination ──────────────────────────── - - #[test] - fn singleton_elimination_no_false_duplicates() { - let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); - - // Root. - let root_off = idx.add_name("."); - let root = idx.get_or_create(ROOT_FRS.into()); - root.stdinfo.set_directory(true); - root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); - root.first_name.parent_frs = Into::into(ROOT_FRS); - - // 10 unique files — all different names and sizes. - for i in 0..10_u64 { - let name = format!("file_{i}.dat"); - let off = idx.add_name(&name); - let ext = idx.intern_extension(&name); - let rec = idx.get_or_create((100 + i).into()); - rec.first_name.name = - IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); - rec.first_name.parent_frs = Into::into(ROOT_FRS); - rec.first_stream.size = SizeInfo { - length: (i + 1) * 100, - allocated: (i + 1) * 512, - }; - rec.stdinfo.flags = 0x20; - } - - let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); - let mut acc = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 2, - ); - - for (i, rec) in drive.records.iter().enumerate() { - acc.feed(rec, &drive, i); - } - - let result = acc.finalize(50); - - // All files are unique → zero duplicate groups. - assert_eq!(result.candidate_groups, 0, "no duplicates expected"); - assert_eq!(result.candidate_files, 0); - assert_eq!(result.total_reclaimable_bytes, 0); - assert!(result.groups.is_empty()); - } - - #[test] - fn zero_byte_files_excluded_from_duplicates() { - let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); - - let root_off = idx.add_name("."); - let root = idx.get_or_create(ROOT_FRS.into()); - root.stdinfo.set_directory(true); - root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); - root.first_name.parent_frs = Into::into(ROOT_FRS); - - // Two zero-byte files with same name — should NOT be duplicates. - for i in 0..2_u64 { - let name = "empty.txt"; - let off = idx.add_name(name); - let ext = idx.intern_extension(name); - let rec = idx.get_or_create((100 + i).into()); - rec.first_name.name = - IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); - rec.first_name.parent_frs = Into::into(ROOT_FRS); - rec.first_stream.size = SizeInfo { - length: 0, - allocated: 0, - }; - rec.stdinfo.flags = 0x20; - } - - let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); - let mut acc = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 2, - ); - - for (i, rec) in drive.records.iter().enumerate() { - acc.feed(rec, &drive, i); - } - - let result = acc.finalize(50); - assert_eq!(result.candidate_groups, 0, "zero-byte files excluded"); - } - - #[test] - fn directories_excluded_from_duplicates() { - let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); - - let root_off = idx.add_name("."); - let root = idx.get_or_create(ROOT_FRS.into()); - root.stdinfo.set_directory(true); - root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); - root.first_name.parent_frs = Into::into(ROOT_FRS); - - // Two directories with same name — should NOT be duplicates. - for i in 0..2_u64 { - let name = "subdir"; - let off = idx.add_name(name); - let ext = idx.intern_extension(name); - let rec = idx.get_or_create((100 + i).into()); - rec.stdinfo.set_directory(true); - rec.stdinfo.flags = 0x10; // directory - rec.first_name.name = - IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); - rec.first_name.parent_frs = Into::into(ROOT_FRS); - rec.first_stream.size = SizeInfo { - length: 4096, - allocated: 4096, - }; - } - - let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); - let mut acc = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::None, - 100_000, - 2, - ); - - for (i, rec) in drive.records.iter().enumerate() { - acc.feed(rec, &drive, i); - } - - let result = acc.finalize(50); - assert_eq!(result.candidate_groups, 0, "directories excluded"); - } - - // ── S4E.4: Windows verified duplicates ────────────────────── - - /// Integration test with real MFT data + file-content verification. - /// - /// Requires Windows with the test-tree created by - /// `scripts/windows/create_mft_test_tree.ps1`. - /// Run with: `cargo test -p uffs-core -- duplicates_verified_windows - /// --ignored` - #[test] - #[ignore = "requires Windows with MFT test tree (create_mft_test_tree.ps1)"] - #[cfg(windows)] - fn duplicates_verified_windows() { - use crate::compact_loader::{MftSource, load_drive}; - - // Load C: drive index. `load_drive` returns `(index, timing)`; - // the test only uses the index itself. - let source = MftSource::Live(uffs_mft::platform::DriveLetter::C); - let (drive, _load_timing) = load_drive(&source, false).expect("failed to load C: drive"); - - let mut acc = DuplicateAccumulator::new( - vec![FieldId::Size, FieldId::Name], - DuplicateVerify::FirstBytes { count: 4096 }, - 500_000, - 5, - ); - - for (idx, rec) in drive.records.iter().enumerate() { - acc.feed(rec, &drive, idx); - } - - let result = acc.finalize(100); - - // On any real Windows install there should be known duplicates - // (e.group., DLLs in System32 and SysWOW64 with same name+size). - assert!( - result.candidate_groups > 0, - "a real Windows C: drive should contain duplicate files" - ); - assert!( - result.total_reclaimable_bytes > 0, - "reclaimable bytes should be non-zero" - ); - - // Verify groups are sorted by reclaimable_bytes descending. - for pair in result.groups.windows(2) { - // `windows(2)` always yields exactly two elements; the - // `else` arm is dead code but keeps clippy's - // missing_asserts_for_indexing lint quiet without - // resorting to `unreachable!()`. - let [prev, curr] = pair else { - continue; - }; - assert!( - prev.reclaimable_bytes >= curr.reclaimable_bytes, - "groups should be sorted by reclaimable_bytes desc" - ); - } - - // Each group must have count ≥ 2. - for group in &result.groups { - assert!(group.count >= 2, "each group must have at least 2 files"); - assert!(group.file_size > 0, "zero-byte files should be excluded"); - } - } -} +mod tests; diff --git a/crates/uffs-core/src/aggregate/duplicates/tests.rs b/crates/uffs-core/src/aggregate/duplicates/tests.rs new file mode 100644 index 000000000..d5332aecc --- /dev/null +++ b/crates/uffs-core/src/aggregate/duplicates/tests.rs @@ -0,0 +1,584 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2025-2026 SKY, LLC. + +//! Unit tests for [`super`] (`duplicates`). +//! +//! Extracted from `duplicates.rs` into a sibling submodule so the lib +//! file stays under the 800 LOC policy ceiling (mirrors the +//! `compact_cache/tests.rs` extraction). + +#![expect( + clippy::indexing_slicing, + reason = "tests assert against fixtures with known shape; indexing panic = test failure" +)] + +use uffs_mft::index::{IndexNameRef, MftIndex, ROOT_FRS, SizeInfo}; + +use super::*; +use crate::compact::build_compact_index; + +#[test] +fn composite_key_equality() { + let key1 = CompositeKey { + components: vec![100, 42], + name_hash: 12345, + }; + let key2 = CompositeKey { + components: vec![100, 42], + name_hash: 12345, + }; + assert_eq!(key1, key2); +} + +#[test] +fn composite_key_inequality() { + let key1 = CompositeKey { + components: vec![100, 42], + name_hash: 12345, + }; + let key2 = CompositeKey { + components: vec![100, 43], + name_hash: 12345, + }; + assert_ne!(key1, key2); +} + +#[test] +fn duplicate_accumulator_new() { + let acc = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 2, + ); + assert!(acc.groups.is_empty()); +} + +/// Build a synthetic drive with known duplicate files. +/// +/// Layout: +/// - root (dir) +/// - "readme.txt" (FRS 100, 500 bytes) — unique +/// - "data.bin" (FRS 101, 1000 bytes) — duplicate (3 copies) +/// - "data.bin" (FRS 102, 1000 bytes) +/// - "data.bin" (FRS 103, 1000 bytes) +/// - "config.ini" (FRS 104, 200 bytes) — duplicate (2 copies) +/// - "config.ini" (FRS 105, 200 bytes) +fn build_dup_drive() -> DriveCompactIndex { + let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); + + // Root directory. + let root_off = idx.add_name("."); + let root = idx.get_or_create(ROOT_FRS.into()); + root.stdinfo.set_directory(true); + root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); + root.first_name.parent_frs = Into::into(ROOT_FRS); + + let add_file = |index: &mut MftIndex, frs: u64, name: &str, size: u64| { + let off = index.add_name(name); + let ext = index.intern_extension(name); + let rec = index.get_or_create(frs.into()); + rec.first_name.name = IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); + rec.first_name.parent_frs = Into::into(ROOT_FRS); + rec.first_stream.size = SizeInfo { + length: size, + allocated: size, + }; + rec.stdinfo.flags = 0x20; // archive + }; + + add_file(&mut idx, 100, "readme.txt", 500); + add_file(&mut idx, 101, "data.bin", 1000); + add_file(&mut idx, 102, "data.bin", 1000); + add_file(&mut idx, 103, "data.bin", 1000); + add_file(&mut idx, 104, "config.ini", 200); + add_file(&mut idx, 105, "config.ini", 200); + + let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); + drive +} + +// ── S4E.2: synthetic duplicates — group count + reclaimable ─── + +#[test] +fn synthetic_duplicates_group_count_and_reclaimable() { + let drive = build_dup_drive(); + let mut acc = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 3, + ); + + for (idx, rec) in drive.records.iter().enumerate() { + acc.feed(rec, &drive, idx); + } + + let result = acc.finalize(50); + + // Should have exactly 2 duplicate groups: + // 1. data.bin (3 copies × 1000 bytes) + // 2. config.ini (2 copies × 200 bytes) + assert_eq!(result.candidate_groups, 2, "expected 2 duplicate groups"); + + // Total duplicate files: 3 + 2 = 5 + assert_eq!( + result.candidate_files, 5, + "expected 5 total files across duplicate groups" + ); + + // Reclaimable: + // data.bin: 3×1000 - 1000 = 2000 + // config.ini: 2×200 - 200 = 200 + // total: 2200 + assert_eq!( + result.total_reclaimable_bytes, 2200, + "expected 2200 reclaimable bytes" + ); + + // Groups sorted by reclaimable desc: data.bin first. + assert_eq!(result.groups.len(), 2); + assert_eq!( + result.groups[0].count, 3, + "first group: data.bin (3 copies)" + ); + assert_eq!(result.groups[0].file_size, 1000); + assert_eq!(result.groups[0].reclaimable_bytes, 2000); + assert_eq!( + result.groups[1].count, 2, + "second group: config.ini (2 copies)" + ); + assert_eq!(result.groups[1].file_size, 200); + assert_eq!(result.groups[1].reclaimable_bytes, 200); + + // Member indices captured (sample=3). + assert_eq!(result.groups[0].member_indices.len(), 3); + assert_eq!(result.groups[1].member_indices.len(), 2); +} + +// ── Cross-drive merge (parallel aggregation reducer path) ─── + +/// Regression guard for the v0.5.39 aggregate-parallelism fix. +/// +/// Before the fix, `GroupAccumulator::merge` silently no-op'd on +/// `(Duplicates, Duplicates)` pairs, so the rayon reducer used by +/// `run_aggregate{,_filtered,_with_filters}` lost every per-drive +/// group that didn't happen to survive the reduce tree, collapsing +/// real duplicate buckets to zero (see `LOG/Output` T140/T147/S4C.*). +/// +/// This test feeds two "drives" that share a `(size=1000, name="data.bin")` +/// group (1 copy on drive X, 2 copies on drive Y → 3 total) and a +/// drive-local singleton, then merges them via the new +/// `DuplicateAccumulator::merge` and asserts the final group count, +/// member count, and drive-ordinal provenance are all preserved. +#[test] +fn merge_sums_groups_across_drives() { + // Drive X — uses build_dup_drive but we'll only feed a subset + // into acc_x to simulate per-drive scans over a shared corpus. + let drive_x = build_dup_drive(); + let drive_y = build_dup_drive(); + + let mut acc_x = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 5, + ); + let mut acc_y = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 5, + ); + + // Drive X is scanned as drive ordinal 0. + acc_x.set_drive_ordinal(0); + for (idx, rec) in drive_x.records.iter().enumerate() { + acc_x.feed(rec, &drive_x, idx); + } + + // Drive Y is scanned as drive ordinal 1. + acc_y.set_drive_ordinal(1); + for (idx, rec) in drive_y.records.iter().enumerate() { + acc_y.feed(rec, &drive_y, idx); + } + + // Reduce: X absorbs Y. + acc_x.merge(&acc_y); + + let result = acc_x.finalize(50); + + // Both drives hold the same synthetic layout (1× readme.txt + + // 3× data.bin + 2× config.ini). After merge: + // * data.bin: 6 copies × 1000 B → reclaimable 5000 + // * config.ini: 4 copies × 200 B → reclaimable 600 + // * readme.txt: 2 copies × 500 B → reclaimable 500 (was a singleton on each + // drive individually — the fact that it becomes a duplicate *only after + // cross-drive merge* is exactly the property the old silent no-op arm was + // destroying.) + assert_eq!( + result.candidate_groups, 3, + "merge must keep all three cross-drive duplicate groups" + ); + assert_eq!( + result.candidate_files, 12, + "3+3 data.bin + 2+2 config.ini + 1+1 readme.txt = 12 duplicate members" + ); + + // Groups sorted desc by reclaimable bytes → data.bin first. + assert_eq!(result.groups[0].count, 6, "6 data.bin across both drives"); + assert_eq!(result.groups[0].total_bytes, 6000); + assert_eq!(result.groups[0].reclaimable_bytes, 5000); + assert_eq!(result.groups[1].count, 4, "4 config.ini across both drives"); + assert_eq!(result.groups[1].total_bytes, 800); + assert_eq!(result.groups[1].reclaimable_bytes, 600); + assert_eq!( + result.groups[2].count, 2, + "readme.txt becomes a duplicate *only* via cross-drive merge" + ); + assert_eq!(result.groups[2].total_bytes, 1000); + assert_eq!(result.groups[2].reclaimable_bytes, 500); + + // member_indices cap = 5; we merged 6 potential members for + // data.bin, so we should see exactly 5 entries, and the union + // must contain BOTH drive ordinals (otherwise the parallel + // reducer would be losing cross-drive provenance). + assert_eq!(result.groups[0].member_indices.len(), 5); + let ordinals: std::collections::HashSet = result.groups[0] + .member_indices + .iter() + .map(|(_, dist)| *dist) + .collect(); + assert!( + ordinals.contains(&0) && ordinals.contains(&1), + "member_indices must include members from BOTH drives after merge, got {ordinals:?}" + ); +} + +/// Regression: the `Extension` composite-key component must be +/// drive-independent. +/// +/// 2026-07-21, found by auditing for the multi-drive rollup key bug +/// (`top_folders` malformed keys): `CompositeKey` pushed the raw +/// per-drive `extension_id`. The same extension gets different intern +/// ids on different drives (missed cross-drive duplicates) and the +/// same id can mean different extensions (false groups). +#[test] +fn extension_key_groups_across_drives_by_name_not_intern_id() { + let build = |letter, files: &[(&str, u64)]| { + let mut idx = MftIndex::new(letter); + let root_off = idx.add_name("."); + let root = idx.get_or_create(ROOT_FRS.into()); + root.stdinfo.set_directory(true); + root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); + root.first_name.parent_frs = Into::into(ROOT_FRS); + for (frs, &(name, size)) in (100_u64..).zip(files) { + let off = idx.add_name(name); + let ext = idx.intern_extension(name); + let rec = idx.get_or_create(frs.into()); + rec.first_name.name = + IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); + rec.first_name.parent_frs = Into::into(ROOT_FRS); + rec.first_stream.size = SizeInfo { + length: size, + allocated: size, + }; + rec.stdinfo.flags = 0x20; + } + let (drive, _, _) = build_compact_index(letter, &idx); + drive + }; + + // Drive X intern order: jpg=1, avi=2. + let drive_x = build(uffs_mft::platform::DriveLetter::X, &[ + ("photo.jpg", 1000), + ("clip.avi", 500), + ]); + // Drive Y intern order: mp3=1, mp4=2, jpg=3 — so "jpg" has a + // DIFFERENT local id than on X, while "mp4" COLLIDES with X's + // "avi" id (and also matches its size). + let drive_y = build(uffs_mft::platform::DriveLetter::Y, &[ + ("song.mp3", 300), + ("video.mp4", 500), + ("photo.jpg", 1000), + ]); + + let mut acc_x = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Extension], + DuplicateVerify::None, + 100_000, + 5, + ); + let mut acc_y = acc_x.clone(); + + acc_x.set_drive_ordinal(0); + for (idx, rec) in drive_x.records.iter().enumerate() { + acc_x.feed(rec, &drive_x, idx); + } + acc_y.set_drive_ordinal(1); + for (idx, rec) in drive_y.records.iter().enumerate() { + acc_y.feed(rec, &drive_y, idx); + } + acc_x.merge(&acc_y); + + let result = acc_x.finalize(50); + + // Exactly one real group: photo.jpg on both drives (size 1000 + + // extension "jpg"). clip.avi / video.mp4 share size 500 and local + // ext id 2 but are DIFFERENT extensions — they must not group. + assert_eq!( + result.candidate_groups, 1, + "only the cross-drive jpg pair should group" + ); + assert_eq!(result.groups[0].count, 2, "photo.jpg on X + photo.jpg on Y"); + assert_eq!( + result.groups[0].file_size, 1000, + "the surviving group must be the jpg pair, not the avi/mp4 id collision" + ); +} + +/// Guard: merge respects the `max_groups` OOM cap when absorbing +/// entirely-new groups from `other`. We build a self with the cap +/// already consumed, then merge another accumulator carrying a +/// brand-new group key — the new group must be dropped, not +/// silently pushed past the cap. +#[test] +fn merge_respects_max_groups_cap() { + let drive_x = build_dup_drive(); + let drive_y = build_dup_drive(); + + // Cap acc_x at exactly the number of groups it will build + // (data.bin + config.ini = 2) so any NEW keys from `other` get + // rejected. + let mut acc_x = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 2, + 5, + ); + let mut acc_y = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 5, + ); + + acc_x.set_drive_ordinal(0); + for (idx, rec) in drive_x.records.iter().enumerate() { + acc_x.feed(rec, &drive_x, idx); + } + + // Feed acc_y with a record whose key is NEW — fabricate by + // switching the size on the fly (still uses drive_y's name table). + acc_y.set_drive_ordinal(1); + // Inject the existing groups plus a synthetic new one. + for (idx, rec) in drive_y.records.iter().enumerate() { + acc_y.feed(rec, &drive_y, idx); + } + + let groups_before = acc_x.groups.len(); + acc_x.merge(&acc_y); + let groups_after = acc_x.groups.len(); + + assert_eq!( + groups_before, groups_after, + "merge must not push past max_groups cap when absorbing new keys" + ); +} + +// ── S4E.3: singleton elimination ──────────────────────────── + +#[test] +fn singleton_elimination_no_false_duplicates() { + let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); + + // Root. + let root_off = idx.add_name("."); + let root = idx.get_or_create(ROOT_FRS.into()); + root.stdinfo.set_directory(true); + root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); + root.first_name.parent_frs = Into::into(ROOT_FRS); + + // 10 unique files — all different names and sizes. + for i in 0..10_u64 { + let name = format!("file_{i}.dat"); + let off = idx.add_name(&name); + let ext = idx.intern_extension(&name); + let rec = idx.get_or_create((100 + i).into()); + rec.first_name.name = IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); + rec.first_name.parent_frs = Into::into(ROOT_FRS); + rec.first_stream.size = SizeInfo { + length: (i + 1) * 100, + allocated: (i + 1) * 512, + }; + rec.stdinfo.flags = 0x20; + } + + let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); + let mut acc = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 2, + ); + + for (i, rec) in drive.records.iter().enumerate() { + acc.feed(rec, &drive, i); + } + + let result = acc.finalize(50); + + // All files are unique → zero duplicate groups. + assert_eq!(result.candidate_groups, 0, "no duplicates expected"); + assert_eq!(result.candidate_files, 0); + assert_eq!(result.total_reclaimable_bytes, 0); + assert!(result.groups.is_empty()); +} + +#[test] +fn zero_byte_files_excluded_from_duplicates() { + let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); + + let root_off = idx.add_name("."); + let root = idx.get_or_create(ROOT_FRS.into()); + root.stdinfo.set_directory(true); + root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); + root.first_name.parent_frs = Into::into(ROOT_FRS); + + // Two zero-byte files with same name — should NOT be duplicates. + for i in 0..2_u64 { + let name = "empty.txt"; + let off = idx.add_name(name); + let ext = idx.intern_extension(name); + let rec = idx.get_or_create((100 + i).into()); + rec.first_name.name = IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); + rec.first_name.parent_frs = Into::into(ROOT_FRS); + rec.first_stream.size = SizeInfo { + length: 0, + allocated: 0, + }; + rec.stdinfo.flags = 0x20; + } + + let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); + let mut acc = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 2, + ); + + for (i, rec) in drive.records.iter().enumerate() { + acc.feed(rec, &drive, i); + } + + let result = acc.finalize(50); + assert_eq!(result.candidate_groups, 0, "zero-byte files excluded"); +} + +#[test] +fn directories_excluded_from_duplicates() { + let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::T); + + let root_off = idx.add_name("."); + let root = idx.get_or_create(ROOT_FRS.into()); + root.stdinfo.set_directory(true); + root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); + root.first_name.parent_frs = Into::into(ROOT_FRS); + + // Two directories with same name — should NOT be duplicates. + for i in 0..2_u64 { + let name = "subdir"; + let off = idx.add_name(name); + let ext = idx.intern_extension(name); + let rec = idx.get_or_create((100 + i).into()); + rec.stdinfo.set_directory(true); + rec.stdinfo.flags = 0x10; // directory + rec.first_name.name = IndexNameRef::new(off, uffs_mft::len_to_u16(name.len()), true, ext); + rec.first_name.parent_frs = Into::into(ROOT_FRS); + rec.first_stream.size = SizeInfo { + length: 4096, + allocated: 4096, + }; + } + + let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::T, &idx); + let mut acc = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::None, + 100_000, + 2, + ); + + for (i, rec) in drive.records.iter().enumerate() { + acc.feed(rec, &drive, i); + } + + let result = acc.finalize(50); + assert_eq!(result.candidate_groups, 0, "directories excluded"); +} + +// ── S4E.4: Windows verified duplicates ────────────────────── + +/// Integration test with real MFT data + file-content verification. +/// +/// Requires Windows with the test-tree created by +/// `scripts/windows/create_mft_test_tree.ps1`. +/// Run with: `cargo test -p uffs-core -- duplicates_verified_windows +/// --ignored` +#[test] +#[ignore = "requires Windows with MFT test tree (create_mft_test_tree.ps1)"] +#[cfg(windows)] +fn duplicates_verified_windows() { + use crate::compact_loader::{MftSource, load_drive}; + + // Load C: drive index. `load_drive` returns `(index, timing)`; + // the test only uses the index itself. + let source = MftSource::Live(uffs_mft::platform::DriveLetter::C); + let (drive, _load_timing) = load_drive(&source, false).expect("failed to load C: drive"); + + let mut acc = DuplicateAccumulator::new( + vec![FieldId::Size, FieldId::Name], + DuplicateVerify::FirstBytes { count: 4096 }, + 500_000, + 5, + ); + + for (idx, rec) in drive.records.iter().enumerate() { + acc.feed(rec, &drive, idx); + } + + let result = acc.finalize(100); + + // On any real Windows install there should be known duplicates + // (e.group., DLLs in System32 and SysWOW64 with same name+size). + assert!( + result.candidate_groups > 0, + "a real Windows C: drive should contain duplicate files" + ); + assert!( + result.total_reclaimable_bytes > 0, + "reclaimable bytes should be non-zero" + ); + + // Verify groups are sorted by reclaimable_bytes descending. + for pair in result.groups.windows(2) { + // `windows(2)` always yields exactly two elements; the + // `else` arm is dead code but keeps clippy's + // missing_asserts_for_indexing lint quiet without + // resorting to `unreachable!()`. + let [prev, curr] = pair else { + continue; + }; + assert!( + prev.reclaimable_bytes >= curr.reclaimable_bytes, + "groups should be sorted by reclaimable_bytes desc" + ); + } + + // Each group must have count ≥ 2. + for group in &result.groups { + assert!(group.count >= 2, "each group must have at least 2 files"); + assert!(group.file_size > 0, "zero-byte files should be excluded"); + } +} diff --git a/crates/uffs-core/src/aggregate/finalize.rs b/crates/uffs-core/src/aggregate/finalize.rs index a78e89240..5c1758631 100644 --- a/crates/uffs-core/src/aggregate/finalize.rs +++ b/crates/uffs-core/src/aggregate/finalize.rs @@ -252,7 +252,7 @@ impl BucketRow { } /// Finalize accumulated results into a response. -/// Finalize aggregate results, optionally using a cross-drive +/// Finalize aggregate results using the cross-drive /// [`crate::aggregate::ExtensionMap`] for correct extension key resolution. pub(crate) fn finalize_with_ext_map( accumulators: Vec, @@ -260,7 +260,7 @@ pub(crate) fn finalize_with_ext_map( drives: &[&DriveCompactIndex], options: &FinalizeOptions, total_matched: u64, - ext_map: Option<&super::ExtensionMap>, + ext_map: &super::ExtensionMap, ) -> AggregateResponse { let global_total_bytes = compute_global_bytes(&accumulators); @@ -301,6 +301,7 @@ fn finalize_accumulator( total_matched: u64, total_bytes: u64, drives: &[&DriveCompactIndex], + ext_map: &super::ExtensionMap, _predicates: &[DrilldownPredicate], ) -> AggregateResult { finalize_one( @@ -309,7 +310,7 @@ fn finalize_accumulator( total_bytes, &FinalizeOptions::default(), drives, - None, + ext_map, ) } @@ -337,7 +338,7 @@ fn finalize_one( total_bytes: u64, options: &FinalizeOptions, drives: &[&DriveCompactIndex], - ext_map: Option<&super::ExtensionMap>, + ext_map: &super::ExtensionMap, ) -> AggregateResult { let label = acc.label.clone(); let field = acc.field; @@ -385,25 +386,14 @@ fn finalize_one( buckets, boundaries, .. - } => { - let rows: Vec<_> = buckets - .iter() - .enumerate() - .filter(|(_, stats)| options.include_empty_buckets || stats.count > 0) - .map(|(i, stats)| { - let key = format_range_key(i, &boundaries); - BucketRow::from_stats(key, stats, total_matched, total_bytes) - }) - .collect(); - - AggregateResultData::Buckets { - field: field_name, - rows, - other_count: 0, - total_groups: buckets.len(), - exact: true, - } - } + } => finalize_histogram( + field_name, + &buckets, + &boundaries, + total_matched, + total_bytes, + options, + ), AccumulatorKind::DateHistogram { buckets, .. } => { let rows: Vec<_> = buckets @@ -438,7 +428,14 @@ fn finalize_one( inner, sub_accumulators, .. - } => finalize_rollup(&inner, sub_accumulators, total_matched, total_bytes, drives), + } => finalize_rollup( + &inner, + sub_accumulators, + total_matched, + total_bytes, + drives, + ext_map, + ), AccumulatorKind::Duplicates { inner, sample_spec } => { finalize_duplicates(inner, sample_spec, drives) @@ -448,6 +445,35 @@ fn finalize_one( AggregateResult { label, data } } +/// Finalize a `Histogram` accumulator: format range keys, drop empty +/// buckets unless requested otherwise. +fn finalize_histogram( + field_name: String, + buckets: &[StatsAccumulator], + boundaries: &[u64], + total_matched: u64, + total_bytes: u64, + options: &FinalizeOptions, +) -> AggregateResultData { + let rows: Vec<_> = buckets + .iter() + .enumerate() + .filter(|(_, stats)| options.include_empty_buckets || stats.count > 0) + .map(|(i, stats)| { + let key = format_range_key(i, boundaries); + BucketRow::from_stats(key, stats, total_matched, total_bytes) + }) + .collect(); + + AggregateResultData::Buckets { + field: field_name, + rows, + other_count: 0, + total_groups: buckets.len(), + exact: true, + } +} + /// Finalize a `Terms` accumulator: sort buckets by count, take top-N, attach /// sample rows and drill-down predicates. #[expect( @@ -465,7 +491,7 @@ fn finalize_terms( total_bytes: u64, options: &FinalizeOptions, drives: &[&DriveCompactIndex], - ext_map: Option<&super::ExtensionMap>, + ext_map: &super::ExtensionMap, ) -> AggregateResultData { let total_groups = groups.len(); @@ -536,32 +562,30 @@ fn finalize_terms( /// rollup, attach nested sub-aggregation rows where present. fn finalize_rollup( inner: &super::rollup::RollupAccumulator, - mut sub_accumulators: Option>, + mut sub_accumulators: Option>, total_matched: u64, total_bytes: u64, drives: &[&DriveCompactIndex], + ext_map: &super::ExtensionMap, ) -> AggregateResultData { let mode_str = match inner.mode { super::spec::RollupMode::Drive => "drive".to_owned(), super::spec::RollupMode::Path { depth } => format!("path(depth={depth})"), - super::spec::RollupMode::Ancestor { record_idx } => { - format!("ancestor(record={record_idx})") + super::spec::RollupMode::Ancestor { record_idx, drive } => { + format!("ancestor(record={record_idx}, drive={drive})") } }; let entries = inner.finalize(); let rows: Vec<_> = entries .into_iter() .map(|(key, stats)| { - let key_str = drives.first().map_or_else( - || format!("{key}"), - |drive| super::rollup::resolve_rollup_key(key, inner.mode, drive), - ); + let key_str = super::rollup::resolve_rollup_key(key, inner.mode, drives); let mut row = BucketRow::from_stats(key_str, stats, total_matched, total_bytes); // Attach sub-aggregation from nested sub-accumulator. if let Some(sub_acc) = sub_accumulators.as_mut().and_then(|map| map.remove(&key)) { let sub_result = - finalize_accumulator(sub_acc, total_matched, total_bytes, drives, &[]); + finalize_accumulator(sub_acc, total_matched, total_bytes, drives, ext_map, &[]); row.sub_buckets = sub_result_to_bucket_rows(&sub_result); } @@ -710,31 +734,19 @@ fn format_field( /// Resolve a u64 group key to a display string. /// -/// For `Extension`, the key encodes `(drive_ordinal << 16) | extension_id`. -/// The extension is resolved using the specific drive's intern table. +/// For `Extension`, group keys are canonical cross-drive IDs from the +/// [`super::ExtensionMap`]; the map is the only correct way to resolve +/// them (a raw per-drive intern table would map the same id to +/// different extensions on different drives). fn resolve_group_key( field: Option, key: u64, - drives: &[&DriveCompactIndex], - ext_map: Option<&super::ExtensionMap>, + _drives: &[&DriveCompactIndex], + ext_map: &super::ExtensionMap, ) -> String { use crate::search::field::FieldId; match field { - Some(FieldId::Extension) => { - // When an ExtensionMap is available, group keys are canonical - // IDs that can be resolved directly. - if let Some(map) = ext_map { - return map.resolve(key); - } - // Legacy fallback: raw extension_id, first-drive lookup. - let ext_id = u16::try_from(key).unwrap_or(u16::MAX); - for drive in drives { - if let Some(name) = drive.ext_names.get(usize::from(ext_id)) { - return name.to_string(); - } - } - format!("ext:{ext_id}") - } + Some(FieldId::Extension) => ext_map.resolve(key), Some(FieldId::Drive) => { let ch = char::from(u8::try_from(key).unwrap_or(b'?')); format!("{ch}:") diff --git a/crates/uffs-core/src/aggregate/integration_tests.rs b/crates/uffs-core/src/aggregate/integration_tests.rs index 88c40c339..ec0eaad7f 100644 --- a/crates/uffs-core/src/aggregate/integration_tests.rs +++ b/crates/uffs-core/src/aggregate/integration_tests.rs @@ -37,7 +37,27 @@ const TS_JUN_2024: i64 = 133_633_536_000_000_000; /// /// Totals (files only): 7 files, 17400 bytes logical, 30628 bytes alloc. fn build_agg_test_drive() -> DriveCompactIndex { - let mut idx = MftIndex::new(uffs_mft::platform::DriveLetter::C); + build_drive_with_folder(uffs_mft::platform::DriveLetter::C, "Projects", &[ + ("main.rs", 101, 2000, 4096, TS_JAN_2024), + ("lib.rs", 102, 3000, 4096, TS_JAN_2024), + ("util.rs", 103, 1000, 4096, TS_MAR_2024), + ("README.md", 104, 500, 512, TS_JAN_2024), + ("CHANGELOG.md", 105, 800, 1024, TS_JUN_2024), + ("config.toml", 106, 100, 512, TS_MAR_2024), + ("data.bin", 107, 10000, 16384, TS_JUN_2024), + ]) +} + +/// Build a synthetic single-folder drive: `:\\`. +/// +/// `files` entries are `(name, frs, size, allocated, modified_timestamp)`. +/// The folder directory record uses FRS 100. +fn build_drive_with_folder( + letter: uffs_mft::platform::DriveLetter, + folder: &str, + files: &[(&str, u64, u64, u64, i64)], +) -> DriveCompactIndex { + let mut idx = MftIndex::new(letter); // Root directory. let root_off = idx.add_name("."); @@ -46,28 +66,16 @@ fn build_agg_test_drive() -> DriveCompactIndex { root.first_name.name = IndexNameRef::new(root_off, 1, true, IndexNameRef::NO_EXTENSION); root.first_name.parent_frs = Into::into(ROOT_FRS); - // Projects directory. - let dir_name = "Projects"; - let dir_off = idx.add_name(dir_name); - let dir_ext = idx.intern_extension(dir_name); + // Folder directory. + let dir_off = idx.add_name(folder); + let dir_ext = idx.intern_extension(folder); let dir = idx.get_or_create(100.into()); dir.stdinfo.set_directory(true); dir.stdinfo.flags = 0x10; dir.first_name.name = - IndexNameRef::new(dir_off, uffs_mft::len_to_u16(dir_name.len()), true, dir_ext); + IndexNameRef::new(dir_off, uffs_mft::len_to_u16(folder.len()), true, dir_ext); dir.first_name.parent_frs = Into::into(ROOT_FRS); - // Files: (name, frs, size, allocated, modified_timestamp) - let files: &[(&str, u64, u64, u64, i64)] = &[ - ("main.rs", 101, 2000, 4096, TS_JAN_2024), - ("lib.rs", 102, 3000, 4096, TS_JAN_2024), - ("util.rs", 103, 1000, 4096, TS_MAR_2024), - ("README.md", 104, 500, 512, TS_JAN_2024), - ("CHANGELOG.md", 105, 800, 1024, TS_JUN_2024), - ("config.toml", 106, 100, 512, TS_MAR_2024), - ("data.bin", 107, 10000, 16384, TS_JUN_2024), - ]; - for &(name, frs, size, allocated, modified) in files { let off = idx.add_name(name); let ext = idx.intern_extension(name); @@ -82,7 +90,7 @@ fn build_agg_test_drive() -> DriveCompactIndex { rec.stdinfo.modified = modified; } - let (drive, _, _) = build_compact_index(uffs_mft::platform::DriveLetter::C, &idx); + let (drive, _, _) = build_compact_index(letter, &idx); drive } @@ -592,6 +600,17 @@ fn s2f4_top_folders_preset() { let total_bytes: u64 = rows.iter().map(|r| r.total_bytes).sum(); assert!(total_bytes > 0, "top_folders should report non-zero bytes"); + + // Keys must be real folder paths, not raw record indices. + let keys: Vec<&str> = rows.iter().map(|r| r.key.as_str()).collect(); + assert!( + keys.contains(&"C:\\Projects"), + "top_folders should resolve the folder name, got {keys:?}" + ); + assert!( + !keys.iter().any(|k| k.starts_with("record_")), + "no unresolvable rollup keys expected, got {keys:?}" + ); } else { panic!( "expected Rollup result from top_folders, got {:?}", @@ -600,6 +619,127 @@ fn s2f4_top_folders_preset() { } } +// ── Regression: multi-drive top_folders keys must resolve per drive ── +// +// 2026-07-21, found live via the MCP `top_folders` preset over multiple +// drives: rollup group keys were bare per-drive record indices. The +// cross-drive merge collided buckets from different drives, and finalize +// resolved every key against the FIRST drive's records — producing +// malformed folder names (or `record_N` fallbacks). Keys must carry the +// drive they came from and resolve against that drive's name table. + +#[test] +fn top_folders_multi_drive_keys_resolve_per_drive() { + let drive_c = build_agg_test_drive(); // C:\Projects — 17400 bytes of files + // Same record layout as drive C (identical compact indices) so a bare + // record-index key collides across drives; different names and sizes so + // the collision is observable. + let drive_d = build_drive_with_folder(uffs_mft::platform::DriveLetter::D, "Photos", &[ + ("img_001.jpg", 101, 5000, 8192, TS_JAN_2024), + ("img_002.jpg", 102, 7000, 8192, TS_MAR_2024), + ]); + + let specs = AggregatePreset::TopFolders.expand(); + let output = run_aggregate(&[&drive_c, &drive_d], &specs, &FinalizeOptions::default()).unwrap(); + let result = &output.response.results[0]; + + let AggregateResultData::Rollup { rows, .. } = &result.data else { + panic!( + "expected Rollup result from top_folders, got {:?}", + result.data + ); + }; + let keys: Vec<&str> = rows.iter().map(|r| r.key.as_str()).collect(); + + // Each drive's folder must appear under its own drive letter and name. + let projects = rows + .iter() + .find(|r| r.key == "C:\\Projects") + .unwrap_or_else(|| panic!("missing C:\\Projects bucket, got {keys:?}")); + let photos = rows + .iter() + .find(|r| r.key == "D:\\Photos") + .unwrap_or_else(|| panic!("missing D:\\Photos bucket, got {keys:?}")); + + // No cross-drive merging: each bucket carries only its own drive's bytes. + assert_eq!( + projects.total_bytes, 17_400, + "C:\\Projects must not absorb drive D records" + ); + assert_eq!( + photos.total_bytes, 12_000, + "D:\\Photos must not absorb drive C records" + ); + + // And no unresolvable keys anywhere. + assert!( + !keys.iter().any(|k| k.starts_with("record_")), + "no unresolvable rollup keys expected, got {keys:?}" + ); +} + +// ── Regression: ancestor rollups are scoped to their drive ────── +// +// An ancestor drilldown's record index is per-drive. Before drive +// scoping, the index was interpreted against EVERY scanned drive: +// records on other drives were bucketed against an arbitrary unrelated +// record that happened to sit at that index (or flooded the result +// with one-file self buckets). Records on other drives must be +// skipped entirely — in the top-level rows AND in nested sub-buckets. + +#[test] +fn ancestor_rollup_scoped_to_named_drive() { + let drive_c = build_agg_test_drive(); // C:\Projects — 7 files, 17400 B + let drive_d = build_drive_with_folder(uffs_mft::platform::DriveLetter::D, "Photos", &[ + ("img_001.jpg", 101, 5000, 8192, TS_JAN_2024), + ("img_002.jpg", 102, 7000, 8192, TS_MAR_2024), + ]); + + // Find C's "Projects" record index in the compact index. + let projects_idx = drive_c + .records + .iter() + .position(|rec| rec.name(&drive_c.names) == "Projects") + .expect("Projects record"); + + // Drill into C:\Projects — its children are the 7 files themselves. + let spec = AggregateSpec::new(AggregateKind::Rollup { + mode: RollupMode::Ancestor { + record_idx: uffs_mft::len_to_u32(projects_idx), + drive: uffs_mft::platform::DriveLetter::C, + }, + top: 30, + metrics: vec![BucketMetric::Count, BucketMetric::TotalBytes], + sample: None, + sub: None, + }); + let output = + run_aggregate(&[&drive_c, &drive_d], &[spec], &FinalizeOptions::default()).unwrap(); + + let AggregateResultData::Rollup { rows, mode } = &output.response.results[0].data else { + panic!("expected Rollup result"); + }; + assert_eq!( + *mode, + format!("ancestor(record={projects_idx}, drive=C)"), + "mode label should carry the drive scope" + ); + + // Every bucket must be a C:\ key — drive D records are out of scope. + for row in rows { + assert!( + row.key.starts_with("C:\\"), + "ancestor drilldown into C must not bucket drive D records, got {}", + row.key + ); + } + let total_bytes: u64 = rows.iter().map(|r| r.total_bytes).sum(); + assert_eq!( + total_bytes, 17_400, + "drilldown totals must cover exactly drive C's Projects files" + ); +} + #[test] fn s2f5_cleanup_preset() { let drive = build_agg_test_drive(); diff --git a/crates/uffs-core/src/aggregate/mod.rs b/crates/uffs-core/src/aggregate/mod.rs index 39fa0729e..46746c76a 100644 --- a/crates/uffs-core/src/aggregate/mod.rs +++ b/crates/uffs-core/src/aggregate/mod.rs @@ -253,7 +253,7 @@ pub fn run_aggregate( .map(|(drive_ordinal, drive)| { let ordinal = u8::try_from(drive_ordinal).unwrap_or(u8::MAX); let per_drive_start = std::time::Instant::now(); - let (local, scanned, matched) = scan_drive(drive, &plan, ordinal, Some(&ext_map)); + let (local, scanned, matched) = scan_drive(drive, &plan, ordinal, &ext_map); tracing::debug!( drive = %drive.letter, scanned, @@ -275,14 +275,8 @@ pub fn run_aggregate( let scan_ms = start.elapsed().as_millis(); // 3. Finalize - let response = finalize::finalize_with_ext_map( - merged, - &plan, - drives, - options, - total_matched, - Some(&ext_map), - ); + let response = + finalize::finalize_with_ext_map(merged, &plan, drives, options, total_matched, &ext_map); let total_ms = start.elapsed().as_millis(); tracing::info!( drives = drives.len(), @@ -367,7 +361,7 @@ pub(crate) fn run_aggregate_filtered( } matched += 1; for acc in &mut local { - acc.feed(record, drive, idx, ordinal, Some(&ext_map)); + acc.feed(record, drive, idx, ordinal, &ext_map); } } (local, scanned, matched) @@ -384,14 +378,8 @@ pub(crate) fn run_aggregate_filtered( let t_fin = std::time::Instant::now(); // 3. Finalize. - let response = finalize::finalize_with_ext_map( - merged, - &plan, - drives, - options, - total_matched, - Some(&ext_map), - ); + let response = + finalize::finalize_with_ext_map(merged, &plan, drives, options, total_matched, &ext_map); tracing::info!( drives = drives.len(), records_scanned = total_scanned, @@ -511,7 +499,7 @@ pub fn run_aggregate_with_filters( matched += 1; for acc in &mut local { - acc.feed(record, drive, idx, ordinal, Some(&ext_map)); + acc.feed(record, drive, idx, ordinal, &ext_map); } } (local, scanned, matched) @@ -528,14 +516,8 @@ pub fn run_aggregate_with_filters( let t_fin = std::time::Instant::now(); // 3. Finalize. - let response = finalize::finalize_with_ext_map( - merged, - &plan, - drives, - options, - total_matched, - Some(&ext_map), - ); + let response = + finalize::finalize_with_ext_map(merged, &plan, drives, options, total_matched, &ext_map); tracing::info!( drives = drives.len(), records_scanned = total_scanned, @@ -591,7 +573,7 @@ fn scan_drive( drive: &DriveCompactIndex, plan: &AggregatePlan, drive_ordinal: u8, - ext_map: Option<&ExtensionMap>, + ext_map: &ExtensionMap, ) -> (Vec, u64, u64) { let records = &drive.records; let mut accumulators = plan.create_accumulators(); diff --git a/crates/uffs-core/src/aggregate/parser.rs b/crates/uffs-core/src/aggregate/parser.rs index 4b721d4c0..c9c9424a1 100644 --- a/crates/uffs-core/src/aggregate/parser.rs +++ b/crates/uffs-core/src/aggregate/parser.rs @@ -285,8 +285,16 @@ fn parse_range(rest: &str) -> Result { })) } -/// Parse "path,depth=N,top=N" or "drive,top=N" or "ancestor,record=N" → Rollup -/// spec. +/// Parse a `drive=` option value into a drive letter. +fn parse_drive_letter(val: &str) -> Result { + val.parse() + .map_err(|_ignored| ParseAggSpecError::InvalidDriveLetter { + val: val.to_owned(), + }) +} + +/// Parse "path,depth=N,top=N" or "drive,top=N" or +/// "ancestor,record=N,drive=C" → Rollup spec. /// /// Nested sub-aggregation syntax: `rollup:drive,sub=terms:type` fn parse_rollup(rest: &str) -> Result { @@ -297,6 +305,7 @@ fn parse_rollup(rest: &str) -> Result { let mut metrics = vec![BucketMetric::Count, BucketMetric::TotalBytes]; let mut sample_count: u8 = 0; let mut record_idx: Option = None; + let mut drive: Option = None; let mut sub_spec: Option> = None; for (key, val) in &opts { @@ -313,6 +322,7 @@ fn parse_rollup(rest: &str) -> Result { .map_err(invalid_int("record index", (*val).to_owned()))?, ); } + "drive" => drive = Some(parse_drive_letter(val)?), "metrics" => { metrics.clear(); for metric in val.split('+') { @@ -338,7 +348,11 @@ fn parse_rollup(rest: &str) -> Result { "path" | "folder" | "dir" => RollupMode::Path { depth }, "ancestor" | "drilldown" => { let idx = record_idx.ok_or(ParseAggSpecError::AncestorRequiresRecord)?; - RollupMode::Ancestor { record_idx: idx } + let letter = drive.ok_or(ParseAggSpecError::AncestorRequiresDrive)?; + RollupMode::Ancestor { + record_idx: idx, + drive: letter, + } } _ => { return Err(ParseAggSpecError::UnknownRollupMode { diff --git a/crates/uffs-core/src/aggregate/parser_error.rs b/crates/uffs-core/src/aggregate/parser_error.rs index 2dacc13e7..3fe55461b 100644 --- a/crates/uffs-core/src/aggregate/parser_error.rs +++ b/crates/uffs-core/src/aggregate/parser_error.rs @@ -81,6 +81,17 @@ pub enum ParseAggSpecError { /// `record=` option (also known as `frs=` / `ancestor=`). #[error("rollup:ancestor requires record= option")] AncestorRequiresRecord, + /// `rollup:ancestor` was supplied without the required + /// `drive=` option. The record index is per-drive, so the + /// drilldown must name the drive it belongs to. + #[error("rollup:ancestor requires drive= option")] + AncestorRequiresDrive, + /// The `drive=` option did not parse as a drive letter (`A`–`Z`). + #[error("Invalid drive letter: `{val}`")] + InvalidDriveLetter { + /// The unrecognised drive-letter value. + val: String, + }, /// The rollup mode did not match `path` / `folder` / `dir` / /// `drive` / `ancestor` / `drilldown`. #[error("Unknown rollup mode: `{mode}`. Use 'path', 'drive', or 'ancestor'.")] diff --git a/crates/uffs-core/src/aggregate/parser_tests.rs b/crates/uffs-core/src/aggregate/parser_tests.rs index be51a7590..3d4fdca7d 100644 --- a/crates/uffs-core/src/aggregate/parser_tests.rs +++ b/crates/uffs-core/src/aggregate/parser_tests.rs @@ -223,9 +223,12 @@ fn parse_rollup_dir_alias() { #[test] fn parse_rollup_ancestor_mode() { - let spec = parse_agg_spec("rollup:ancestor,record=42,top=10").unwrap(); + let spec = parse_agg_spec("rollup:ancestor,record=42,drive=C,top=10").unwrap(); if let AggregateKind::Rollup { mode, top, .. } = &spec.kind { - assert_eq!(*mode, RollupMode::Ancestor { record_idx: 42 }); + assert_eq!(*mode, RollupMode::Ancestor { + record_idx: 42, + drive: uffs_mft::platform::DriveLetter::C, + }); assert_eq!(*top, 10); } else { panic!("expected Rollup"); @@ -234,9 +237,12 @@ fn parse_rollup_ancestor_mode() { #[test] fn parse_rollup_ancestor_frs_alias() { - let spec = parse_agg_spec("rollup:ancestor,frs=100").unwrap(); + let spec = parse_agg_spec("rollup:ancestor,frs=100,drive=D").unwrap(); if let AggregateKind::Rollup { mode, .. } = &spec.kind { - assert_eq!(*mode, RollupMode::Ancestor { record_idx: 100 }); + assert_eq!(*mode, RollupMode::Ancestor { + record_idx: 100, + drive: uffs_mft::platform::DriveLetter::D, + }); } else { panic!("expected Rollup"); } @@ -244,15 +250,36 @@ fn parse_rollup_ancestor_frs_alias() { #[test] fn parse_rollup_ancestor_missing_record_errors() { - let err = parse_agg_spec("rollup:ancestor,top=10"); + let err = parse_agg_spec("rollup:ancestor,drive=C,top=10"); assert!(err.is_err(), "ancestor without record= should fail"); } +#[test] +fn parse_rollup_ancestor_missing_drive_errors() { + let err = parse_agg_spec("rollup:ancestor,record=42,top=10").expect_err("must error"); + assert_eq!( + err, + ParseAggSpecError::AncestorRequiresDrive, + "ancestor without drive= should fail — the record index is per-drive" + ); +} + +#[test] +fn parse_rollup_ancestor_invalid_drive_errors() { + let err = parse_agg_spec("rollup:ancestor,record=42,drive=nope").expect_err("must error"); + assert_eq!(err, ParseAggSpecError::InvalidDriveLetter { + val: "nope".to_owned(), + }); +} + #[test] fn parse_rollup_drilldown_alias() { - let spec = parse_agg_spec("rollup:drilldown,record=5").unwrap(); + let spec = parse_agg_spec("rollup:drilldown,record=5,drive=C").unwrap(); if let AggregateKind::Rollup { mode, .. } = &spec.kind { - assert_eq!(*mode, RollupMode::Ancestor { record_idx: 5 }); + assert_eq!(*mode, RollupMode::Ancestor { + record_idx: 5, + drive: uffs_mft::platform::DriveLetter::C, + }); } else { panic!("expected Rollup"); } diff --git a/crates/uffs-core/src/aggregate/rollup.rs b/crates/uffs-core/src/aggregate/rollup.rs index 28cf7e094..9232cfb3b 100644 --- a/crates/uffs-core/src/aggregate/rollup.rs +++ b/crates/uffs-core/src/aggregate/rollup.rs @@ -15,17 +15,25 @@ use crate::compact::{CompactRecord, DriveCompactIndex}; /// A rollup accumulator — groups records by a key derived from /// path ancestry or drive letter. +/// +/// Group keys are drive-qualified: the drive ordinal lives in the high +/// 32 bits and the per-drive record index (or drive-letter byte) in the +/// low 32 bits. Bare record indices would collide across drives when the +/// per-drive accumulators are merged, silently folding one drive's +/// folders into another's (and resolving their names against the wrong +/// drive's name table). #[derive(Debug, Clone)] pub struct RollupAccumulator { - /// Per-group statistics keyed by ancestor record index (or drive ordinal). - pub groups: HashMap, + /// Per-group statistics keyed by drive-qualified group key + /// (see `encode_rollup_key`). + pub groups: HashMap, /// Rollup mode. pub mode: RollupMode, /// Max groups to track. pub top: u16, /// Last computed group key (used by nested rollups to route /// sub-accumulator feeding without recomputing the key). - pub last_key: u32, + pub last_key: u64, } impl RollupAccumulator { @@ -41,19 +49,45 @@ impl RollupAccumulator { } /// Feed a record into the rollup. + /// + /// `drive_ordinal` is the position of `drive` in the aggregation's + /// drive slice; it is encoded into the group key so keys from + /// different drives never collide and resolve against the right + /// drive's name table. + /// + /// Returns `true` when the record was bucketed. `Ancestor` rollups + /// are scoped to the drive named in the spec — records on other + /// drives are skipped (`false`), and callers must not feed nested + /// sub-accumulators for skipped records. #[inline] - pub fn feed(&mut self, record: &CompactRecord, drive: &DriveCompactIndex, idx: usize) { - let key = match self.mode { + pub fn feed( + &mut self, + record: &CompactRecord, + drive: &DriveCompactIndex, + idx: usize, + drive_ordinal: u8, + ) -> bool { + let low = match self.mode { RollupMode::Drive => { u32::from(u8::try_from(u32::from(drive.letter.as_byte())).unwrap_or(b'?')) } RollupMode::Path { depth } => ancestor_at_depth(record, drive, idx, depth), - RollupMode::Ancestor { record_idx } => child_of_ancestor(drive, idx, record_idx), + RollupMode::Ancestor { + record_idx, + drive: target, + } => { + if drive.letter != target { + return false; + } + child_of_ancestor(drive, idx, record_idx) + } }; + let key = encode_rollup_key(drive_ordinal, low); self.last_key = key; let stats = self.groups.entry(key).or_default(); stats.feed_value(record.size, record.allocated); + true } /// The group key computed by the most recent `feed()` call. @@ -62,7 +96,7 @@ impl RollupAccumulator { /// without recomputing the key. #[inline] #[must_use] - pub const fn last_key(&self) -> u32 { + pub const fn last_key(&self) -> u64 { self.last_key } @@ -81,9 +115,10 @@ impl RollupAccumulator { } /// Finalize: sort by total bytes descending, truncate to top-N. - /// Returns (key, stats) pairs. + /// Returns (key, stats) pairs; keys are drive-qualified + /// (see `encode_rollup_key`). #[must_use] - pub fn finalize(&self) -> Vec<(u32, &StatsAccumulator)> { + pub fn finalize(&self) -> Vec<(u64, &StatsAccumulator)> { let mut entries: Vec<_> = self.groups.iter().map(|(&key, val)| (key, val)).collect(); entries.sort_by_key(|entry| core::cmp::Reverse(entry.1.sum)); entries.truncate(usize::from(self.top)); @@ -164,26 +199,54 @@ fn child_of_ancestor(drive: &DriveCompactIndex, idx: usize, ancestor_idx: u32) - uffs_mft::len_to_u32(idx) } -/// Resolve a rollup key (record index) to a display name. +/// Encode a drive-qualified rollup group key: drive ordinal in the high +/// 32 bits, per-drive value (record index or drive-letter byte) in the +/// low 32 bits. +#[inline] +#[must_use] +pub(crate) fn encode_rollup_key(drive_ordinal: u8, low: u32) -> u64 { + (u64::from(drive_ordinal) << 32_u32) | u64::from(low) +} + +/// Split a drive-qualified rollup key back into (drive ordinal, low bits). +#[inline] +#[must_use] +fn decode_rollup_key(key: u64) -> (usize, u32) { + // Both conversions are infallible after the shift/mask; the fallbacks + // exist only to satisfy the no-panic lint policy. + let ordinal = usize::try_from(key >> 32_u32).unwrap_or(usize::MAX); + let low = u32::try_from(key & u64::from(u32::MAX)).unwrap_or(u32::MAX); + (ordinal, low) +} + +/// Resolve a drive-qualified rollup key to a display name. /// -/// For drive rollups, key is the drive letter ordinal. -/// For path/ancestor rollups, key is the record index → look up name. +/// For drive rollups, the low bits are the drive-letter byte. +/// For path/ancestor rollups, the low bits are a record index into the +/// drive selected by the key's ordinal — never any other drive. #[must_use] -pub(crate) fn resolve_rollup_key(key: u32, mode: RollupMode, drive: &DriveCompactIndex) -> String { +pub(crate) fn resolve_rollup_key( + key: u64, + mode: RollupMode, + drives: &[&DriveCompactIndex], +) -> String { + let (ordinal, low) = decode_rollup_key(key); match mode { RollupMode::Drive => { - let ch = char::from(u8::try_from(key).unwrap_or(b'?')); + let ch = char::from(u8::try_from(low).unwrap_or(b'?')); format!("{ch}:") } RollupMode::Path { .. } | RollupMode::Ancestor { .. } => { - let idx = uffs_mft::u32_as_usize(key); - drive.records.get(idx).map_or_else( - || format!("record_{key}"), - |record| { - let name = record.name(&drive.names); - format!("{}:\\{name}", drive.letter) - }, - ) + let idx = uffs_mft::u32_as_usize(low); + drives + .get(ordinal) + .and_then(|drive| { + drive.records.get(idx).map(|record| { + let name = record.name(&drive.names); + format!("{}:\\{name}", drive.letter) + }) + }) + .unwrap_or_else(|| format!("record_{low}")) } } } @@ -213,8 +276,12 @@ mod tests { #[test] fn rollup_accumulator_ancestor_mode() { - let acc = RollupAccumulator::new(RollupMode::Ancestor { record_idx: 42 }, 20); - assert_eq!(acc.mode, RollupMode::Ancestor { record_idx: 42 }); + let ancestor = RollupMode::Ancestor { + record_idx: 42, + drive: uffs_mft::platform::DriveLetter::C, + }; + let acc = RollupAccumulator::new(ancestor, 20); + assert_eq!(acc.mode, ancestor); assert_eq!(acc.top, 20); } @@ -371,8 +438,29 @@ mod tests { #[test] fn resolve_rollup_key_ancestor_mode() { let drive = build_ancestor_test_drive(); - let mode = RollupMode::Ancestor { record_idx: 0 }; - let name = resolve_rollup_key(1, mode, &drive); + let mode = RollupMode::Ancestor { + record_idx: 0, + drive: uffs_mft::platform::DriveLetter::C, + }; + let name = resolve_rollup_key(encode_rollup_key(0, 1), mode, &[&drive]); assert_eq!(name, "C:\\folder_a"); } + + #[test] + fn rollup_key_roundtrip_encodes_drive_ordinal() { + let key = encode_rollup_key(3, 0x00AB_CDEF); + assert_eq!(decode_rollup_key(key), (3, 0x00AB_CDEF)); + // Same record index on different drives must produce distinct keys. + assert_ne!(encode_rollup_key(0, 7), encode_rollup_key(1, 7)); + } + + #[test] + fn resolve_rollup_key_out_of_range_ordinal_falls_back() { + let drive = build_ancestor_test_drive(); + let mode = RollupMode::Path { depth: 1 }; + // Ordinal 5 with only one drive present — must not resolve against + // the wrong drive. + let name = resolve_rollup_key(encode_rollup_key(5, 1), mode, &[&drive]); + assert_eq!(name, "record_1"); + } } diff --git a/crates/uffs-core/src/aggregate/spec.rs b/crates/uffs-core/src/aggregate/spec.rs index 4e3267435..3ef81a907 100644 --- a/crates/uffs-core/src/aggregate/spec.rs +++ b/crates/uffs-core/src/aggregate/spec.rs @@ -151,9 +151,16 @@ pub enum RollupMode { /// whichever direct child of that record they descend from. /// Files *directly* inside the ancestor (i.e. whose parent IS /// the ancestor) use themselves as the group key. + /// + /// `record_idx` is a per-drive record index, so the drilldown is + /// scoped to exactly one drive: records on other drives are skipped + /// (interpreting the index against a different drive's records + /// would bucket by an arbitrary unrelated folder). Ancestor { /// Record index of the ancestor to drill into. record_idx: u32, + /// The drive the ancestor's `record_idx` belongs to. + drive: uffs_mft::platform::DriveLetter, }, } diff --git a/crates/uffs-daemon/src/index/wire_spec.rs b/crates/uffs-daemon/src/index/wire_spec.rs index 4de8a75d1..439ea10e6 100644 --- a/crates/uffs-daemon/src/index/wire_spec.rs +++ b/crates/uffs-daemon/src/index/wire_spec.rs @@ -94,6 +94,11 @@ pub(crate) enum WireSpecError { /// The unrecognised kind identifier as supplied on the wire. kind: String, }, + /// An `ancestor`/`drilldown` rollup arrived without a usable + /// `drive` slot. The rollup's record index is per-drive, so the + /// drilldown must name the drive it belongs to. + #[error("ancestor rollup requires a valid 'drive' letter")] + AncestorDriveMissing, } impl IndexManager { @@ -211,9 +216,16 @@ impl IndexManager { let mode = match mode_str { "drive" => RollupMode::Drive, "ancestor" | "drilldown" => { - // Use interval field as the record index. + // Use interval field as the record index. The + // index is per-drive, so the wire spec must also + // name the drive it belongs to. let record_idx = u32::try_from(ws.interval.unwrap_or(0)).unwrap_or(0); - RollupMode::Ancestor { record_idx } + let drive = ws + .drive + .as_deref() + .and_then(|letter| letter.parse().ok()) + .ok_or(WireSpecError::AncestorDriveMissing)?; + RollupMode::Ancestor { record_idx, drive } } _ => { let depth = u32::try_from(ws.interval.unwrap_or(1)).unwrap_or(1); @@ -481,4 +493,28 @@ mod tests { },); assert_eq!(err.to_string(), "unknown aggregate kind: `bogus-kind`"); } + + #[test] + fn ancestor_rollup_without_drive_errors() { + let mut spec = ws("rollup"); + spec.field = Some("ancestor".to_owned()); + spec.interval = Some(42); + let err = IndexManager::convert_wire_spec(&spec) + .expect_err("ancestor rollup without 'drive' must error"); + assert_eq!(err, WireSpecError::AncestorDriveMissing); + assert_eq!( + err.to_string(), + "ancestor rollup requires a valid 'drive' letter" + ); + } + + #[test] + fn ancestor_rollup_with_drive_converts() { + let mut spec = ws("rollup"); + spec.field = Some("ancestor".to_owned()); + spec.interval = Some(42); + spec.drive = Some("C".to_owned()); + let specs = IndexManager::convert_wire_spec(&spec).expect("valid ancestor rollup"); + assert_eq!(specs.len(), 1); + } } diff --git a/crates/uffs-mcp/src/tools/aggregate.rs b/crates/uffs-mcp/src/tools/aggregate.rs index 86b7c3a7f..161f06f15 100644 --- a/crates/uffs-mcp/src/tools/aggregate.rs +++ b/crates/uffs-mcp/src/tools/aggregate.rs @@ -225,5 +225,6 @@ fn default_agg_spec() -> AggregateSpecWire { sample_desc: None, verify: None, verify_bytes: None, + drive: None, } } diff --git a/crates/uffs-mcp/src/tools/facet_values.rs b/crates/uffs-mcp/src/tools/facet_values.rs index d6ce790a9..33ab28e49 100644 --- a/crates/uffs-mcp/src/tools/facet_values.rs +++ b/crates/uffs-mcp/src/tools/facet_values.rs @@ -85,6 +85,7 @@ pub(crate) async fn run( sample_desc: None, verify: None, verify_bytes: None, + drive: None, }; let mut params = SearchParams {