Parallelize test suite with pytest-xdist - #3353
Open
singlerider wants to merge 14 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #3353 +/- ##
======================================
+ Coverage 90% 90% +1%
======================================
Files 450 454 +4
Lines 46327 46588 +261
======================================
+ Hits 41587 41851 +264
+ Misses 4740 4737 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Test servers bound fixed ports (8888/9999/5556) and 58 test files hardcoded those in URLs, so the suite could only ever run serially. CI spent ~58 min in a single pytest call while using one of four cores. - bbot/test/ports.py: resolve server ports per xdist worker - conftest.py: tolerate listener=None (workers inherit _BBOT_LOGGING_SETUP) - 57 test files: hardcoded host:port -> constants from bbot.test.ports localhost and 127.0.0.1 spellings are kept distinct; virtualhost tests assert on the Host header, where they are not interchangeable. test_regexes.py is untouched, its literals are regex fixtures, not references to the test server.
Beyond the server ports, three things were process-global and would corrupt each other under xdist: - BBOT home (/tmp/.bbot_test): shared caches and scan output, and pytest_sessionfinish rmtree's it. The first worker to finish would delete the directory out from under every worker still running. - uvicorn (8978) and websockets (8765) servers: bound fixed ports. - docker-backed tests (kafka, elastic, mongo, mysql, nats, postgres, rabbitmq): fixed container names and fixed host port bindings, so they genuinely cannot run concurrently. They are now pinned to a single worker via xdist_group, keyed off the skip_distro_tests flag they already set. Renamed ports.py -> worker.py since it now covers more than ports. Port bases were chosen with distinct mod-100 residues; verified collision-free and clear of the ephemeral range for up to 64 workers.
pytest-xdist added to the dev group; workflow runs with -n logical --dist loadgroup. loadgroup (not loadfile) is required so the xdist_group marker on the docker-backed tests is honored.
Two problems surfaced running the suite 16-way on a 16-core/15GB box. Elasticsearch was OOM-killed (exit 137): 16 workers hold ~10GB, leaving under 3GB, and it wants ~2GB of heap. The test then waited for it in an unbounded 'while True', so a container that was already dead hung the run at 99% until the 1200s global timeout. Same unbounded loop in the mongo test. Both now stop after CONTAINER_READY_TIMEOUT (180s, overridable) and say what happened, including the exit-137 hint. '-n logical' is the wrong worker count when cores outnumber GB. worker_count.py bounds by cores AND by available memory, keeping 5GB free for the docker-backed services. A 4-core/16GB github runner is unaffected (still 4); this box goes 16 -> 15. Override with BBOT_TEST_WORKERS.
Running the suite under xdist surfaced failures that were my own, not
pre-existing. Four separate gaps:
Logging. Workers inherit _BBOT_LOGGING_SETUP from the pytest parent, so
BBOTLogger skipped setup entirely and debug.log was never written. The
previous 'if listener is not None' guard hid that instead of fixing it,
and log-reading tests failed with 'assert -retries 1 in ""'. Workers now
clear the flag so each sets up its own logging, and conftest raises if a
process somehow still has no listener.
Port literals the first pass did not match, because it only looked for
127.0.0.1:PORT and localhost:PORT:
* synthetic hostnames carrying the port (test.example:8888, bad.dns:8888)
* the bbot_other_httpservers fixture, six servers hardcoded to 8888/8889
* config values like {"ports": "8888"}, which are not URLs
* 127.0.0.0:9999 (the .0 address, excluded along with .1)
* filename/slug spellings: 127.0.0.1.8888 and http-127-0-0-1-8888
* mocked masscan JSON, whose port is what the module 'discovers'
Hardcoded /tmp/.bbot_test paths, which my own per-worker home broke.
Adds BBOT_TEST_PORT_OFFSET so a second copy of the suite can run beside
one that already owns the base ports, instead of the two colliding and
looking like a worker-isolation bug.
Full-suite A/B surfaced the last regressions from parallelization. asset_inventory: TestAsset_InventoryEmitPrevious runs with use_previous=True, which reads the asset-inventory.csv left behind by an earlier scan of the same name. It was relying on TestAsset_Inventory having already run. That is order-dependent, and xdist guarantees neither order nor worker affinity, so it failed whenever it was scheduled first or landed on another worker (each worker has its own BBOT home). It now seeds the CSV itself in setup_before_prep, which makes it independent and drops its runtime from ~20s to ~4s since it no longer needs the portscan. Port comparisons written as bare integers (e.port == 8888) in test_scope.py and test_module_fingerprintx.py. The earlier passes only rewrote string literals, so these were missed. Also groups tests that share a scan_name onto one worker, alongside the existing docker grouping. Not sufficient on its own for the ordering problem above, but correct for co-locating tests that share a scan home.
git_clone writes cloned repos to git_repos/<bbot home name>/<repo>, so the
home directory's *basename* shows up inside the paths these tests assert on.
They spelled it '.bbot_test', which stopped matching once each xdist worker
got its own home ('.bbot_test_gw0').
Export BBOT_TEST_DIR_NAME from worker.py and use it instead.
Worth noting these reproduce with '-n 1', a single worker with no concurrency
at all. They were never race conditions, just the per-worker home rename, and
checking '-n 1' before '-n 4' would have found that much faster.
test_module_robots matched the server URL with a raw regex string, where the port sits behind escaped dots (r"http://127\.0\.0\.1:8888/..."). The earlier rewrites looked for plain literals and skipped it, so the test failed whenever it landed on a worker whose port was not 8888. test_cli_presets asserts a scan output dir does not exist yet, and removes it at the end of the test. Any earlier failure or interrupted run leaves it in place, and every later run against that BBOT home then fails on the assertion rather than on the real problem. It now clears the directory up front, so the precondition is established instead of assumed. Both showed up as intermittent (~25% of runs) rather than deterministic, which is what made them the last two to be found.
Net removal pass over the parallelization work. Removed: - BBOT_TEST_PORT_OFFSET and port_offset(). Added for running two copies of the suite side by side while benchmarking. Nothing outside worker.py used it. - LOCALHOST_SSL_HOSTPORT, the BASE_* port aliases, and the worker_index() wrapper. All unused or single-use indirection. - OTHER_HTTPSERVER_PORT, which was literally 'OTHER_HTTPSERVER_PORT = HTTPSERVER_PORT'. Renamed to HTTPSERVER_PORT/HTTPSERVER_PORT_ALT at its one call site. - The scan_name xdist grouping. It compensated for asset_inventory's ordering dependency, which the previous commit removed by having the test seed its own input, so the grouping no longer does anything. Verified: 3x green at 4 workers without it. - worker_count's -v flag and MIN_WORKERS floor. The floor guaranteed 2 workers even when the memory arithmetic said there was room for none, which is an assumption I never validated; a 2-core/4GB box now correctly gets 1. - A dead 'import time' the refactor exposed. De-duplicated: - The container readiness loop was copy-pasted between elastic (sync) and mongo (async), each with its own deadline handling and OOM hint. Now one wait_for_container() helper that takes either kind of callable. worker.py 154 -> 100 lines, worker_count.py 64 -> 38, conftest hook 40 -> 13.
Caught by CI on this PR, not locally: gowitness needs its binary installed, so
both classes error out on my machine and never reached the assertions.
Two bugs:
The multiport test asserted on bare port substrings ('8888' in url) rather than
URL-shaped literals, so the earlier rewrites did not match them. On any worker
but the first the ports are 9188/10299 and the assertion could never pass.
Both classes also pinned an explicit home directory under /tmp and rmtree'd it
at class-definition time, which every worker does to the same path. Workers
deleted each other's dependency installs mid-run, which is where the
FileNotFoundError on the depsinstaller artifact came from. Now worker-scoped.
Worth noting these run with deps.behavior=force_install, so each worker
installs its own copy of chrome, ~650MB. That is fine on a runner where /tmp is
disk-backed, but it multiplies with worker count.
singlerider
force-pushed
the
ci-parallel-tests
branch
from
July 28, 2026 18:32
9c2ab0a to
216cef3
Compare
The force_basehost wildcard-canary check still matched a literal 8888, so on any worker but gw0 it never matched. Both that branch and the fallthrough return 404, which is why it did not show up as a failure.
Same class of bug as the gowitness home directory that CI caught: paths that are fine serially but shared once workers run concurrently. - test_python_api used one BBOT home across two tests, so the dep installer wrote and read the same artifacts directory from two workers - test_presets wrote a fixed custom preset dir - test_command and test_web wrote fixed scratch files, then read them back
A pull_request run from a fork gets a read-only GITHUB_TOKEN regardless of the
permissions block, so github-script fails with
HttpError: Resource not accessible by integration (403)
POST /repos/.../issues/{n}/comments
and takes the whole job red. Every fork PR has a failing Performance Benchmarks
check for a reason that has nothing to do with performance, which trains people
to ignore a red check.
Skip the comment step when the head repo is not this repo. The benchmark still
runs and the report is still uploaded as an artifact, so nothing is lost except
a comment that could never have been posted.
singlerider
marked this pull request as ready for review
July 28, 2026 23:28
Codecov flagged worker_count.py at 0 percent: the workflow runs it as a script, so pytest never imported it. worker.py was only covered incidentally. These two modules decide how many workers run and how each one gets its own ports and directories, so a silent regression there either overloads the runner or lets workers collide. Verified by mutation: removing the max(1, ...) clamp, dropping the memory cap, and swapping sched_getaffinity for os.cpu_count each fail these tests.
liquidsec
approved these changes
Jul 30, 2026
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
2 participants
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.
Summary
Runs the test suite under
pytest-xdist. On a 4-core runner this takes CI from about two hours to about half an hour, with the same tests passing.dev(serial)Pass counts are identical, which is the point. Wall time drops about 3.7x on that pair of runs, but the number that matters is that the same 791 tests pass either way.
The 1 remaining failure (
test_cli.py::test_cli_args) and all 8 errors (gowitness, jadx, unarchive) reproduce on unmodifieddevin the same environment. They are missing binaries in my local runner container, not something this branch introduces.Validation
Both sides were run as the real
Tests/testworkflow job underact, not a barepytestinvocation, so the comparison includes the same dependency installs, service containers and workflow steps CI actually performs:Same machine, same container image, same Python (3.14.6), pytest 9.1.1, pytest-xdist 3.8.0. Baseline is
devatfab03bed1, checked out in a separate worktree and run unmodified.Final run
Measured against
5ce87d99c, which was the branch tip at the time.The branch has since been rebased onto current
devand the numbers above have not been re-measured on the rebased tree. The original branch was cut fromfab03bed1and had drifted 24 commits behind; left alone it would have revertedactions/setup-pythonv7 to v6 across four workflows and narrowed thepytest-envandfastapibounds back down, purely as rebase collateral. The rebase was clean and the intervening commits are dependency bumps plus adocker_pullfix, none of which plausibly move a 2-hour wall time, but the honest statement is that the measurement predates the rebase.Pass counts match exactly. The failing test and the 8 errors are the same node IDs on both sides:
test_cli.py::test_cli_args(fails on unmodifieddevhere as well)test_module_gowitness.py, 2xtest_module_unarchive.py, 1xtest_module_jadx.py— missing binaries in my runner container (7zand friends), unrelated to this changeDiffing the before/after failure and error sets yields zero regressions.
Convergence across runs
The parallel side was re-run after each round of fixes. Included because the first numbers were not clean and it would be misleading to show only the last:
filedownloadcould not fetch its wordlistRuns 4-6 differ only in how the dependency downloads went, which is why ~30m is the figure worth quoting rather than 23m43s. Across the three clean runs the spread is 23m43s to 33m44s, all with the same pass count, so the speedup is somewhere in the 3.7x-5.2x range depending on network luck. The serial baseline is a single 124m sample; I did not re-run it, so treat it as one observation.
port collisions: 0in every run, grepped from the logs as an explicit check rather than inferred from the pass count.Per-test verification
Each fix was confirmed individually at 4 workers before the full re-run, and flaky-looking failures were repeated 3-4 times rather than declared fixed on a single green run. Two examples:
test_module_robotsandtest_cli_presetseach failed roughly 1 run in 4. Both were run 4x after the fix, 4/4 green.test_module_trufflehoglooked like a race. It reproduced at-n 1— one worker, no concurrency — which showed it was the per-worker home rename, not a race. Worth knowing when triaging anything similar: if it fails at-n 1it is an isolation bug, not a concurrency bug.Why 4 workers
CI is
ubuntu-latest, 4 cores, so 4 workers is the number that predicts CI time. My box has 16 cores but roughly a quarter the RAM per core, so a 16-way measurement here would describe hardware nobody runs the suite on.How it works
Each xdist worker gets its own:
/tmp/.bbot_test_gw0), sincepytest_sessionfinishdeletes it and whichever worker finished first was deleting it out from under the othersSerial runs are unchanged: no worker id means the original path and the original ports, so running
pytestwithout-nbehaves exactly as before.Docker-backed tests (kafka, elastic, mongo, mysql, nats, postgres, rabbitmq) bind fixed container names and host ports, so they are pinned to a single worker via
xdist_group. They are identified by theskip_distro_testsclass attribute they already set.Notable fixes along the way
_BBOT_LOGGING_SETUPfrom the parent, soBBOTLoggerskipped setup anddebug.logwas never written; tests reading it failed against an empty file.conftestnow raises if it ever finds itself without a real listener, rather than proceeding with logging quietly broken.while True. When a container was OOM-killed the suite hung until the global timeout rather than reporting anything useful. Now bounded, with a hint about exit 137.8888or.bbot_testin places a naive search misses: filenames, path slugs, regex literals, bare integer comparisons, and mocked tool output. The last one of these was a canary regex intest_module_virtualhostthat still matched a literal8888, so on any worker butgw0it silently stopped matching. It did not show up as a failure because that branch and the fallthrough both return 404 — worth calling out, since a green run did not prove that particular line correct.TestAsset_InventoryEmitPreviousdepended on a sibling test having run first to produce its input CSV. It now seeds its own, which also drops it from ~20s to ~4s.Measured on real CI
The numbers above come from my own machine under
act. Now that both a parallel and a serial run have completed on GitHub's own runners, here is the honest comparison, per Python version, same workflow, sameubuntu-latesthardware:Whole-workflow wall time: 60m serial, 24m parallel.
That is about 2.6x, noticeably less than the 3.7x I measured locally. A 4-core runner does not give 4x, and the local figure was flattered by my own hardware. 2.6x on the actual runners is the number worth quoting, and it still takes the matrix from an hour to under half an hour.
The serial side is the #3355 branch, which changes only workflow YAML and no Python, so it is a valid stand-in for
dev's serial timing.Bugs CI caught that local runs did not
Worth stating plainly, because it is the main risk in a change like this: two rounds of fixes here came from CI, not from my own runs.
TestGowitness_Socialerrored on all five Python versions with aFileNotFoundErrorunder/tmp/.bbot_gowitness_test/cache/depsinstaller/.... Two tests shared one BBOT home, so their dependency installers raced over the same artifacts directory. It never reproduced locally because gowitness's binary is absent on my machine, so both tests errored out early and never got far enough to collide.That prompted a sweep for the same shape of bug rather than just fixing the one instance. Four more fixed paths were shared across workers: the
test_python_apiBBOT home (same dep-installer race, two tests), thetest_presetscustom preset dir, and the scratch files intest_commandandtest_webthat are written and then read back. All now carry a per-worker name.The general lesson: a hardcoded path in a test is only safe while the suite is serial, and a locally-green run does not prove otherwise when a missing binary is masking the code path.
CI status on this PR
benchmarkfails with a 403 posting its comment. That is a fork PR getting a read-onlyGITHUB_TOKEN; it fails the same way on the other two CI PRs and passes on same-repo branches. Not something this branch can fix.test-distros (debian)run failed ontest_cli_args, the same test that already fails ondevin a container missing binaries. The distro suite passed on this branch on earlier pushes, anddevitself had a distro job go red on 07-27, so this is the existing flakiness rather than a regression. The distro workflow is untouched here and still runs serially.