Skip to content

test harness: env-overridable container ports + deterministic host load - #537

Merged
WilfordGrimley merged 1 commit into
masterfrom
fix/test-harness-isolation
Jul 29, 2026
Merged

test harness: env-overridable container ports + deterministic host load#537
WilfordGrimley merged 1 commit into
masterfrom
fix/test-harness-isolation

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Test infrastructure only. No production module is touched; no envelope threshold is changed.

Several agents share this box and keep colliding in the test harness. Two distinct causes, both reproduced live tonight.

Problem 1 — fixed container host ports

conftest.POSTGRES_PORT/ELASTICSEARCH_PORT were hardcoded to 47000/9300. A second concurrent suite dies at container start with Bind for 0.0.0.0:9300 failed: port is already allocated, which surfaces as ~2500 collection ERRORs — reading as catastrophic breakage rather than a port clash.

Both are now environment-overridable, defaulting to the historical values so CI and a single local run are byte-identical:

POSTGRES_PORT = int(os.environ.get("TEST_POSTGRES_PORT", 47000))
ELASTICSEARCH_PORT = int(os.environ.get("TEST_ELASTICSEARCH_PORT", 9300))
TEST_POSTGRES_PORT=47010 TEST_ELASTICSEARCH_PORT=9310 pytest cardpicker/tests/

The Elasticsearch half works too. The old comment warned that 9300 "is the default expected by elasticsearch_nooproc". pytest_elasticsearch.config.get_config reads config.getoption("elasticsearch_port") or config.getini("elasticsearch_port") and falls back to a hardcoded 9300, so a new conftest.pytest_configure threads our value into the plugin's own config. Nothing in this suite currently resolves elasticsearch_nooproc (the local elasticsearch fixture shadows the plugin's and never requests the noproc process), so this is belt-and-braces — but without it, TEST_ELASTICSEARCH_PORT would leave a latent 9300 behind for the first test that eventually does.

pytest-xdist worker-id offsets were considered and skipped: xdist is not installed here, and a second mechanism silently competing with the env override would be worse than either alone.

Problem 2 — tests sampled the real host load average

stage_e_dispatch._sample_envelope_signals samples the real os.getloadavg()[0] before every dispatch decision — correct in production, but it made every Stage E dispatch test a function of whatever else was running on the machine. Above HOST_LOAD_CEILING (7.0) they return halted-new-trip and fail; below it they pass. These are the worst kind of flake: plausible, specific, and entirely about the neighbouring process.

New autouse deterministic_host_load fixture in conftest.py pins os.getloadavg() to 0.5 for every test, with a @pytest.mark.real_host_load opt-out (registered in pytest.ini; nothing uses it yet).

The seam is os.getloadavg itself, not _sample_envelope_signals, deliberately. Stubbing the whole sampler would also flatten _window.failures_and_total() and get_process_rss_mb(), which several dispatch tests legitimately drive (the fetch-failure-rate bar in particular). Only the ambient host signal is pinned; every other envelope input still comes from the real code path.

HOST_LOAD_CEILING, RSS_MB_PER_WORKER_CEILING and every other ratified threshold are untouched, as is production sampling. No production code has a new seam.

Which tests were affected

Found by pinning the load high (TEST_HOST_LOAD_AVG=8.67, the value observed in the wild) rather than waiting for a busy box — 37 tests fail halted-new-trip, more than the 18 originally reported:

file count
test_stage_e_dispatch.py 25
test_stage_e_shakedown.py 6
test_stream_full_catalog.py 6

All 37 pass with the fixture's default. No test was found that deliberately asserts real-load sampling — the tests that pin envelope inputs (test_operating_envelope.py, test_resolve_envelope_trip.py, and the existing monkeypatch.setattr(..., "_sample_envelope_signals", ...) sites) construct EnvelopeSignals directly and never reach os.getloadavg. test_process_metrics.py samples real RSS, not load, and is unaffected.

New tests

cardpicker/tests/test_harness_isolation.py (9 tests) exists so neither fix can rot into a silent no-op — a harness fix that has stopped taking effect is worse than no fix, because the suite stays green and the flakes come back looking like product bugs. Notably it asserts the pinned value is what the production sampler returns (_sample_envelope_signals().load_avg), not merely that the fixture ran; that the Django DB settings and the ES DSL actually follow the overridden ports; and that the real_host_load opt-out genuinely restores real sampling (compared against /proc/loadavg, so a stub that merely forwards is still distinguishable).

Verification

