fix(test): un-ignore four flaky tests; fix the content_status race - #22
Merged
Conversation
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 <noreply@anthropic.com>
|
Documentation for this PR has been generated and is available at: https://holon-technologies.github.io/iroh/pr/22/docs/krikos/ Last updated: 2026-08-01T23:15:45Z |
… 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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four of the five tests carrying
#[ignore = "flaky"]are now enabled. Three were not flaky any more; the fourth had a real race with a measurable cause.Measured before changing anything
Five runs of all five ignored tests:
connect_via_relay_becomes_direct_and_sends_directsync_restart_nodesync_bigsync_full_basictest_roundtrip_bytes_smallThree had been left ignored long after whatever made them flaky stopped happening. Nothing re-checks an
#[ignore], so they just stayed off.The real race:
sync_full_basicIt passes in isolation and fails only when run alongside other tests — and not as a timeout:
The matchers required
content_status: ContentStatus::Missing. Butcontent_statusis 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.Rather than guess how far to loosen, I measured which values actually occur, over 20 runs under full CPU load, by accepting any status and logging it:
peer0/from-peer1peer2/hash0peer2/hash1CompleteSo "not yet complete" is a real assertion that holds, while the
Missing/Incompletesplit 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.Verification
sync_full_basic4 times in 5krikos-docs102/102,krikos155/155, with nothing skipped (previously 3 and 1)check-flaky-sweep-scope.shtracked the change automatically: now reports 1 flaky test watched, down from 5Not fixed here:
test_roundtrip_bytes_smallIt stays ignored, and it needs a decision rather than a fix. It fails 5/5, so it is broken, not flaky — it has never given signal, and labelling it flaky obscured that.
It asserts:
that
get_bytesreturns the same allocationadd_byteswas handed — zero-copy identity. The store does not guarantee that, as the ignore reason itself concedes: "I need a reliable way to keep the handle alive."The options are to drop the address assertion (keeping the round-trip, hash and completion checks, which are real), or to build genuine handle retention so the guarantee exists. The first loses a narrow ability to detect the store starting to copy unnecessarily. Worth an explicit call rather than a quiet choice.
🤖 Generated with Claude Code