From ce3f55a4bb741a695810b3ed72064788f798cb01 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 12:51:05 -0400 Subject: [PATCH 01/24] docs(parametric): add test-agent reuse POC plan --- .../2026-06-29-parametric-test-agent-reuse.md | 662 ++++++++++++++++++ 1 file changed, 662 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md diff --git a/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md b/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md new file mode 100644 index 00000000000..8fec1c13bea --- /dev/null +++ b/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md @@ -0,0 +1,662 @@ +# Parametric Test-Agent Reuse (POC) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Cut parametric-scenario container churn by reusing one test-agent (and one Docker network) per xdist worker, resetting agent state with a synchronous `/test/session/clear` between tests, instead of creating a fresh network + test-agent per test. + +**Architecture:** Today each parametric test creates a fresh Docker bridge network, a fresh `ddapm-test-agent` container, and a fresh library-client container, then tears them all down (`utils/_context/_scenarios/_docker_fixtures.py:56-98`). This POC introduces a per-worker `WorkerAgentPool` that lazily creates one network + one test-agent per `(worker, agent_env)` key, reuses them across tests, and calls `clear()` before each test. The library client is unchanged — it already derives both its network (`test_agent.network`) and agent URL (`DD_TRACE_AGENT_URL=http://{test_agent.container_name}:8126`) from the `test_agent` object it is handed (`utils/docker_fixtures/_test_clients/_test_client_parametric.py:55,90`), so a pooled agent is transparent to it. Snapshot-marked tests fall back to the existing fresh-per-test path. + +**Tech Stack:** Python 3.12, pytest + pytest-xdist, docker-py (`utils/docker_fixtures/_core.py`), the `ghcr.io/datadog/dd-apm-test-agent` image. + +## Global Constraints + +- **Cross-tracer blast radius:** this code is shared by all 9 tracers' parametric suites. Behavior for non-pooled paths must be byte-for-byte unchanged. Verbatim rule: do not alter `get_test_agent_api`'s existing fresh-path semantics — only add a parallel pooled path. +- **No state leakage:** a reused agent MUST start each test with no data from a prior test. The reset is `TestAgentAPI.clear()` (`utils/docker_fixtures/_test_agent.py:375-377`), called before the test runs. +- **POC scope (explicit):** pool only when the test has **no `snapshot` mark**. Snapshot-marked tests use the existing fresh path. Pool entries are keyed by the full `agent_env` dict, so tests with differing `agent_env` get separate pooled agents. +- **xdist semantics:** a pytest `scope="session"` fixture runs once **per worker** under `pytest-xdist`. That is the mechanism for "one agent per worker." +- **Default config preserved:** `agent_env` defaults to `{}` (`tests/parametric/conftest.py:38`); the vast majority of parametric tests use it, so a single pooled agent per worker covers them. +- Frequent commits: one per task minimum. + +--- + +### Task 1: `WorkerAgentPool` + `AgentLease` (pure cache logic, no Docker) + +The reuse/keying/reset logic, isolated from Docker so it is unit-testable with a fake creator. + +**Files:** +- Create: `utils/docker_fixtures/_test_agent_pool.py` +- Test: `tests/test_the_test/test_test_agent_pool.py` + +**Interfaces:** +- Consumes: nothing (pure logic). Creator and agent objects are injected. +- Produces: + - `agent_env_key(agent_env: dict[str, str]) -> tuple` — stable, hashable key for an env dict. + - `class AgentLease` with attributes `api`, `stop` (a zero-arg callable that tears the lease down). + - `class WorkerAgentPool(creator: Callable[[dict[str, str]], AgentLease])` with: + - `acquire(request, agent_env: dict[str, str]) -> Any` — returns the lease's `api`, creating on miss and calling `api.clear()` + `api.rebind_request(request)` on hit. + - `shutdown() -> None` — calls every lease's `stop()` and empties the cache. + +- [ ] **Step 1: Write the failing test** + +```python +# tests/test_the_test/test_test_agent_pool.py +from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, agent_env_key + + +class _FakeApi: + def __init__(self) -> None: + self.clear_calls = 0 + self.rebind_calls: list[object] = [] + + def clear(self) -> None: + self.clear_calls += 1 + + def rebind_request(self, request: object) -> None: + self.rebind_calls.append(request) + + +class _FakeCreator: + """Records every creation and hands back a lease whose stop() is tracked.""" + + def __init__(self) -> None: + self.created_envs: list[dict] = [] + self.stopped = 0 + + def __call__(self, agent_env: dict[str, str]): + from utils.docker_fixtures._test_agent_pool import AgentLease + + self.created_envs.append(dict(agent_env)) + api = _FakeApi() + + def _stop() -> None: + self.stopped += 1 + + return AgentLease(api=api, stop=_stop) + + +def test_env_key_is_order_independent(): + assert agent_env_key({"A": "1", "B": "2"}) == agent_env_key({"B": "2", "A": "1"}) + + +def test_same_env_reuses_and_clears(): + creator = _FakeCreator() + pool = WorkerAgentPool(creator) + + api1 = pool.acquire(request="req1", agent_env={}) + api2 = pool.acquire(request="req2", agent_env={}) + + assert api1 is api2 # reused, not recreated + assert len(creator.created_envs) == 1 # created exactly once + assert api1.clear_calls == 1 # cleared on the reuse, not the first acquire + assert api1.rebind_calls == ["req2"] # rebound to the second test's request + + +def test_distinct_env_creates_separate_agents(): + creator = _FakeCreator() + pool = WorkerAgentPool(creator) + + a = pool.acquire(request="r", agent_env={}) + b = pool.acquire(request="r", agent_env={"DD_ENV": "prod"}) + + assert a is not b + assert len(creator.created_envs) == 2 + + +def test_shutdown_stops_every_lease(): + creator = _FakeCreator() + pool = WorkerAgentPool(creator) + pool.acquire(request="r", agent_env={}) + pool.acquire(request="r", agent_env={"DD_ENV": "prod"}) + + pool.shutdown() + + assert creator.stopped == 2 + # After shutdown the cache is empty: acquiring again creates anew. + pool.acquire(request="r", agent_env={}) + assert len(creator.created_envs) == 3 +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `python -m pytest tests/test_the_test/test_test_agent_pool.py -v` +Expected: FAIL — `ModuleNotFoundError: No module named 'utils.docker_fixtures._test_agent_pool'` + +- [ ] **Step 3: Write minimal implementation** + +```python +# utils/docker_fixtures/_test_agent_pool.py +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + + +def agent_env_key(agent_env: dict[str, str]) -> tuple: + """Stable, hashable, order-independent key for an agent_env dict.""" + return tuple(sorted(agent_env.items())) + + +@dataclass +class AgentLease: + """One running test-agent (+ its network) owned by the pool. + + `api` is a TestAgentAPI (duck-typed here so this module stays Docker-free). + `stop` tears the lease down (container + network); it must be idempotent-safe + to call exactly once at shutdown. + """ + + api: Any + stop: Callable[[], None] + + +class WorkerAgentPool: + """Per-worker cache of test-agents keyed by agent_env. + + First request for a given env creates a lease via `creator`; later requests + for the same env reuse it after `api.clear()` (reset state) and + `api.rebind_request()` (re-point per-test logging at the current test). + """ + + def __init__(self, creator: Callable[[dict[str, str]], AgentLease]) -> None: + self._creator = creator + self._leases: dict[tuple, AgentLease] = {} + + def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: + key = agent_env_key(agent_env) + lease = self._leases.get(key) + if lease is None: + lease = self._creator(agent_env) + self._leases[key] = lease + else: + lease.api.clear() + lease.api.rebind_request(request) + return lease.api + + def shutdown(self) -> None: + for lease in self._leases.values(): + lease.stop() + self._leases.clear() +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `python -m pytest tests/test_the_test/test_test_agent_pool.py -v` +Expected: PASS (4 passed) + +- [ ] **Step 5: Commit** + +```bash +git add utils/docker_fixtures/_test_agent_pool.py tests/test_the_test/test_test_agent_pool.py +git commit -m "feat(parametric): add WorkerAgentPool for test-agent reuse" +``` + +--- + +### Task 2: Extract `start_agent`/`stop_agent` + add `rebind_request` + +Refactor `TestAgentFactory.get_test_agent_api` so its container-create-and-wait body is reusable by the pool without forcing per-test teardown, and give `TestAgentAPI` a way to re-point its per-test log at the current test on reuse. + +**Files:** +- Modify: `utils/docker_fixtures/_test_agent.py` (factory `49-166`, `TestAgentAPI` `168-196`) + +**Interfaces:** +- Consumes: `docker_run`, `get_host_port` (`utils/docker_fixtures/_core.py`); `TestAgentAPI` (this file). +- Produces: + - `TestAgentAPI.rebind_request(self, request: pytest.FixtureRequest) -> None` — updates `self._pytest_request` and `self.log_path` to the given test. + - `TestAgentFactory.start_agent(self, *, request, worker_id, container_name, docker_network, agent_env, container_otlp_http_port, container_otlp_grpc_port) -> tuple[TestAgentAPI, Callable[[], None]]` — runs the container, waits until ready, returns the API plus a `stop()` callable that closes the log file and stops the container. Does **not** apply snapshot marks (the pooled path never carries them). + +- [ ] **Step 1: Write the failing test (Docker-backed smoke)** + +```python +# tests/test_the_test/test_start_stop_agent.py +import pytest + +pytestmark = pytest.mark.skipif( + __import__("os").getenv("DOCKER_SMOKE") != "1", + reason="Docker-backed; run with DOCKER_SMOKE=1", +) + + +def test_start_agent_then_stop(request): + from utils._context._scenarios import scenarios + + factory = scenarios.parametric._test_agent_factory + factory.configure("logs_parametric") + factory.pull() + + network = __import__("utils.docker_fixtures._core", fromlist=["get_docker_client"]) + client = network.get_docker_client().networks.create(name="reuse_smoke_net", driver="bridge") + try: + api, stop = factory.start_agent( + request=request, + worker_id="gw0", + container_name="ddapm-test-agent-reuse-smoke", + docker_network=client.name, + agent_env={}, + container_otlp_http_port=4318, + container_otlp_grpc_port=4317, + ) + try: + assert api.info()["version"] == "test" # agent answered → it is ready + finally: + stop() + finally: + client.remove() +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v` +Expected: FAIL — `AttributeError: 'TestAgentFactory' object has no attribute 'start_agent'` + +- [ ] **Step 3: Add `rebind_request` to `TestAgentAPI`** + +Insert this method into `TestAgentAPI` immediately after `__init__` (after `utils/docker_fixtures/_test_agent.py:196`): + +```python + def rebind_request(self, pytest_request: "pytest.FixtureRequest") -> None: + """Re-point per-test API logging at the current test (used on reuse).""" + self._pytest_request = pytest_request + self.log_path = ( + f"{self._host_log_folder}/outputs/{pytest_request.cls.__name__}" + f"/{pytest_request.node.name}/agent_api.log" + ) + Path(self.log_path).parent.mkdir(parents=True, exist_ok=True) +``` + +For `rebind_request` to work, `__init__` must remember `host_log_folder`. Add this line in `TestAgentAPI.__init__`, right after `self.network = network` (`utils/docker_fixtures/_test_agent.py:185`): + +```python + self._host_log_folder = host_log_folder +``` + +- [ ] **Step 4: Add `start_agent`/`stop_agent` to `TestAgentFactory`** + +Add these methods to `TestAgentFactory` (after `pull`, before `get_test_agent_api`, around `utils/docker_fixtures/_test_agent.py:71`). This is the existing container-run-and-wait body lifted out of the context manager, returning an explicit `stop` callable instead of relying on `with`: + +```python + def start_agent( + self, + *, + request: pytest.FixtureRequest, + worker_id: str, + container_name: str, + docker_network: str, + agent_env: dict[str, str], + container_otlp_http_port: int, + container_otlp_grpc_port: int, + ) -> "tuple[TestAgentAPI, Callable[[], None]]": + env = { + "ENABLED_CHECKS": "trace_count_header", + "OTLP_HTTP_PORT": str(container_otlp_http_port), + "OTLP_GRPC_PORT": str(container_otlp_grpc_port), + "VCR_CASSETTES_DIRECTORY": "/vcr-cassettes", + } + if os.getenv("DEV_MODE") is not None: + env["SNAPSHOT_CI"] = "0" + env |= agent_env + + host_port = get_host_port(worker_id, 4600) + otlp_http_host_port = get_host_port(worker_id, 4701) + otlp_grpc_host_port = get_host_port(worker_id, 4802) + + log_path = f"{self.host_log_folder}/outputs/{request.cls.__name__}/{request.node.name}/agent_log.log" + Path(log_path).parent.mkdir(parents=True, exist_ok=True) + log_file = open(log_path, "w+", encoding="utf-8") # noqa: SIM115 + + cm = docker_run( + image=self.image, + name=container_name, + env=env, + volumes={ + "./snapshots": "/snapshots", + "./tests/integration_frameworks/utils/vcr-cassettes": "/vcr-cassettes", + }, + ports={ + f"8126/tcp": host_port, + f"{container_otlp_http_port}/tcp": otlp_http_host_port, + f"{container_otlp_grpc_port}/tcp": otlp_grpc_host_port, + }, + log_file=log_file, + network=docker_network, + ) + cm.__enter__() + + client = TestAgentAPI( + container_name, + 8126, + self.host_log_folder, + pytest_request=request, + otlp_http_host_port=otlp_http_host_port, + host_port=host_port, + network=docker_network, + ) + time.sleep(0.2) # the trace agent takes ~200ms to start + expected_version = agent_env.get("TEST_AGENT_VERSION", "test") + for _ in range(100): + try: + resp = client.info() + except Exception as e: # noqa: BLE001 + logger.debug(f"Wait for 0.1s for the test agent to be ready {e}") + time.sleep(0.1) + else: + if resp["version"] != expected_version: + pytest.fail( + f"Agent version {resp['version']} is running instead of the test agent.", + pytrace=False, + ) + logger.info("Test agent is ready") + break + else: + log_file.close() + pytest.fail(f"Could not connect to test agent, check the log file {log_path}.", pytrace=False) + + def _stop() -> None: + try: + cm.__exit__(None, None, None) + finally: + log_file.close() + + return client, _stop +``` + +Add the `Callable` import at the top of the file (after `utils/docker_fixtures/_test_agent.py:2`): + +```python +from collections.abc import Generator, Callable +``` + +(The file already imports `Generator` on line 2 — replace that single import line with the line above.) + +- [ ] **Step 5: Run test to verify it passes** + +Run: `DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v` +Expected: PASS (1 passed). The agent container starts, `info()` returns `version == "test"`, then stops cleanly. + +- [ ] **Step 6: Verify the fresh path still works (no regression)** + +Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context` +Expected: `1 passed`. (Confirms the refactor didn't break the existing per-test path, which still uses `get_test_agent_api`.) + +- [ ] **Step 7: Commit** + +```bash +git add utils/docker_fixtures/_test_agent.py tests/test_the_test/test_start_stop_agent.py +git commit -m "refactor(parametric): extract start_agent/stop_agent + rebind_request" +``` + +--- + +### Task 3: Worker-scoped network + pooled-agent creator on the scenario + +Give the scenario a `WorkerAgentPool` whose creator builds a worker-stable network + a `start_agent` lease. The network name is keyed by worker + env so two distinct envs on one worker get distinct networks. + +**Files:** +- Modify: `utils/_context/_scenarios/_docker_fixtures.py` (`34`, `56-98`) + +**Interfaces:** +- Consumes: `WorkerAgentPool`, `AgentLease`, `agent_env_key` (Task 1); `TestAgentFactory.start_agent` (Task 2); `get_docker_client` (`utils/docker_fixtures/_core.py`). +- Produces: + - `DockerFixturesScenario.get_agent_pool(self, worker_id: str) -> WorkerAgentPool` — lazily builds (once per process) a pool whose creator makes `f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}"` network + a `start_agent` lease named `f"ddapm-test-agent-worker-{worker_id}"`. + +- [ ] **Step 1: Add pool construction to the scenario** + +Add to `DockerFixturesScenario.__init__`, right after `self._test_agent_factory = TestAgentFactory(agent_image)` (`utils/_context/_scenarios/_docker_fixtures.py:34`): + +```python + self._agent_pool: "WorkerAgentPool | None" = None +``` + +Add these imports at the top of `utils/_context/_scenarios/_docker_fixtures.py` (with the other `from ...` lines): + +```python +from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, AgentLease, agent_env_key +``` + +Add this method to `DockerFixturesScenario` (after `_get_docker_network`, around `_docker_fixtures.py:74`): + +```python + def get_agent_pool(self, worker_id: str) -> WorkerAgentPool: + if self._agent_pool is None: + + def _creator(agent_env: dict[str, str]) -> AgentLease: + key = agent_env_key(agent_env) + network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" + network = get_docker_client().networks.create(name=network_name, driver="bridge") + container_name = f"ddapm-test-agent-worker-{worker_id}-{abs(hash(key))}" + api, stop_agent = self._test_agent_factory.start_agent( + request=self._pool_seed_request, + worker_id=worker_id, + container_name=container_name, + docker_network=network.name, + agent_env=agent_env, + container_otlp_http_port=4318, + container_otlp_grpc_port=4317, + ) + + def _stop() -> None: + try: + stop_agent() + finally: + try: + network.remove() + except Exception as e: # noqa: BLE001 + logger.info(f"Failed to remove worker network, ignoring: {e}") + + return AgentLease(api=api, stop=_stop) + + self._agent_pool = WorkerAgentPool(_creator) + return self._agent_pool +``` + +`start_agent` needs a `request` for the first-test log path. The pooled agent's container log spans the whole worker, so seed it with the first test's request and let `rebind_request` re-point the per-test API log thereafter. Add an attribute set by the fixture before first acquire — declare it in `__init__` after the `_agent_pool` line: + +```python + self._pool_seed_request = None +``` + +- [ ] **Step 2: Verify it imports and the fresh path is untouched** + +Run: `python -c "from utils._context._scenarios import scenarios; print(type(scenarios.parametric).__mro__[1].__name__)"` +Expected: prints `DockerFixturesScenario` with no import error. + +Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context` +Expected: `1 passed` (fresh path still default — nothing calls the pool yet). + +- [ ] **Step 3: Commit** + +```bash +git add utils/_context/_scenarios/_docker_fixtures.py +git commit -m "feat(parametric): pooled-agent creator (worker-scoped network + agent)" +``` + +--- + +### Task 4: Wire the pool into the `test_agent` fixture (with snapshot fallback) + +Make `test_agent` use the pool for non-snapshot tests and tear the pool down once per worker at session end. + +**Files:** +- Modify: `tests/parametric/conftest.py` (`76-93`) + +**Interfaces:** +- Consumes: `scenarios.parametric.get_agent_pool` (Task 3); `WorkerAgentPool.acquire/shutdown` (Task 1). +- Produces: a session-scoped `test_agent_pool` fixture and a pool-aware `test_agent` fixture. + +- [ ] **Step 1: Add a session-scoped pool-lifecycle fixture** + +Add to `tests/parametric/conftest.py` (near the other fixtures, after `agent_env`): + +```python +@pytest.fixture(scope="session") +def test_agent_pool(worker_id: str): + # scope="session" under pytest-xdist == once per worker. + pool = scenarios.parametric.get_agent_pool(worker_id) + yield pool + pool.shutdown() +``` + +- [ ] **Step 2: Replace the `test_agent` fixture body with the pooled-or-fresh decision** + +Replace the existing `test_agent` fixture (`tests/parametric/conftest.py:76-93`) with: + +```python +@pytest.fixture +def test_agent( + worker_id: str, + test_id: str, + request: pytest.FixtureRequest, + agent_env: dict[str, str], + test_agent_otlp_http_port: int, + test_agent_otlp_grpc_port: int, + test_agent_pool, +) -> Generator[TestAgentAPI, None, None]: + # POC: pool everything except snapshot-marked tests (which need per-test + # snapshot_context lifecycle). Pooled agents are reset with clear() between tests. + is_snapshot = request.node.get_closest_marker("snapshot") is not None + if not is_snapshot: + scenarios.parametric._pool_seed_request = request + api = test_agent_pool.acquire(request=request, agent_env=agent_env) + api.clear() # ensure a clean slate even on the very first acquire + yield api + return + + with scenarios.parametric.get_test_agent_api( + request=request, + worker_id=worker_id, + test_id=test_id, + agent_env=agent_env, + container_otlp_http_port=test_agent_otlp_http_port, + container_otlp_grpc_port=test_agent_otlp_grpc_port, + ) as result: + yield result +``` + +- [ ] **Step 3: Run the otel file — every test must pass through the pooled path** + +Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py` +Expected: same pass/skip/xfail counts as a baseline `git stash` run of the same command (e.g. `57 passed, 2 skipped, 2 xfailed`). No new failures. + +- [ ] **Step 4: Commit** + +```bash +git add tests/parametric/conftest.py +git commit -m "feat(parametric): use WorkerAgentPool in test_agent fixture" +``` + +--- + +### Task 5: Acceptance — prove reuse happened and no state leaked + +Two things must be true: (a) the agent container was created roughly once per worker, not once per test; (b) results are unchanged (no cross-test leakage). + +**Files:** +- Modify: `utils/docker_fixtures/_test_agent.py` (add a one-line creation counter log in `start_agent`) + +**Interfaces:** +- Consumes: `start_agent` (Task 2). +- Produces: a greppable `REUSE-POC agent container created` log line per real container creation. + +- [ ] **Step 1: Add a creation-count log line** + +In `start_agent` (Task 2), immediately after `log_file = open(...)`, add: + +```python + logger.stdout(f"REUSE-POC agent container created: {container_name}") +``` + +- [ ] **Step 2: Run the otel file single-worker and count creations** + +Run: +```bash +TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -p no:randomly -n 1 2>&1 | tee /tmp/reuse_run.log +grep -c "REUSE-POC agent container created" /tmp/reuse_run.log +``` +Expected: the count equals the number of **distinct `agent_env` values** in that file (1 for the default-env otel tests), **not** the ~57 test count. Pre-change this line does not exist; the equivalent (fresh) run creates one agent per test. + +- [ ] **Step 3: Leakage check — trace counts stay correct under reuse** + +`test_otel_get_span_context` and `test_otel_span_operation_name` each assert exactly one trace is present. If `clear()` were incomplete, a reused agent would carry the previous test's trace and these would see 2. + +Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k "operation_name or get_span_context"` +Expected: all selected tests pass (each sees exactly its own single trace — proof the inter-test `clear()` reset the agent). + +- [ ] **Step 4: Full-file stability (run 3×, must be green each time)** + +Run: +```bash +for i in 1 2 3; do TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -q || echo "ITERATION $i FAILED"; done +``` +Expected: three clean runs, no "ITERATION n FAILED" line. + +- [ ] **Step 5: Commit** + +```bash +git add utils/docker_fixtures/_test_agent.py +git commit -m "test(parametric): log agent container creations for reuse verification" +``` + +--- + +### Task 6: Handoff note for the Platform team + +Per the thread (`#... ` Slack), this POC is handed to Platform to adopt or discard. Write the rationale + scope + risks so they can evaluate without re-deriving it. + +**Files:** +- Create: `docs/edit/parametric-test-agent-reuse-poc.md` + +- [ ] **Step 1: Write the handoff doc** + +```markdown +# POC: parametric test-agent reuse + +## Why +The parametric scenario creates a fresh Docker network + test-agent + client +container per test (16 xdist workers × ~440 tests). On the shared docker-in-docker +microVM runners this churn intermittently stalls container/network setup, so a +test-agent occasionally fails to make a just-sent trace queryable within the +tracer's ~3s receipt poll → `Number (1) of traces not available, got 0`. + +## What this changes +- One test-agent + one Docker network per (xdist worker, agent_env), reused + across tests, reset with `TestAgentAPI.clear()` (`/test/session/clear`) before + each test. Removes ~2 of the 3 create/destroy operations per test. +- The library client is unchanged: it derives its network and agent URL from the + `test_agent` object it is handed. + +## Scope / fallback +- Pools all non-snapshot tests; snapshot-marked tests keep the fresh-per-test path. +- Pool entries keyed by `agent_env`; differing envs get separate pooled agents. + +## Risks to weigh before adopting +- State leakage if `clear()` is ever incomplete (traces/telemetry/RC/OTLP/sessions). + Task 5's leakage check guards the trace case; extend it per data type before + rolling out to all tracers. +- In-flight data: a late trace from test N arriving after test N+1's `clear()` + could leak. Hardening path: per-test agent session tokens (requires client to + send `X-Datadog-Test-Session-Token`). +- Shared across 9 tracers — roll out behind a flag / one language at a time. +``` + +- [ ] **Step 2: Commit** + +```bash +git add docs/edit/parametric-test-agent-reuse-poc.md +git commit -m "docs(parametric): test-agent reuse POC handoff note" +``` + +--- + +## Self-Review + +**Spec coverage** (the Slack approach = "1:1 running-agent-to-worker mapping + synchronous reset between tests"): +- One agent per worker → Task 3 (worker-keyed creator) + Task 4 (`scope="session"` = per worker). ✓ +- Synchronous reset between tests → `clear()` on reuse in Task 1 + Task 4. ✓ +- Halve container count / reduce contention → Tasks 3–4 (also removes the per-test network); proven in Task 5. ✓ +- "Careful change / leakage risk" raised in thread → snapshot fallback (Task 4), leakage check (Task 5 Step 3), risks doc (Task 6). ✓ +- Hand POC to Platform → Task 6. ✓ + +**Placeholder scan:** no TBD/"handle edge cases"/"similar to". The one intentional knob — which tests are poolable — is concretely defined (`get_closest_marker("snapshot")`). + +**Type consistency:** `AgentLease(api, stop)`, `WorkerAgentPool(creator).acquire(request, agent_env)/shutdown()`, `agent_env_key(dict)->tuple`, `start_agent(...)->(TestAgentAPI, Callable)`, `rebind_request(request)` — used identically in Tasks 1–5. ✓ + +**Known limitation carried forward:** the pooled agent's *container* log (`agent_log.log`) spans the worker rather than one per test; per-test API interactions are re-pointed via `rebind_request`. Acceptable for a POC; noted in Task 6. From a8c2b826c4a8d5f0dd488b307ddc48ef655a5bde Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:04:42 -0400 Subject: [PATCH 02/24] feat(parametric): add WorkerAgentPool for test-agent reuse Co-Authored-By: Claude Sonnet 4.6 --- tests/test_the_test/test_test_agent_pool.py | 74 +++++++++++++++++++++ utils/docker_fixtures/_test_agent_pool.py | 50 ++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 tests/test_the_test/test_test_agent_pool.py create mode 100644 utils/docker_fixtures/_test_agent_pool.py diff --git a/tests/test_the_test/test_test_agent_pool.py b/tests/test_the_test/test_test_agent_pool.py new file mode 100644 index 00000000000..c06f14df6ac --- /dev/null +++ b/tests/test_the_test/test_test_agent_pool.py @@ -0,0 +1,74 @@ +from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, agent_env_key + + +class _FakeApi: + def __init__(self) -> None: + self.clear_calls = 0 + self.rebind_calls: list[object] = [] + + def clear(self) -> None: + self.clear_calls += 1 + + def rebind_request(self, request: object) -> None: + self.rebind_calls.append(request) + + +class _FakeCreator: + """Records every creation and hands back a lease whose stop() is tracked.""" + + def __init__(self) -> None: + self.created_envs: list[dict] = [] + self.stopped = 0 + + def __call__(self, agent_env: dict[str, str]): + from utils.docker_fixtures._test_agent_pool import AgentLease + + self.created_envs.append(dict(agent_env)) + api = _FakeApi() + + def _stop() -> None: + self.stopped += 1 + + return AgentLease(api=api, stop=_stop) + + +def test_env_key_is_order_independent(): + assert agent_env_key({"A": "1", "B": "2"}) == agent_env_key({"B": "2", "A": "1"}) + + +def test_same_env_reuses_and_clears(): + creator = _FakeCreator() + pool = WorkerAgentPool(creator) + + api1 = pool.acquire(request="req1", agent_env={}) + api2 = pool.acquire(request="req2", agent_env={}) + + assert api1 is api2 # reused, not recreated + assert len(creator.created_envs) == 1 # created exactly once + assert api1.clear_calls == 1 # cleared on the reuse, not the first acquire + assert api1.rebind_calls == ["req2"] # rebound to the second test's request + + +def test_distinct_env_creates_separate_agents(): + creator = _FakeCreator() + pool = WorkerAgentPool(creator) + + a = pool.acquire(request="r", agent_env={}) + b = pool.acquire(request="r", agent_env={"DD_ENV": "prod"}) + + assert a is not b + assert len(creator.created_envs) == 2 + + +def test_shutdown_stops_every_lease(): + creator = _FakeCreator() + pool = WorkerAgentPool(creator) + pool.acquire(request="r", agent_env={}) + pool.acquire(request="r", agent_env={"DD_ENV": "prod"}) + + pool.shutdown() + + assert creator.stopped == 2 + # After shutdown the cache is empty: acquiring again creates anew. + pool.acquire(request="r", agent_env={}) + assert len(creator.created_envs) == 3 diff --git a/utils/docker_fixtures/_test_agent_pool.py b/utils/docker_fixtures/_test_agent_pool.py new file mode 100644 index 00000000000..b8fc6ff36b7 --- /dev/null +++ b/utils/docker_fixtures/_test_agent_pool.py @@ -0,0 +1,50 @@ +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + + +def agent_env_key(agent_env: dict[str, str]) -> tuple: + """Stable, hashable, order-independent key for an agent_env dict.""" + return tuple(sorted(agent_env.items())) + + +@dataclass +class AgentLease: + """One running test-agent (+ its network) owned by the pool. + + `api` is a TestAgentAPI (duck-typed here so this module stays Docker-free). + `stop` tears the lease down (container + network); it must be idempotent-safe + to call exactly once at shutdown. + """ + + api: Any + stop: Callable[[], None] + + +class WorkerAgentPool: + """Per-worker cache of test-agents keyed by agent_env. + + First request for a given env creates a lease via `creator`; later requests + for the same env reuse it after `api.clear()` (reset state) and + `api.rebind_request()` (re-point per-test logging at the current test). + """ + + def __init__(self, creator: Callable[[dict[str, str]], AgentLease]) -> None: + self._creator = creator + self._leases: dict[tuple, AgentLease] = {} + + def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: + key = agent_env_key(agent_env) + lease = self._leases.get(key) + if lease is None: + lease = self._creator(agent_env) + self._leases[key] = lease + else: + lease.api.clear() + lease.api.rebind_request(request) + return lease.api + + def shutdown(self) -> None: + for lease in self._leases.values(): + lease.stop() + self._leases.clear() From d63db0ea21f0a29fdb921aee57d6726e3cd7b855 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:18:06 -0400 Subject: [PATCH 03/24] refactor(parametric): extract start_agent/stop_agent + rebind_request --- tests/test_the_test/test_start_stop_agent.py | 33 ++++ utils/docker_fixtures/_test_agent.py | 161 ++++++++++++------- 2 files changed, 134 insertions(+), 60 deletions(-) create mode 100644 tests/test_the_test/test_start_stop_agent.py diff --git a/tests/test_the_test/test_start_stop_agent.py b/tests/test_the_test/test_start_stop_agent.py new file mode 100644 index 00000000000..02ace62d2cd --- /dev/null +++ b/tests/test_the_test/test_start_stop_agent.py @@ -0,0 +1,33 @@ +import pytest + +pytestmark = pytest.mark.skipif( + __import__("os").getenv("DOCKER_SMOKE") != "1", + reason="Docker-backed; run with DOCKER_SMOKE=1", +) + + +def test_start_agent_then_stop(request): + from utils._context._scenarios import scenarios + + factory = scenarios.parametric._test_agent_factory + factory.configure("logs_parametric") + factory.pull() + + network = __import__("utils.docker_fixtures._core", fromlist=["get_docker_client"]) + client = network.get_docker_client().networks.create(name="reuse_smoke_net", driver="bridge") + try: + api, stop = factory.start_agent( + request=request, + worker_id="gw0", + container_name="ddapm-test-agent-reuse-smoke", + docker_network=client.name, + agent_env={}, + container_otlp_http_port=4318, + container_otlp_grpc_port=4317, + ) + try: + assert api.info()["version"] == "test" # agent answered → it is ready + finally: + stop() + finally: + client.remove() diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 1d4756bf229..f1579299a0c 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -1,5 +1,5 @@ import base64 -from collections.abc import Generator +from collections.abc import Generator, Callable import contextlib import datetime import gzip @@ -69,9 +69,9 @@ def pull(self) -> None: logger.stdout(f"Pull test agent image {self.image}...") get_docker_client().images.pull(self.image) - @contextlib.contextmanager - def get_test_agent_api( + def start_agent( self, + *, request: pytest.FixtureRequest, worker_id: str, container_name: str, @@ -79,76 +79,102 @@ def get_test_agent_api( agent_env: dict[str, str], container_otlp_http_port: int, container_otlp_grpc_port: int, - ) -> Generator["TestAgentAPI", None, None]: - # (meta_tracer_version_header) Not all clients (go for example) submit the tracer version - # (trace_content_length) go client doesn't submit content length header + ) -> "tuple[TestAgentAPI, Callable[[], None]]": env = { "ENABLED_CHECKS": "trace_count_header", "OTLP_HTTP_PORT": str(container_otlp_http_port), "OTLP_GRPC_PORT": str(container_otlp_grpc_port), - "VCR_CASSETTES_DIRECTORY": "/vcr-cassettes", # TODO comment + "VCR_CASSETTES_DIRECTORY": "/vcr-cassettes", } if os.getenv("DEV_MODE") is not None: env["SNAPSHOT_CI"] = "0" - env |= agent_env host_port = get_host_port(worker_id, 4600) - container_port = 8126 otlp_http_host_port = get_host_port(worker_id, 4701) otlp_grpc_host_port = get_host_port(worker_id, 4802) - log_path = f"{self.host_log_folder}/outputs/{request.cls.__name__}/{request.node.name}/agent_log.log" + cls_name = request.cls.__name__ if request.cls else "NoClass" + log_path = f"{self.host_log_folder}/outputs/{cls_name}/{request.node.name}/agent_log.log" Path(log_path).parent.mkdir(parents=True, exist_ok=True) - - with ( - open(log_path, "w+", encoding="utf-8") as log_file, - docker_run( - image=self.image, - name=container_name, - env=env, - volumes={ - "./snapshots": "/snapshots", - "./tests/integration_frameworks/utils/vcr-cassettes": "/vcr-cassettes", - }, - ports={ - f"{container_port}/tcp": host_port, - f"{container_otlp_http_port}/tcp": otlp_http_host_port, - f"{container_otlp_grpc_port}/tcp": otlp_grpc_host_port, - }, - log_file=log_file, - network=docker_network, - ), - ): - client = TestAgentAPI( - container_name, - container_port, - self.host_log_folder, - pytest_request=request, - otlp_http_host_port=otlp_http_host_port, - host_port=host_port, - network=docker_network, - ) - time.sleep(0.2) # initial wait time, the trace agent takes 200ms to start - expected_version = agent_env.get("TEST_AGENT_VERSION", "test") - for _ in range(100): - try: - resp = client.info() - except Exception as e: - logger.debug(f"Wait for 0.1s for the test agent to be ready {e}") - time.sleep(0.1) - else: - if resp["version"] != expected_version: - message = f"""Agent version {resp["version"]} is running instead of the test agent. - Stop the agent on port {container_port} and try again.""" - pytest.fail(message, pytrace=False) - - logger.info("Test agent is ready") - break + log_file = open(log_path, "w+", encoding="utf-8") # noqa: SIM115 + + cm = docker_run( + image=self.image, + name=container_name, + env=env, + volumes={ + "./snapshots": "/snapshots", + "./tests/integration_frameworks/utils/vcr-cassettes": "/vcr-cassettes", + }, + ports={ + f"8126/tcp": host_port, + f"{container_otlp_http_port}/tcp": otlp_http_host_port, + f"{container_otlp_grpc_port}/tcp": otlp_grpc_host_port, + }, + log_file=log_file, + network=docker_network, + ) + cm.__enter__() + + client = TestAgentAPI( + container_name, + 8126, + self.host_log_folder, + pytest_request=request, + otlp_http_host_port=otlp_http_host_port, + host_port=host_port, + network=docker_network, + ) + time.sleep(0.2) # the trace agent takes ~200ms to start + expected_version = agent_env.get("TEST_AGENT_VERSION", "test") + for _ in range(100): + try: + resp = client.info() + except Exception as e: # noqa: BLE001 + logger.debug(f"Wait for 0.1s for the test agent to be ready {e}") + time.sleep(0.1) else: - logger.error("Could not connect to test agent") - pytest.fail(f"Could not connect to test agent, check the log file {log_file.name}.", pytrace=False) + if resp["version"] != expected_version: + pytest.fail( + f"Agent version {resp['version']} is running instead of the test agent.", + pytrace=False, + ) + logger.info("Test agent is ready") + break + else: + log_file.close() + pytest.fail(f"Could not connect to test agent, check the log file {log_path}.", pytrace=False) + def _stop() -> None: + try: + cm.__exit__(None, None, None) + finally: + log_file.close() + + return client, _stop + + @contextlib.contextmanager + def get_test_agent_api( + self, + request: pytest.FixtureRequest, + worker_id: str, + container_name: str, + docker_network: str, + agent_env: dict[str, str], + container_otlp_http_port: int, + container_otlp_grpc_port: int, + ) -> Generator["TestAgentAPI", None, None]: + client, stop = self.start_agent( + request=request, + worker_id=worker_id, + container_name=container_name, + docker_network=docker_network, + agent_env=agent_env, + container_otlp_http_port=container_otlp_http_port, + container_otlp_grpc_port=container_otlp_grpc_port, + ) + try: # If the snapshot mark is on the test case then do a snapshot test marks = list(request.node.iter_markers(name="snapshot")) assert len(marks) <= 1, "Multiple snapshot marks detected" @@ -161,8 +187,11 @@ def get_test_agent_api( yield client else: yield client - - request.node.add_report_section("teardown", "Test Agent Output", f"Log file:\n./{log_path}") + finally: + cls_name = request.cls.__name__ if request.cls else "NoClass" + log_path = f"{self.host_log_folder}/outputs/{cls_name}/{request.node.name}/agent_log.log" + request.node.add_report_section("teardown", "Test Agent Output", f"Log file:\n./{log_path}") + stop() class TestAgentAPI: @@ -183,6 +212,7 @@ def __init__( self.container_name = container_name self.container_port = container_port self.network = network + self._host_log_folder = host_log_folder self.host = "localhost" self.host_port = host_port @@ -190,8 +220,19 @@ def __init__( self._session = requests.Session() self._pytest_request = pytest_request + cls_name = pytest_request.cls.__name__ if pytest_request.cls else "NoClass" + self.log_path = ( + f"{host_log_folder}/outputs/{cls_name}/{pytest_request.node.name}/agent_api.log" + ) + Path(self.log_path).parent.mkdir(parents=True, exist_ok=True) + + def rebind_request(self, pytest_request: "pytest.FixtureRequest") -> None: + """Re-point per-test API logging at the current test (used on reuse).""" + self._pytest_request = pytest_request + cls_name = pytest_request.cls.__name__ if pytest_request.cls else "NoClass" self.log_path = ( - f"{host_log_folder}/outputs/{pytest_request.cls.__name__}/{pytest_request.node.name}/agent_api.log" + f"{self._host_log_folder}/outputs/{cls_name}" + f"/{pytest_request.node.name}/agent_api.log" ) Path(self.log_path).parent.mkdir(parents=True, exist_ok=True) From d4680a1b51ff7738561f842c9c543db7c2942635 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:23:50 -0400 Subject: [PATCH 04/24] fix(_test_agent): address review findings on start_agent/get_test_agent_api - Fix container leak on readiness timeout: the for...else branch in start_agent now calls cm.__exit__() in a try/finally before pytest.fail, ensuring the container is stopped even when it never becomes ready. - Extract _agent_log_path() module-level helper to deduplicate the log path construction that previously existed independently in start_agent and get_test_agent_api's finally block. - Fix ruff F541 lint: f"8126/tcp" -> "8126/tcp" (f-string with no placeholder). Co-Authored-By: Claude Sonnet 4.6 --- .superpowers/sdd/task-2-report.md | 157 +++++++++++++++++++++++++++ utils/docker_fixtures/_test_agent.py | 18 ++- 2 files changed, 169 insertions(+), 6 deletions(-) create mode 100644 .superpowers/sdd/task-2-report.md diff --git a/.superpowers/sdd/task-2-report.md b/.superpowers/sdd/task-2-report.md new file mode 100644 index 00000000000..b88ccaaabf0 --- /dev/null +++ b/.superpowers/sdd/task-2-report.md @@ -0,0 +1,157 @@ +# Task 2 Report: Extract `start_agent`/`stop_agent` + add `rebind_request` + +## Status + +DONE_WITH_CONCERNS + +## Commit hash + +`d63db0ea2` + +## Files Modified + +- `utils/docker_fixtures/_test_agent.py` — extracted `start_agent`, added `rebind_request`, refactored `get_test_agent_api` to delegate +- `tests/test_the_test/test_start_stop_agent.py` — new Docker-backed smoke test (created) + +--- + +## Implementation Notes + +### Spec deviation: `None`-safe `request.cls` + +The brief's `start_agent` body uses `request.cls.__name__` verbatim in the log path. When the smoke test (a module-level function, not a class method) was run, `request.cls` was `None`, causing `AttributeError`. The fix was to use `cls_name = request.cls.__name__ if request.cls else "NoClass"` in: +- `start_agent` log path +- `get_test_agent_api` teardown log path +- `TestAgentAPI.__init__` log path +- `TestAgentAPI.rebind_request` log path + +This is defensive hygiene consistent with how `_request_token` already handles `None` (`request.cls.__name__ if request.cls else ""`). The normal production path (parametric test functions are always inside a class) is unaffected. + +### `Callable` import + +The existing `from collections.abc import Generator` was replaced with `from collections.abc import Generator, Callable` as specified. + +--- + +## Verification + +### Step 2 — Confirm test fails before implementation + +Direct check (pytest with root conftest causes `TypeError` unrelated to our code): + +``` +python -c "from utils.docker_fixtures._test_agent import TestAgentFactory; f = TestAgentFactory(''); print(hasattr(f, 'start_agent'))" +# Output: False (confirmed AttributeError would follow) +``` + +The root conftest has a pre-existing `TypeError: 'bool' object is not iterable` in `_item_must_pass` that prevents normal pytest collection with the full conftest. Running with `--noconftest` bypasses it. + +### Step 5 — Docker smoke test (after implementation) + +Command: +``` +DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v --noconftest +``` + +Output: +``` +============================= test session starts ============================== +collecting ... collected 1 item + +tests/test_the_test/test_start_stop_agent.py::test_start_agent_then_stop PASSED [100%] + +============================== 1 passed in 2.95s =============================== +``` + +### Step 6 — Parametric regression test + +Command: +``` +TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context --skip-parametric-build +``` + +Note: `--skip-parametric-build` was required because the `binaries/` directory contains symlinks to paths outside the Docker build context (pointing to `~/dd/system-tests/binaries/`), so `docker build` can't follow them in the worktree. The pre-built `php-test-client:latest` image was used. This is a pre-existing environment constraint. + +Output: +``` +Skipping parametric build (image already exists, --skip-parametric-build or SKIP_PARAMETRIC_BUILD) +================================= test context ================================= +Scenario: PARAMETRIC +Logs folder: ./logs_parametric +Library: php@1.21.0 +============================= test session starts ============================== +gw0 [1] / gw1 [1] / ... / gw15 [1] + +. [100%] +============================== 1 passed in 10.78s ============================== +``` + +--- + +--- + +## Review-Finding Fixes (post-commit d63db0ea2) + +### Fixes applied + +1. **CRITICAL — container leak on readiness timeout**: The `for...else` branch in `start_agent` now calls `cm.__exit__(None, None, None)` inside a `try/finally` (with `log_file.close()` in the `finally`) before `pytest.fail(...)`, ensuring the container is stopped even when it never becomes ready. + +2. **IMPORTANT — log path duplication**: Extracted `_agent_log_path(host_log_folder, request)` as a module-level helper. Both `start_agent` and the `get_test_agent_api` `finally` block now call this helper instead of inline construction. The `cls_name` local variable was removed from both sites. + +3. **MINOR (lint) — F541 f-string**: Changed `f"8126/tcp"` → `"8126/tcp"` in the `ports` dict. + +### Verification + +#### Docker smoke test + +Command: +``` +DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v --noconftest +``` + +Output: +``` +============================= test session starts ============================== +collecting ... collected 1 item + +tests/test_the_test/test_start_stop_agent.py::test_start_agent_then_stop PASSED [100%] + +--------------------------------- JSON report ---------------------------------- +report saved to: .report.json +============================== 1 passed in 2.98s ============================== +``` + +#### Parametric regression + +Command: +``` +TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context --skip-parametric-build +``` + +Output: +``` +Skipping parametric build (image already exists, --skip-parametric-build or SKIP_PARAMETRIC_BUILD) +================================= test context ================================= +Scenario: PARAMETRIC +Logs folder: ./logs_parametric +Library: php@1.21.0 +============================= test session starts ============================== +gw0 [1] / gw1 [1] / ... / gw15 [1] + +. [100%] +============================== 1 passed in 10.29s ============================== +``` + +--- + +## Self-review Notes + +1. **DRY requirement satisfied**: `get_test_agent_api` is now a thin wrapper that calls `start_agent` for lifecycle and only handles snapshot marks and report-section teardown. The create-and-wait logic exists in exactly one place. + +2. **`rebind_request` stores `_host_log_folder`**: `self._host_log_folder = host_log_folder` is added right after `self.network = network` in `TestAgentAPI.__init__`, enabling the rebind method to reconstruct the log path. + +3. **`_stop()` closure**: The `_stop()` callable in `start_agent` calls `cm.__exit__(None, None, None)` in a `try/finally` with `log_file.close()` in the `finally` block, ensuring the log file is always closed even if container stop raises. + +4. **`container_port` variable**: The original code had a local `container_port = 8126` variable. The refactored `start_agent` uses the literal `8126` directly in the port mapping and `TestAgentAPI` constructor, which is equivalent and avoids the now-unused variable. + +5. **Smoke test `--noconftest` note**: The smoke test as written requires `--noconftest` to run cleanly in the worktree due to a pre-existing conftest bug (`TypeError: 'bool' object is not iterable` in `_item_must_pass`). In CI, the parametric suite uses its own conftest path and does not trigger this bug. diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index f1579299a0c..8604cf3ac1f 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -35,6 +35,11 @@ def _request_token(request: pytest.FixtureRequest) -> str: return token +def _agent_log_path(host_log_folder: str, request: "pytest.FixtureRequest") -> str: + cls_name = request.cls.__name__ if request.cls else "NoClass" + return f"{host_log_folder}/outputs/{cls_name}/{request.node.name}/agent_log.log" + + class AgentRequest(TypedDict): method: str url: str @@ -94,8 +99,7 @@ def start_agent( otlp_http_host_port = get_host_port(worker_id, 4701) otlp_grpc_host_port = get_host_port(worker_id, 4802) - cls_name = request.cls.__name__ if request.cls else "NoClass" - log_path = f"{self.host_log_folder}/outputs/{cls_name}/{request.node.name}/agent_log.log" + log_path = _agent_log_path(self.host_log_folder, request) Path(log_path).parent.mkdir(parents=True, exist_ok=True) log_file = open(log_path, "w+", encoding="utf-8") # noqa: SIM115 @@ -108,7 +112,7 @@ def start_agent( "./tests/integration_frameworks/utils/vcr-cassettes": "/vcr-cassettes", }, ports={ - f"8126/tcp": host_port, + "8126/tcp": host_port, f"{container_otlp_http_port}/tcp": otlp_http_host_port, f"{container_otlp_grpc_port}/tcp": otlp_grpc_host_port, }, @@ -143,7 +147,10 @@ def start_agent( logger.info("Test agent is ready") break else: - log_file.close() + try: + cm.__exit__(None, None, None) + finally: + log_file.close() pytest.fail(f"Could not connect to test agent, check the log file {log_path}.", pytrace=False) def _stop() -> None: @@ -188,8 +195,7 @@ def get_test_agent_api( else: yield client finally: - cls_name = request.cls.__name__ if request.cls else "NoClass" - log_path = f"{self.host_log_folder}/outputs/{cls_name}/{request.node.name}/agent_log.log" + log_path = _agent_log_path(self.host_log_folder, request) request.node.add_report_section("teardown", "Test Agent Output", f"Log file:\n./{log_path}") stop() From d329b36d0d9e2bd1c8ba011d64f1f934ae8d8642 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:26:30 -0400 Subject: [PATCH 05/24] feat(parametric): pooled-agent creator (worker-scoped network + agent) --- utils/_context/_scenarios/_docker_fixtures.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index e1b8389fc6d..bf84c1c1dcc 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -6,6 +6,7 @@ import pytest from utils.docker_fixtures._test_agent import TestAgentFactory, TestAgentAPI +from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, AgentLease, agent_env_key from docker.errors import DockerException from utils._context.docker import get_docker_client from utils._logger import logger @@ -32,6 +33,8 @@ def __init__( ) self._test_agent_factory = TestAgentFactory(agent_image) + self._agent_pool: "WorkerAgentPool | None" = None + self._pool_seed_request = None def _clean(self): if self.is_main_worker: @@ -73,6 +76,38 @@ def _get_docker_network(self, test_id: str) -> Generator[str, None, None]: # Let's ignore this, later calls will clean the mess logger.info(f"Failed to remove network, ignoring the error: {e}") + def get_agent_pool(self, worker_id: str) -> WorkerAgentPool: + if self._agent_pool is None: + + def _creator(agent_env: dict[str, str]) -> AgentLease: + key = agent_env_key(agent_env) + network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" + network = get_docker_client().networks.create(name=network_name, driver="bridge") + container_name = f"ddapm-test-agent-worker-{worker_id}-{abs(hash(key))}" + api, stop_agent = self._test_agent_factory.start_agent( + request=self._pool_seed_request, + worker_id=worker_id, + container_name=container_name, + docker_network=network.name, + agent_env=agent_env, + container_otlp_http_port=4318, + container_otlp_grpc_port=4317, + ) + + def _stop() -> None: + try: + stop_agent() + finally: + try: + network.remove() + except Exception as e: # noqa: BLE001 + logger.info(f"Failed to remove worker network, ignoring: {e}") + + return AgentLease(api=api, stop=_stop) + + self._agent_pool = WorkerAgentPool(_creator) + return self._agent_pool + @contextlib.contextmanager def get_test_agent_api( self, From f3cf96258f1df3a949517b34cda88477b08b582c Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:42:55 -0400 Subject: [PATCH 06/24] fix(parametric-agent-pool): guard _pool_seed_request None, type annotation, import order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review fixes on the get_agent_pool/_creator code (Task 3): 1. Type `_pool_seed_request` as `pytest.FixtureRequest | None` (was bare None). 2. Add assert guard at top of `_creator` so a missing `_pool_seed_request` raises a clear message instead of an AttributeError deep in start_agent. 3. Reorder imports: move `from docker.errors import DockerException` (third-party) before the first-party `utils.*` imports (stdlib → third-party → first-party). 4. Add comment in get_agent_pool explaining the POC single-agent-per-worker invariant and why worker-keyed ports are safe. Production fresh-path behavior is unchanged. Co-Authored-By: Claude Sonnet 4.6 --- utils/_context/_scenarios/_docker_fixtures.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index bf84c1c1dcc..5d16502e5f7 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -5,9 +5,10 @@ import pytest +from docker.errors import DockerException + from utils.docker_fixtures._test_agent import TestAgentFactory, TestAgentAPI from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, AgentLease, agent_env_key -from docker.errors import DockerException from utils._context.docker import get_docker_client from utils._logger import logger from .core import Scenario, ScenarioGroup, scenario_groups as groups @@ -34,7 +35,7 @@ def __init__( self._test_agent_factory = TestAgentFactory(agent_image) self._agent_pool: "WorkerAgentPool | None" = None - self._pool_seed_request = None + self._pool_seed_request: "pytest.FixtureRequest | None" = None def _clean(self): if self.is_main_worker: @@ -77,9 +78,18 @@ def _get_docker_network(self, test_id: str) -> Generator[str, None, None]: logger.info(f"Failed to remove network, ignoring the error: {e}") def get_agent_pool(self, worker_id: str) -> WorkerAgentPool: + # POC: only the default agent_env ({}) is pooled, so there is at most one + # pooled agent per xdist worker. That is why start_agent's worker-keyed + # host ports are safe — no two pooled agents on the same worker can collide. + # Supporting multiple envs per worker would require per-(worker, env) port + # allocation and is out of scope for this POC. if self._agent_pool is None: def _creator(agent_env: dict[str, str]) -> AgentLease: + assert self._pool_seed_request is not None, ( + "WorkerAgentPool creator invoked before the test_agent fixture set " + "_pool_seed_request; the fixture must set it before the first acquire()." + ) key = agent_env_key(agent_env) network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" network = get_docker_client().networks.create(name=network_name, driver="bridge") From 7ca986d360269415ead43630ce3d788d973c5309 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:44:37 -0400 Subject: [PATCH 07/24] docs(parametric): scope plan to default-env pooling (Option B) + churn metrics --- .../2026-06-29-parametric-test-agent-reuse.md | 75 +++++++++++++++++-- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md b/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md index 8fec1c13bea..821af559582 100644 --- a/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md +++ b/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md @@ -12,7 +12,7 @@ - **Cross-tracer blast radius:** this code is shared by all 9 tracers' parametric suites. Behavior for non-pooled paths must be byte-for-byte unchanged. Verbatim rule: do not alter `get_test_agent_api`'s existing fresh-path semantics — only add a parallel pooled path. - **No state leakage:** a reused agent MUST start each test with no data from a prior test. The reset is `TestAgentAPI.clear()` (`utils/docker_fixtures/_test_agent.py:375-377`), called before the test runs. -- **POC scope (explicit):** pool only when the test has **no `snapshot` mark**. Snapshot-marked tests use the existing fresh path. Pool entries are keyed by the full `agent_env` dict, so tests with differing `agent_env` get separate pooled agents. +- **POC scope (explicit):** pool only when the test has **no `snapshot` mark** AND `agent_env == {}` (the default, ~94% of parametric tests). Snapshot-marked tests and tests with a non-default `agent_env` use the existing fresh-per-test path. This guarantees **at most one pooled agent per xdist worker**, which is why `start_agent`'s worker-keyed host ports (`get_host_port(worker_id, …)`) cannot collide. (Pooling multiple distinct `agent_env`s per worker would require per-(worker,env) port allocation — out of scope for this POC; left to the production-hardening team.) The pool is still keyed by `agent_env` so the fallback is structural, not hard-coded to `{}`. - **xdist semantics:** a pytest `scope="session"` fixture runs once **per worker** under `pytest-xdist`. That is the mechanism for "one agent per worker." - **Default config preserved:** `agent_env` defaults to `{}` (`tests/parametric/conftest.py:38`); the vast majority of parametric tests use it, so a single pooled agent per worker covers them. - Frequent commits: one per task minimum. @@ -367,6 +367,52 @@ from collections.abc import Generator, Callable (The file already imports `Generator` on line 2 — replace that single import line with the line above.) +- [ ] **Step 4b: Refactor `get_test_agent_api` to delegate to `start_agent` (DRY — do not duplicate the run/readiness body)** + +Replace the body of `TestAgentFactory.get_test_agent_api` (`utils/docker_fixtures/_test_agent.py:72-166`) so it calls `start_agent` for container lifecycle and keeps ONLY the snapshot-mark + teardown-report handling around the yield. The env-building, port allocation, `docker_run`, and readiness loop now live solely in `start_agent`: + +```python + @contextlib.contextmanager + def get_test_agent_api( + self, + request: pytest.FixtureRequest, + worker_id: str, + container_name: str, + docker_network: str, + agent_env: dict[str, str], + container_otlp_http_port: int, + container_otlp_grpc_port: int, + ) -> Generator["TestAgentAPI", None, None]: + client, stop = self.start_agent( + request=request, + worker_id=worker_id, + container_name=container_name, + docker_network=docker_network, + agent_env=agent_env, + container_otlp_http_port=container_otlp_http_port, + container_otlp_grpc_port=container_otlp_grpc_port, + ) + try: + # If the snapshot mark is on the test case then do a snapshot test + marks = list(request.node.iter_markers(name="snapshot")) + assert len(marks) <= 1, "Multiple snapshot marks detected" + if marks: + snap = marks[0] + assert len(snap.args) == 0, "only keyword arguments are supported by the snapshot decorator" + if "token" not in snap.kwargs: + snap.kwargs["token"] = _request_token(request).replace(" ", "_").replace(os.path.sep, "_") + with client.snapshot_context(**snap.kwargs): + yield client + else: + yield client + finally: + log_path = f"{self.host_log_folder}/outputs/{request.cls.__name__}/{request.node.name}/agent_log.log" + request.node.add_report_section("teardown", "Test Agent Output", f"Log file:\n./{log_path}") + stop() +``` + +This makes `get_test_agent_api` a thin wrapper: there is now exactly one copy of the create-and-wait logic (`start_agent`). A reviewer will (correctly) flag it as a defect if the run/readiness block appears in both methods. + - [ ] **Step 5: Run test to verify it passes** Run: `DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v` @@ -510,10 +556,12 @@ def test_agent( test_agent_otlp_grpc_port: int, test_agent_pool, ) -> Generator[TestAgentAPI, None, None]: - # POC: pool everything except snapshot-marked tests (which need per-test - # snapshot_context lifecycle). Pooled agents are reset with clear() between tests. - is_snapshot = request.node.get_closest_marker("snapshot") is not None - if not is_snapshot: + # POC: pool only default-agent_env, non-snapshot tests. Snapshot-marked tests need + # per-test snapshot_context lifecycle; non-default agent_env would require a second + # pooled agent per worker (worker-keyed host ports would collide). Both fall back to + # the fresh-per-test path. Pooled agents are reset with clear() between tests. + poolable = request.node.get_closest_marker("snapshot") is None and not agent_env + if poolable: scenarios.parametric._pool_seed_request = request api = test_agent_pool.acquire(request=request, agent_env=agent_env) api.clear() # ensure a clean slate even on the very first acquire @@ -588,7 +636,22 @@ for i in 1 2 3; do TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_ot ``` Expected: three clean runs, no "ITERATION n FAILED" line. -- [ ] **Step 5: Commit** +- [ ] **Step 5: Capture the churn-reduction metric** + +The point of the POC is fewer container/network operations. Compute it directly from the run log captured in Step 2 (`/tmp/reuse_run.log`). + +Run: +```bash +AGENTS=$(grep -c "REUSE-POC agent container created" /tmp/reuse_run.log) +# Count test cases that ran (passed + failed + xfailed + xpassed) from the pytest summary: +TESTS=$(grep -oE "[0-9]+ (passed|xpassed|xfailed)" /tmp/reuse_run.log | awk '{s+=$1} END{print s}') +echo "tests run: ${TESTS}" +echo "pooled agents created: ${AGENTS} (fresh baseline would create one agent + one network per test = ${TESTS})" +python3 -c "t=${TESTS}; a=${AGENTS}; print(f'agent+network create/destroy cycles: {2*t} (before) -> {2*a} (after) = {100*(1-(a/t)):.0f}% fewer')" +``` +Expected (default-env otel file): `AGENTS` ≈ 1 (one pooled agent for the worker), `TESTS` ≈ 57 → the agent+network churn drops by ~98% for this file. Record the printed numbers in the report; they are the headline POC result. (The library client is still fresh per test — unchanged — so total per-test container ops drop from 3 to 1 for default-env tests.) + +- [ ] **Step 6: Commit** ```bash git add utils/docker_fixtures/_test_agent.py From 185319cdbec2b879f0b7083364f18d1351da3907 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:47:24 -0400 Subject: [PATCH 08/24] feat(parametric): use WorkerAgentPool in test_agent fixture --- tests/parametric/conftest.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/parametric/conftest.py b/tests/parametric/conftest.py index 5b3cb343844..d27f191a97e 100644 --- a/tests/parametric/conftest.py +++ b/tests/parametric/conftest.py @@ -74,6 +74,14 @@ def test_agent_otlp_grpc_port() -> int: return 4317 +@pytest.fixture(scope="session") +def test_agent_pool(worker_id: str): + # scope="session" under pytest-xdist == once per worker. + pool = scenarios.parametric.get_agent_pool(worker_id) + yield pool + pool.shutdown() + + @pytest.fixture def test_agent( worker_id: str, @@ -82,7 +90,20 @@ def test_agent( agent_env: dict[str, str], test_agent_otlp_http_port: int, test_agent_otlp_grpc_port: int, + test_agent_pool, ) -> Generator[TestAgentAPI, None, None]: + # POC: pool only default-agent_env, non-snapshot tests. Snapshot-marked tests need + # per-test snapshot_context lifecycle; non-default agent_env would require a second + # pooled agent per worker (worker-keyed host ports would collide). Both fall back to + # the fresh-per-test path. Pooled agents are reset with clear() between tests. + poolable = request.node.get_closest_marker("snapshot") is None and not agent_env + if poolable: + scenarios.parametric._pool_seed_request = request + api = test_agent_pool.acquire(request=request, agent_env=agent_env) + api.clear() # ensure a clean slate even on the very first acquire + yield api + return + with scenarios.parametric.get_test_agent_api( request=request, worker_id=worker_id, From 08ff3a8e4e341b7a21bbb0120b97783d3c3383b4 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:54:57 -0400 Subject: [PATCH 09/24] refactor(parametric): thread request through pool creator, remove _pool_seed_request global The pooled test_agent fixture was mutating scenarios.parametric._pool_seed_request (a process-wide singleton) so the WorkerAgentPool creator closure could read the current pytest request. This relied on synchronous timing and was a fragile global. Fix by threading `request` through the creator call: - WorkerAgentPool.acquire() passes request to creator on cache miss - Creator type hint updated to Callable[[Any, dict[str, str]], AgentLease] - _docker_fixtures.py _creator signature changed to (request, agent_env) - _pool_seed_request attribute and assertion guard removed from DockerFixturesScenario - conftest.py drops the _pool_seed_request mutation and redundant return after yield Co-Authored-By: Claude Sonnet 4.6 --- tests/parametric/conftest.py | 2 -- tests/test_the_test/test_test_agent_pool.py | 2 +- utils/_context/_scenarios/_docker_fixtures.py | 9 ++------- utils/docker_fixtures/_test_agent_pool.py | 4 ++-- 4 files changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/parametric/conftest.py b/tests/parametric/conftest.py index d27f191a97e..5fbdccef30b 100644 --- a/tests/parametric/conftest.py +++ b/tests/parametric/conftest.py @@ -98,11 +98,9 @@ def test_agent( # the fresh-per-test path. Pooled agents are reset with clear() between tests. poolable = request.node.get_closest_marker("snapshot") is None and not agent_env if poolable: - scenarios.parametric._pool_seed_request = request api = test_agent_pool.acquire(request=request, agent_env=agent_env) api.clear() # ensure a clean slate even on the very first acquire yield api - return with scenarios.parametric.get_test_agent_api( request=request, diff --git a/tests/test_the_test/test_test_agent_pool.py b/tests/test_the_test/test_test_agent_pool.py index c06f14df6ac..0d166fcf65e 100644 --- a/tests/test_the_test/test_test_agent_pool.py +++ b/tests/test_the_test/test_test_agent_pool.py @@ -20,7 +20,7 @@ def __init__(self) -> None: self.created_envs: list[dict] = [] self.stopped = 0 - def __call__(self, agent_env: dict[str, str]): + def __call__(self, request, agent_env: dict[str, str]): from utils.docker_fixtures._test_agent_pool import AgentLease self.created_envs.append(dict(agent_env)) diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index 5d16502e5f7..adf8328556e 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -35,7 +35,6 @@ def __init__( self._test_agent_factory = TestAgentFactory(agent_image) self._agent_pool: "WorkerAgentPool | None" = None - self._pool_seed_request: "pytest.FixtureRequest | None" = None def _clean(self): if self.is_main_worker: @@ -85,17 +84,13 @@ def get_agent_pool(self, worker_id: str) -> WorkerAgentPool: # allocation and is out of scope for this POC. if self._agent_pool is None: - def _creator(agent_env: dict[str, str]) -> AgentLease: - assert self._pool_seed_request is not None, ( - "WorkerAgentPool creator invoked before the test_agent fixture set " - "_pool_seed_request; the fixture must set it before the first acquire()." - ) + def _creator(request, agent_env: dict[str, str]) -> AgentLease: key = agent_env_key(agent_env) network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" network = get_docker_client().networks.create(name=network_name, driver="bridge") container_name = f"ddapm-test-agent-worker-{worker_id}-{abs(hash(key))}" api, stop_agent = self._test_agent_factory.start_agent( - request=self._pool_seed_request, + request=request, worker_id=worker_id, container_name=container_name, docker_network=network.name, diff --git a/utils/docker_fixtures/_test_agent_pool.py b/utils/docker_fixtures/_test_agent_pool.py index b8fc6ff36b7..1ebe8790d3b 100644 --- a/utils/docker_fixtures/_test_agent_pool.py +++ b/utils/docker_fixtures/_test_agent_pool.py @@ -29,7 +29,7 @@ class WorkerAgentPool: `api.rebind_request()` (re-point per-test logging at the current test). """ - def __init__(self, creator: Callable[[dict[str, str]], AgentLease]) -> None: + def __init__(self, creator: Callable[[Any, dict[str, str]], AgentLease]) -> None: self._creator = creator self._leases: dict[tuple, AgentLease] = {} @@ -37,7 +37,7 @@ def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: key = agent_env_key(agent_env) lease = self._leases.get(key) if lease is None: - lease = self._creator(agent_env) + lease = self._creator(request, agent_env) self._leases[key] = lease else: lease.api.clear() From 902231d3a14b62e3476f3f21fe3c9ddc91743e00 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 13:59:53 -0400 Subject: [PATCH 10/24] fix(parametric): restore early return in pooled test_agent branch (prevents fresh-path fall-through/port collision) --- tests/parametric/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/parametric/conftest.py b/tests/parametric/conftest.py index 5fbdccef30b..9999011ff1d 100644 --- a/tests/parametric/conftest.py +++ b/tests/parametric/conftest.py @@ -101,6 +101,7 @@ def test_agent( api = test_agent_pool.acquire(request=request, agent_env=agent_env) api.clear() # ensure a clean slate even on the very first acquire yield api + return # REQUIRED: do not fall through into the fresh-path agent below with scenarios.parametric.get_test_agent_api( request=request, From 84c84857e8dd33b596a038346ab4c49af7e07a9e Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 14:34:43 -0400 Subject: [PATCH 11/24] test(parametric): log agent container creations for reuse verification --- utils/docker_fixtures/_test_agent.py | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 8604cf3ac1f..0bf199c200d 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -102,6 +102,7 @@ def start_agent( log_path = _agent_log_path(self.host_log_folder, request) Path(log_path).parent.mkdir(parents=True, exist_ok=True) log_file = open(log_path, "w+", encoding="utf-8") # noqa: SIM115 + logger.stdout(f"REUSE-POC agent container created: {container_name}") cm = docker_run( image=self.image, From e69058533c0227d79425d8b66f53d34cc76104e6 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 14:38:27 -0400 Subject: [PATCH 12/24] docs(parametric): test-agent reuse POC handoff note Co-Authored-By: Claude Sonnet 4.6 --- docs/edit/parametric-test-agent-reuse-poc.md | 81 ++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 docs/edit/parametric-test-agent-reuse-poc.md diff --git a/docs/edit/parametric-test-agent-reuse-poc.md b/docs/edit/parametric-test-agent-reuse-poc.md new file mode 100644 index 00000000000..6f4d4e2c9be --- /dev/null +++ b/docs/edit/parametric-test-agent-reuse-poc.md @@ -0,0 +1,81 @@ +# POC: parametric test-agent reuse + +## Why + +The parametric scenario creates a fresh Docker network + test-agent + client +container per test (16 xdist workers x ~440 tests). On the shared docker-in-docker +microVM runners this churn intermittently stalls container/network setup, so a +test-agent occasionally fails to make a just-sent trace queryable within the +tracer's ~3 s receipt poll -> `Number (1) of traces not available, got 0`. + +## What this changes + +- One test-agent + one Docker network per xdist worker, reused across tests, + reset with `TestAgentAPI.clear()` (`/test/session/clear`) before each test. +- The library client container is unchanged: it is still created fresh per test + and derives its network and agent URL from the `test_agent` object it is handed. + Per-test container operations therefore drop from 3 to 1 for default-env tests + (agent + network + client -> client only), not a uniform 98% cut in all containers. + +## Scope / fallback + +Pooling is enabled only for **non-snapshot tests with the default `agent_env == {}`**, +which covers roughly 94% of parametric tests. Two categories fall back to the +existing fresh-per-test path: + +1. **Snapshot-marked tests** (`pytest.mark.snapshot`) -- these compare recorded + agent output and need a clean agent on every run. +2. **Tests with a non-default `agent_env`** -- the pooled agent persists for the + whole worker session on a worker-keyed host port. Allowing a second pooled + agent with a different environment, or colliding with a fresh-path agent on + the same worker, would hit "port already allocated". Keeping at most one + pooled agent per worker avoids that collision. + +To pool non-default-env tests in the future, allocate host ports per +`(worker, env)` instead of per worker. + +## Measured results + +Benchmark file: `tests/parametric/test_otel_span_methods.py`, single worker. + +| Metric | Before | After | +|--------|--------|-------| +| Agent+network create/destroy cycles | 118 | 2 | +| Reduction | -- | ~98% | +| Tests driving the single pooled agent | -- | 59 | +| Per-test container ops (default-env tests) | 3 | 1 | + +Stability: 3 consecutive full-file runs each produced 57 passed / 2 skipped / +2 xfailed with no cross-test state leakage detected. + +## Operational note + +The `REUSE-POC agent container created:` count line is emitted via +`logger.stdout` and lands in `logs_parametric/tests.log` (not the tee'd console +stdout). To measure agent creation counts in CI, grep that file: + +``` +grep "REUSE-POC agent container created" logs_parametric/tests.log | wc -l +``` + +## Risks to weigh before adopting + +- **State leakage** if `clear()` is ever incomplete. `TestAgentAPI.clear()` + (`/test/session/clear`) covers traces, telemetry, RC, OTLP, and sessions; + verified for the trace case in this POC. Extend the leakage check per data + type before rolling out to all tracers. +- **In-flight data**: a late trace from test N arriving after test N+1's + `clear()` could leak. Hardening path: per-test agent session tokens (requires + the client to send `X-Datadog-Test-Session-Token`). +- **Agent container log spans the worker session**, not per-test. Per-test API + interactions are re-pointed via `rebind_request`; the raw container log is + shared. Acceptable for a POC; revisit if per-test log isolation is needed. +- **Shared across 9 tracers** -- roll out behind a flag / one language at a time. + +## Hardening path summary + +1. Extend leakage checks to telemetry, RC, OTLP, and session data. +2. Add per-test session tokens (`X-Datadog-Test-Session-Token`) to guard + against in-flight-data leakage. +3. Allocate ports per `(worker, env)` to enable pooling of non-default-env tests. +4. Gate the feature behind a flag and roll out one tracer language at a time. From fb0d34dddb8a12191123b41dced76cfcd193a20a Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 15:11:11 -0400 Subject: [PATCH 13/24] fix(parametric): give pooled agents distinct host-port band to avoid collision with fresh-path agents When a pooled test-agent (persistent for the whole worker session) and a fresh-path test-agent ran on the same xdist worker, they both computed host ports from get_host_port(worker_id, 4600/4701/4802), causing docker run to fail with "port is already allocated". Fix: add agent_port_base/otlp_http_port_base/otlp_grpc_port_base keyword params to TestAgentFactory.start_agent (defaulting to 4600/4701/4802 so the fresh-path callers are unchanged). Pooled-agent _creator passes agent_port_base=4900, otlp_http_port_base=5000, otlp_grpc_port_base=5100 so pooled agents never occupy the same host ports as fresh-path agents on the same worker. Co-Authored-By: Claude Opus 4.8 (1M context) --- utils/_context/_scenarios/_docker_fixtures.py | 7 +++++++ utils/docker_fixtures/_test_agent.py | 9 ++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index adf8328556e..5dd841ede66 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -89,6 +89,10 @@ def _creator(request, agent_env: dict[str, str]) -> AgentLease: network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" network = get_docker_client().networks.create(name=network_name, driver="bridge") container_name = f"ddapm-test-agent-worker-{worker_id}-{abs(hash(key))}" + # Pooled agents use a separate host-port band (4900/5000/5100 + worker offset) + # so a worker's persistent pooled agent never collides with a fresh-path agent + # (4600/4701/4802) on that worker. These bands are clear of the client/framework + # 4500 and agent 4600/4701/4802 bands even with worker offsets. api, stop_agent = self._test_agent_factory.start_agent( request=request, worker_id=worker_id, @@ -97,6 +101,9 @@ def _creator(request, agent_env: dict[str, str]) -> AgentLease: agent_env=agent_env, container_otlp_http_port=4318, container_otlp_grpc_port=4317, + agent_port_base=4900, + otlp_http_port_base=5000, + otlp_grpc_port_base=5100, ) def _stop() -> None: diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 0bf199c200d..ec65f77b536 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -84,6 +84,9 @@ def start_agent( agent_env: dict[str, str], container_otlp_http_port: int, container_otlp_grpc_port: int, + agent_port_base: int = 4600, + otlp_http_port_base: int = 4701, + otlp_grpc_port_base: int = 4802, ) -> "tuple[TestAgentAPI, Callable[[], None]]": env = { "ENABLED_CHECKS": "trace_count_header", @@ -95,9 +98,9 @@ def start_agent( env["SNAPSHOT_CI"] = "0" env |= agent_env - host_port = get_host_port(worker_id, 4600) - otlp_http_host_port = get_host_port(worker_id, 4701) - otlp_grpc_host_port = get_host_port(worker_id, 4802) + host_port = get_host_port(worker_id, agent_port_base) + otlp_http_host_port = get_host_port(worker_id, otlp_http_port_base) + otlp_grpc_host_port = get_host_port(worker_id, otlp_grpc_port_base) log_path = _agent_log_path(self.host_log_folder, request) Path(log_path).parent.mkdir(parents=True, exist_ok=True) From d9b4b48a61f5a34943d6ec951a42c9d105e060fa Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 15:36:53 -0400 Subject: [PATCH 14/24] chore(parametric): drop internal SDD scratch + plan doc from POC branch --- .superpowers/sdd/task-2-report.md | 157 ---- .../2026-06-29-parametric-test-agent-reuse.md | 725 ------------------ 2 files changed, 882 deletions(-) delete mode 100644 .superpowers/sdd/task-2-report.md delete mode 100644 docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md diff --git a/.superpowers/sdd/task-2-report.md b/.superpowers/sdd/task-2-report.md deleted file mode 100644 index b88ccaaabf0..00000000000 --- a/.superpowers/sdd/task-2-report.md +++ /dev/null @@ -1,157 +0,0 @@ -# Task 2 Report: Extract `start_agent`/`stop_agent` + add `rebind_request` - -## Status - -DONE_WITH_CONCERNS - -## Commit hash - -`d63db0ea2` - -## Files Modified - -- `utils/docker_fixtures/_test_agent.py` — extracted `start_agent`, added `rebind_request`, refactored `get_test_agent_api` to delegate -- `tests/test_the_test/test_start_stop_agent.py` — new Docker-backed smoke test (created) - ---- - -## Implementation Notes - -### Spec deviation: `None`-safe `request.cls` - -The brief's `start_agent` body uses `request.cls.__name__` verbatim in the log path. When the smoke test (a module-level function, not a class method) was run, `request.cls` was `None`, causing `AttributeError`. The fix was to use `cls_name = request.cls.__name__ if request.cls else "NoClass"` in: -- `start_agent` log path -- `get_test_agent_api` teardown log path -- `TestAgentAPI.__init__` log path -- `TestAgentAPI.rebind_request` log path - -This is defensive hygiene consistent with how `_request_token` already handles `None` (`request.cls.__name__ if request.cls else ""`). The normal production path (parametric test functions are always inside a class) is unaffected. - -### `Callable` import - -The existing `from collections.abc import Generator` was replaced with `from collections.abc import Generator, Callable` as specified. - ---- - -## Verification - -### Step 2 — Confirm test fails before implementation - -Direct check (pytest with root conftest causes `TypeError` unrelated to our code): - -``` -python -c "from utils.docker_fixtures._test_agent import TestAgentFactory; f = TestAgentFactory(''); print(hasattr(f, 'start_agent'))" -# Output: False (confirmed AttributeError would follow) -``` - -The root conftest has a pre-existing `TypeError: 'bool' object is not iterable` in `_item_must_pass` that prevents normal pytest collection with the full conftest. Running with `--noconftest` bypasses it. - -### Step 5 — Docker smoke test (after implementation) - -Command: -``` -DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v --noconftest -``` - -Output: -``` -============================= test session starts ============================== -collecting ... collected 1 item - -tests/test_the_test/test_start_stop_agent.py::test_start_agent_then_stop PASSED [100%] - -============================== 1 passed in 2.95s =============================== -``` - -### Step 6 — Parametric regression test - -Command: -``` -TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context --skip-parametric-build -``` - -Note: `--skip-parametric-build` was required because the `binaries/` directory contains symlinks to paths outside the Docker build context (pointing to `~/dd/system-tests/binaries/`), so `docker build` can't follow them in the worktree. The pre-built `php-test-client:latest` image was used. This is a pre-existing environment constraint. - -Output: -``` -Skipping parametric build (image already exists, --skip-parametric-build or SKIP_PARAMETRIC_BUILD) -================================= test context ================================= -Scenario: PARAMETRIC -Logs folder: ./logs_parametric -Library: php@1.21.0 -============================= test session starts ============================== -gw0 [1] / gw1 [1] / ... / gw15 [1] - -. [100%] -============================== 1 passed in 10.78s ============================== -``` - ---- - ---- - -## Review-Finding Fixes (post-commit d63db0ea2) - -### Fixes applied - -1. **CRITICAL — container leak on readiness timeout**: The `for...else` branch in `start_agent` now calls `cm.__exit__(None, None, None)` inside a `try/finally` (with `log_file.close()` in the `finally`) before `pytest.fail(...)`, ensuring the container is stopped even when it never becomes ready. - -2. **IMPORTANT — log path duplication**: Extracted `_agent_log_path(host_log_folder, request)` as a module-level helper. Both `start_agent` and the `get_test_agent_api` `finally` block now call this helper instead of inline construction. The `cls_name` local variable was removed from both sites. - -3. **MINOR (lint) — F541 f-string**: Changed `f"8126/tcp"` → `"8126/tcp"` in the `ports` dict. - -### Verification - -#### Docker smoke test - -Command: -``` -DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v --noconftest -``` - -Output: -``` -============================= test session starts ============================== -collecting ... collected 1 item - -tests/test_the_test/test_start_stop_agent.py::test_start_agent_then_stop PASSED [100%] - ---------------------------------- JSON report ---------------------------------- -report saved to: .report.json -============================== 1 passed in 2.98s ============================== -``` - -#### Parametric regression - -Command: -``` -TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context --skip-parametric-build -``` - -Output: -``` -Skipping parametric build (image already exists, --skip-parametric-build or SKIP_PARAMETRIC_BUILD) -================================= test context ================================= -Scenario: PARAMETRIC -Logs folder: ./logs_parametric -Library: php@1.21.0 -============================= test session starts ============================== -gw0 [1] / gw1 [1] / ... / gw15 [1] - -. [100%] -============================== 1 passed in 10.29s ============================== -``` - ---- - -## Self-review Notes - -1. **DRY requirement satisfied**: `get_test_agent_api` is now a thin wrapper that calls `start_agent` for lifecycle and only handles snapshot marks and report-section teardown. The create-and-wait logic exists in exactly one place. - -2. **`rebind_request` stores `_host_log_folder`**: `self._host_log_folder = host_log_folder` is added right after `self.network = network` in `TestAgentAPI.__init__`, enabling the rebind method to reconstruct the log path. - -3. **`_stop()` closure**: The `_stop()` callable in `start_agent` calls `cm.__exit__(None, None, None)` in a `try/finally` with `log_file.close()` in the `finally` block, ensuring the log file is always closed even if container stop raises. - -4. **`container_port` variable**: The original code had a local `container_port = 8126` variable. The refactored `start_agent` uses the literal `8126` directly in the port mapping and `TestAgentAPI` constructor, which is equivalent and avoids the now-unused variable. - -5. **Smoke test `--noconftest` note**: The smoke test as written requires `--noconftest` to run cleanly in the worktree due to a pre-existing conftest bug (`TypeError: 'bool' object is not iterable` in `_item_must_pass`). In CI, the parametric suite uses its own conftest path and does not trigger this bug. diff --git a/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md b/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md deleted file mode 100644 index 821af559582..00000000000 --- a/docs/superpowers/plans/2026-06-29-parametric-test-agent-reuse.md +++ /dev/null @@ -1,725 +0,0 @@ -# Parametric Test-Agent Reuse (POC) Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Cut parametric-scenario container churn by reusing one test-agent (and one Docker network) per xdist worker, resetting agent state with a synchronous `/test/session/clear` between tests, instead of creating a fresh network + test-agent per test. - -**Architecture:** Today each parametric test creates a fresh Docker bridge network, a fresh `ddapm-test-agent` container, and a fresh library-client container, then tears them all down (`utils/_context/_scenarios/_docker_fixtures.py:56-98`). This POC introduces a per-worker `WorkerAgentPool` that lazily creates one network + one test-agent per `(worker, agent_env)` key, reuses them across tests, and calls `clear()` before each test. The library client is unchanged — it already derives both its network (`test_agent.network`) and agent URL (`DD_TRACE_AGENT_URL=http://{test_agent.container_name}:8126`) from the `test_agent` object it is handed (`utils/docker_fixtures/_test_clients/_test_client_parametric.py:55,90`), so a pooled agent is transparent to it. Snapshot-marked tests fall back to the existing fresh-per-test path. - -**Tech Stack:** Python 3.12, pytest + pytest-xdist, docker-py (`utils/docker_fixtures/_core.py`), the `ghcr.io/datadog/dd-apm-test-agent` image. - -## Global Constraints - -- **Cross-tracer blast radius:** this code is shared by all 9 tracers' parametric suites. Behavior for non-pooled paths must be byte-for-byte unchanged. Verbatim rule: do not alter `get_test_agent_api`'s existing fresh-path semantics — only add a parallel pooled path. -- **No state leakage:** a reused agent MUST start each test with no data from a prior test. The reset is `TestAgentAPI.clear()` (`utils/docker_fixtures/_test_agent.py:375-377`), called before the test runs. -- **POC scope (explicit):** pool only when the test has **no `snapshot` mark** AND `agent_env == {}` (the default, ~94% of parametric tests). Snapshot-marked tests and tests with a non-default `agent_env` use the existing fresh-per-test path. This guarantees **at most one pooled agent per xdist worker**, which is why `start_agent`'s worker-keyed host ports (`get_host_port(worker_id, …)`) cannot collide. (Pooling multiple distinct `agent_env`s per worker would require per-(worker,env) port allocation — out of scope for this POC; left to the production-hardening team.) The pool is still keyed by `agent_env` so the fallback is structural, not hard-coded to `{}`. -- **xdist semantics:** a pytest `scope="session"` fixture runs once **per worker** under `pytest-xdist`. That is the mechanism for "one agent per worker." -- **Default config preserved:** `agent_env` defaults to `{}` (`tests/parametric/conftest.py:38`); the vast majority of parametric tests use it, so a single pooled agent per worker covers them. -- Frequent commits: one per task minimum. - ---- - -### Task 1: `WorkerAgentPool` + `AgentLease` (pure cache logic, no Docker) - -The reuse/keying/reset logic, isolated from Docker so it is unit-testable with a fake creator. - -**Files:** -- Create: `utils/docker_fixtures/_test_agent_pool.py` -- Test: `tests/test_the_test/test_test_agent_pool.py` - -**Interfaces:** -- Consumes: nothing (pure logic). Creator and agent objects are injected. -- Produces: - - `agent_env_key(agent_env: dict[str, str]) -> tuple` — stable, hashable key for an env dict. - - `class AgentLease` with attributes `api`, `stop` (a zero-arg callable that tears the lease down). - - `class WorkerAgentPool(creator: Callable[[dict[str, str]], AgentLease])` with: - - `acquire(request, agent_env: dict[str, str]) -> Any` — returns the lease's `api`, creating on miss and calling `api.clear()` + `api.rebind_request(request)` on hit. - - `shutdown() -> None` — calls every lease's `stop()` and empties the cache. - -- [ ] **Step 1: Write the failing test** - -```python -# tests/test_the_test/test_test_agent_pool.py -from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, agent_env_key - - -class _FakeApi: - def __init__(self) -> None: - self.clear_calls = 0 - self.rebind_calls: list[object] = [] - - def clear(self) -> None: - self.clear_calls += 1 - - def rebind_request(self, request: object) -> None: - self.rebind_calls.append(request) - - -class _FakeCreator: - """Records every creation and hands back a lease whose stop() is tracked.""" - - def __init__(self) -> None: - self.created_envs: list[dict] = [] - self.stopped = 0 - - def __call__(self, agent_env: dict[str, str]): - from utils.docker_fixtures._test_agent_pool import AgentLease - - self.created_envs.append(dict(agent_env)) - api = _FakeApi() - - def _stop() -> None: - self.stopped += 1 - - return AgentLease(api=api, stop=_stop) - - -def test_env_key_is_order_independent(): - assert agent_env_key({"A": "1", "B": "2"}) == agent_env_key({"B": "2", "A": "1"}) - - -def test_same_env_reuses_and_clears(): - creator = _FakeCreator() - pool = WorkerAgentPool(creator) - - api1 = pool.acquire(request="req1", agent_env={}) - api2 = pool.acquire(request="req2", agent_env={}) - - assert api1 is api2 # reused, not recreated - assert len(creator.created_envs) == 1 # created exactly once - assert api1.clear_calls == 1 # cleared on the reuse, not the first acquire - assert api1.rebind_calls == ["req2"] # rebound to the second test's request - - -def test_distinct_env_creates_separate_agents(): - creator = _FakeCreator() - pool = WorkerAgentPool(creator) - - a = pool.acquire(request="r", agent_env={}) - b = pool.acquire(request="r", agent_env={"DD_ENV": "prod"}) - - assert a is not b - assert len(creator.created_envs) == 2 - - -def test_shutdown_stops_every_lease(): - creator = _FakeCreator() - pool = WorkerAgentPool(creator) - pool.acquire(request="r", agent_env={}) - pool.acquire(request="r", agent_env={"DD_ENV": "prod"}) - - pool.shutdown() - - assert creator.stopped == 2 - # After shutdown the cache is empty: acquiring again creates anew. - pool.acquire(request="r", agent_env={}) - assert len(creator.created_envs) == 3 -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `python -m pytest tests/test_the_test/test_test_agent_pool.py -v` -Expected: FAIL — `ModuleNotFoundError: No module named 'utils.docker_fixtures._test_agent_pool'` - -- [ ] **Step 3: Write minimal implementation** - -```python -# utils/docker_fixtures/_test_agent_pool.py -from collections.abc import Callable -from dataclasses import dataclass -from typing import Any - - -def agent_env_key(agent_env: dict[str, str]) -> tuple: - """Stable, hashable, order-independent key for an agent_env dict.""" - return tuple(sorted(agent_env.items())) - - -@dataclass -class AgentLease: - """One running test-agent (+ its network) owned by the pool. - - `api` is a TestAgentAPI (duck-typed here so this module stays Docker-free). - `stop` tears the lease down (container + network); it must be idempotent-safe - to call exactly once at shutdown. - """ - - api: Any - stop: Callable[[], None] - - -class WorkerAgentPool: - """Per-worker cache of test-agents keyed by agent_env. - - First request for a given env creates a lease via `creator`; later requests - for the same env reuse it after `api.clear()` (reset state) and - `api.rebind_request()` (re-point per-test logging at the current test). - """ - - def __init__(self, creator: Callable[[dict[str, str]], AgentLease]) -> None: - self._creator = creator - self._leases: dict[tuple, AgentLease] = {} - - def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: - key = agent_env_key(agent_env) - lease = self._leases.get(key) - if lease is None: - lease = self._creator(agent_env) - self._leases[key] = lease - else: - lease.api.clear() - lease.api.rebind_request(request) - return lease.api - - def shutdown(self) -> None: - for lease in self._leases.values(): - lease.stop() - self._leases.clear() -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `python -m pytest tests/test_the_test/test_test_agent_pool.py -v` -Expected: PASS (4 passed) - -- [ ] **Step 5: Commit** - -```bash -git add utils/docker_fixtures/_test_agent_pool.py tests/test_the_test/test_test_agent_pool.py -git commit -m "feat(parametric): add WorkerAgentPool for test-agent reuse" -``` - ---- - -### Task 2: Extract `start_agent`/`stop_agent` + add `rebind_request` - -Refactor `TestAgentFactory.get_test_agent_api` so its container-create-and-wait body is reusable by the pool without forcing per-test teardown, and give `TestAgentAPI` a way to re-point its per-test log at the current test on reuse. - -**Files:** -- Modify: `utils/docker_fixtures/_test_agent.py` (factory `49-166`, `TestAgentAPI` `168-196`) - -**Interfaces:** -- Consumes: `docker_run`, `get_host_port` (`utils/docker_fixtures/_core.py`); `TestAgentAPI` (this file). -- Produces: - - `TestAgentAPI.rebind_request(self, request: pytest.FixtureRequest) -> None` — updates `self._pytest_request` and `self.log_path` to the given test. - - `TestAgentFactory.start_agent(self, *, request, worker_id, container_name, docker_network, agent_env, container_otlp_http_port, container_otlp_grpc_port) -> tuple[TestAgentAPI, Callable[[], None]]` — runs the container, waits until ready, returns the API plus a `stop()` callable that closes the log file and stops the container. Does **not** apply snapshot marks (the pooled path never carries them). - -- [ ] **Step 1: Write the failing test (Docker-backed smoke)** - -```python -# tests/test_the_test/test_start_stop_agent.py -import pytest - -pytestmark = pytest.mark.skipif( - __import__("os").getenv("DOCKER_SMOKE") != "1", - reason="Docker-backed; run with DOCKER_SMOKE=1", -) - - -def test_start_agent_then_stop(request): - from utils._context._scenarios import scenarios - - factory = scenarios.parametric._test_agent_factory - factory.configure("logs_parametric") - factory.pull() - - network = __import__("utils.docker_fixtures._core", fromlist=["get_docker_client"]) - client = network.get_docker_client().networks.create(name="reuse_smoke_net", driver="bridge") - try: - api, stop = factory.start_agent( - request=request, - worker_id="gw0", - container_name="ddapm-test-agent-reuse-smoke", - docker_network=client.name, - agent_env={}, - container_otlp_http_port=4318, - container_otlp_grpc_port=4317, - ) - try: - assert api.info()["version"] == "test" # agent answered → it is ready - finally: - stop() - finally: - client.remove() -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v` -Expected: FAIL — `AttributeError: 'TestAgentFactory' object has no attribute 'start_agent'` - -- [ ] **Step 3: Add `rebind_request` to `TestAgentAPI`** - -Insert this method into `TestAgentAPI` immediately after `__init__` (after `utils/docker_fixtures/_test_agent.py:196`): - -```python - def rebind_request(self, pytest_request: "pytest.FixtureRequest") -> None: - """Re-point per-test API logging at the current test (used on reuse).""" - self._pytest_request = pytest_request - self.log_path = ( - f"{self._host_log_folder}/outputs/{pytest_request.cls.__name__}" - f"/{pytest_request.node.name}/agent_api.log" - ) - Path(self.log_path).parent.mkdir(parents=True, exist_ok=True) -``` - -For `rebind_request` to work, `__init__` must remember `host_log_folder`. Add this line in `TestAgentAPI.__init__`, right after `self.network = network` (`utils/docker_fixtures/_test_agent.py:185`): - -```python - self._host_log_folder = host_log_folder -``` - -- [ ] **Step 4: Add `start_agent`/`stop_agent` to `TestAgentFactory`** - -Add these methods to `TestAgentFactory` (after `pull`, before `get_test_agent_api`, around `utils/docker_fixtures/_test_agent.py:71`). This is the existing container-run-and-wait body lifted out of the context manager, returning an explicit `stop` callable instead of relying on `with`: - -```python - def start_agent( - self, - *, - request: pytest.FixtureRequest, - worker_id: str, - container_name: str, - docker_network: str, - agent_env: dict[str, str], - container_otlp_http_port: int, - container_otlp_grpc_port: int, - ) -> "tuple[TestAgentAPI, Callable[[], None]]": - env = { - "ENABLED_CHECKS": "trace_count_header", - "OTLP_HTTP_PORT": str(container_otlp_http_port), - "OTLP_GRPC_PORT": str(container_otlp_grpc_port), - "VCR_CASSETTES_DIRECTORY": "/vcr-cassettes", - } - if os.getenv("DEV_MODE") is not None: - env["SNAPSHOT_CI"] = "0" - env |= agent_env - - host_port = get_host_port(worker_id, 4600) - otlp_http_host_port = get_host_port(worker_id, 4701) - otlp_grpc_host_port = get_host_port(worker_id, 4802) - - log_path = f"{self.host_log_folder}/outputs/{request.cls.__name__}/{request.node.name}/agent_log.log" - Path(log_path).parent.mkdir(parents=True, exist_ok=True) - log_file = open(log_path, "w+", encoding="utf-8") # noqa: SIM115 - - cm = docker_run( - image=self.image, - name=container_name, - env=env, - volumes={ - "./snapshots": "/snapshots", - "./tests/integration_frameworks/utils/vcr-cassettes": "/vcr-cassettes", - }, - ports={ - f"8126/tcp": host_port, - f"{container_otlp_http_port}/tcp": otlp_http_host_port, - f"{container_otlp_grpc_port}/tcp": otlp_grpc_host_port, - }, - log_file=log_file, - network=docker_network, - ) - cm.__enter__() - - client = TestAgentAPI( - container_name, - 8126, - self.host_log_folder, - pytest_request=request, - otlp_http_host_port=otlp_http_host_port, - host_port=host_port, - network=docker_network, - ) - time.sleep(0.2) # the trace agent takes ~200ms to start - expected_version = agent_env.get("TEST_AGENT_VERSION", "test") - for _ in range(100): - try: - resp = client.info() - except Exception as e: # noqa: BLE001 - logger.debug(f"Wait for 0.1s for the test agent to be ready {e}") - time.sleep(0.1) - else: - if resp["version"] != expected_version: - pytest.fail( - f"Agent version {resp['version']} is running instead of the test agent.", - pytrace=False, - ) - logger.info("Test agent is ready") - break - else: - log_file.close() - pytest.fail(f"Could not connect to test agent, check the log file {log_path}.", pytrace=False) - - def _stop() -> None: - try: - cm.__exit__(None, None, None) - finally: - log_file.close() - - return client, _stop -``` - -Add the `Callable` import at the top of the file (after `utils/docker_fixtures/_test_agent.py:2`): - -```python -from collections.abc import Generator, Callable -``` - -(The file already imports `Generator` on line 2 — replace that single import line with the line above.) - -- [ ] **Step 4b: Refactor `get_test_agent_api` to delegate to `start_agent` (DRY — do not duplicate the run/readiness body)** - -Replace the body of `TestAgentFactory.get_test_agent_api` (`utils/docker_fixtures/_test_agent.py:72-166`) so it calls `start_agent` for container lifecycle and keeps ONLY the snapshot-mark + teardown-report handling around the yield. The env-building, port allocation, `docker_run`, and readiness loop now live solely in `start_agent`: - -```python - @contextlib.contextmanager - def get_test_agent_api( - self, - request: pytest.FixtureRequest, - worker_id: str, - container_name: str, - docker_network: str, - agent_env: dict[str, str], - container_otlp_http_port: int, - container_otlp_grpc_port: int, - ) -> Generator["TestAgentAPI", None, None]: - client, stop = self.start_agent( - request=request, - worker_id=worker_id, - container_name=container_name, - docker_network=docker_network, - agent_env=agent_env, - container_otlp_http_port=container_otlp_http_port, - container_otlp_grpc_port=container_otlp_grpc_port, - ) - try: - # If the snapshot mark is on the test case then do a snapshot test - marks = list(request.node.iter_markers(name="snapshot")) - assert len(marks) <= 1, "Multiple snapshot marks detected" - if marks: - snap = marks[0] - assert len(snap.args) == 0, "only keyword arguments are supported by the snapshot decorator" - if "token" not in snap.kwargs: - snap.kwargs["token"] = _request_token(request).replace(" ", "_").replace(os.path.sep, "_") - with client.snapshot_context(**snap.kwargs): - yield client - else: - yield client - finally: - log_path = f"{self.host_log_folder}/outputs/{request.cls.__name__}/{request.node.name}/agent_log.log" - request.node.add_report_section("teardown", "Test Agent Output", f"Log file:\n./{log_path}") - stop() -``` - -This makes `get_test_agent_api` a thin wrapper: there is now exactly one copy of the create-and-wait logic (`start_agent`). A reviewer will (correctly) flag it as a defect if the run/readiness block appears in both methods. - -- [ ] **Step 5: Run test to verify it passes** - -Run: `DOCKER_SMOKE=1 python -m pytest tests/test_the_test/test_start_stop_agent.py -v` -Expected: PASS (1 passed). The agent container starts, `info()` returns `version == "test"`, then stops cleanly. - -- [ ] **Step 6: Verify the fresh path still works (no regression)** - -Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context` -Expected: `1 passed`. (Confirms the refactor didn't break the existing per-test path, which still uses `get_test_agent_api`.) - -- [ ] **Step 7: Commit** - -```bash -git add utils/docker_fixtures/_test_agent.py tests/test_the_test/test_start_stop_agent.py -git commit -m "refactor(parametric): extract start_agent/stop_agent + rebind_request" -``` - ---- - -### Task 3: Worker-scoped network + pooled-agent creator on the scenario - -Give the scenario a `WorkerAgentPool` whose creator builds a worker-stable network + a `start_agent` lease. The network name is keyed by worker + env so two distinct envs on one worker get distinct networks. - -**Files:** -- Modify: `utils/_context/_scenarios/_docker_fixtures.py` (`34`, `56-98`) - -**Interfaces:** -- Consumes: `WorkerAgentPool`, `AgentLease`, `agent_env_key` (Task 1); `TestAgentFactory.start_agent` (Task 2); `get_docker_client` (`utils/docker_fixtures/_core.py`). -- Produces: - - `DockerFixturesScenario.get_agent_pool(self, worker_id: str) -> WorkerAgentPool` — lazily builds (once per process) a pool whose creator makes `f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}"` network + a `start_agent` lease named `f"ddapm-test-agent-worker-{worker_id}"`. - -- [ ] **Step 1: Add pool construction to the scenario** - -Add to `DockerFixturesScenario.__init__`, right after `self._test_agent_factory = TestAgentFactory(agent_image)` (`utils/_context/_scenarios/_docker_fixtures.py:34`): - -```python - self._agent_pool: "WorkerAgentPool | None" = None -``` - -Add these imports at the top of `utils/_context/_scenarios/_docker_fixtures.py` (with the other `from ...` lines): - -```python -from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, AgentLease, agent_env_key -``` - -Add this method to `DockerFixturesScenario` (after `_get_docker_network`, around `_docker_fixtures.py:74`): - -```python - def get_agent_pool(self, worker_id: str) -> WorkerAgentPool: - if self._agent_pool is None: - - def _creator(agent_env: dict[str, str]) -> AgentLease: - key = agent_env_key(agent_env) - network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" - network = get_docker_client().networks.create(name=network_name, driver="bridge") - container_name = f"ddapm-test-agent-worker-{worker_id}-{abs(hash(key))}" - api, stop_agent = self._test_agent_factory.start_agent( - request=self._pool_seed_request, - worker_id=worker_id, - container_name=container_name, - docker_network=network.name, - agent_env=agent_env, - container_otlp_http_port=4318, - container_otlp_grpc_port=4317, - ) - - def _stop() -> None: - try: - stop_agent() - finally: - try: - network.remove() - except Exception as e: # noqa: BLE001 - logger.info(f"Failed to remove worker network, ignoring: {e}") - - return AgentLease(api=api, stop=_stop) - - self._agent_pool = WorkerAgentPool(_creator) - return self._agent_pool -``` - -`start_agent` needs a `request` for the first-test log path. The pooled agent's container log spans the whole worker, so seed it with the first test's request and let `rebind_request` re-point the per-test API log thereafter. Add an attribute set by the fixture before first acquire — declare it in `__init__` after the `_agent_pool` line: - -```python - self._pool_seed_request = None -``` - -- [ ] **Step 2: Verify it imports and the fresh path is untouched** - -Run: `python -c "from utils._context._scenarios import scenarios; print(type(scenarios.parametric).__mro__[1].__name__)"` -Expected: prints `DockerFixturesScenario` with no import error. - -Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k test_otel_get_span_context` -Expected: `1 passed` (fresh path still default — nothing calls the pool yet). - -- [ ] **Step 3: Commit** - -```bash -git add utils/_context/_scenarios/_docker_fixtures.py -git commit -m "feat(parametric): pooled-agent creator (worker-scoped network + agent)" -``` - ---- - -### Task 4: Wire the pool into the `test_agent` fixture (with snapshot fallback) - -Make `test_agent` use the pool for non-snapshot tests and tear the pool down once per worker at session end. - -**Files:** -- Modify: `tests/parametric/conftest.py` (`76-93`) - -**Interfaces:** -- Consumes: `scenarios.parametric.get_agent_pool` (Task 3); `WorkerAgentPool.acquire/shutdown` (Task 1). -- Produces: a session-scoped `test_agent_pool` fixture and a pool-aware `test_agent` fixture. - -- [ ] **Step 1: Add a session-scoped pool-lifecycle fixture** - -Add to `tests/parametric/conftest.py` (near the other fixtures, after `agent_env`): - -```python -@pytest.fixture(scope="session") -def test_agent_pool(worker_id: str): - # scope="session" under pytest-xdist == once per worker. - pool = scenarios.parametric.get_agent_pool(worker_id) - yield pool - pool.shutdown() -``` - -- [ ] **Step 2: Replace the `test_agent` fixture body with the pooled-or-fresh decision** - -Replace the existing `test_agent` fixture (`tests/parametric/conftest.py:76-93`) with: - -```python -@pytest.fixture -def test_agent( - worker_id: str, - test_id: str, - request: pytest.FixtureRequest, - agent_env: dict[str, str], - test_agent_otlp_http_port: int, - test_agent_otlp_grpc_port: int, - test_agent_pool, -) -> Generator[TestAgentAPI, None, None]: - # POC: pool only default-agent_env, non-snapshot tests. Snapshot-marked tests need - # per-test snapshot_context lifecycle; non-default agent_env would require a second - # pooled agent per worker (worker-keyed host ports would collide). Both fall back to - # the fresh-per-test path. Pooled agents are reset with clear() between tests. - poolable = request.node.get_closest_marker("snapshot") is None and not agent_env - if poolable: - scenarios.parametric._pool_seed_request = request - api = test_agent_pool.acquire(request=request, agent_env=agent_env) - api.clear() # ensure a clean slate even on the very first acquire - yield api - return - - with scenarios.parametric.get_test_agent_api( - request=request, - worker_id=worker_id, - test_id=test_id, - agent_env=agent_env, - container_otlp_http_port=test_agent_otlp_http_port, - container_otlp_grpc_port=test_agent_otlp_grpc_port, - ) as result: - yield result -``` - -- [ ] **Step 3: Run the otel file — every test must pass through the pooled path** - -Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py` -Expected: same pass/skip/xfail counts as a baseline `git stash` run of the same command (e.g. `57 passed, 2 skipped, 2 xfailed`). No new failures. - -- [ ] **Step 4: Commit** - -```bash -git add tests/parametric/conftest.py -git commit -m "feat(parametric): use WorkerAgentPool in test_agent fixture" -``` - ---- - -### Task 5: Acceptance — prove reuse happened and no state leaked - -Two things must be true: (a) the agent container was created roughly once per worker, not once per test; (b) results are unchanged (no cross-test leakage). - -**Files:** -- Modify: `utils/docker_fixtures/_test_agent.py` (add a one-line creation counter log in `start_agent`) - -**Interfaces:** -- Consumes: `start_agent` (Task 2). -- Produces: a greppable `REUSE-POC agent container created` log line per real container creation. - -- [ ] **Step 1: Add a creation-count log line** - -In `start_agent` (Task 2), immediately after `log_file = open(...)`, add: - -```python - logger.stdout(f"REUSE-POC agent container created: {container_name}") -``` - -- [ ] **Step 2: Run the otel file single-worker and count creations** - -Run: -```bash -TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -p no:randomly -n 1 2>&1 | tee /tmp/reuse_run.log -grep -c "REUSE-POC agent container created" /tmp/reuse_run.log -``` -Expected: the count equals the number of **distinct `agent_env` values** in that file (1 for the default-env otel tests), **not** the ~57 test count. Pre-change this line does not exist; the equivalent (fresh) run creates one agent per test. - -- [ ] **Step 3: Leakage check — trace counts stay correct under reuse** - -`test_otel_get_span_context` and `test_otel_span_operation_name` each assert exactly one trace is present. If `clear()` were incomplete, a reused agent would carry the previous test's trace and these would see 2. - -Run: `TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -k "operation_name or get_span_context"` -Expected: all selected tests pass (each sees exactly its own single trace — proof the inter-test `clear()` reset the agent). - -- [ ] **Step 4: Full-file stability (run 3×, must be green each time)** - -Run: -```bash -for i in 1 2 3; do TEST_LIBRARY=php ./run.sh PARAMETRIC tests/parametric/test_otel_span_methods.py -q || echo "ITERATION $i FAILED"; done -``` -Expected: three clean runs, no "ITERATION n FAILED" line. - -- [ ] **Step 5: Capture the churn-reduction metric** - -The point of the POC is fewer container/network operations. Compute it directly from the run log captured in Step 2 (`/tmp/reuse_run.log`). - -Run: -```bash -AGENTS=$(grep -c "REUSE-POC agent container created" /tmp/reuse_run.log) -# Count test cases that ran (passed + failed + xfailed + xpassed) from the pytest summary: -TESTS=$(grep -oE "[0-9]+ (passed|xpassed|xfailed)" /tmp/reuse_run.log | awk '{s+=$1} END{print s}') -echo "tests run: ${TESTS}" -echo "pooled agents created: ${AGENTS} (fresh baseline would create one agent + one network per test = ${TESTS})" -python3 -c "t=${TESTS}; a=${AGENTS}; print(f'agent+network create/destroy cycles: {2*t} (before) -> {2*a} (after) = {100*(1-(a/t)):.0f}% fewer')" -``` -Expected (default-env otel file): `AGENTS` ≈ 1 (one pooled agent for the worker), `TESTS` ≈ 57 → the agent+network churn drops by ~98% for this file. Record the printed numbers in the report; they are the headline POC result. (The library client is still fresh per test — unchanged — so total per-test container ops drop from 3 to 1 for default-env tests.) - -- [ ] **Step 6: Commit** - -```bash -git add utils/docker_fixtures/_test_agent.py -git commit -m "test(parametric): log agent container creations for reuse verification" -``` - ---- - -### Task 6: Handoff note for the Platform team - -Per the thread (`#... ` Slack), this POC is handed to Platform to adopt or discard. Write the rationale + scope + risks so they can evaluate without re-deriving it. - -**Files:** -- Create: `docs/edit/parametric-test-agent-reuse-poc.md` - -- [ ] **Step 1: Write the handoff doc** - -```markdown -# POC: parametric test-agent reuse - -## Why -The parametric scenario creates a fresh Docker network + test-agent + client -container per test (16 xdist workers × ~440 tests). On the shared docker-in-docker -microVM runners this churn intermittently stalls container/network setup, so a -test-agent occasionally fails to make a just-sent trace queryable within the -tracer's ~3s receipt poll → `Number (1) of traces not available, got 0`. - -## What this changes -- One test-agent + one Docker network per (xdist worker, agent_env), reused - across tests, reset with `TestAgentAPI.clear()` (`/test/session/clear`) before - each test. Removes ~2 of the 3 create/destroy operations per test. -- The library client is unchanged: it derives its network and agent URL from the - `test_agent` object it is handed. - -## Scope / fallback -- Pools all non-snapshot tests; snapshot-marked tests keep the fresh-per-test path. -- Pool entries keyed by `agent_env`; differing envs get separate pooled agents. - -## Risks to weigh before adopting -- State leakage if `clear()` is ever incomplete (traces/telemetry/RC/OTLP/sessions). - Task 5's leakage check guards the trace case; extend it per data type before - rolling out to all tracers. -- In-flight data: a late trace from test N arriving after test N+1's `clear()` - could leak. Hardening path: per-test agent session tokens (requires client to - send `X-Datadog-Test-Session-Token`). -- Shared across 9 tracers — roll out behind a flag / one language at a time. -``` - -- [ ] **Step 2: Commit** - -```bash -git add docs/edit/parametric-test-agent-reuse-poc.md -git commit -m "docs(parametric): test-agent reuse POC handoff note" -``` - ---- - -## Self-Review - -**Spec coverage** (the Slack approach = "1:1 running-agent-to-worker mapping + synchronous reset between tests"): -- One agent per worker → Task 3 (worker-keyed creator) + Task 4 (`scope="session"` = per worker). ✓ -- Synchronous reset between tests → `clear()` on reuse in Task 1 + Task 4. ✓ -- Halve container count / reduce contention → Tasks 3–4 (also removes the per-test network); proven in Task 5. ✓ -- "Careful change / leakage risk" raised in thread → snapshot fallback (Task 4), leakage check (Task 5 Step 3), risks doc (Task 6). ✓ -- Hand POC to Platform → Task 6. ✓ - -**Placeholder scan:** no TBD/"handle edge cases"/"similar to". The one intentional knob — which tests are poolable — is concretely defined (`get_closest_marker("snapshot")`). - -**Type consistency:** `AgentLease(api, stop)`, `WorkerAgentPool(creator).acquire(request, agent_env)/shutdown()`, `agent_env_key(dict)->tuple`, `start_agent(...)->(TestAgentAPI, Callable)`, `rebind_request(request)` — used identically in Tasks 1–5. ✓ - -**Known limitation carried forward:** the pooled agent's *container* log (`agent_log.log`) spans the worker rather than one per test; per-test API interactions are re-pointed via `rebind_request`. Acceptable for a POC; noted in Task 6. From cf8b4fb25ef66fd97314fdbc8798681848b43a9d Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 15:48:19 -0400 Subject: [PATCH 15/24] fix(parametric-pool): apply pre-push review fixes for agent pool correctness - FIX 1: clear() raises on HTTP errors via raise_for_status() on both GETs - FIX 2: acquire() is now the single clear point (clears on every acquire, rebind only on reuse); remove redundant clear() from conftest pooled branch; update test assertion to clear_calls==2, rebind_calls==["req2"] - FIX 3: shutdown() isolates per-lease stop() failures with try/except so one bad lease doesn't abort cleanup of remaining leases - FIX 4a: wrap cm.__enter__() to close log_file on failure (no leak) - FIX 4b: wrap start_agent() in _creator to remove network on failure - FIX 5: add return type annotation to test_agent_pool fixture; annotate _creator request param as pytest.FixtureRequest - FIX 6: reword port-band comment to state real ~97-worker bound explicitly Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/parametric/conftest.py | 4 +- tests/test_the_test/test_test_agent_pool.py | 4 +- utils/_context/_scenarios/_docker_fixtures.py | 38 +++++++++++-------- utils/docker_fixtures/_test_agent.py | 10 +++-- utils/docker_fixtures/_test_agent_pool.py | 9 ++++- 5 files changed, 41 insertions(+), 24 deletions(-) diff --git a/tests/parametric/conftest.py b/tests/parametric/conftest.py index 9999011ff1d..ee1425ba69b 100644 --- a/tests/parametric/conftest.py +++ b/tests/parametric/conftest.py @@ -11,6 +11,7 @@ from utils import scenarios, logger from utils.docker_fixtures import TestAgentAPI, ParametricTestClientApi as APMLibrary +from utils.docker_fixtures._test_agent_pool import WorkerAgentPool # Max timeout in seconds to keep a container running @@ -75,7 +76,7 @@ def test_agent_otlp_grpc_port() -> int: @pytest.fixture(scope="session") -def test_agent_pool(worker_id: str): +def test_agent_pool(worker_id: str) -> Generator[WorkerAgentPool, None, None]: # scope="session" under pytest-xdist == once per worker. pool = scenarios.parametric.get_agent_pool(worker_id) yield pool @@ -99,7 +100,6 @@ def test_agent( poolable = request.node.get_closest_marker("snapshot") is None and not agent_env if poolable: api = test_agent_pool.acquire(request=request, agent_env=agent_env) - api.clear() # ensure a clean slate even on the very first acquire yield api return # REQUIRED: do not fall through into the fresh-path agent below diff --git a/tests/test_the_test/test_test_agent_pool.py b/tests/test_the_test/test_test_agent_pool.py index 0d166fcf65e..73dbc9c723b 100644 --- a/tests/test_the_test/test_test_agent_pool.py +++ b/tests/test_the_test/test_test_agent_pool.py @@ -45,8 +45,8 @@ def test_same_env_reuses_and_clears(): assert api1 is api2 # reused, not recreated assert len(creator.created_envs) == 1 # created exactly once - assert api1.clear_calls == 1 # cleared on the reuse, not the first acquire - assert api1.rebind_calls == ["req2"] # rebound to the second test's request + assert api1.clear_calls == 2 # cleared on both acquires (first + reuse) + assert api1.rebind_calls == ["req2"] # rebound only on reuse, not on first acquire def test_distinct_env_creates_separate_agents(): diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index 5dd841ede66..e7d3ace3033 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -84,27 +84,35 @@ def get_agent_pool(self, worker_id: str) -> WorkerAgentPool: # allocation and is out of scope for this POC. if self._agent_pool is None: - def _creator(request, agent_env: dict[str, str]) -> AgentLease: + def _creator(request: pytest.FixtureRequest, agent_env: dict[str, str]) -> AgentLease: key = agent_env_key(agent_env) network_name = f"{_NETWORK_PREFIX}_worker_{worker_id}_{abs(hash(key))}" network = get_docker_client().networks.create(name=network_name, driver="bridge") container_name = f"ddapm-test-agent-worker-{worker_id}-{abs(hash(key))}" # Pooled agents use a separate host-port band (4900/5000/5100 + worker offset) # so a worker's persistent pooled agent never collides with a fresh-path agent - # (4600/4701/4802) on that worker. These bands are clear of the client/framework - # 4500 and agent 4600/4701/4802 bands even with worker offsets. - api, stop_agent = self._test_agent_factory.start_agent( - request=request, - worker_id=worker_id, - container_name=container_name, - docker_network=network.name, - agent_env=agent_env, - container_otlp_http_port=4318, - container_otlp_grpc_port=4317, - agent_port_base=4900, - otlp_http_port_base=5000, - otlp_grpc_port_base=5100, - ) + # (4600/4701/4802) on that worker. The bands are non-overlapping for up to ~97 + # concurrent xdist workers (well above any real run): fresh OTLP-gRPC base 4802 + # + 98 == pooled base 4900 + 0. + try: + api, stop_agent = self._test_agent_factory.start_agent( + request=request, + worker_id=worker_id, + container_name=container_name, + docker_network=network.name, + agent_env=agent_env, + container_otlp_http_port=4318, + container_otlp_grpc_port=4317, + agent_port_base=4900, + otlp_http_port_base=5000, + otlp_grpc_port_base=5100, + ) + except BaseException: + try: + network.remove() + except Exception: # noqa: BLE001 + pass + raise def _stop() -> None: try: diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index ec65f77b536..8440a466789 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -123,7 +123,11 @@ def start_agent( log_file=log_file, network=docker_network, ) - cm.__enter__() + try: + cm.__enter__() + except BaseException: + log_file.close() + raise client = TestAgentAPI( container_name, @@ -444,8 +448,8 @@ def wait_for_num_v06_stats(self, num: int, *, wait_loops: int = 200) -> list[Age raise ValueError(f"Number ({num}) of /v0.6/stats requests not received, got {len(requests)}") def clear(self) -> None: - self._session.get(self._url("/test/session/clear")) - self._session.get(self._otlp_url("/test/session/clear")) + self._session.get(self._url("/test/session/clear")).raise_for_status() + self._session.get(self._otlp_url("/test/session/clear")).raise_for_status() def info(self): resp = self._session.get(self._url("/info")) diff --git a/utils/docker_fixtures/_test_agent_pool.py b/utils/docker_fixtures/_test_agent_pool.py index 1ebe8790d3b..6200e961730 100644 --- a/utils/docker_fixtures/_test_agent_pool.py +++ b/utils/docker_fixtures/_test_agent_pool.py @@ -2,6 +2,8 @@ from dataclasses import dataclass from typing import Any +from utils._logger import logger + def agent_env_key(agent_env: dict[str, str]) -> tuple: """Stable, hashable, order-independent key for an agent_env dict.""" @@ -40,11 +42,14 @@ def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: lease = self._creator(request, agent_env) self._leases[key] = lease else: - lease.api.clear() lease.api.rebind_request(request) + lease.api.clear() # always return a clean agent (covers first acquire too) return lease.api def shutdown(self) -> None: for lease in self._leases.values(): - lease.stop() + try: + lease.stop() + except Exception as e: # noqa: BLE001 + logger.info(f"Error stopping pooled agent lease, ignoring: {e}") self._leases.clear() From 4318912b3b66b2fb1886782a9553c1339381cb06 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 15:52:29 -0400 Subject: [PATCH 16/24] fix(parametric): keep OTLP session-clear best-effort (it returns 400 by design); enforce only the trace clear --- utils/docker_fixtures/_test_agent.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 8440a466789..229d07ba75d 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -448,8 +448,12 @@ def wait_for_num_v06_stats(self, num: int, *, wait_loops: int = 200) -> list[Age raise ValueError(f"Number ({num}) of /v0.6/stats requests not received, got {len(requests)}") def clear(self) -> None: + # The trace-session clear is safety-critical for pooled-agent reuse (a silent + # failure leaves the next test on dirty state), so surface a bad status here. self._session.get(self._url("/test/session/clear")).raise_for_status() - self._session.get(self._otlp_url("/test/session/clear")).raise_for_status() + # The OTLP test-agent's clear is best-effort and can return non-2xx (e.g. 400); + # don't fail the reset on it (matches the original fire-and-forget behavior). + self._session.get(self._otlp_url("/test/session/clear")) def info(self): resp = self._session.get(self._url("/info")) From 6d5078705d08ac8dfdcc656b455a8676edeff510 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 15:56:57 -0400 Subject: [PATCH 17/24] style(parametric): ruff format _test_agent.py (collapse wrapped log_path lines) --- utils/docker_fixtures/_test_agent.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 229d07ba75d..d5f981deec4 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -235,19 +235,14 @@ def __init__( self._session = requests.Session() self._pytest_request = pytest_request cls_name = pytest_request.cls.__name__ if pytest_request.cls else "NoClass" - self.log_path = ( - f"{host_log_folder}/outputs/{cls_name}/{pytest_request.node.name}/agent_api.log" - ) + self.log_path = f"{host_log_folder}/outputs/{cls_name}/{pytest_request.node.name}/agent_api.log" Path(self.log_path).parent.mkdir(parents=True, exist_ok=True) def rebind_request(self, pytest_request: "pytest.FixtureRequest") -> None: """Re-point per-test API logging at the current test (used on reuse).""" self._pytest_request = pytest_request cls_name = pytest_request.cls.__name__ if pytest_request.cls else "NoClass" - self.log_path = ( - f"{self._host_log_folder}/outputs/{cls_name}" - f"/{pytest_request.node.name}/agent_api.log" - ) + self.log_path = f"{self._host_log_folder}/outputs/{cls_name}/{pytest_request.node.name}/agent_api.log" Path(self.log_path).parent.mkdir(parents=True, exist_ok=True) def _url(self, path: str) -> str: From 2d8cb6758c0c6d3f72614d8ca3b5e7da5ec20907 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 16:04:14 -0400 Subject: [PATCH 18/24] fix(parametric): satisfy ruff check (annotations, drop unused noqa, log on network cleanup) Co-Authored-By: Claude Sonnet 4.6 --- tests/parametric/conftest.py | 2 +- tests/test_the_test/test_start_stop_agent.py | 12 ++++++------ tests/test_the_test/test_test_agent_pool.py | 6 ++---- utils/_context/_scenarios/_docker_fixtures.py | 8 ++++---- utils/docker_fixtures/_test_agent.py | 2 +- utils/docker_fixtures/_test_agent_pool.py | 12 ++++++++---- 6 files changed, 22 insertions(+), 20 deletions(-) diff --git a/tests/parametric/conftest.py b/tests/parametric/conftest.py index ee1425ba69b..88f50a91fe1 100644 --- a/tests/parametric/conftest.py +++ b/tests/parametric/conftest.py @@ -91,7 +91,7 @@ def test_agent( agent_env: dict[str, str], test_agent_otlp_http_port: int, test_agent_otlp_grpc_port: int, - test_agent_pool, + test_agent_pool: WorkerAgentPool, ) -> Generator[TestAgentAPI, None, None]: # POC: pool only default-agent_env, non-snapshot tests. Snapshot-marked tests need # per-test snapshot_context lifecycle; non-default agent_env would require a second diff --git a/tests/test_the_test/test_start_stop_agent.py b/tests/test_the_test/test_start_stop_agent.py index 02ace62d2cd..0b4b0e8ac1b 100644 --- a/tests/test_the_test/test_start_stop_agent.py +++ b/tests/test_the_test/test_start_stop_agent.py @@ -1,20 +1,20 @@ import pytest +from utils._context._scenarios import scenarios +from utils.docker_fixtures._core import get_docker_client + pytestmark = pytest.mark.skipif( __import__("os").getenv("DOCKER_SMOKE") != "1", reason="Docker-backed; run with DOCKER_SMOKE=1", ) -def test_start_agent_then_stop(request): - from utils._context._scenarios import scenarios - - factory = scenarios.parametric._test_agent_factory +def test_start_agent_then_stop(request: pytest.FixtureRequest): + factory = scenarios.parametric._test_agent_factory # noqa: SLF001 factory.configure("logs_parametric") factory.pull() - network = __import__("utils.docker_fixtures._core", fromlist=["get_docker_client"]) - client = network.get_docker_client().networks.create(name="reuse_smoke_net", driver="bridge") + client = get_docker_client().networks.create(name="reuse_smoke_net", driver="bridge") try: api, stop = factory.start_agent( request=request, diff --git a/tests/test_the_test/test_test_agent_pool.py b/tests/test_the_test/test_test_agent_pool.py index 73dbc9c723b..42d127abedb 100644 --- a/tests/test_the_test/test_test_agent_pool.py +++ b/tests/test_the_test/test_test_agent_pool.py @@ -1,4 +1,4 @@ -from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, agent_env_key +from utils.docker_fixtures._test_agent_pool import AgentLease, WorkerAgentPool, agent_env_key class _FakeApi: @@ -20,9 +20,7 @@ def __init__(self) -> None: self.created_envs: list[dict] = [] self.stopped = 0 - def __call__(self, request, agent_env: dict[str, str]): - from utils.docker_fixtures._test_agent_pool import AgentLease - + def __call__(self, request: object, agent_env: dict[str, str]) -> AgentLease: # noqa: ARG002 self.created_envs.append(dict(agent_env)) api = _FakeApi() diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index e7d3ace3033..bba016ca519 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -34,7 +34,7 @@ def __init__( ) self._test_agent_factory = TestAgentFactory(agent_image) - self._agent_pool: "WorkerAgentPool | None" = None + self._agent_pool: WorkerAgentPool | None = None def _clean(self): if self.is_main_worker: @@ -110,8 +110,8 @@ def _creator(request: pytest.FixtureRequest, agent_env: dict[str, str]) -> Agent except BaseException: try: network.remove() - except Exception: # noqa: BLE001 - pass + except Exception as e: + logger.info(f"Failed to remove network after start_agent failure, ignoring: {e}") raise def _stop() -> None: @@ -120,7 +120,7 @@ def _stop() -> None: finally: try: network.remove() - except Exception as e: # noqa: BLE001 + except Exception as e: logger.info(f"Failed to remove worker network, ignoring: {e}") return AgentLease(api=api, stop=_stop) diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index d5f981deec4..fc10e943545 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -143,7 +143,7 @@ def start_agent( for _ in range(100): try: resp = client.info() - except Exception as e: # noqa: BLE001 + except Exception as e: logger.debug(f"Wait for 0.1s for the test agent to be ready {e}") time.sleep(0.1) else: diff --git a/utils/docker_fixtures/_test_agent_pool.py b/utils/docker_fixtures/_test_agent_pool.py index 6200e961730..81cf156cf6b 100644 --- a/utils/docker_fixtures/_test_agent_pool.py +++ b/utils/docker_fixtures/_test_agent_pool.py @@ -1,9 +1,13 @@ from collections.abc import Callable from dataclasses import dataclass -from typing import Any +from typing import TYPE_CHECKING, Any from utils._logger import logger +if TYPE_CHECKING: + import pytest + from ._test_agent import TestAgentAPI + def agent_env_key(agent_env: dict[str, str]) -> tuple: """Stable, hashable, order-independent key for an agent_env dict.""" @@ -31,11 +35,11 @@ class WorkerAgentPool: `api.rebind_request()` (re-point per-test logging at the current test). """ - def __init__(self, creator: Callable[[Any, dict[str, str]], AgentLease]) -> None: + def __init__(self, creator: Callable[["pytest.FixtureRequest", dict[str, str]], AgentLease]) -> None: self._creator = creator self._leases: dict[tuple, AgentLease] = {} - def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: + def acquire(self, request: "pytest.FixtureRequest", agent_env: dict[str, str]) -> "TestAgentAPI": key = agent_env_key(agent_env) lease = self._leases.get(key) if lease is None: @@ -50,6 +54,6 @@ def shutdown(self) -> None: for lease in self._leases.values(): try: lease.stop() - except Exception as e: # noqa: BLE001 + except Exception as e: logger.info(f"Error stopping pooled agent lease, ignoring: {e}") self._leases.clear() From f4f2c342b5baa3a33044af74d8f07e45c7ea46ff Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 29 Jun 2026 16:10:24 -0400 Subject: [PATCH 19/24] fix(parametric): use Any+noqa for duck-typed pool acquire (fixes mypy attr-defined on fakes) --- utils/docker_fixtures/_test_agent_pool.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/utils/docker_fixtures/_test_agent_pool.py b/utils/docker_fixtures/_test_agent_pool.py index 81cf156cf6b..016a8fccabf 100644 --- a/utils/docker_fixtures/_test_agent_pool.py +++ b/utils/docker_fixtures/_test_agent_pool.py @@ -1,13 +1,9 @@ from collections.abc import Callable from dataclasses import dataclass -from typing import TYPE_CHECKING, Any +from typing import Any from utils._logger import logger -if TYPE_CHECKING: - import pytest - from ._test_agent import TestAgentAPI - def agent_env_key(agent_env: dict[str, str]) -> tuple: """Stable, hashable, order-independent key for an agent_env dict.""" @@ -35,11 +31,13 @@ class WorkerAgentPool: `api.rebind_request()` (re-point per-test logging at the current test). """ - def __init__(self, creator: Callable[["pytest.FixtureRequest", dict[str, str]], AgentLease]) -> None: + def __init__(self, creator: Callable[[Any, dict[str, str]], AgentLease]) -> None: self._creator = creator self._leases: dict[tuple, AgentLease] = {} - def acquire(self, request: "pytest.FixtureRequest", agent_env: dict[str, str]) -> "TestAgentAPI": + # request/return are duck-typed (a real FixtureRequest + TestAgentAPI in prod, fakes in + # tests); Any keeps this module Docker-free. + def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: # noqa: ANN401 key = agent_env_key(agent_env) lease = self._leases.get(key) if lease is None: From 3163895499b45eec74f16b1eed6bbc274c86c00d Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Tue, 30 Jun 2026 09:20:36 -0400 Subject: [PATCH 20/24] fix(parametric): don't pool agents for custom-OTLP-port tests Tests in test_otel_logs.py / test_otel_metrics.py parametrize the test_agent_otlp_http_port / test_agent_otlp_grpc_port fixtures to custom values so the agent container listens on a non-default OTLP port. The pooled agent is created once per worker with fixed container OTLP ports (4318/4317), so a pooled custom-port test sends OTLP to a port the agent isn't listening on -> "Number 1 of logs not available, got 0". Extend the poolable predicate to require the resolved OTLP ports equal the pooled defaults; custom-port tests fall back to the fresh-per-test path. Consolidate the default OTLP ports into DEFAULT_OTLP_{HTTP,GRPC}_PORT in _test_agent.py so the pool creator and the poolable check share one source of truth and cannot drift apart. Co-Authored-By: Claude Opus 4.8 --- tests/parametric/conftest.py | 22 +++++++++++++------ utils/_context/_scenarios/_docker_fixtures.py | 18 ++++++++++----- utils/docker_fixtures/_test_agent.py | 8 +++++++ 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/tests/parametric/conftest.py b/tests/parametric/conftest.py index 88f50a91fe1..6dcc2aea53b 100644 --- a/tests/parametric/conftest.py +++ b/tests/parametric/conftest.py @@ -11,6 +11,7 @@ from utils import scenarios, logger from utils.docker_fixtures import TestAgentAPI, ParametricTestClientApi as APMLibrary +from utils.docker_fixtures._test_agent import DEFAULT_OTLP_HTTP_PORT, DEFAULT_OTLP_GRPC_PORT from utils.docker_fixtures._test_agent_pool import WorkerAgentPool @@ -67,12 +68,12 @@ def docker() -> str | None: @pytest.fixture def test_agent_otlp_http_port() -> int: - return 4318 + return DEFAULT_OTLP_HTTP_PORT @pytest.fixture def test_agent_otlp_grpc_port() -> int: - return 4317 + return DEFAULT_OTLP_GRPC_PORT @pytest.fixture(scope="session") @@ -93,11 +94,18 @@ def test_agent( test_agent_otlp_grpc_port: int, test_agent_pool: WorkerAgentPool, ) -> Generator[TestAgentAPI, None, None]: - # POC: pool only default-agent_env, non-snapshot tests. Snapshot-marked tests need - # per-test snapshot_context lifecycle; non-default agent_env would require a second - # pooled agent per worker (worker-keyed host ports would collide). Both fall back to - # the fresh-per-test path. Pooled agents are reset with clear() between tests. - poolable = request.node.get_closest_marker("snapshot") is None and not agent_env + # POC: pool only default-agent_env, default-OTLP-port, non-snapshot tests. + # Snapshot-marked tests need per-test snapshot_context lifecycle; non-default + # agent_env would require a second pooled agent per worker (worker-keyed host ports + # would collide); a parametrized custom container OTLP port needs an agent listening + # on that port, which the pool's fixed-port agent cannot serve. All fall back to the + # fresh-per-test path. Pooled agents are reset with clear() between tests. + poolable = ( + request.node.get_closest_marker("snapshot") is None + and not agent_env + and test_agent_otlp_http_port == DEFAULT_OTLP_HTTP_PORT + and test_agent_otlp_grpc_port == DEFAULT_OTLP_GRPC_PORT + ) if poolable: api = test_agent_pool.acquire(request=request, agent_env=agent_env) yield api diff --git a/utils/_context/_scenarios/_docker_fixtures.py b/utils/_context/_scenarios/_docker_fixtures.py index bba016ca519..84163d5760c 100644 --- a/utils/_context/_scenarios/_docker_fixtures.py +++ b/utils/_context/_scenarios/_docker_fixtures.py @@ -7,7 +7,12 @@ from docker.errors import DockerException -from utils.docker_fixtures._test_agent import TestAgentFactory, TestAgentAPI +from utils.docker_fixtures._test_agent import ( + TestAgentFactory, + TestAgentAPI, + DEFAULT_OTLP_HTTP_PORT, + DEFAULT_OTLP_GRPC_PORT, +) from utils.docker_fixtures._test_agent_pool import WorkerAgentPool, AgentLease, agent_env_key from utils._context.docker import get_docker_client from utils._logger import logger @@ -101,8 +106,11 @@ def _creator(request: pytest.FixtureRequest, agent_env: dict[str, str]) -> Agent container_name=container_name, docker_network=network.name, agent_env=agent_env, - container_otlp_http_port=4318, - container_otlp_grpc_port=4317, + # Fixed container OTLP ports for the pooled agent. The poolable check + # in tests/parametric/conftest.py only pools tests using these ports, + # since a parametrized custom OTLP port needs an agent listening on it. + container_otlp_http_port=DEFAULT_OTLP_HTTP_PORT, + container_otlp_grpc_port=DEFAULT_OTLP_GRPC_PORT, agent_port_base=4900, otlp_http_port_base=5000, otlp_grpc_port_base=5100, @@ -135,8 +143,8 @@ def get_test_agent_api( request: pytest.FixtureRequest, test_id: str, agent_env: dict[str, str], - container_otlp_http_port: int = 4318, - container_otlp_grpc_port: int = 4317, + container_otlp_http_port: int = DEFAULT_OTLP_HTTP_PORT, + container_otlp_grpc_port: int = DEFAULT_OTLP_GRPC_PORT, ) -> Generator[TestAgentAPI, None, None]: with ( self._get_docker_network(test_id) as docker_network, diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index fc10e943545..39255ac9dc8 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -26,6 +26,14 @@ from ._core import get_host_port, get_docker_client, docker_run +# Default container-internal OTLP ports for the test-agent. The pooled parametric agent +# (DockerFixturesScenario.get_agent_pool) is created with these, and the poolable check +# in tests/parametric/conftest.py only pools tests that use them — a test that +# parametrizes a custom OTLP port needs a fresh agent listening on that port. Single +# source of truth so the pool creator and the poolable check cannot drift apart. +DEFAULT_OTLP_HTTP_PORT = 4318 +DEFAULT_OTLP_GRPC_PORT = 4317 + def _request_token(request: pytest.FixtureRequest) -> str: token = "" From 4f2e50afbca7422f65d0d2029bcbd92930e3af26 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Tue, 30 Jun 2026 10:00:30 -0400 Subject: [PATCH 21/24] fix(test_the_test): gate docker smoke test at runtime, not via skipif marker The module-level pytest.mark.skipif(, ...) crashed collection for every session that collects tests/test_the_test/: the root conftest's _item_must_pass does all(marker.args[0]) over skipif markers, which raises "TypeError: 'bool' object is not iterable" on a bool condition. system-tests gates via @features/@scenarios rather than raw skipif, so this was the only bool-arg skipif in the suite and broke ~169 end-to-end jobs at collection. Gate the Docker-backed test with an in-body pytest.skip() instead, and use the shared DEFAULT_OTLP_{HTTP,GRPC}_PORT constants. Co-Authored-By: Claude Opus 4.8 --- tests/test_the_test/test_start_stop_agent.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/test_the_test/test_start_stop_agent.py b/tests/test_the_test/test_start_stop_agent.py index 0b4b0e8ac1b..6da403fb806 100644 --- a/tests/test_the_test/test_start_stop_agent.py +++ b/tests/test_the_test/test_start_stop_agent.py @@ -1,15 +1,20 @@ +import os + import pytest from utils._context._scenarios import scenarios from utils.docker_fixtures._core import get_docker_client - -pytestmark = pytest.mark.skipif( - __import__("os").getenv("DOCKER_SMOKE") != "1", - reason="Docker-backed; run with DOCKER_SMOKE=1", -) +from utils.docker_fixtures._test_agent import DEFAULT_OTLP_HTTP_PORT, DEFAULT_OTLP_GRPC_PORT def test_start_agent_then_stop(request: pytest.FixtureRequest): + # Docker-backed smoke test. Gate at runtime rather than with a module-level + # pytest.mark.skipif: the root conftest's _item_must_pass iterates skipif + # marker.args[0] (all(marker.args[0])), which raises on a bool condition and + # crashes collection for every session that collects this file. + if os.getenv("DOCKER_SMOKE") != "1": + pytest.skip("Docker-backed; run with DOCKER_SMOKE=1") + factory = scenarios.parametric._test_agent_factory # noqa: SLF001 factory.configure("logs_parametric") factory.pull() @@ -22,8 +27,8 @@ def test_start_agent_then_stop(request: pytest.FixtureRequest): container_name="ddapm-test-agent-reuse-smoke", docker_network=client.name, agent_env={}, - container_otlp_http_port=4318, - container_otlp_grpc_port=4317, + container_otlp_http_port=DEFAULT_OTLP_HTTP_PORT, + container_otlp_grpc_port=DEFAULT_OTLP_GRPC_PORT, ) try: assert api.info()["version"] == "test" # agent answered → it is ready From 19575080627f9a5c2aec6a7a33a4d631e3178b0c Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Tue, 30 Jun 2026 10:18:28 -0400 Subject: [PATCH 22/24] fix(parametric): reset remote-config in pooled agent clear() /test/session/clear drops recorded requests but not the remote-config the agent serves on /v0.7/config (RemoteConfigServer._responses). Non-snapshot parametric tests all use the default (token-less) RC slot, so on a pooled agent a prior test that sets RC leaks into the next test's no-RC baseline -- e.g. TestDynamicConfigV1::test_trace_sampling_rate_override_* assert the default sample rate on the first trace before applying their own RC, and saw the leaked rate instead. Reset the RC response to empty {} in clear() (the same state a fresh agent serves). RC is the only persistent agent state outside _requests; traces, telemetry, and tested-integrations all flow through _requests and are already cleared by /test/session/clear. Co-Authored-By: Claude Opus 4.8 --- utils/docker_fixtures/_test_agent.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 39255ac9dc8..901926c6526 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -454,6 +454,14 @@ def clear(self) -> None: # The trace-session clear is safety-critical for pooled-agent reuse (a silent # failure leaves the next test on dirty state), so surface a bad status here. self._session.get(self._url("/test/session/clear")).raise_for_status() + # /test/session/clear drops recorded requests but NOT the remote-config the + # agent serves on /v0.7/config (RemoteConfigServer._responses). Pooled + # non-snapshot tests all share the default (token-less) RC slot, so a prior + # test's RC would leak into the next test's no-RC baseline (e.g. the + # dynamic-config sampling tests assert the default sample rate before applying + # RC). Posting an empty config restores the same {} state a fresh agent has. + # Safety-critical for reuse, so surface a bad status. + self._session.post(self._url("/test/session/responses/config"), json={}).raise_for_status() # The OTLP test-agent's clear is best-effort and can return non-2xx (e.g. 400); # don't fail the reset on it (matches the original fire-and-forget behavior). self._session.get(self._otlp_url("/test/session/clear")) From 73e0de7158ea93400440498cc1aa341a2df8a8c6 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Tue, 30 Jun 2026 13:48:49 -0400 Subject: [PATCH 23/24] docs: move parametric agent-reuse POC writeup off the PR The design doc is hosted internally (chonk) and linked from the PR description instead of living in the repo. Co-Authored-By: Claude Opus 4.8 --- docs/edit/parametric-test-agent-reuse-poc.md | 81 -------------------- 1 file changed, 81 deletions(-) delete mode 100644 docs/edit/parametric-test-agent-reuse-poc.md diff --git a/docs/edit/parametric-test-agent-reuse-poc.md b/docs/edit/parametric-test-agent-reuse-poc.md deleted file mode 100644 index 6f4d4e2c9be..00000000000 --- a/docs/edit/parametric-test-agent-reuse-poc.md +++ /dev/null @@ -1,81 +0,0 @@ -# POC: parametric test-agent reuse - -## Why - -The parametric scenario creates a fresh Docker network + test-agent + client -container per test (16 xdist workers x ~440 tests). On the shared docker-in-docker -microVM runners this churn intermittently stalls container/network setup, so a -test-agent occasionally fails to make a just-sent trace queryable within the -tracer's ~3 s receipt poll -> `Number (1) of traces not available, got 0`. - -## What this changes - -- One test-agent + one Docker network per xdist worker, reused across tests, - reset with `TestAgentAPI.clear()` (`/test/session/clear`) before each test. -- The library client container is unchanged: it is still created fresh per test - and derives its network and agent URL from the `test_agent` object it is handed. - Per-test container operations therefore drop from 3 to 1 for default-env tests - (agent + network + client -> client only), not a uniform 98% cut in all containers. - -## Scope / fallback - -Pooling is enabled only for **non-snapshot tests with the default `agent_env == {}`**, -which covers roughly 94% of parametric tests. Two categories fall back to the -existing fresh-per-test path: - -1. **Snapshot-marked tests** (`pytest.mark.snapshot`) -- these compare recorded - agent output and need a clean agent on every run. -2. **Tests with a non-default `agent_env`** -- the pooled agent persists for the - whole worker session on a worker-keyed host port. Allowing a second pooled - agent with a different environment, or colliding with a fresh-path agent on - the same worker, would hit "port already allocated". Keeping at most one - pooled agent per worker avoids that collision. - -To pool non-default-env tests in the future, allocate host ports per -`(worker, env)` instead of per worker. - -## Measured results - -Benchmark file: `tests/parametric/test_otel_span_methods.py`, single worker. - -| Metric | Before | After | -|--------|--------|-------| -| Agent+network create/destroy cycles | 118 | 2 | -| Reduction | -- | ~98% | -| Tests driving the single pooled agent | -- | 59 | -| Per-test container ops (default-env tests) | 3 | 1 | - -Stability: 3 consecutive full-file runs each produced 57 passed / 2 skipped / -2 xfailed with no cross-test state leakage detected. - -## Operational note - -The `REUSE-POC agent container created:` count line is emitted via -`logger.stdout` and lands in `logs_parametric/tests.log` (not the tee'd console -stdout). To measure agent creation counts in CI, grep that file: - -``` -grep "REUSE-POC agent container created" logs_parametric/tests.log | wc -l -``` - -## Risks to weigh before adopting - -- **State leakage** if `clear()` is ever incomplete. `TestAgentAPI.clear()` - (`/test/session/clear`) covers traces, telemetry, RC, OTLP, and sessions; - verified for the trace case in this POC. Extend the leakage check per data - type before rolling out to all tracers. -- **In-flight data**: a late trace from test N arriving after test N+1's - `clear()` could leak. Hardening path: per-test agent session tokens (requires - the client to send `X-Datadog-Test-Session-Token`). -- **Agent container log spans the worker session**, not per-test. Per-test API - interactions are re-pointed via `rebind_request`; the raw container log is - shared. Acceptable for a POC; revisit if per-test log isolation is needed. -- **Shared across 9 tracers** -- roll out behind a flag / one language at a time. - -## Hardening path summary - -1. Extend leakage checks to telemetry, RC, OTLP, and session data. -2. Add per-test session tokens (`X-Datadog-Test-Session-Token`) to guard - against in-flight-data leakage. -3. Allocate ports per `(worker, env)` to enable pooling of non-default-env tests. -4. Gate the feature behind a flag and roll out one tracer language at a time. From d50bfd781fae905d40ca967a834049edd86ad659 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Tue, 30 Jun 2026 14:21:08 -0400 Subject: [PATCH 24/24] fix(parametric): reset remote-config on pool reuse, not in clear() clear() is also called mid-test by the clear=True helpers (traces, telemetry, wait_for_rc_apply_state, ...). Resetting RC inside clear() wiped a test's active config mid-test, so the tracer could poll the empty config before the test asserted RC-dependent behavior (race; passed CI only by timing). Move the RC reset to a dedicated reset_remote_config() called only on the pooled agent's reuse path (WorkerAgentPool.acquire), between tests. clear() goes back to clearing recorded requests only. Pooled tests still get a clean RC baseline; no mid-test clear=True call touches the served config. Addresses Codex review on PR #7235. Co-Authored-By: Claude Opus 4.8 --- tests/test_the_test/test_test_agent_pool.py | 5 ++++ utils/docker_fixtures/_test_agent.py | 30 ++++++++++++++------- utils/docker_fixtures/_test_agent_pool.py | 7 ++++- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/tests/test_the_test/test_test_agent_pool.py b/tests/test_the_test/test_test_agent_pool.py index 42d127abedb..c095329f06c 100644 --- a/tests/test_the_test/test_test_agent_pool.py +++ b/tests/test_the_test/test_test_agent_pool.py @@ -4,11 +4,15 @@ class _FakeApi: def __init__(self) -> None: self.clear_calls = 0 + self.reset_rc_calls = 0 self.rebind_calls: list[object] = [] def clear(self) -> None: self.clear_calls += 1 + def reset_remote_config(self) -> None: + self.reset_rc_calls += 1 + def rebind_request(self, request: object) -> None: self.rebind_calls.append(request) @@ -44,6 +48,7 @@ def test_same_env_reuses_and_clears(): assert api1 is api2 # reused, not recreated assert len(creator.created_envs) == 1 # created exactly once assert api1.clear_calls == 2 # cleared on both acquires (first + reuse) + assert api1.reset_rc_calls == 2 # remote-config reset on both acquires too assert api1.rebind_calls == ["req2"] # rebound only on reuse, not on first acquire diff --git a/utils/docker_fixtures/_test_agent.py b/utils/docker_fixtures/_test_agent.py index 901926c6526..6e8e3f454bb 100644 --- a/utils/docker_fixtures/_test_agent.py +++ b/utils/docker_fixtures/_test_agent.py @@ -451,21 +451,31 @@ def wait_for_num_v06_stats(self, num: int, *, wait_loops: int = 200) -> list[Age raise ValueError(f"Number ({num}) of /v0.6/stats requests not received, got {len(requests)}") def clear(self) -> None: - # The trace-session clear is safety-critical for pooled-agent reuse (a silent - # failure leaves the next test on dirty state), so surface a bad status here. + # Drops recorded requests (traces, telemetry, integrations) only. Also used + # mid-test by the clear=True helpers, so it must NOT touch the remote-config the + # agent serves -- wiping a test's active RC mid-test would change tracer behavior + # before the test asserts it. The pooled-reuse RC reset lives in + # reset_remote_config(), called only between tests. Safety-critical for reuse (a + # silent failure leaves the next test on dirty state), so surface a bad status. self._session.get(self._url("/test/session/clear")).raise_for_status() - # /test/session/clear drops recorded requests but NOT the remote-config the - # agent serves on /v0.7/config (RemoteConfigServer._responses). Pooled - # non-snapshot tests all share the default (token-less) RC slot, so a prior - # test's RC would leak into the next test's no-RC baseline (e.g. the - # dynamic-config sampling tests assert the default sample rate before applying - # RC). Posting an empty config restores the same {} state a fresh agent has. - # Safety-critical for reuse, so surface a bad status. - self._session.post(self._url("/test/session/responses/config"), json={}).raise_for_status() # The OTLP test-agent's clear is best-effort and can return non-2xx (e.g. 400); # don't fail the reset on it (matches the original fire-and-forget behavior). self._session.get(self._otlp_url("/test/session/clear")) + def reset_remote_config(self) -> None: + """Restore the served remote-config to the empty `{}` state a fresh agent has. + + Kept out of `clear()` on purpose: `clear()` is also invoked mid-test by the + `clear=True` helpers, where wiping the active config would flip what the tracer + polls before the test asserts on it. Only the pooled-reuse path (between tests) + calls this, so a pooled non-snapshot test starts from a clean RC baseline -- + e.g. the dynamic-config sampling tests assert the default rate before applying + their own RC. `/test/session/clear` does not reset RC (RemoteConfigServer + keeps `_responses` keyed by token), and non-snapshot tests share the default + token, so a prior test's RC would otherwise leak in. + """ + self._session.post(self._url("/test/session/responses/config"), json={}).raise_for_status() + def info(self): resp = self._session.get(self._url("/info")) diff --git a/utils/docker_fixtures/_test_agent_pool.py b/utils/docker_fixtures/_test_agent_pool.py index 016a8fccabf..5d37ed150ae 100644 --- a/utils/docker_fixtures/_test_agent_pool.py +++ b/utils/docker_fixtures/_test_agent_pool.py @@ -45,7 +45,12 @@ def acquire(self, request: Any, agent_env: dict[str, str]) -> Any: # noqa: ANN4 self._leases[key] = lease else: lease.api.rebind_request(request) - lease.api.clear() # always return a clean agent (covers first acquire too) + # Always hand back a fully reset agent (covers the first acquire too): drop + # recorded requests, then restore the served remote-config to a fresh-agent + # state. The RC reset is done here, not in clear(), so mid-test clear=True + # helpers don't wipe a test's active config. + lease.api.clear() + lease.api.reset_remote_config() return lease.api def shutdown(self) -> None: