speed up integration tests - #698
Conversation
|
We definitely got a nice speedup here, IT went from 30 mins to 20 mins! 🔥 |
|
thanks for the initiative @GitGab19 SRI community desperately needs to speed up our development process, since we're running on GitHub free tiers and Loupe reports are arriving at a pace in which our meatlayer processes are not being able to keep up with we're all very grateful for Anthropic's FOSS support with out free-tier Claude Max accounts, but we've also been kinda disappointed at Fable and Opus performance (even when locked into the borderline mandatory Claude Code walled gardened harness, which sorta goes agaist FOSS spirit) and Kimi K3 has been kicking ass across the entire Bitcoin Ecosystem lately, so we might as well try to leverage on our benefit too so I'll try to leverage Kimi K3 for the creation of a plan, to be executed later by budget-friendly DeepSeek V4 Pro (both inside PROMPT to Kimi K3:
I'll come back later with some commit suggestions to be cherry-picked |
PLAN by Kimi K3:
|
EXECUTION report by DeekSeek V4 Pro:
proposed commits for cherry-picking: |
|
PERFORMANCE report by DeekSeek V4 Pro:
from ~30min to ~9min tbh 9 min feels still a bit prohibitive IMO, but great improvements overall a good next step would be to try to explore the following area, which has been left out of plan scope as YAGNI (and is also somewhat aligned to what was done on #600):
not sure I want to push for it now though, feels like an area that deserves deeper reasoning before we get our hands dirty for example, one important aspect worth of consideration: this will also affect the dynamics of how the global org-level Github Runner quota will be consumed, which impacts how concurrent PRs affect eachother (which the model called "queue" on the quoted sentence above) |
b19dfe9 to
5701391
Compare
|
@plebhash I cherry-picked your commits and added a1896b9 and 5701391 on top of them. I did that to address the flakiness exposed by parallel execution of your PR (plebhash#4). The first makes minerd setup and teardown concurrency-safe, replaces a fixed startup delay with a readiness check, and properly reaps child processes to prevent nextest LEAK reports. The latest Ubuntu and macOS runs completed without flaky or leaky tests. The results are:
|
b38fe09 to
2ae3023
Compare
|
Update: with the cache commit (f334f5b), now we're running all the jobs in ~8min! |
2ae3023 to
df8ecd7
Compare
Four tests polled on one condition and then asserted on another that
settles later. Each was masked by a coarse 1s poll interval that happened
to return late enough for the second condition to hold, rather than by
any actual synchronisation:
- `SnifferSV1::wait_and_assert` waited using a fuzzy matcher (which also
accepts an `OkResponse` whose serialized form merely *contains* the
filter string) and then re-fetched with a strict predicate, panicking
"Message disappeared after wait_for_message" when the two disagreed.
It now polls on exactly the predicate it fetches with.
- `test_extension_negotiation_with_tlv_in_submit_shares` popped
`RequestExtensions` off the queue without waiting for it. It is sent
*after* `SetupConnectionSuccess`, so the pop could precede the send.
- `pool_api_endpoints_with_miner` and `jdc_api_endpoints_with_miner`
polled until a client was registered, then asserted that client already
had a channel. The channel opens after registration. Both now poll on
the channel count; `poll_until` takes `&str` so it can address the
dynamic `/clients/{id}/channels` route.
- `non_aggregated_translator_correctly_deals_with_group_channels`
compared a `mining.notify` prevhash before and after a chain tip
update without clearing the queue, so the stale pre-update notify
could be matched and the prevhash appeared unchanged. Adds
`SnifferSV1::clean_queue`, mirroring the Sv2 sniffer.
Each was reproduced deterministically before being fixed.
The suite spent most of its wall clock asleep. Bucketing every timestamp gap across a full run showed ~613s of dead time in loops that checked a condition, missed, and then slept a full second — so a message arriving in 30ms still cost ~1000ms. Introduces two intervals in `utils` rather than one: - `POLL_INTERVAL` (200ms) for message-wait loops, which exit as soon as their message lands. - `CONNECT_RETRY_INTERVAL` (1s) for the unbounded connect-retry loops, which have no timeout and spin for as long as a peer is absent — something several tests arrange deliberately. Every test runs on a bare `#[tokio::test]`, i.e. a single-threaded runtime, so those loops must not busy-poll against the test's own work. 200ms is not a tuning artefact. Measured across full runs: 1s leaves ~613s of dead time, 200ms leaves ~87s, and 50ms leaves ~61s. The residual at 50ms is real message latency rather than poll delay, so 200ms already captures ~96% of everything recoverable and anything in the 150-250ms range lands in the same place. 50ms was also actively harmful: it starved the single-threaded runtime and produced a 120s hang (nextest's slow-timeout terminate-after) in 5 of 6 runs. At 200ms: 0 hangs in 7 runs.
…tadirs
`TemplateProvider::start` slept a flat 2s after spawning Bitcoin Core and
a further 3s after spawning sv2-tp — 5s per template provider, ~79 times
per run, whether or not either process was ready.
A fixed sleep couples two things that should be independent: how long you
wait when the machine is slow (safety) and how long you wait when it is
fast (speed). Polling separates them, so the ceilings here are *more*
generous than the sleeps they replace (30s) while the common path returns
as soon as the process is actually serving. Measured: Bitcoin Core IPC
socket ready in ~110us, sv2-tp in ~151ms.
Both gates prove serviceability rather than existence. The IPC gate
connects to `node.sock` rather than checking that the path exists,
because datadirs are keyed by port and a stale socket file from an
earlier test can satisfy a `Path::exists` check while refusing traffic —
that failure mode caused `tdp_io_integration_v30x` to run for 108s
against a node that never answered, and `jdp_io_integration_v30x` to time
out entirely.
Note that a connect probe is only safe against a listener that ascribes
no meaning to a bare connection. The pool is not such a listener — it
accepts every connection as a protocol session — so `start_pool` keeps a
plain sleep. Probing it created phantom downstreams that failed setup and
hung `jds_isolates_state_for_colliding_request_ids_across_downstreams` in
4 of 10 runs.
Also removes each node's datadir on drop. Nothing cleaned them up, so a
full run left ~1.5GB of `.bitcoin-{port}` directories behind and
successive runs accumulated until the filesystem filled. Retention is
still available via SV2_KEEP_TEST_DATADIR for post-mortem inspection of
debug.log and chainstate, since that is plausibly why a persistent
`staticdir` was chosen in the first place.
The IPC readiness gate connected to node.sock and dropped the connection immediately. Core's libmultiprocess layer sets TCP_NODELAY on every accepted connection, so that setsockopt ran against a socket which was already gone. Linux tolerates it; macOS returns EINVAL, which surfaced as mp/proxy.cpp:45: error: Uncaught exception in daemonized task.; exception = kj/async-io-unix.c++:1365: failed: setsocketopt(IPPROTO_TCP, TCP_NODELAY): Invalid argument and took down Core's IPC listener. sv2-tp could then never connect, so it never bound its Sv2 port, and the gate after it failed with timeout after 30s waiting for sv2-tp to listen on 127.0.0.1:49215 This is the rule already documented for start_pool in efc4dca — a connect probe is only safe against a listener that ascribes no meaning to a bare connection — applied to a capnp RPC endpoint, where it plainly does not hold. It went unnoticed because every run validating this branch was on Linux. The gate now waits for the socket to appear rather than connecting to it. Stat-ing was previously rejected because a stale node.sock from an earlier test on the same port could satisfy it, so this also removes any leftover datadir for the port before starting the node, which closes that window directly instead of by probing around it.
Replace the per-process-only UNIQUE_PORTS in-process dedup set with
flock-based per-port lockfiles held for the process lifetime. Two
concurrent nextest test processes probing bind(0) to find free ports
can now never pick the same port because the non-blocking exclusive
flock is forced to fail on the loser.
Bitcoin Core datadirs are keyed by port (.bitcoin-{port}), so a
port collision meant two nodes sharing one datadir, not just a bind
error. The lockfiles live under $TMPDIR/sv2-it-ports/ and are
released by the kernel on exit (including kill -9), so stale files
are harmless and never need cleanup.
…oss processes Concurrent nextest processes on a cold CI runner would all see the bitcoin-core / sv2-tp / high_diff_chain directories as missing and simultaneously download+unpack into the same shared tree. Guard each artifact with a blocking exclusive flock so only the first process actually downloads; the rest wait on the lock, then skip via an inner exists() re-check. Also give tarball::unpack a pid-unique temp filename so two processes can never share the same staging file even if the lock is bypassed.
The mining_device auto mode spawns (logical_cpus - 1) hashing threads per test process. With test-threads > 1 this oversubscribes the CI runner and makes timing-sensitive share-rate assertions flaky (SHARES_PER_MINUTE). Call set_cores(1) in the test-harness helper start_mining_device_sv2 so every test that mines through the harness gets a single hashing thread. The standalone binary and benches (not part of ITFCI) keep auto mode.
Replace test-threads = 1 with test-threads = "num-cpus" so the Integration Tests CI drains the job queue faster on free-tier runners (4 vCPU on ubuntu-latest, 3 on macos-latest per GitHub docs). Comment documents the NEXTEST_TEST_THREADS=1 override for local sequential debugging.
With test-threads > 1, live stdout from N interleaving tests is unreadable. nextest captures per-test output and prints it only for failing/flaky tests at the end. The RUST_LOG=debug logs remain captured; re-run with --no-capture is documented in the step comment for manual triage.
Use a common target directory for workspace commands and configure rust-cache from the integration-tests workspace. Replace ineffective caches and fix the MSRV cache ordering while keeping the existing checks unchanged.
df8ecd7 to
1111a69
Compare
Publish downloaded artifacts atomically after extraction and signing, retain the minerd proxy listener until startup, and tolerate vardiff SetTarget messages at the affected assertion sites. Also fix Cargo cache invalidation across workspaces, remove the unused readiness probe, and correct stale diagnostics and documentation.
1e50034 to
30b7dfb
Compare
3 participants
This is a test PR with some improvements on the IT in order to speed up tests execution and improve flakiness.