fix(intake): handle concurrent docker-run Conflict in run_clickhouse.sh#893
fix(intake): handle concurrent docker-run Conflict in run_clickhouse.sh#893ironcommit wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe ClickHouse launcher and evaluator integration tests now support configurable container endpoints, synchronized startup, TCP readiness checks, worker-level readiness logging, and propagation of the resolved ClickHouse URL to Intake services. ChangesClickHouse integration startup
Sequence Diagram(s)sequenceDiagram
participant TestFixture
participant ClickHouseScript
participant Docker
participant IntakeServices
TestFixture->>ClickHouseScript: start with configured endpoint
ClickHouseScript->>Docker: lock, inspect, and start container
Docker-->>ClickHouseScript: configured ports become available
TestFixture->>TestFixture: wait for TCP readiness
TestFixture->>IntakeServices: pass NMP_INTAKE_CLICKHOUSE_URL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/intake/scripts/spans/run_clickhouse.sh`:
- Around line 41-44: Restrict the conflict handling in the container startup
flow to cases where the specific ${container_name} is confirmed running after
docker run fails. Before proceeding to docker exec, recheck the container with
the existing docker-ps mechanism and only swallow the error when it is running;
propagate port-binding and other failures instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61308ed5-777b-429f-94af-3d7ebaf0dacd
📒 Files selected for processing (1)
services/intake/scripts/spans/run_clickhouse.sh
|
The evaluator publish integration tests share one local ClickHouse and platform stack because ClickHouse is expensive to duplicate. Under pytest-xdist, worker restarts can instantiate session fixtures in multiple workers, so concurrent setup must be serialized instead of relying on Docker exit 125 or parsing Docker stderr text. Add a cross-process startup lock to run_clickhouse.sh, keep the default nmp-intake-clickhouse singleton, and expose env overrides for the container name and host ports for callers that need isolation. The helper logs the resolved HTTP and native endpoints on every successful path. Hold a fixture-level lock across the evaluator ClickHouse/platform lifecycle so another worker cannot start or tear down the singleton while it is in use. Wire the fixture through the ClickHouse URL, container, and port overrides. Forward the ClickHouse endpoint diagnostic through pytest-xdist worker output so it remains visible in passing CI logs, not only in captured failure output. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
72b386a to
80b5937
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py (1)
66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
CLICKHOUSE_CI_LOG_KEYstring constant across two files. Same literal independently declared in both; if one drifts,pytest_testnodedownsilently drops messages with no error.
plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py#L66: keep as the single source of truth (or move to a shared module) and import it fromconftest.py.plugins/nemo-evaluator/tests/integration/conftest.py#L37: importCLICKHOUSE_CI_LOG_KEYfromtest_publish_to_intake.py(or shared module) instead of redeclaring the literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py` at line 66, Use a single source of truth for CLICKHOUSE_CI_LOG_KEY: retain the definition in plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py:66, and update plugins/nemo-evaluator/tests/integration/conftest.py:37 to import and reuse it instead of redeclaring the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py`:
- Around line 151-172: Update the _clickhouse fixture so _file_lock protects
only the startup subprocess.run call, allowing xdist workers to use the shared
container concurrently. Replace the unconditional docker rm -f teardown with
coordination that removes CLICKHOUSE_CONTAINER only after the final worker
releases the shared session, preserving the container for workers still using
it.
---
Nitpick comments:
In `@plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py`:
- Line 66: Use a single source of truth for CLICKHOUSE_CI_LOG_KEY: retain the
definition in
plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py:66, and
update plugins/nemo-evaluator/tests/integration/conftest.py:37 to import and
reuse it instead of redeclaring the literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fd8b601a-23b0-404e-a829-c8ed95a10e79
📒 Files selected for processing (3)
plugins/nemo-evaluator/tests/integration/conftest.pyplugins/nemo-evaluator/tests/integration/test_publish_to_intake.pyservices/intake/scripts/spans/run_clickhouse.sh
| def _clickhouse(request: pytest.FixtureRequest) -> Iterator[None]: | ||
| with _file_lock(CLICKHOUSE_TEST_LOCK_FILE): | ||
| if not _docker_available(): | ||
| pytest.skip("Docker not available; required for ClickHouse-backed Intake") | ||
| with _pytest_capture_disabled(request): | ||
| subprocess.run( | ||
| ["bash", str(REPO_ROOT / "services/intake/scripts/spans/run_clickhouse.sh")], | ||
| check=True, | ||
| cwd=REPO_ROOT, | ||
| env={ | ||
| **os.environ, | ||
| "CLICKHOUSE_CONTAINER_NAME": CLICKHOUSE_CONTAINER, | ||
| "CLICKHOUSE_HTTP_PORT": str(CLICKHOUSE_HTTP_PORT), | ||
| "CLICKHOUSE_NATIVE_PORT": str(CLICKHOUSE_NATIVE_PORT), | ||
| }, | ||
| ) | ||
| try: | ||
| _wait_for_tcp(CLICKHOUSE_HOST, CLICKHOUSE_HTTP_PORT, timeout=60) | ||
| _log_clickhouse_endpoint(request) | ||
| yield | ||
| finally: | ||
| subprocess.run(["docker", "rm", "-f", CLICKHOUSE_CONTAINER], check=False) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
File lock spans the whole session, serializing all xdist workers.
with _file_lock(...) wraps yield and the teardown, not just the run_clickhouse.sh call. Since _clickhouse is session-scoped and xdist runs one full session per worker, worker B's lock acquisition blocks until worker A's entire test session and its docker rm -f finish. This defeats the goal of letting workers share one container while running in parallel — they now run fully serialized.
Separately, even with a narrower lock, the unconditional docker rm -f in finally removes the shared container as soon as any single worker's session ends, breaking other workers still using it.
🔒 Narrow the lock to just startup (teardown-safety still needs a reference-count/last-worker design)
def _clickhouse(request: pytest.FixtureRequest) -> Iterator[None]:
- with _file_lock(CLICKHOUSE_TEST_LOCK_FILE):
- if not _docker_available():
- pytest.skip("Docker not available; required for ClickHouse-backed Intake")
- with _pytest_capture_disabled(request):
- subprocess.run(
- ["bash", str(REPO_ROOT / "services/intake/scripts/spans/run_clickhouse.sh")],
- check=True,
- cwd=REPO_ROOT,
- env={
- **os.environ,
- "CLICKHOUSE_CONTAINER_NAME": CLICKHOUSE_CONTAINER,
- "CLICKHOUSE_HTTP_PORT": str(CLICKHOUSE_HTTP_PORT),
- "CLICKHOUSE_NATIVE_PORT": str(CLICKHOUSE_NATIVE_PORT),
- },
- )
- try:
- _wait_for_tcp(CLICKHOUSE_HOST, CLICKHOUSE_HTTP_PORT, timeout=60)
- _log_clickhouse_endpoint(request)
- yield
- finally:
- subprocess.run(["docker", "rm", "-f", CLICKHOUSE_CONTAINER], check=False)
+ if not _docker_available():
+ pytest.skip("Docker not available; required for ClickHouse-backed Intake")
+ with _file_lock(CLICKHOUSE_TEST_LOCK_FILE):
+ with _pytest_capture_disabled(request):
+ subprocess.run(
+ ["bash", str(REPO_ROOT / "services/intake/scripts/spans/run_clickhouse.sh")],
+ check=True,
+ cwd=REPO_ROOT,
+ env={
+ **os.environ,
+ "CLICKHOUSE_CONTAINER_NAME": CLICKHOUSE_CONTAINER,
+ "CLICKHOUSE_HTTP_PORT": str(CLICKHOUSE_HTTP_PORT),
+ "CLICKHOUSE_NATIVE_PORT": str(CLICKHOUSE_NATIVE_PORT),
+ },
+ )
+ _wait_for_tcp(CLICKHOUSE_HOST, CLICKHOUSE_HTTP_PORT, timeout=60)
+ _log_clickhouse_endpoint(request)
+ yield
+ # TODO: teardown still needs a reference-count/last-worker check before
+ # `docker rm -f` — otherwise the last worker to finish kills a container
+ # other workers may still depend on.🧰 Tools
🪛 ast-grep (0.44.1)
[error] 155-165: Command coming from incoming request
Context: subprocess.run(
["bash", str(REPO_ROOT / "services/intake/scripts/spans/run_clickhouse.sh")],
check=True,
cwd=REPO_ROOT,
env={
**os.environ,
"CLICKHOUSE_CONTAINER_NAME": CLICKHOUSE_CONTAINER,
"CLICKHOUSE_HTTP_PORT": str(CLICKHOUSE_HTTP_PORT),
"CLICKHOUSE_NATIVE_PORT": str(CLICKHOUSE_NATIVE_PORT),
},
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 171-171: Command coming from incoming request
Context: subprocess.run(["docker", "rm", "-f", CLICKHOUSE_CONTAINER], check=False)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py` around
lines 151 - 172, Update the _clickhouse fixture so _file_lock protects only the
startup subprocess.run call, allowing xdist workers to use the shared container
concurrently. Replace the unconditional docker rm -f teardown with coordination
that removes CLICKHOUSE_CONTAINER only after the final worker releases the
shared session, preserving the container for workers still using it.
|
|
||
| @pytest.fixture(scope="session") | ||
| def _clickhouse(request: pytest.FixtureRequest) -> Iterator[None]: | ||
| with _file_lock(CLICKHOUSE_TEST_LOCK_FILE): |
There was a problem hiding this comment.
Consider dropping this fixture-level _file_lock entirely and relying on the flock now added to run_clickhouse.sh. Each xdist worker spawns run_clickhouse.sh, and the script serializes the check-then-docker run under flock on ${TMPDIR}/<container>.lock (same container name -> same lock path), so the second caller falls into the "already running" branch and a Docker Conflict is no longer reachable. The startup race is thus fully handled at the shell level, making this Python-side lock redundant. It's also the specific cause of the full-session serialization flagged in the critical review comment, since it wraps yield and teardown rather than just startup. Removing it fixes that regression by construction.
| _log_clickhouse_endpoint(request) | ||
| yield | ||
| finally: | ||
| subprocess.run(["docker", "rm", "-f", CLICKHOUSE_CONTAINER], check=False) |
There was a problem hiding this comment.
Even once the lock is narrowed to startup, this unconditional docker rm -f still races: the first worker to finish teardown removes the shared singleton while other workers are still mid-test. Note that "just leak the container" isn't a safe alternative here either, because session_id_for() is deterministic (f"{run_id}:{trial_id}", see plugins/nemo-evaluator/src/nemo_evaluator/intake/mapping.py:62) -- stale rows under the same session id would contaminate re-runs, and this teardown is what currently guarantees a clean slate. This needs a last-worker / reference-count guard (or an "only remove if this process created it" check, keyed off run_clickhouse.sh's created-vs-reused signal) before calling docker rm -f.
| native_port="${CLICKHOUSE_NATIVE_PORT:-9000}" | ||
| script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" | ||
| repo_root="$(cd -- "${script_dir}/../../../.." && pwd)" | ||
| data_dir="${CLICKHOUSE_DATA_DIR:-${repo_root}/tmp/intake-clickhouse}" |
There was a problem hiding this comment.
data_dir isn't namespaced by container_name, so the new CLICKHOUSE_CONTAINER_NAME / port overrides don't actually give isolation. A caller who sets only the container name (or only the ports) without also setting CLICKHOUSE_DATA_DIR gets a second server bound to the same /var/lib/clickhouse volume -> lock contention / data corruption. Either derive data_dir (and the lock file) from container_name, or document that CLICKHOUSE_CONTAINER_NAME, the ports, and CLICKHOUSE_DATA_DIR must be overridden together to isolate an instance.
| return sock.connect_ex((host, port)) == 0 | ||
|
|
||
|
|
||
| def pytest_testnodedown(node, error) -> None: # noqa: ANN001, ARG001 |
There was a problem hiding this comment.
I'm assuming this weird method name matches up with a hook from the xdist package?
| clickhouse_user="${CLICKHOUSE_USER:-default}" | ||
| clickhouse_password="${CLICKHOUSE_PASSWORD:-}" | ||
| http_port="${CLICKHOUSE_HTTP_PORT:-8123}" | ||
| native_port="${CLICKHOUSE_NATIVE_PORT:-9000}" |
3 participants
Summary
test_publish_skips_nan_and_failed_scoresfailed at fixture setup withCalledProcessError(exit 125) fromrun_clickhouse.shdocker pschecks and then raced todocker run, with the second caller receiving a Docker Conflict error because the first had already creatednmp-intake-clickhousedocker runto capture stderr and treat a "Conflict / already in use" error as a non-fatal condition — the container was created by a concurrent caller and is already running; real docker errors still propagate normallyRoot cause
run_clickhouse.shuses a check-then-create pattern that is not atomic:Under
--dist loadscopewith--max-worker-restart=2, a worker restart can cause tests from the same module to be split across two workers with independent session scopes, each independently calling the fixture that invokes this script.Fix
Test plan
bash -nsyntax check passesshellcheckpasses with no warningsThe failing commit (
8adcee4, Anonymizer Studio scaffold) is entirely unrelated to this failure — it touches only TypeScript frontend files. This is a pre-existing flake in test infrastructure.Summary by CodeRabbit
Bug Fixes
Tests