Skip to content

Commit ba2ec54

Browse files
[client] fix flaky test connectivity (#12729)
test_connectivity flakes on teardown: TestNet::drop tears down tokio before the storage backend's background threads release their files, so temp.close() races a DirectoryNotEmpty. Retry fs::remove_dir_all a few times instead of failing on it. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 parent c8a200a commit ba2ec54

2 files changed

Lines changed: 35 additions & 3 deletions

File tree

prdoc/pr_12729.prdoc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
title: '[client] fix flaky test connectivity'
2+
doc:
3+
- audience: Runtime Dev
4+
description: 'test_connectivity flakes on teardown: TestNet::drop tears down tokio
5+
before the storage backend''s background threads release their files, so temp.close()
6+
races a DirectoryNotEmpty. Retry fs::remove_dir_all a few times instead of failing
7+
on it.'
8+
crates: []

substrate/client/service/test/src/lib.rs

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ use sc_service::{
3939
use sc_transaction_pool_api::TransactionPool;
4040
use sp_blockchain::HeaderBackend;
4141
use sp_runtime::traits::Block as BlockT;
42-
use std::{iter, net::Ipv4Addr, pin::Pin, sync::Arc, task::Context, time::Duration};
42+
use std::{iter, net::Ipv4Addr, pin::Pin, sync::Arc, task::Context, thread, time::Duration};
4343
use tempfile::TempDir;
4444
use tokio::{runtime::Runtime, time};
4545

@@ -364,6 +364,30 @@ fn tempdir_with_prefix(prefix: &str) -> TempDir {
364364
.expect("Error creating test dir")
365365
}
366366

367+
/// Removes `temp`'s directory, retrying on failure.
368+
///
369+
/// A node's storage backend (e.g. RocksDB) runs background compaction/WAL threads outside the
370+
/// tokio runtime, so they can still hold file handles open for a beat after `TestNet`'s `Drop`
371+
/// (which tears down the runtime) has already returned. Retrying absorbs that race instead of
372+
/// failing the test on it.
373+
fn close_tempdir_retrying(temp: TempDir) {
374+
const MAX_ATTEMPTS: u32 = 10;
375+
const RETRY_DELAY: Duration = Duration::from_millis(200);
376+
377+
for attempt in 1..=MAX_ATTEMPTS {
378+
match std::fs::remove_dir_all(temp.path()) {
379+
Ok(()) => return,
380+
Err(err) if attempt < MAX_ATTEMPTS => {
381+
debug!(
382+
"Removing temp dir failed (attempt {attempt}/{MAX_ATTEMPTS}), retrying: {err}"
383+
);
384+
thread::sleep(RETRY_DELAY);
385+
},
386+
Err(err) => panic!("Error removing temp dir after {MAX_ATTEMPTS} attempts: {err}"),
387+
}
388+
}
389+
}
390+
367391
pub fn connectivity<E, Fb, F>(spec: GenericChainSpec<E>, full_builder: Fb)
368392
where
369393
E: ChainSpecExtension + Clone + 'static + Send + Sync,
@@ -402,7 +426,7 @@ where
402426
});
403427
};
404428

405-
temp.close().expect("Error removing temp dir");
429+
close_tempdir_retrying(temp);
406430
}
407431
{
408432
let temp = tempdir_with_prefix("substrate-connectivity-test");
@@ -436,7 +460,7 @@ where
436460
connected == expected_full_connections
437461
});
438462
}
439-
temp.close().expect("Error removing temp dir");
463+
close_tempdir_retrying(temp);
440464
}
441465
}
442466

0 commit comments

Comments
 (0)