From e62c4e23822957812c3eeb9a41684bba261e301b Mon Sep 17 00:00:00 2001 From: jonaswre Date: Sat, 1 Aug 2026 22:32:17 +0000 Subject: [PATCH 1/3] fix(test): un-ignore four flaky tests; fix the content_status race Four of the five tests carrying #[ignore = "flaky"] are now enabled. Three of them were not flaky at all any more, and the fourth had a real race with a measurable cause. Measured first, before changing anything -- 5 runs of all five ignored tests: connect_via_relay_becomes_direct_and_sends_direct 5/5 pass sync_restart_node 5/5 pass sync_big 5/5 pass sync_full_basic 4/5 FAIL test_roundtrip_bytes_small 5/5 FAIL The three that pass were left ignored long after whatever made them flaky stopped happening. Nothing re-checks an #[ignore], so they simply stayed off. sync_full_basic passes in isolation and fails only alongside other tests. The failure is not a timeout: Event didn't match any matcher: InsertRemote { ..., content_status: Incomplete } Its matchers required `content_status: ContentStatus::Missing`. But content_status is evaluated when the event is CONVERTED, not when the entry synced -- the same timing-dependent field behind the double-count fixed in #20 -- so whether a download has started by then is a race. Measured which values actually occur, over 20 runs under full CPU load, by accepting any status and logging it: peer0/from-peer1 Missing 20/20 peer2/hash0 Incomplete 12, Missing 8 peer2/hash1 Missing 13, Incomplete 7 Complete never observed So "not yet complete" is a real assertion that holds, while the Missing/Incomplete split is pure timing. Match both. Applied to all six sites, including sync_restart_node's three -- those pass today but carry the identical latent race. Verified: the four run together 12 times under full CPU saturation with 0 failures, in the configuration that previously failed sync_full_basic 4 times in 5. Full suites green with nothing skipped -- krikos-docs 102/102, krikos 155/155. test_roundtrip_bytes_small stays ignored and needs a decision, not a fix: it fails 5/5, so it is broken rather than flaky and has never given signal. It asserts `expected.addr() == actual.addr()`, i.e. that get_bytes hands back the same allocation add_bytes was given. That is an implementation detail the store does not guarantee, which the ignore reason itself concedes ("I need a reliable way to keep the handle alive"). Co-Authored-By: Claude Opus 5 --- krikos/src/endpoint/tests.rs | 1 - protocols/krikos-docs/tests/sync.rs | 22 +++++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/krikos/src/endpoint/tests.rs b/krikos/src/endpoint/tests.rs index 84d0ed977e4..769ad622ce2 100644 --- a/krikos/src/endpoint/tests.rs +++ b/krikos/src/endpoint/tests.rs @@ -1865,7 +1865,6 @@ async fn test_bind_addr_prefix_len_0_not_default() -> Result { Ok(()) } -#[ignore = "flaky"] #[tokio::test] #[traced_test] async fn connect_via_relay_becomes_direct_and_sends_direct() -> Result { diff --git a/protocols/krikos-docs/tests/sync.rs b/protocols/krikos-docs/tests/sync.rs index 853345ee9b7..35ab8989ff5 100644 --- a/protocols/krikos-docs/tests/sync.rs +++ b/protocols/krikos-docs/tests/sync.rs @@ -253,7 +253,6 @@ async fn sync_gossip_bulk() -> Result<()> { /// This tests basic sync and gossip with 3 peers. #[tokio::test] #[traced_test] -#[ignore = "flaky"] async fn sync_full_basic() -> testresult::TestResult<()> { let mut rng = test_rng(b"sync_full_basic"); let mut nodes = spawn_nodes(2, &mut rng).await?; @@ -334,6 +333,13 @@ async fn sync_full_basic() -> testresult::TestResult<()> { ) .await; + // `content_status` is evaluated when the event is CONVERTED, not when the + // entry synced, so whether a download has started by then is a race. These + // matchers accepted only `Missing` and broke whenever it read `Incomplete`. + // Measured over 20 runs under CPU load: peer2's two InsertRemote matchers + // saw Incomplete 12/20 and 7/20 respectively; `Complete` never appeared. + // So "not yet complete" is a real assertion that holds, while the + // Missing/Incomplete split is pure timing. Match both. // peer0: assert events for entry received via gossip info!("peer0: wait for 2 events (gossip'ed entry from peer1)"); assert_next( @@ -341,7 +347,7 @@ async fn sync_full_basic() -> testresult::TestResult<()> { TIMEOUT, vec![ Box::new( - move |e| matches!(e, LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing, .. } if *from == peer1), + move |e| matches!(e, LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing | ContentStatus::Incomplete, .. } if *from == peer1), ), Box::new(move |e| matches!(e, LiveEvent::ContentReady { hash } if *hash == hash1)), ], @@ -375,10 +381,10 @@ async fn sync_full_basic() -> testresult::TestResult<()> { Box::new(move |e| match_sync_finished(e, peer1)), // 2 InsertRemote events Box::new( - move |e| matches!(e, LiveEvent::InsertRemote { entry, content_status: ContentStatus::Missing, .. } if entry.content_hash() == hash0), + move |e| matches!(e, LiveEvent::InsertRemote { entry, content_status: ContentStatus::Missing | ContentStatus::Incomplete, .. } if entry.content_hash() == hash0), ), Box::new( - move |e| matches!(e, LiveEvent::InsertRemote { entry, content_status: ContentStatus::Missing, .. } if entry.content_hash() == hash1), + move |e| matches!(e, LiveEvent::InsertRemote { entry, content_status: ContentStatus::Missing | ContentStatus::Incomplete, .. } if entry.content_hash() == hash1), ), // 2 ContentReady events Box::new(move |e| matches!(e, LiveEvent::ContentReady { hash } if *hash == hash0)), @@ -607,7 +613,6 @@ async fn test_sync_via_relay() -> Result<()> { #[tokio::test] #[traced_test] -#[ignore = "flaky"] #[cfg(feature = "fs-store")] async fn sync_restart_node() -> Result<()> { use crate::util::endpoint; @@ -656,7 +661,7 @@ async fn sync_restart_node() -> Result<()> { vec![ match_event!(LiveEvent::NeighborUp(n) if *n == id2), match_event!(LiveEvent::SyncFinished(e) if e.peer == id2 && e.result.is_ok()), - match_event!(LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing, .. } if *from == id2), + match_event!(LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing | ContentStatus::Incomplete, .. } if *from == id2), match_event!(LiveEvent::ContentReady { hash } if *hash == hash_a), match_event!(LiveEvent::PendingContentReady), ], @@ -701,7 +706,7 @@ async fn sync_restart_node() -> Result<()> { vec![ match_event!(LiveEvent::NeighborUp(n) if *n== id2), match_event!(LiveEvent::SyncFinished(e) if e.peer == id2 && e.result.is_ok()), - match_event!(LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing, .. } if *from == id2), + match_event!(LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing | ContentStatus::Incomplete, .. } if *from == id2), match_event!(LiveEvent::ContentReady { hash } if *hash == hash_b), ], vec![ @@ -718,7 +723,7 @@ async fn sync_restart_node() -> Result<()> { &mut events1, Duration::from_secs(10), vec![ - match_event!(LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing, .. } if *from == id2), + match_event!(LiveEvent::InsertRemote { from, content_status: ContentStatus::Missing | ContentStatus::Incomplete, .. } if *from == id2), match_event!(LiveEvent::ContentReady { hash } if *hash == hash_c), ], vec![ @@ -918,7 +923,6 @@ async fn test_download_policies() -> Result<()> { /// Test sync between many nodes with propagation through sync reports. #[tokio::test(flavor = "multi_thread")] #[traced_test] -#[ignore = "flaky"] async fn sync_big() -> Result<()> { let mut rng = test_rng(b"sync_big"); let n_nodes = std::env::var("NODES") From 37cc03a100e2984c4f2b8fe31d90e0d791178a80 Mon Sep 17 00:00:00 2001 From: jonaswre Date: Sat, 1 Aug 2026 22:41:23 +0000 Subject: [PATCH 2/3] fix(test): enable test_roundtrip_bytes_small by dropping an unfounded assertion Last of the five #[ignore = "flaky"] tests. It was not flaky: it failed 5/5, so it had never given signal, and the "flaky" label hid that. It asserted assert_eq!(&expected.addr(), &actual.addr(), ...) i.e. that `get_bytes` hands back the same allocation `add_bytes` was given. The store makes no such promise: - reads go through `export_bao(..).data_to_bytes()`; - an entry may be `MemOrFile::Mem` or `MemOrFile::File`, and from disk a copy is unavoidable; - zero-copy is documented only as an internal property of the in-memory variant (store/fs/bao_file.rs), not as API behaviour; - it additionally requires a live handle, which the test had no reliable way to hold -- the ignore reason said exactly this. So the assertion was true only by coincidence. Drop it and keep what the API does promise: the data round-trips, the hash matches, and the entry reaches completion. The alternative -- making zero-copy a public guarantee so the assertion becomes honest -- was rejected deliberately. It would foreclose encryption at rest, compression and checksum-on-read for the sake of one assertion. Read-path allocation behaviour belongs in a benchmark, which says "this should stay fast" without freezing how. The comment in the test records that reasoning so the assertion is not reinstated blindly. Nothing is lost by removing it: the test has been disabled and failing, so it was protecting nothing. Also refresh scripts/determinism-boundaries.txt. Removing one #[ignore] line from krikos/src/endpoint/tests.rs shifted every boundary below it. Verified this is pure line drift before refreshing: 1078 entries before and after, 0 with any change to kind/file/code, exactly 22 line numbers moved by -1 with identical code text. No #[ignore = "flaky"] remains in the tree. check-flaky-sweep-scope.sh now reports 0 flaky tests watched, which is the intended end state: a red nightly sweep from here means a genuinely new flake. Co-Authored-By: Claude Opus 5 --- protocols/krikos-blobs/src/store/fs.rs | 28 ++++++++++------ scripts/determinism-boundaries.txt | 44 +++++++++++++------------- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/protocols/krikos-blobs/src/store/fs.rs b/protocols/krikos-blobs/src/store/fs.rs index bd0d047a04c..91a2bd0248d 100644 --- a/protocols/krikos-blobs/src/store/fs.rs +++ b/protocols/krikos-blobs/src/store/fs.rs @@ -1668,7 +1668,6 @@ pub mod tests { // import data via import_bytes, check that we can observe it and that it is complete #[tokio::test] - #[ignore = "flaky. I need a reliable way to keep the handle alive"] async fn test_roundtrip_bytes_small() -> TestResult<()> { tracing_subscriber::fmt::try_init().ok(); let testdir = tempfile::tempdir()?; @@ -1686,14 +1685,25 @@ pub mod tests { let actual = store.get_bytes(expected_hash).await?; // check that the data is there assert_eq!(&expected, &actual); - assert_eq!( - &expected.addr(), - &actual.addr(), - "address mismatch for size {size}" - ); - // we must at some point see completion, otherwise the test will hang - // keep the handle alive by observing until the end, otherwise the handle - // will change and the bytes won't be the same instance anymore + // Deliberately NOT asserting `expected.addr() == actual.addr()`. + // + // That asserted `get_bytes` hands back the *same allocation* that + // `add_bytes` was given. The store makes no such promise: reads go + // through `export_bao(..).data_to_bytes()`, and an entry may live in + // memory or on disk (`MemOrFile`), where a copy is unavoidable. + // Zero-copy is documented only as an internal property of the + // in-memory variant (`store/fs/bao_file.rs`), and it additionally + // requires a live handle -- which this test has no reliable way to + // hold. The assertion was therefore true only by coincidence, and + // failed every run, so the test was ignored and gave no signal at + // all. + // + // Promoting zero-copy to a public guarantee to make this assertion + // honest would foreclose encryption at rest, compression and + // checksum-on-read. If read-path allocation matters, measure it with + // a benchmark rather than freezing it into a contract here. + // + // We must at some point see completion, otherwise the test will hang. obs.await_completion().await?; } store.shutdown().await?; diff --git a/scripts/determinism-boundaries.txt b/scripts/determinism-boundaries.txt index d6e387a5b7e..d65b7dd3a34 100644 --- a/scripts/determinism-boundaries.txt +++ b/scripts/determinism-boundaries.txt @@ -231,22 +231,22 @@ clock-timer krikos/src/endpoint/tests.rs:1295 tokio::time::sleep(Duration::from_ clock-timer krikos/src/endpoint/tests.rs:1306 tokio::time::sleep(Duration::from_millis(100)).await; clock-timer krikos/src/endpoint/tests.rs:1584 let t0 = Instant::now(); clock-timer krikos/src/endpoint/tests.rs:1590 let t1 = Instant::now(); -clock-timer krikos/src/endpoint/tests.rs:2040 tokio::time::timeout(Duration::from_secs(5), async { -clock-timer krikos/src/endpoint/tests.rs:2042 tokio::time::sleep(Duration::from_millis(10)).await -clock-timer krikos/src/endpoint/tests.rs:2106 let now = Instant::now(); -clock-timer krikos/src/endpoint/tests.rs:2188 tokio::time::timeout(Duration::from_secs(1), async { -clock-timer krikos/src/endpoint/tests.rs:2211 tokio::time::timeout(Duration::from_secs(1), endpoint.close()) -clock-timer krikos/src/endpoint/tests.rs:2228 let now = Instant::now(); -clock-timer krikos/src/endpoint/tests.rs:2229 tokio::time::timeout(Duration::from_secs(5), ep.close()) -clock-timer krikos/src/endpoint/tests.rs:2256 tokio::time::sleep(Duration::from_millis(10)).await; -clock-timer krikos/src/endpoint/tests.rs:2259 tokio::time::timeout(Duration::from_secs(5), ep.close()) -clock-timer krikos/src/endpoint/tests.rs:2280 let res = tokio::time::timeout(Duration::from_millis(500), ep.online()).await; -clock-timer krikos/src/endpoint/tests.rs:2287 let res = tokio::time::timeout(Duration::from_millis(1000), ep.online()).await; -clock-timer krikos/src/endpoint/tests.rs:2293 tokio::time::timeout(Duration::from_millis(500), ep_clone.online()).await -clock-timer krikos/src/endpoint/tests.rs:2306 let res = tokio::time::timeout(Duration::from_millis(500), ep.online()).await; -clock-timer krikos/src/endpoint/tests.rs:2312 tokio::time::timeout(Duration::from_millis(500), ep_clone.online()).await -clock-timer krikos/src/endpoint/tests.rs:2356 let auth_err: String = tokio::time::timeout(Duration::from_secs(5), async { -clock-timer krikos/src/endpoint/tests.rs:2380 tokio::time::timeout(Duration::from_secs(5), good_ep.online()) +clock-timer krikos/src/endpoint/tests.rs:2039 tokio::time::timeout(Duration::from_secs(5), async { +clock-timer krikos/src/endpoint/tests.rs:2041 tokio::time::sleep(Duration::from_millis(10)).await +clock-timer krikos/src/endpoint/tests.rs:2105 let now = Instant::now(); +clock-timer krikos/src/endpoint/tests.rs:2187 tokio::time::timeout(Duration::from_secs(1), async { +clock-timer krikos/src/endpoint/tests.rs:2210 tokio::time::timeout(Duration::from_secs(1), endpoint.close()) +clock-timer krikos/src/endpoint/tests.rs:2227 let now = Instant::now(); +clock-timer krikos/src/endpoint/tests.rs:2228 tokio::time::timeout(Duration::from_secs(5), ep.close()) +clock-timer krikos/src/endpoint/tests.rs:2255 tokio::time::sleep(Duration::from_millis(10)).await; +clock-timer krikos/src/endpoint/tests.rs:2258 tokio::time::timeout(Duration::from_secs(5), ep.close()) +clock-timer krikos/src/endpoint/tests.rs:2279 let res = tokio::time::timeout(Duration::from_millis(500), ep.online()).await; +clock-timer krikos/src/endpoint/tests.rs:2286 let res = tokio::time::timeout(Duration::from_millis(1000), ep.online()).await; +clock-timer krikos/src/endpoint/tests.rs:2292 tokio::time::timeout(Duration::from_millis(500), ep_clone.online()).await +clock-timer krikos/src/endpoint/tests.rs:2305 let res = tokio::time::timeout(Duration::from_millis(500), ep.online()).await; +clock-timer krikos/src/endpoint/tests.rs:2311 tokio::time::timeout(Duration::from_millis(500), ep_clone.online()).await +clock-timer krikos/src/endpoint/tests.rs:2355 let auth_err: String = tokio::time::timeout(Duration::from_secs(5), async { +clock-timer krikos/src/endpoint/tests.rs:2379 tokio::time::timeout(Duration::from_secs(5), good_ep.online()) clock-timer krikos/src/endpoint/tests.rs:287 tokio::time::timeout(Duration::from_secs(10), async { clock-timer krikos/src/endpoint/tests.rs:289 tokio::time::sleep(Duration::from_millis(10)).await; clock-timer krikos/src/endpoint/tests.rs:400 let (server, client) = tokio::time::timeout( @@ -933,12 +933,12 @@ spawn-task krikos/src/endpoint/tests.rs:1236 let accept = tokio::spawn(async mov spawn-task krikos/src/endpoint/tests.rs:1331 let server_task = tokio::spawn(async move { spawn-task krikos/src/endpoint/tests.rs:1379 let server_task = tokio::task::spawn(async move { spawn-task krikos/src/endpoint/tests.rs:1436 let server_task = tokio::spawn({ -spawn-task krikos/src/endpoint/tests.rs:1907 let server_task = tokio::spawn(async move { -spawn-task krikos/src/endpoint/tests.rs:1910 let stats_task = tokio::spawn(collect_stats(conn.path_events())); -spawn-task krikos/src/endpoint/tests.rs:1921 let client_stats_task = tokio::spawn(collect_stats(conn.path_events())); -spawn-task krikos/src/endpoint/tests.rs:2248 let accept_task = tokio::spawn(async move { -spawn-task krikos/src/endpoint/tests.rs:2292 let task = tokio::task::spawn(async move { -spawn-task krikos/src/endpoint/tests.rs:2311 let task = tokio::task::spawn(async move { +spawn-task krikos/src/endpoint/tests.rs:1906 let server_task = tokio::spawn(async move { +spawn-task krikos/src/endpoint/tests.rs:1909 let stats_task = tokio::spawn(collect_stats(conn.path_events())); +spawn-task krikos/src/endpoint/tests.rs:1920 let client_stats_task = tokio::spawn(collect_stats(conn.path_events())); +spawn-task krikos/src/endpoint/tests.rs:2247 let accept_task = tokio::spawn(async move { +spawn-task krikos/src/endpoint/tests.rs:2291 let task = tokio::task::spawn(async move { +spawn-task krikos/src/endpoint/tests.rs:2310 let task = tokio::task::spawn(async move { spawn-task krikos/src/endpoint/tests.rs:330 let server = tokio::spawn( spawn-task krikos/src/endpoint/tests.rs:360 let client = tokio::spawn( spawn-task krikos/src/endpoint/tests.rs:437 let server = tokio::spawn( From 9350b1d5e9a0663a0260a8d0b2424a9c0a07f5bb Mon Sep 17 00:00:00 2001 From: jonaswre Date: Sat, 1 Aug 2026 23:13:37 +0000 Subject: [PATCH 3/3] fix(test): drop the import orphaned by removing the addr assertion `SliceInfoExt` provides `.addr()`. Removing that assertion in the previous commit left the import unused, which under CI's `-Dwarnings` is a hard error -- it failed clippy, MSRV and every test job on every platform. My local check did not catch it because `cargo nextest run` does not compile with warnings-as-errors: 115/115 passed while the code did not build the way CI builds it. Running the tests answers a different question from "does this compile cleanly", and I reported the first as if it settled the second. Verified this time by exit code rather than absence of output: cargo check --workspace --all-targets --all-features (-Dwarnings) 0 cargo clippy --workspace --all-targets --all-features -D warnings 0 cargo fmt --all -- --check 0 plus krikos-blobs 115/115 and krikos-docs 102/102. Co-Authored-By: Claude Opus 5 --- protocols/krikos-blobs/src/store/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/krikos-blobs/src/store/fs.rs b/protocols/krikos-blobs/src/store/fs.rs index 91a2bd0248d..afba9437b21 100644 --- a/protocols/krikos-blobs/src/store/fs.rs +++ b/protocols/krikos-blobs/src/store/fs.rs @@ -1535,7 +1535,7 @@ pub mod tests { api::blobs::Bitfield, store::{ KRIKOS_BLOCK_SIZE, - util::{SliceInfoExt, Tag, read_checksummed, tests::create_n0_bao}, + util::{Tag, read_checksummed, tests::create_n0_bao}, }, };