gsadmin/rustfs
1
0
Fork 0
mirror of https://github.com/rustfs/rustfs.git synced 2026-07-26 08:18:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(scanner,data-usage): fix add() logic inversion and usize underflow in reduce_children_of (#3142)

Browse Source 
* fix(scanner,data-usage): fix add() logic inversion and usize underflow in reduce_children_of

Bug #1: add() had inverted logic — returned early when children were present,
making the recursive child traversal dead code. Leaf compaction path was
completely non-functional. Fixed by recursing into children when present and
collecting leaves only at leaf nodes.

Bug #2: reduce_children_of used  which underflows in debug
(panic) or release (wraps to usize::MAX, compacts entire cache). Fixed with
saturating_sub.

Both bugs existed identically in crates/scanner and crates/data-usage.

Added 5 tests covering add() subtree traversal, edge cases, and
reduce_children_of compaction behavior.

Closes rustfs/backlog#652

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: collect internal nodes as compaction candidates, not leaves

The previous fix for add() collected leaf nodes (children empty) as
compaction candidates. This is incorrect: total_children_rec returns 0
for leaves, so compacting them never decrements  — the
compaction loop makes no progress.

Collect internal nodes (children non-empty) instead. Compacting an
internal node removes its subtree via delete_recursive, which actually
reduces the total child count. Leaf nodes are skipped since they have
no children to remove.

Updated tests to match the corrected semantics.

* refactor: rename leaves→candidates, add data-usage regression tests

- Rename misleading  variable/parameter to  in
  add() and reduce_children_of() for both crates
- Add 4 regression tests in crates/data-usage covering add() candidate
  selection and reduce_children_of() compaction + saturating_sub

