From d78a1086025540245a7feea92c4820b6c50e8249 Mon Sep 17 00:00:00 2001 From: grumbach Date: Tue, 15 Sep 2026 18:46:31 +0900 Subject: [PATCH 1/3] fix(storage): stop counting a peer that disconnects mid-tally as unreported `tally_peers` lists the connected peers and then reads each one's user agent. saorsa-core drops a peer's agent when its last channel closes, so a peer that disconnects between the two reads has no agent, and the tally counted it as a node running a build from before the migration signal. On a 990-node testnet this put a departing client in `peers_unreported` on one tick in 21,402, which is exactly the count the release gate for removing the LMDB store reads. A peer with no agent that is no longer connected is now skipped: it is not a peer this node can see. A peer that is still connected with no agent recorded keeps its place in the unreported bucket, so the count stays conservative with or without the saorsa-core change that records the agent under the peer's connection entry. --- src/storage/migration_signal.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/storage/migration_signal.rs b/src/storage/migration_signal.rs index e698561b..24a0ffde 100644 --- a/src/storage/migration_signal.rs +++ b/src/storage/migration_signal.rs @@ -332,10 +332,14 @@ pub async fn tally_peers(p2p: &Arc) -> PeerTally { let transport = p2p.transport(); let observer = p2p.peer_id().to_hex(); for peer in transport.connected_peers().await { - // No agent recorded is not the same as a peer that reported nothing, but it is - // just as far from evidence of completion, so it lands in the same bucket rather - // than being skipped. let agent = transport.peer_user_agent(&peer).await; + // A peer that disconnected after the list above was taken has no agent any more. It is + // not a peer this node can see, so it is skipped: counting it put a departing client in + // the unreported bucket on a testnet. A peer still connected with no agent recorded is + // not evidence of completion either, so that one stays in the unreported bucket. + if agent.is_none() && !transport.is_peer_connected(&peer).await { + continue; + } let state = agent .as_deref() .map_or(PeerMigrationState::Unreported, peer_state); From f50e6a497ee41871eeda2cd51f88a0b3f9d1e969 Mon Sep 17 00:00:00 2001 From: grumbach Date: Thu, 24 Sep 2026 16:52:54 +0900 Subject: [PATCH 2/3] fix(storage): read a peer's agent again when it reconnects mid-tally saorsa-core records a peer's user agent and removes it under the same lock as the peer's connection entry, so a connected peer always has an agent. When `tally_peers` reads no agent and then finds the peer connected, the peer disconnected and came back between the two reads. The tally counted it as unreported from the first, stale read, even when the peer had announced a finished store or was a client. That is the same false `unreported` reading the previous commit removes for a departing peer. The agent is now read a second time in that case and the peer is classified from what it announced. If the second read still finds no agent, the peer stays in the unreported bucket, so a connected peer is never dropped from the count. --- src/storage/migration_signal.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/storage/migration_signal.rs b/src/storage/migration_signal.rs index 24a0ffde..6ae6bc7e 100644 --- a/src/storage/migration_signal.rs +++ b/src/storage/migration_signal.rs @@ -332,13 +332,21 @@ pub async fn tally_peers(p2p: &Arc) -> PeerTally { let transport = p2p.transport(); let observer = p2p.peer_id().to_hex(); for peer in transport.connected_peers().await { - let agent = transport.peer_user_agent(&peer).await; - // A peer that disconnected after the list above was taken has no agent any more. It is - // not a peer this node can see, so it is skipped: counting it put a departing client in - // the unreported bucket on a testnet. A peer still connected with no agent recorded is - // not evidence of completion either, so that one stays in the unreported bucket. - if agent.is_none() && !transport.is_peer_connected(&peer).await { - continue; + let mut agent = transport.peer_user_agent(&peer).await; + if agent.is_none() { + // saorsa-core records a peer's agent and drops it together with the peer's + // connection entry, so no agent means the peer was not connected when it was read: + // it disconnected after the list above was taken. A peer that is still gone is not + // a peer this node can see, so it is skipped. Counting it put a departing client in + // the unreported bucket on a testnet. + if !transport.is_peer_connected(&peer).await { + continue; + } + // It came back between the two reads, so the first one is stale and would put a + // peer that may have finished in the unreported bucket. Read what it announced this + // time. If that is still nothing, it stays in the unreported bucket: no agent is not + // evidence of completion. + agent = transport.peer_user_agent(&peer).await; } let state = agent .as_deref() From 8218e8dfafeafc2495bf014babe795fcb3f066da Mon Sep 17 00:00:00 2001 From: grumbach Date: Fri, 2 Oct 2026 13:19:42 +0900 Subject: [PATCH 3/3] test(storage): read settled free space in the btrfs reclaim test retiring_the_legacy_environment_returns_its_bytes_to_the_filesystem fails intermittently on the btrfs CI job, on main and on unrelated branches, always at the recovery check with about 26.3 MB recovered of a 54.1 MB environment. The space does come back. The peak was read before btrfs had settled its accounting, and the end could be read while the retired environment was still being deleted. The free space btrfs reports can lag what it has allocated and freed until its transaction commits, every 30 seconds by default, and the fsync after each chunk file does not commit it. Read straight after the copy, the peak missed whatever part of the file store was not committed yet. Passing CI runs read a peak cost of 79.5 to 99.6 MB, 7 to 27 MB short of the 106,647,552 bytes it settles to on a test machine, and the failing runs fit a 28 MB shortfall: once it passes half the environment (27 MB), the recovery measured from it comes up short however much space comes back. Every free-space reading on Linux now follows a syncfs(2) of the volume, which on btrfs commits the running transaction. Reading promptly exposed the second problem. Retirement renames the environment to a tombstone before a detached thread deletes it, so the old name is gone before anything is deleted, and a reading taken while the tombstone was still being deleted saw only part of the environment back: enough to pass the recovery check, with the one-copy check passing by as little as 0.6 MB. The test now also waits for the tombstone to go, for up to 80 seconds, which is what the old wait and the space poll after it gave the deletion between them. Production code is unchanged. Measured on a fresh 3 GiB loop image per run, made the way CI makes it: - Unchanged test, btrfs, idle machine: 22 of 22 passed, every one reading a peak cost of 88,555,520 bytes, as 20 of 32 passing CI runs did. In paired runs the commit adds exactly 18,092,032 bytes to that peak cost, making it 106,647,552. - Unchanged test, btrfs, with a disk writer and busy CPUs alongside it, standing in for a runner that has just finished a build: 2 of 6 failed at the same assertion as on CI, recovering 25,382,912 and 26,562,560 bytes of the 54,145,024-byte environment. With the disk writer alone, 6 of 6 passed. - This change, btrfs: 86 of 86 passed (50 as CI runs them, 10 with the start delayed by 0 to 27 seconds, 26 with readings logged, 6 of those under the same load), each with the same numbers: peak cost 106,647,552, end cost 52,428,800 (the payload), recovered 54,218,752 (all of the environment). Under load, the reading without the commit was as much as 29,360,128 bytes short at the peak in those same runs. - This change, ext4 and XFS: 13 of 13 each, with the same numbers as CI on ext4 and within 20,480 bytes of them on XFS. The commit moves no reading on ext4 and one by 28,672 bytes on XFS. - With the environment's data file held open across retirement, so its blocks stay allocated, this change still fails on btrfs, ext4 and XFS. --- tests/migration_reclaims_disk.rs | 68 +++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/tests/migration_reclaims_disk.rs b/tests/migration_reclaims_disk.rs index 55cf07f3..0bd76c64 100644 --- a/tests/migration_reclaims_disk.rs +++ b/tests/migration_reclaims_disk.rs @@ -24,7 +24,11 @@ )] use ant_node::storage::migration::{MigrationPhase, MIN_RETIRE_DELAY_HOURS}; -use ant_node::storage::{ChunkStore, ChunkStoreConfig, LmdbStorage, LmdbStorageConfig}; +use ant_node::storage::{ + ChunkStore, ChunkStoreConfig, LmdbStorage, LmdbStorageConfig, LEGACY_ENV_DIR, +}; +#[cfg(target_os = "linux")] +use std::os::fd::AsRawFd; use std::path::Path; use std::sync::Arc; use tempfile::TempDir; @@ -86,10 +90,55 @@ fn walk(path: &Path, size: &dyn Fn(&std::fs::Metadata) -> u64) -> u64 { /// disappearing proves nothing: unlink a file that something still holds open and every /// name is gone while every block is still spoken for, which is a fair description of the /// bug that started all this. +/// +/// On Linux, read only once the filesystem has committed what it has been given. The free +/// space btrfs reports can lag what it has allocated and freed until its transaction +/// commits, every 30 seconds by default, and the fsync after each chunk file does not +/// commit it. Read straight after the copy, the peak has missed up to 28 MB of the file +/// store, and the recovery measured from it then came up short however much space came +/// back. `syncfs` is Linux only, so other targets read straight away, as before. fn free_space(path: &Path) -> u64 { + #[cfg(target_os = "linux")] + { + let committed = commit_filesystem(path); + assert!( + committed.is_ok(), + "could not commit the filesystem before reading it: {committed:?}" + ); + } fs2::available_space(path).expect("the filesystem should report its free space") } +/// `syncfs(2)` on the filesystem holding `path`. It returns once that filesystem has +/// written out its pending changes, which on btrfs means once the running transaction +/// has committed. +#[cfg(target_os = "linux")] +fn commit_filesystem(path: &Path) -> std::io::Result<()> { + let dir = std::fs::File::open(path)?; + // SAFETY: `dir` is open for the whole call, so its descriptor is valid, and `syncfs` + // only reads the descriptor and keeps nothing. + #[allow(clippy::undocumented_unsafe_blocks, unsafe_code)] + let synced = unsafe { libc::syncfs(dir.as_raw_fd()) }; + if synced == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +/// What is left under `root` of the legacy environment: the environment itself, and any +/// tombstone retirement renamed it to. +fn legacy_entries(root: &Path) -> std::io::Result> { + let mut left = Vec::new(); + for entry in std::fs::read_dir(root)? { + let name = entry?.file_name().to_string_lossy().into_owned(); + if name.starts_with(LEGACY_ENV_DIR) { + left.push(name); + } + } + Ok(left) +} + /// Deterministic content for chunk `n`, filled so it does not compress to nothing. /// /// `n` goes in verbatim at the front rather than being folded into the fill, because a @@ -250,16 +299,23 @@ async fn retiring_the_legacy_environment_returns_its_bytes_to_the_filesystem() { assert_eq!(store.migration_phase(), MigrationPhase::FilesOnly); assert!(freed > 0, "retirement reported no bytes freed"); - // The deletion runs on a detached thread so the node can serve while it happens. - for _ in 0..400 { - if !environment.exists() && allocated_bytes(&root) < environment_blocks + payload { + // The deletion runs on a detached thread so the node can serve while it happens. The + // thread renames the environment to a tombstone first and removes the tombstone last, + // so the old name is gone before anything is deleted and only the tombstone going says + // the deletion is over. Space read before then comes back in pieces: on btrfs, a + // reading taken mid-delete has seen half the environment back. The wait is up to 80 + // seconds, as long as the old wait and the space poll after it gave the deletion + // between them, which leaves room for the thread to retry after 10, 30 and 60 seconds. + for _ in 0..1600 { + if legacy_entries(&root).is_ok_and(|left| left.is_empty()) { break; } tokio::time::sleep(std::time::Duration::from_millis(50)).await; } + let left = legacy_entries(&root); assert!( - !environment.exists(), - "the environment directory is still on disk" + left.as_ref().is_ok_and(Vec::is_empty), + "the environment is still on disk: {left:?}" ); // Dropping the store closes every handle. A file that is unlinked while something