run result
origin/master, default ports, real host load ~2.0 2503 passed, 7 skipped, 0 failed
this branch, ports 47010/9310 2512 passed, 7 skipped, 0 failed (+9 new harness tests)
this branch, TEST_HOST_LOAD_AVG=8.67 38 failed (37 load-sensitive + the guard test asserting the shipped default; that one now skips when the variable is set)
two full suites concurrently, 47010/9310 and 47020/9320 both 2512 passed, 7 skipped, 0 failed — zero port collisions

Two independent live confirmations of problem 1 during this work:

  • An unmodified origin/master run started while another agent held 9300 produced 29 errors, every one Bind for 0.0.0.0:9300 failed: port is already allocated.
  • A later run of this branch deliberately left on the default ports was killed the same way mid-session when another agent grabbed 9300 — while its sibling on 9320 completed clean. Same commit, same host, same minute; only the port differed.

All pre-commit hooks pass (ruff, isort, black, mypy, prettier).

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

Test infrastructure only - no production module is touched, and no envelope
threshold is changed.

Two ways concurrent runs on one host were corrupting each other's results,
both observed live on this box tonight:

1. FIXED CONTAINER PORTS. `conftest.POSTGRES_PORT`/`ELASTICSEARCH_PORT` were
   hardcoded to 47000/9300, so a second suite died at container start with
   `Bind for 0.0.0.0:9300 failed: port is already allocated` - which surfaces
   as ~2500 collection ERRORs that read as catastrophic breakage rather than
   as a port clash. Both are now `int(os.environ.get("TEST_<X>_PORT", <old>))`,
   defaulting to the historical values, so CI and a single local run are
   byte-identical.

   `conftest.pytest_configure` also threads the port into pytest-elasticsearch's
   own config, which is where `elasticsearch_nooproc` reads it from (falling
   back to a hardcoded 9300 - the "default expected by `elasticsearch_nooproc`"
   the old comment silently relied on). Nothing resolves that fixture today;
   this stops `TEST_ELASTICSEARCH_PORT` leaving a latent 9300 behind for the
   first test that eventually does.

2. AMBIENT HOST LOAD. `stage_e_dispatch._sample_envelope_signals` samples the
   real `os.getloadavg()[0]` before every dispatch decision, exactly as it
   should in production - which made every Stage E dispatch test a function of
   whatever else was running on the machine. Above `HOST_LOAD_CEILING` (7.0)
   they return `halted-new-trip` and fail; below it they pass. New autouse
   `deterministic_host_load` fixture pins `os.getloadavg()` to 0.5 for the
   duration of every test, with a `@pytest.mark.real_host_load` opt-out
   (registered in pytest.ini; nothing uses it yet).

   The seam is `os.getloadavg` rather than `_sample_envelope_signals`
   deliberately: stubbing the whole sampler would also flatten
   `_window.failures_and_total()` and `get_process_rss_mb()`, which several
   dispatch tests legitimately drive. Only the ambient host signal is pinned.

   `HOST_LOAD_CEILING`, `RSS_MB_PER_WORKER_CEILING` and every other ratified
   threshold are untouched, as is production sampling.

`cardpicker/tests/test_harness_isolation.py` covers both fixes so neither can
rot into a silent no-op - in particular it asserts the pinned load is what the
PRODUCTION sampler returns, not merely that the fixture ran.

Evidence:
  - origin/master, default ports, real load ~2.0: 2503 passed, 7 skipped.
  - this branch: 2512 passed, 7 skipped (the 9 new harness tests).
  - `TEST_HOST_LOAD_AVG=8.67` on this branch: 37 tests fail `halted-new-trip`
    (25 in test_stage_e_dispatch.py, 6 in test_stage_e_shakedown.py, 6 in
    test_stream_full_catalog.py) - the exact class of flake this fixes,
    reproduced deterministically instead of waiting for a busy box.
  - two full suites run CONCURRENTLY on 47010/9310 and 47020/9320: both
    2512 passed, 7 skipped, zero port collisions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