* fix: address data usage compaction review feedback

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: houseme <housemecn@gmail.com>
This commit is contained in:
安正超
2026-06-01 05:52:53 +08:00
committed by GitHub
parent 315c2e33f0
commit a14657f517
2 changed files with 392 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/data-usage/src/data_usage.rs
+179 -22
View File
@@ -688,15 +688,14 @@ impl DataUsageCache {
return;
}
let mut leaves = Vec::new();
let mut candidates = Vec::new();
let mut remove = total - limit;
add(self, path, &mut leaves);
leaves.sort_by_key(|a| a.objects);
add(self, path, &mut candidates);
candidates.sort_by_key(|a| a.objects);
while remove > 0 && !leaves.is_empty() {
let Some(e) = leaves.first() else {
break;
};
let mut candidate_index = 0;
while remove > 0 && candidate_index < candidates.len() {
let e = &candidates[candidate_index];
let candidate = e.path.clone();
if candidate == *path && !compact_self {
break;
@@ -705,7 +704,7 @@ impl DataUsageCache {
let mut flat = match self.size_recursive(&candidate.key()) {
Some(flat) => flat,
None => {
leaves.remove(0);
candidate_index += 1;
continue;
}
};
@@ -714,8 +713,8 @@ impl DataUsageCache {
self.delete_recursive(&candidate);
self.replace_hashed(&candidate, &None, &flat);
remove -= removing;
leaves.remove(0);
remove = remove.saturating_sub(removing);
candidate_index += 1;
}
}
@@ -869,23 +868,24 @@ struct Inner {
path: DataUsageHash,
}
fn add(data_usage_cache: &DataUsageCache, path: &DataUsageHash, leaves: &mut Vec<Inner>) {
fn add(data_usage_cache: &DataUsageCache, path: &DataUsageHash, candidates: &mut Vec<Inner>) -> usize {
let e = match data_usage_cache.cache.get(&path.key()) {
Some(e) => e,
None => return,
None => return 0,
};
if !e.children.is_empty() {
return;
}
let sz = data_usage_cache.size_recursive(&path.key()).unwrap_or_default();
leaves.push(Inner {
objects: sz.objects,
path: path.clone(),
});
let mut objects = e.objects;
for ch in e.children.iter() {
add(data_usage_cache, &DataUsageHash(ch.clone()), leaves);
objects += add(data_usage_cache, &DataUsageHash(ch.clone()), candidates);
}
// Collect internal nodes (with children) as compaction candidates.
// Leaf nodes have no children to remove, so compacting them is a no-op.
if !e.children.is_empty() {
candidates.push(Inner {
objects,
path: path.clone(),
});
}
objects
}
fn mark(duc: &DataUsageCache, entry: &DataUsageEntry, found: &mut HashSet<String>) {
@@ -1361,4 +1361,161 @@ mod tests {
assert_eq!(child_entry.size, 30);
assert_eq!(child_entry.objects, 3);
}
// --- Tests for `add` and `reduce_children_of` (bug fixes) ---
/// Build a small tree: root -> child1 (leaf), child2 -> grandchild (leaf).
fn build_test_tree() -> (DataUsageCache, DataUsageHash) {
let root = hash_path("bucket");
let c1 = hash_path("bucket/a");
let c2 = hash_path("bucket/b");
let gc = hash_path("bucket/b/c");
let mut cache = DataUsageCache::default();
cache.replace_hashed(&root, &None, &DataUsageEntry::default());
cache.replace_hashed(
&c1,
&Some(root.clone()),
&DataUsageEntry {
objects: 1,
size: 10,
..Default::default()
},
);
cache.replace_hashed(
&c2,
&Some(root.clone()),
&DataUsageEntry {
objects: 2,
size: 20,
..Default::default()
},
);
cache.replace_hashed(
&gc,
&Some(c2.clone()),
&DataUsageEntry {
objects: 3,
size: 30,
..Default::default()
},
);
(cache, root)
}
fn build_underflow_test_tree() -> (DataUsageCache, DataUsageHash) {
let root = hash_path("bucket");
let small = hash_path("bucket/small");
let small_a = hash_path("bucket/small/a");
let small_b = hash_path("bucket/small/b");
let large = hash_path("bucket/large");
let large_a = hash_path("bucket/large/a");
let large_b = hash_path("bucket/large/b");
let mut cache = DataUsageCache::default();
cache.replace_hashed(
&root,
&None,
&DataUsageEntry {
objects: 100,
..Default::default()
},
);
cache.replace_hashed(&small, &Some(root.clone()), &DataUsageEntry::default());
cache.replace_hashed(
&small_a,
&Some(small.clone()),
&DataUsageEntry {
objects: 1,
..Default::default()
},
);
cache.replace_hashed(
&small_b,
&Some(small.clone()),
&DataUsageEntry {
objects: 1,
..Default::default()
},
);
cache.replace_hashed(&large, &Some(root.clone()), &DataUsageEntry::default());
cache.replace_hashed(
&large_a,
&Some(large.clone()),
&DataUsageEntry {
objects: 10,
..Default::default()
},
);
cache.replace_hashed(
&large_b,
&Some(large.clone()),
&DataUsageEntry {
objects: 10,
..Default::default()
},
);
(cache, root)
}
#[test]
fn test_add_collects_internal_nodes_as_compaction_candidates() {
let (cache, root) = build_test_tree();
let mut candidates = Vec::new();
add(&cache, &root, &mut candidates);
let mut paths: Vec<String> = candidates.iter().map(|l| l.path.key()).collect();
paths.sort();
assert_eq!(paths.len(), 2, "add() should find internal nodes with children");
assert!(paths.contains(&hash_path("bucket").key()));
assert!(paths.contains(&hash_path("bucket/b").key()));
}
#[test]
fn test_add_skips_leaf_node() {
let mut cache = DataUsageCache::default();
let h = hash_path("single-leaf");
cache.replace_hashed(
&h,
&None,
&DataUsageEntry {
objects: 5,
size: 50,
..Default::default()
},
);
let mut candidates = Vec::new();
add(&cache, &h, &mut candidates);
assert!(candidates.is_empty(), "leaf node should not be a compaction candidate");
}
#[test]
fn test_reduce_children_of_compacts_internal_node() {
let (mut cache, root) = build_test_tree();
cache.reduce_children_of(&root, 2, false);
let entry_c2 = cache.find("bucket/b").unwrap();
assert!(entry_c2.compacted, "internal node 'bucket/b' should be compacted");
let entry_c1 = cache.find("bucket/a").unwrap();
assert!(!entry_c1.compacted, "leaf 'bucket/a' should not be compacted");
assert!(cache.find("bucket/b/c").is_none(), "grandchild should be removed");
}
#[test]
fn test_reduce_children_of_usize_underflow_saturates() {
let (mut cache, root) = build_underflow_test_tree();
// total children=6, limit=5, remove=1. The smallest candidate removes
// two descendants, so plain subtraction would underflow and compact the
// next candidate too.
cache.reduce_children_of(&root, 5, false);
assert!(cache.find("bucket/small").is_some_and(|entry| entry.compacted));
assert!(cache.find("bucket/small/a").is_none());
assert!(cache.find("bucket/small/b").is_none());
assert!(cache.find("bucket/large").is_some_and(|entry| !entry.compacted));
assert!(cache.find("bucket/large/a").is_some());
assert!(cache.find("bucket/large/b").is_some());
}
}