test(cli): pin the forwarded worker option against argv, not merged output

[nilsmechtel]

A CLI test was reported failing roughly one run in twenty inside the full suite, which would make any single green suite run weaker evidence than it looks. I could not reproduce it — 126 consecutive full-suite runs green in the configuration CI uses — but the assertion it used is brittle by construction in a way I could demonstrate, so this replaces it with one that cannot be perturbed by anything else the process prints. Read this as removing a known-fragile assertion, not as a confirmed cure for a failure anybody has caught.

What the assertion depended on

test_an_option_the_cli_also_defines_still_reaches_the_worker asserted result.output.strip().endswith("--workspace-dir /data/ws"). The worker image ships click 8.5, where CliRunner no longer has mix_stderr: Result.output is stdout and stderr merged in write order, and only Result.stdout is stdout alone. Any line written to stderr inside the invoke window therefore lands after the echoed command and turns the suffix check false while the command itself is perfectly correct.

That half is confirmed rather than assumed. Running a positive control inside the image — echo the exact expected command, then write one line to stderr — gives output with the stray line appended, stdout clean, and endswith False.

What the test does now

The command is run for real with subprocess.call faked by the _fake_runtime helper that already exists in this file, and the recorded argv is compared element by element against ["python", "-m", "bioengine.worker", "--workspace-dir", "/data/ws"]. This is strictly stronger than the old suffix check: it pins the whole argv, not its tail, so it still fails if the CLI's own --workspace-dir option swallows the forwarded one, and it additionally fails if anything extra is appended. It is not a widened assertion.

Non-vacuity checked with a negative control: mutating build_command to drop worker_args from the entrypoint makes the new assertion fail, so it is genuinely exercising the forwarding path.

Reproduction, and why there is none attached

The first job was to catch the failure with its text. It did not happen. In the image, with the suite scope CI uses:

where	runs	failures
origin/main, image 0.16.6-dev3	80	0
origin/main, image 0.16.25	6	0
4ae0b10 (token passthrough fix)	20	0
74e3d52 (its parent)	20	0
this branch	25	0

The token-passthrough commit was suspected of having disturbed this test, since it rewrote the same file heavily. Twenty runs either side of it are clean, so it is not implicated.

One mechanism that was proposed can be ruled out specifically. The suite emits RuntimeWarning: coroutine 'ProxyDeployment.__del__' was never awaited at nondeterministic points, including once from inside click/testing.py during a CLI test, which made it a natural suspect for the stray stderr line. It cannot be: pytest's warnings plugin captures that warning, which is why it appears in the warnings summary rather than in any stream. A purpose-built probe that garbage-collects an object with an async __del__ inside a CliRunner invoke window returns an empty result.stderr and passes the endswith. Some other writer to the live sys.stderr could still do it; that one cannot.

There is one configuration that does make the suite unstable, and it is worth knowing about independently of this test: a .env at the repo root. tests/conftest.py calls load_dotenv(), so HYPHA_TOKEN becomes available and roughly twenty-two tests/apps/model-runner tests that otherwise skip start running against the live cluster. Suite runtime goes from about 30 seconds to about 330, and a live-cluster test failed on the fourth run. The shared checkout has a .env; a git worktree does not. Runtime is the cheap tell — a 30-second run and a 330-second run are not measuring the same thing, and a suite number quoted without it is ambiguous.

Not touched here

ProxyDeployment.__del__ is an async def. Ray Serve awaits async destructors on replica shutdown, which is why it is written that way, but under ordinary CPython garbage collection the destructor returns a coroutine that nothing awaits, so the body never runs at all — no deregistration, no disconnect — and a warning is emitted wherever the collector happened to fire. The full suite produces ten of these per run, attributed to scattered unrelated call sites. That is real cross-test noise and the deregistration path is load-bearing elsewhere, so it wants its own change rather than a drive-by in a test-only PR.

Also left alone: test_a_dry_run_neither_creates_the_workspace_nor_needs_the_runtime asserts result.output.startswith("podman run"), which has the same exposure to merged output at the front rather than the back. It has never been reported failing and rewriting it on an unreproduced mechanism would be speculative, so it is noted rather than changed.

🤖 Generated with Claude Code