@WilfordGrimley
WilfordGrimley merged commit 753ecfc into master Jul 29, 2026
5 checks passed
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…RSS sample (#547)

* test: un-vacuum current_trip's ordering test, and derive the remaining envelope signal literals from their ceilings

THE BUG. test_operating_envelope.py::TestCurrentTrip::
test_returns_the_most_recent_open_trip_when_several_exist has been green and
VACUOUS since 70225df raised RSS_MB_PER_WORKER_CEILING 512->768. It fed a
literal `rss_mb_per_worker=600.0`, chosen to clear the old 512 ceiling; against
768 `check_envelope` returns None, so `second` was None. `first` was
acknowledged on the line above, so `current_trip()` was None too, and the whole
assertion was `None == None`. The test created ZERO open trips and verified
nothing about "the most recent of several". This is the same rot ea72abf fixed
in this file's other RSS tests - it just went silent here instead of red,
because the trip it failed to create was also the trip being asserted on.

Two separate defects, both fixed:

1. The literal. `600.0` -> `RSS_MB_PER_WORKER_CEILING + 0.1`, the ceiling-
   relative form ea72abf already established next door, plus an explicit
   `assert second is not None`-style guard (the open-trip count) so this can
   never go silent again.

2. The test could not fail even with the literal repaired. It acknowledged
   `first` before creating `second`, so only ONE trip was ever open and the
   ordering in its own name was never exercised - a `current_trip` that returned
   the OLDEST open trip would still have passed. Both trips now stay open, the
   count is asserted, and the acknowledged-trip case the old body actually
   covered is kept under its own honest name
   (test_an_acknowledged_trip_does_not_shadow_a_later_one).

MUTATION-VERIFIED. With `current_trip` temporarily mutated to
`order_by("tripped_at")` (oldest open trip, not newest):
  - the fixed test FAILS, and is the only failure in the file (1 failed, 29
    passed);
  - the pre-fix body PASSES (blind);
  - the pre-fix body with ONLY the literal repaired also PASSES (still blind,
    because of defect 2).
Mutation reverted; operating_envelope.py is untouched by this commit.

AUDIT of every other threshold-relative signal literal in this file and in
test_resolve_envelope_trip.py. Two more converted, both because they are
"chosen relative to a bar" rather than "obviously miles from a bar":

  - TestHostLoadBar: `load_avg=7.1` -> `HOST_LOAD_CEILING + 0.1`, and the test
    renamed test_trips_above_7_0 -> test_trips_just_above_the_host_load_ceiling
    (ea72abf's own precedent for a name that hardcodes a ratified number).
  - TestFetchFailureRateBar: `5/500` and `6/500` now derive from
    FETCH_FAILURE_RATE_CEILING * FETCH_FAILURE_WINDOW via a small helper. The
    `exactly at the ceiling` case is the file's other latent silent-vacuity
    risk: it asserts `is None`, so if the rate ceiling ever rose it would stay
    GREEN while no longer sitting on the `>` vs `>=` boundary it exists to pin.
    The helper's caller asserts `failures / WINDOW == CEILING` as a tether, so a
    future non-exact ceiling/window pair fails loudly instead.

Everything else audited and deliberately left alone: values that are orders of
magnitude clear of a bar (1.0 / 100.0 / 190.0 load-and-RSS clear cases) or
grossly across one (8.0, 9.0, 99.0, 99999.0, 1-in-10, 500-of-500). None can go
silent - each sits under an `assert trip is not None` or an `is None` that fails
LOUDLY the moment a ceiling move invalidates it, which is the distinction that
matters. test_resolve_envelope_trip.py was already fully ceiling-derived
(ea72abf) and needs no change.

No production code touched. No ratified threshold changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

* test harness: pin the envelope's RSS sample, the second ambient sensor #537 left live

#537 pinned `os.getloadavg` so no Stage E dispatch test depends on ambient host
load. The envelope has ONE gate with TWO ambient sensors, and the other one was
still live: `stage_e_dispatch._sample_envelope_signals` reads this process's
real RSS before every dispatch decision, so the same 37 tests remained a
function of the pytest process's own memory against the ratified 768MB bar. The
margin is comfortable today (~348MB VmHWM, ~2.2x) but it is a margin, not a
guarantee - it moves with fixture growth and with the other agents sharing this
box, and an RSS-driven failure looks exactly like an envelope bug.

Same shape as #537: an autouse `deterministic_process_rss` fixture, a
`TEST_PROCESS_RSS_MB` env knob (default 128.0), a `real_process_rss` opt-out
marker registered in pytest.ini, and harness tests that stop the fix rotting
into a silent no-op. Same three files, nothing else.

THE SEAM IS `cardpicker.stage_e_dispatch.get_process_rss_mb`, NOT
`cardpicker.process_metrics.get_process_rss_mb`, and getting that wrong makes
the whole change a no-op. #537 could patch `os.getloadavg` because
`stage_e_dispatch` does `import os` and resolves the attribute at CALL time.
RSS is different: stage_e_dispatch.py:117 does `from cardpicker.process_metrics
import get_process_rss_mb`, binding the function object into its own namespace
at IMPORT time, so patching `process_metrics` afterwards rebinds a name nobody
reads. That one module-local name covers both consumers - the envelope sample
(:275) and the ledger's `peak_rss_mb` (:971) - and `stage_e_dispatch` is the
only production importer of the helper in the tree.

Proven to bite, not merely to run:
  - `test_stub_reaches_the_production_sampler` asserts
    `stage_e_dispatch._sample_envelope_signals().rss_mb_per_worker ==
    TEST_PROCESS_RSS_MB`. Repointing the fixture at `process_metrics` turns that
    red (verified locally, along with three of its neighbours) - which is
    exactly the seam mistake it exists to catch.
  - `test_patching_process_metrics_instead_would_not_bite` asserts the negative
    half directly, so the reasoning survives in executable form.
  - `TEST_PROCESS_RSS_MB=900` fails 25 tests in test_stage_e_dispatch, 6 in
    test_stage_e_shakedown and 6 in test_stream_full_catalog - the measured
    RSS-sensitive set, confirming the pin reaches the gate.

`test_process_metrics.py` stays honest for free: it imports the helper from
`process_metrics` directly and never reaches `stage_e_dispatch`, so it keeps
sampling real /proc RSS with no opt-out needed. Verified falsifiably - it still
passes its `rss_mb > 0` assertion under `TEST_PROCESS_RSS_MB=-1`, which would be
red if the pin reached it - and `test_process_metrics_own_tests_still_sample_
real_rss` now locks that in so nobody widens the patch to the shared module
later. `run_image_evidence_cohort._get_rss_mb` is a separate duplicated
implementation and is untouched.

`test_other_envelope_signals_are_still_sampled_for_real` updated: it asserted
RSS came from /proc, which is no longer true under test, so it now pins the
fetch-failure window instead - still guarding the thing that assertion was for
(a future change stubbing `_sample_envelope_signals` wholesale and silently
flattening the fetch-rate bar).

Test-side only. Production sampling behaviour is unchanged - the envelope still
samples for real in production. No ratified threshold changed; no production
module touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

* test harness: correct the RSS-sensitive test count in the two comments that quote it

Comment-only follow-up to the previous commit (same task, split out rather than
amended so the pushed history is never rewritten).

Both comments quoted "37 tests are RSS-sensitive: 25/6/6", the figure measured
during PR #542's investigation. Re-measured on current master with
TEST_PROCESS_RSS_MB=900 it is 38: 25 in test_stage_e_dispatch, 6 in
test_stage_e_shakedown, 7 in test_stream_full_catalog - #545 added one to
test_stream_full_catalog in between. The conftest comment now says so, points at
the override as the way to re-measure rather than trusting a number in a
comment, and the test_harness_isolation docstring simply drops the count.

No behaviour change; comments only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…s cannot collide (#571)

The session-scoped testcontainers in `cardpicker/tests/conftest.py` bound
FIXED host ports (47000 / 9300). A second `pytest cardpicker/` on this
shared box died at container start with `Bind for 0.0.0.0:9300 failed:
port is already allocated`, which surfaces as thousands of collection
ERRORs that read as catastrophic breakage rather than a port clash - three
separate sessions lost real time to it in one day.

PR #537 made those ports env-overridable, but nothing ASSIGNED distinct
values, so the default still collided. Overridability without allocation
does not solve concurrency.

Both containers are now started with no host binding at all (their
testcontainers constructors already call `with_exposed_ports`), so Docker
assigns a free ephemeral port per container and the fixtures read back
what it actually assigned via `get_exposed_port()`. Nothing wants a
specific port, so nothing can collide - a closed race, not a narrowed
window. New session fixtures `postgres_port` / `elasticsearch_port`
publish the resolved values; Django's `DATABASES["default"]["PORT"]`,
`ELASTICSEARCH_DSL`, `settings.ELASTICSEARCH_PORT` and the
pytest-elasticsearch plugin's own `elasticsearch_port` option all follow
them, so nothing can be left dialling a stale 9300.

`TEST_POSTGRES_PORT` / `TEST_ELASTICSEARCH_PORT` still pin a deterministic
host port for CI, debugging, or attaching a client; each is independent,
and setting one re-introduces the collision risk for that run by design.
Isolation is unchanged - every run still gets its own containers, its own
`test_*` database and its own index.

`test_harness_isolation.py` is extended rather than relaxed: it now pins
that the DEFAULT requests no host binding (the assertion that actually
closes the race), that both overrides are still honoured when set, and
that every consumer follows the port Docker really assigned. Port-map keys
are normalised to `str` because testcontainers is unpinned and 4.14.x /
4.15.x disagree on the key type.

Docs: `docs/troubleshooting.md`'s collision entry is rewritten as FIXED
with the new allocation described, keeping only the parts that are still
true (general resource contention on a shared box, which ephemeral ports
do not address). `docs/lessons.md`'s "different ports (47000/9300)" aside
is corrected - its point (testcontainers never touch the prod 5432/9200)
is unchanged.


Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant