Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 67 additions & 15 deletions chain/src/txhashset/txhashset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use crate::util::secp::pedersen::{Commitment, RangeProof};
use crate::util::{file, secp_static, zip, StopState};
use crate::SyncState;
use croaring::Bitmap;
use grin_store::pmmr::{clean_files_by_prefix, PMMRBackend};
use grin_store::pmmr::PMMRBackend;
use std::cmp::Ordering;
use std::fs::{self, File};
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -2056,6 +2056,61 @@ impl<'a> Extension<'a> {
}
}

/// Remove other `txhashset_snapshot_*.zip` files in `data_dir`, keeping only

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we shorten this? The key detail here is that cleanup runs before both reuse and creation

/// the snapshot for the header currently being served (`keep`).
///
/// Snapshot zips are multi-GB; retaining more than one wastes disk and can
/// exhaust low-end nodes (see #3647). Cleanup runs on every `zip_read`,
/// including when reusing an existing zip, so stale files do not linger when
/// no new archive is created.
pub fn cleanup_old_txhashset_zips(data_dir: &Path, keep: &Path) -> u32 {
let prefix = format!("{}_", TXHASHSET_ZIP);
let keep_name = keep.file_name();

let entries = match fs::read_dir(data_dir) {
Ok(entries) => entries,
Err(e) => {
debug!(
"cleanup_old_txhashset_zips: cannot read {:?}: {}",
data_dir, e
);
return 0;
}
};

let mut removed = 0u32;
for entry in entries.flatten() {
let path = entry.path();
if !path.is_file() {
continue;
}
if keep_name.is_some() && path.file_name() == keep_name {
continue;
}
let name = match entry.file_name().into_string() {
Ok(n) => n,
Err(_) => continue,
};
// Match `txhashset_snapshot_<hash>.zip` only.
if !name.starts_with(&prefix) || !name.ends_with(".zip") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says .zip, but this matches any text between the prefix and .zip.

Generated snapshot names use a 12-character hex hash. Could we restrict cleanup to that exact format?

continue;
}
match fs::remove_file(&path) {
Ok(()) => {
debug!("cleanup_old_txhashset_zips: removed {:?}", path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zip_read() already logs the total number removed. Could this per file message be trace! or be dropped to avoid duplicate debug output during a rollover?

removed += 1;
}
Err(e) => {
warn!(
"cleanup_old_txhashset_zips: failed to remove {:?}: {}",
path, e
);
}
}
}
removed
}

/// Packages the txhashset data files into a zip and returns a Read to the
/// resulting file
pub fn zip_read(root_dir: String, header: &BlockHeader) -> Result<File, Error> {
Expand All @@ -2064,28 +2119,25 @@ pub fn zip_read(root_dir: String, header: &BlockHeader) -> Result<File, Error> {
let txhashset_path = Path::new(&root_dir).join(TXHASHSET_SUBDIR);
let zip_path = Path::new(&root_dir).join(txhashset_zip);

// Always free disk: keep at most the zip for this header.
let n = cleanup_old_txhashset_zips(Path::new(&root_dir), &zip_path);
if n > 0 {
debug!(
"zip_read: cleaned up {} old txhashset zip file(s) in {:?}",
n,
Path::new(&root_dir)
);
}

// if file exist, just re-use it
let zip_file = File::open(zip_path.clone());
if let Ok(zip) = zip_file {
if let Ok(zip) = File::open(zip_path.clone()) {
debug!(
"zip_read: {} at {}: reusing existing zip file: {:?}",
header.hash(),
header.height,
zip_path
);
return Ok(zip);
} else {
// clean up old zips.
// Theoretically, we only need clean-up those zip files older than STATE_SYNC_THRESHOLD.
// But practically, these zip files are not small ones, we just keep the zips in last 24 hours
let data_dir = Path::new(&root_dir);
let pattern = format!("{}_", TXHASHSET_ZIP);
if let Ok(n) = clean_files_by_prefix(data_dir, &pattern, 24 * 60 * 60) {
debug!(
"{} zip files have been clean up in folder: {:?}",
n, data_dir
);
}
}

// otherwise, create the zip archive
Expand Down
34 changes: 34 additions & 0 deletions chain/tests/test_txhashset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,37 @@ fn test_unexpected_zip() {
// Cleanup chain directory
clean_output_dir(&db_root);
}

#[test]
fn test_cleanup_old_txhashset_zips() {
let db_root = Path::new(".grin_txhashset_zip_cleanup");
let _ = fs::remove_dir_all(db_root);
fs::create_dir_all(db_root).unwrap();

let keep = db_root.join("txhashset_snapshot_keepme.zip");
let old_a = db_root.join("txhashset_snapshot_aaaa.zip");
let old_b = db_root.join("txhashset_snapshot_bbbb.zip");
let unrelated = db_root.join("other_file.bin");
let not_zip = db_root.join("txhashset_snapshot_cccc.txt");

File::create(&keep).unwrap();
File::create(&old_a).unwrap();
File::create(&old_b).unwrap();
File::create(&unrelated).unwrap();
File::create(&not_zip).unwrap();

let removed = txhashset::cleanup_old_txhashset_zips(db_root, &keep);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also exercise zip_read() with the current archive already present and one stale archive beside it? The original regression was specifically that the reuse path skipped cleanup, while this test only covers the helper directly.

assert_eq!(removed, 2);
assert!(keep.exists());
assert!(!old_a.exists());
assert!(!old_b.exists());
assert!(unrelated.exists());
assert!(not_zip.exists());

// Second pass: only the keep file remains matching the pattern.
let removed_again = txhashset::cleanup_old_txhashset_zips(db_root, &keep);
assert_eq!(removed_again, 0);
assert!(keep.exists());

let _ = fs::remove_dir_all(db_root);
}
Loading