From 49e9f597483d0d986d25457de0c830cb0df92d3b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 17:57:44 +0000 Subject: [PATCH 01/12] fix: thread project_prefix through container cleanup inventory + fix image ref field (#1065 review) _inventory_resources/_list_images/_list_containers/_list_volumes ignored the caller's project_prefix and always classified against the hard-coded DEFAULT_UAT_PROJECT_PREFIX, so `--prefix foo` left foo-owned leftovers uncleaned while still targeting default benchbox-uat resources. Thread project_prefix through all four functions. Also: current `container image ls --format json` renders the image reference at the top-level displayReference field, not configuration.name (which is empty on those rows), so owned-mode image reclaim never matched. Read displayReference first, falling back to configuration.name for older schema variants. Also corrects throughput-timeout-leak-and-success-gates.yaml's root-cause diagnosis (#1068 review): StreamRunner.execute's timeout enforcement is dead code, not just unable to cancel a running thread -- as_completed() only ever yields an already-done future, so the subsequent future.result(timeout=...) can never itself raise TimeoutError. w1 is corrected to fix detection (a wait()-with-deadline poll loop) before surfacing leaked streams. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- ...ughput-timeout-leak-and-success-gates.yaml | 45 ++++++++---- tests/uat/container_cleanup.py | 34 +++++---- tests/uat/test_container_cleanup.py | 72 +++++++++++++++++++ 3 files changed, 123 insertions(+), 28 deletions(-) diff --git a/_project/TODO/main/planning/throughput-timeout-leak-and-success-gates.yaml b/_project/TODO/main/planning/throughput-timeout-leak-and-success-gates.yaml index d7d140202e..712b387fea 100644 --- a/_project/TODO/main/planning/throughput-timeout-leak-and-success-gates.yaml +++ b/_project/TODO/main/planning/throughput-timeout-leak-and-success-gates.yaml @@ -8,17 +8,36 @@ category: Core Functionality description: | Two smaller robustness/consistency issues in the throughput executor. - 1. TIMED-OUT STREAM LEAK: StreamRunner.execute enforces per-stream timeout via - `future.result(timeout=...)` but cannot cancel a running thread - (benchbox/core/throughput/runner.py:87-113). A timed-out stream keeps - executing in the background, still holding its connection and consuming - CPU/server resources, while the executor records it as failed and moves on. - During a long throughput run this means the reported TTT window can overlap - zombie streams that are still hammering the DB -- skewing both the failed - run's accounting and any subsequent phase (e.g. maintenance) that starts - while zombies run. At minimum this should be surfaced; ideally streams - should be cooperatively cancellable or isolated so a timeout actually stops - work. + 1. TIMED-OUT STREAM LEAK: StreamRunner.execute's timeout enforcement is dead + code, not just unable to cancel a running thread. It iterates + `for future in concurrent.futures.as_completed(future_to_stream_id.keys())` + with no `timeout=` argument to `as_completed` itself + (benchbox/core/throughput/runner.py:88), then calls + `future.result(timeout=timeout)` (line 93). `as_completed()` only ever + yields a future once that future is ALREADY done, so by the time the loop + body runs, `.result(timeout=...)` is called on a completed future and + returns/raises immediately -- it can never itself raise + `concurrent.futures.TimeoutError`, because there is nothing left to wait + for. The `except concurrent.futures.TimeoutError` branch (lines 108-113) is + therefore unreachable: a genuinely hung stream is never detected as timed + out at all, and the loop just blocks indefinitely inside `as_completed` + waiting for that stream's future to eventually finish (if ever), rather + than moving on and recording a timeout. This must be fixed BEFORE "cannot + cancel a running thread" becomes the relevant limitation: detecting the + timeout at all requires polling with an actual deadline (e.g. + `concurrent.futures.wait(pending, timeout=remaining, return_when=FIRST_COMPLETED)` + in a loop, checking elapsed time against `config.stream_timeout` each + iteration) instead of a bare `as_completed()` walk. Once detection works, + the still-true secondary problem applies: Python cannot cancel a running + thread, so a detected-as-timed-out stream keeps executing in the + background, still holding its connection and consuming CPU/server + resources, while the executor records it as failed and moves on. During a + long throughput run this means the reported TTT window can overlap zombie + streams that are still hammering the DB -- skewing both the failed run's + accounting and any subsequent phase (e.g. maintenance) that starts while + zombies run. At minimum the leak should be surfaced; ideally streams + should be cooperatively cancellable or isolated so a timeout actually + stops work. 2. DIVERGENT SUCCESS GATES: TPC-H throughput uses a configurable min_success_rate (default 0.99) (benchbox/core/tpch/throughput_test.py:38, @@ -34,12 +53,12 @@ description: | prior_art: - "benchbox/core/throughput/runner.py:StreamRunner.execute — timeout handling + the as_completed loop." - "benchbox/core/tpch/throughput_test.py:TPCHThroughputTestConfig.min_success_rate — the configurable-gate pattern to mirror for TPC-DS." - - "benchbox/experimental/concurrency/executor.py — has a resource-monitor thread pattern usable to detect/report zombie streams." + - "benchbox/experimental/load_testing/executor.py — has a resource-monitor thread pattern usable to detect/report zombie streams." - "docs/development/throughput-result-alignment.md — documents the current divergence; update alongside any change." work: - id: w1 - summary: "Surface timed-out (leaked) streams: annotate the result and log that background execution may continue" + summary: "Fix timeout detection first (as_completed never yields an unfinished future, so future.result(timeout=) is dead code) via a wait()-with-deadline poll loop, then annotate leaked streams" status: pending - id: w2 summary: "Add a cooperative-cancellation or isolation option so a stream timeout can actually stop work (opt-in)" diff --git a/tests/uat/container_cleanup.py b/tests/uat/container_cleanup.py index 614be78853..a657c7394d 100644 --- a/tests/uat/container_cleanup.py +++ b/tests/uat/container_cleanup.py @@ -154,7 +154,7 @@ def reclaim_container_usage( ) run = runner or docker_assets.run_docker_command - resources = _inventory_resources(run) + resources = _inventory_resources(run, project_prefix) targets, retained = _partition_targets(resources, mode) planned = _plan_commands(targets, mode) footprint_before = _read_footprint(run) @@ -321,27 +321,33 @@ def _is_forbidden(argv: tuple[str, ...]) -> bool: # -------------------------------------------------------------------------- -def _inventory_resources(run: ContainerRunner) -> tuple[ContainerResource, ...]: +def _inventory_resources(run: ContainerRunner, project_prefix: str) -> tuple[ContainerResource, ...]: resources: list[ContainerResource] = [] - resources.extend(_list_images(run)) - resources.extend(_list_containers(run)) - resources.extend(_list_volumes(run)) + resources.extend(_list_images(run, project_prefix)) + resources.extend(_list_containers(run, project_prefix)) + resources.extend(_list_volumes(run, project_prefix)) return tuple(resources) -def _list_images(run: ContainerRunner) -> list[ContainerResource]: +def _list_images(run: ContainerRunner, project_prefix: str) -> list[ContainerResource]: rows = _run_json_array(run, [CONTAINER_BIN, "image", "ls", "--format", "json"]) out: list[ContainerResource] = [] for row in rows: + row = row if isinstance(row, dict) else {} config = row.get("configuration") if isinstance(row, dict) else None config = config if isinstance(config, dict) else {} - name = str(config.get("name") or "") - identifier = str(row.get("id") or "") + # Current `container image ls --format json` renders ImageResource rows + # with the reference at the top-level `displayReference` field (see + # upstream ImageList.swift); `configuration.name` is empty on those + # rows. Fall back to `configuration.name` for older/other CLI versions + # that still populate it there. + name = str(row.get("displayReference") or config.get("name") or "") + identifier = str(row.get("id") or row.get("digest") or "") created_at = str(config.get("creationDate") or "") category: ContainerCategory if _BUILDER_IMAGE_MARKER in name: category = "system" - elif _is_owned_image_name(name, DEFAULT_UAT_PROJECT_PREFIX): + elif _is_owned_image_name(name, project_prefix): category = "owned" else: category = "shared" @@ -357,7 +363,7 @@ def _list_images(run: ContainerRunner) -> list[ContainerResource]: return out -def _list_containers(run: ContainerRunner) -> list[ContainerResource]: +def _list_containers(run: ContainerRunner, project_prefix: str) -> list[ContainerResource]: rows = _run_json_array(run, [CONTAINER_BIN, "ls", "-a", "--format", "json"]) out: list[ContainerResource] = [] for row in rows: @@ -376,9 +382,7 @@ def _list_containers(run: ContainerRunner) -> list[ContainerResource]: if labels.get(_BUILDER_ROLE_LABEL) == "builder" or _BUILDER_IMAGE_MARKER in image_ref: category = "system" - elif _is_owned(project, DEFAULT_UAT_PROJECT_PREFIX) or _is_owned_image_name( - image_ref, DEFAULT_UAT_PROJECT_PREFIX - ): + elif _is_owned(project, project_prefix) or _is_owned_image_name(image_ref, project_prefix): category = "owned" else: category = "shared" @@ -395,7 +399,7 @@ def _list_containers(run: ContainerRunner) -> list[ContainerResource]: return out -def _list_volumes(run: ContainerRunner) -> list[ContainerResource]: +def _list_volumes(run: ContainerRunner, project_prefix: str) -> list[ContainerResource]: rows = _run_json_array(run, [CONTAINER_BIN, "volume", "ls", "--format", "json"]) out: list[ContainerResource] = [] for row in rows: @@ -404,7 +408,7 @@ def _list_volumes(run: ContainerRunner) -> list[ContainerResource]: name = str(row.get("name") or row.get("Name") or "") if not name: continue - category: ContainerCategory = "owned" if _is_owned(name, DEFAULT_UAT_PROJECT_PREFIX) else "shared" + category: ContainerCategory = "owned" if _is_owned(name, project_prefix) else "shared" out.append( ContainerResource( kind="volume", diff --git a/tests/uat/test_container_cleanup.py b/tests/uat/test_container_cleanup.py index 37aabffd79..a3499620e2 100644 --- a/tests/uat/test_container_cleanup.py +++ b/tests/uat/test_container_cleanup.py @@ -183,3 +183,75 @@ def test_report_renders_mode_and_store_path(): assert "mode: owned (dry-run)" in rendered assert "com.apple.container" in rendered assert "Retained (widen --mode to reclaim):" in rendered + + +def test_custom_project_prefix_reaches_classification(): + """#1065 review: --prefix must actually classify resources, not just be + echoed in the report. _inventory_resources/_list_images/_list_containers/ + _list_volumes previously ignored the caller's project_prefix and always + classified against the hard-coded DEFAULT_UAT_PROJECT_PREFIX.""" + custom_images = [ + {"id": "img-custom", "configuration": {"name": "foo-tpc-h:latest", "creationDate": "2026-07-08T00:00:00Z"}}, + ] + custom_containers = [ + { + "id": "custom-leftover", + "configuration": { + "id": "custom-leftover", + "image": {"reference": "postgres:18"}, + "labels": {"com.docker.compose.project": "foo-smoke-postgresql"}, + }, + "status": {"state": "stopped"}, + }, + ] + + def fake(argv, **kwargs): + argv_tuple = tuple(argv) + if argv_tuple[:4] == ("container", "image", "ls", "--format"): + return docker_assets.DockerCommandResult(argv_tuple, 0, json.dumps(custom_images), "") + if argv_tuple[:3] == ("container", "ls", "-a"): + return docker_assets.DockerCommandResult(argv_tuple, 0, json.dumps(custom_containers), "") + if argv_tuple[:4] == ("container", "volume", "ls", "--format"): + return docker_assets.DockerCommandResult(argv_tuple, 0, "[]", "") + if argv_tuple == ("container", "system", "df"): + return docker_assets.DockerCommandResult(argv_tuple, 0, _SYSTEM_DF, "") + return docker_assets.DockerCommandResult(argv_tuple, 0, "", "") + + report = container_cleanup.reclaim_container_usage(mode="owned", apply=False, project_prefix="foo", runner=fake) + assert _targets_by_kind(report) == { + "image": {"foo-tpc-h:latest"}, + "container": {"custom-leftover"}, + } + + +def test_image_reference_read_from_display_reference_field(): + """#1065 review: current `container image ls --format json` renders + ImageResource rows with the reference at the top-level `displayReference` + field, not `configuration.name` (which is empty on those rows).""" + images = [ + { + "id": "sha256:abc123", + "displayReference": "benchbox/tpc-h-linux-arm64:latest", + "configuration": {"descriptor": {"digest": "sha256:abc123"}}, + }, + { + "id": "sha256:def456", + "displayReference": "postgres:18", + "configuration": {"descriptor": {"digest": "sha256:def456"}}, + }, + ] + + def fake(argv, **kwargs): + argv_tuple = tuple(argv) + if argv_tuple[:4] == ("container", "image", "ls", "--format"): + return docker_assets.DockerCommandResult(argv_tuple, 0, json.dumps(images), "") + if argv_tuple[:3] == ("container", "ls", "-a"): + return docker_assets.DockerCommandResult(argv_tuple, 0, "[]", "") + if argv_tuple[:4] == ("container", "volume", "ls", "--format"): + return docker_assets.DockerCommandResult(argv_tuple, 0, "[]", "") + if argv_tuple == ("container", "system", "df"): + return docker_assets.DockerCommandResult(argv_tuple, 0, _SYSTEM_DF, "") + return docker_assets.DockerCommandResult(argv_tuple, 0, "", "") + + report = container_cleanup.reclaim_container_usage(mode="owned", apply=False, runner=fake) + assert _targets_by_kind(report) == {"image": {"benchbox/tpc-h-linux-arm64:latest"}} From 9e90b688d81312708c6bda468fe6605f5ad6eb36 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:01:27 +0000 Subject: [PATCH 02/12] fix: restore trailing-delimiter normalization as a fallback safety net (#1069 review) #1069 removed normalize_tbl_trailing_delimiters() entirely, reasoning all bundled dbgen binaries are now compiled with -DEOL_HANDLING. But TPCCompiler.needs_compilation() only checks whether a binary file already exists -- it has no staleness/CFLAGS check against current source -- so a pre-existing, locally-built _sources/tpc-h/dbgen/dbgen predating that fix (or a source compile on a platform outside the precompiled matrix) is reused as-is and would silently reintroduce the trailing pipe with no safety net. Restored the normalization pass in _finalize_generation. The O(1) tail probe keeps it a no-op for the now-clean bundled binaries (matching the original design rationale), while still normalizing the fallback case. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- benchbox/core/tpch/generator.py | 110 ++++++++++++++++++ ...t_tpch_trailing_delimiter_normalization.py | 108 +++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 tests/unit/core/tpch/test_tpch_trailing_delimiter_normalization.py diff --git a/benchbox/core/tpch/generator.py b/benchbox/core/tpch/generator.py index d826aa2058..2186819317 100644 --- a/benchbox/core/tpch/generator.py +++ b/benchbox/core/tpch/generator.py @@ -45,6 +45,87 @@ def _load_generator_specs() -> dict[str, Any]: _TPCH_TABLE_CODES = dict(_GENERATOR_SPECS["table_codes"]) _TPCH_BASE_ROW_COUNTS = dict(_GENERATOR_SPECS["base_row_counts"]) +# Chunk size for the streaming trailing-delimiter rewrite (bytes). +_NORMALIZE_CHUNK_SIZE = 1 << 20 + + +def _has_trailing_delimiter(path: Path) -> bool: + """Probe the last bytes of ``path`` for a classic dbgen trailing delimiter. + + dbgen row framing is decided at compile time (EOL_HANDLING), so every row + in a file shares the same framing; inspecting the file tail is sufficient + to classify the whole file. + """ + size = path.stat().st_size + if size == 0: + return False + with path.open("rb") as handle: + handle.seek(max(0, size - 2)) + tail = handle.read() + return tail.endswith((b"|\n", b"|")) + + +def normalize_tbl_trailing_delimiters(path: Path) -> bool: + """Strip exactly one trailing ``|`` before each newline, rewriting in place. + + The bundled dbgen binaries are all compiled with -DEOL_HANDLING (#1069) + and emit no trailing delimiter, so this is normally an O(1) tail-probe + no-op. It remains a safety net for classic dbgen file-mode output (a + stale locally-built binary predating that CFLAGS fix, or any source + compile on a platform outside the precompiled matrix): TPCCompiler's + ``needs_compilation()`` only checks whether a binary file already exists, + with no staleness/CFLAGS check against current source, so a pre-existing + ``_sources/tpc-h/dbgen/dbgen`` is reused as-is and never recompiled. + + Only the delimiter immediately before the line terminator is removed; + interior empty fields are preserved. In ``.tbl`` format a trailing empty + field is a delimiter artifact, not a NULL (see + :func:`benchbox.utils.file_format.get_data_extension`). + + The rewrite streams fixed-size chunks through a temporary file, so large + scale-factor outputs are never loaded into memory. Files that are already + clean are detected with an O(1) tail probe and left untouched, which also + makes the operation idempotent. + + Args: + path: Uncompressed ``.tbl`` (or ``.tbl.N`` chunk) file to normalize. + + Returns: + True if the file was rewritten, False if it was already clean. + """ + if not _has_trailing_delimiter(path): + return False + + tmp_path = path.with_name(path.name + ".normalize.tmp") + try: + with path.open("rb") as src, tmp_path.open("wb") as dst: + carry = b"" + while True: + chunk = src.read(_NORMALIZE_CHUNK_SIZE) + if not chunk: + break + data = carry + chunk + # Hold back a chunk-final "|": it may be followed by "\n" in + # the next chunk and must be stripped together with it. + if data.endswith(b"|"): + carry = b"|" + data = data[:-1] + else: + carry = b"" + # "\n" only occurs at row boundaries, so "|\n" is always the + # trailing delimiter; str.replace removes exactly one "|" per + # newline and never touches interior empty fields. + dst.write(data.replace(b"|\n", b"\n")) + # A held-back "|" at EOF means the final row is unterminated but + # still carries the trailing delimiter; drop it. + with contextlib.suppress(OSError): + shutil.copystat(path, tmp_path) + os.replace(tmp_path, path) + finally: + with contextlib.suppress(OSError): + tmp_path.unlink(missing_ok=True) + return True + class TPCHDataGenerator(CompressionMixin, CloudStorageGeneratorMixin, VerbosityMixin): """TPC-H data generator. @@ -974,6 +1055,16 @@ def _finalize_generation(self, target_dir: Path) -> dict[str, Path | list[Path]] """ table_paths, precompressed_tables = self._gather_generated_table_paths(target_dir) + # Classic (non -z) dbgen writes a trailing field delimiter on every + # row. Normalize file-mode output before any compression or data + # organization so results match the streaming path byte-for-byte. + # The bundled binaries are clean (#1069), so this is normally an O(1) + # no-op; it remains a safety net for a stale locally-built dbgen that + # predates -DEOL_HANDLING (needs_compilation() has no staleness + # check, so an existing binary is never recompiled - see + # normalize_tbl_trailing_delimiters's docstring). + self._normalize_table_delimiters(table_paths, precompressed_tables) + # Data organization is a post-generation output mode. When configured, # prefer organized outputs over raw compression artifacts. if self._data_organization_config is not None: @@ -995,6 +1086,25 @@ def _finalize_generation(self, target_dir: Path) -> dict[str, Path | list[Path]] self._write_manifest(target_dir, table_paths) return table_paths + def _normalize_table_delimiters( + self, + table_paths: dict[str, Path | list[Path]], + precompressed_tables: set[str], + ) -> None: + """Strip classic dbgen trailing row delimiters from uncompressed outputs. + + Covers both single-file mode (``customer.tbl``) and parallel chunked + mode (``customer.tbl.1`` ... ``customer.tbl.N``). Pre-compressed + tables are skipped: they come from the streaming (-z) path or a + previous run and are already normalized. + """ + for table_name, paths in table_paths.items(): + if table_name in precompressed_tables: + continue + for file_path in paths if isinstance(paths, list) else [paths]: + if normalize_tbl_trailing_delimiters(file_path): + self.log_verbose(f"Stripped trailing row delimiters from {file_path.name}") + def _apply_data_organization( self, target_dir: Path, diff --git a/tests/unit/core/tpch/test_tpch_trailing_delimiter_normalization.py b/tests/unit/core/tpch/test_tpch_trailing_delimiter_normalization.py new file mode 100644 index 0000000000..f94beb2d48 --- /dev/null +++ b/tests/unit/core/tpch/test_tpch_trailing_delimiter_normalization.py @@ -0,0 +1,108 @@ +"""Fast guard: TPC-H file-mode output has its trailing delimiter stripped. + +The bundled dbgen binaries are compiled with -DEOL_HANDLING (#1069) and emit +no trailing field separator, but TPCCompiler.needs_compilation() only checks +whether a binary file already exists -- it has no staleness/CFLAGS check +against the current source -- so a pre-existing, locally-built +``_sources/tpc-h/dbgen/dbgen`` predating that fix (or a source compile on a +platform outside the precompiled matrix) is reused as-is and would silently +reintroduce the trailing pipe without this normalization safety net (#1069 +review). See ``normalize_tbl_trailing_delimiters`` in generator.py. + +Copyright 2026 Joe Harris / BenchBox Project + +Licensed under the MIT License. See LICENSE file in the project root for details. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from benchbox.core.tpch.generator import TPCHDataGenerator, normalize_tbl_trailing_delimiters + +pytestmark = [ + pytest.mark.unit, + pytest.mark.fast, +] + + +def test_strips_exactly_one_trailing_pipe_per_row(tmp_path: Path) -> None: + path = tmp_path / "customer.tbl" + path.write_text("1|Customer#1|BUILDING|comment.|\n2|Customer#2|AUTOMOBILE|comment2.|\n", encoding="utf-8") + + rewritten = normalize_tbl_trailing_delimiters(path) + + assert rewritten is True + assert path.read_text(encoding="utf-8") == "1|Customer#1|BUILDING|comment.\n2|Customer#2|AUTOMOBILE|comment2.\n" + + +def test_interior_empty_fields_are_preserved(tmp_path: Path) -> None: + path = tmp_path / "orders.tbl" + path.write_text("1||BUILDING||comment.|\n", encoding="utf-8") + + normalize_tbl_trailing_delimiters(path) + + assert path.read_text(encoding="utf-8") == "1||BUILDING||comment.\n" + + +def test_already_clean_file_is_left_untouched(tmp_path: Path) -> None: + path = tmp_path / "nation.tbl" + original = "0|ALGERIA|0|comment.\n1|ARGENTINA|1|comment2.\n" + path.write_text(original, encoding="utf-8") + mtime_before = path.stat().st_mtime_ns + + rewritten = normalize_tbl_trailing_delimiters(path) + + assert rewritten is False + assert path.read_text(encoding="utf-8") == original + assert path.stat().st_mtime_ns == mtime_before + + +def test_idempotent_on_repeated_calls(tmp_path: Path) -> None: + path = tmp_path / "region.tbl" + path.write_text("0|AFRICA|comment.|\n", encoding="utf-8") + + first = normalize_tbl_trailing_delimiters(path) + second = normalize_tbl_trailing_delimiters(path) + + assert first is True + assert second is False + assert path.read_text(encoding="utf-8") == "0|AFRICA|comment.\n" + + +def test_chunk_boundary_carry_does_not_corrupt_rows(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """A trailing '|' that lands exactly at a chunk boundary must still be + stripped, not left dangling or merged into the next row.""" + import benchbox.core.tpch.generator as generator_module + + monkeypatch.setattr(generator_module, "_NORMALIZE_CHUNK_SIZE", 5) + path = tmp_path / "supplier.tbl.1" + path.write_text("1|AB|\n2|CD|\n", encoding="utf-8") + + normalize_tbl_trailing_delimiters(path) + + assert path.read_text(encoding="utf-8") == "1|AB\n2|CD\n" + + +def test_finalize_generation_normalizes_fallback_file_mode_output(tmp_path: Path) -> None: + """#1069 review: _finalize_generation must still normalize file-mode + output for the fallback path (a stale, non-bundled dbgen build that + predates -DEOL_HANDLING), not just trust that all reachable binaries are + already clean.""" + dirty_path = tmp_path / "customer.tbl" + dirty_path.write_text("1|Customer#1|BUILDING|comment.|\n", encoding="utf-8") + + gen = TPCHDataGenerator.__new__(TPCHDataGenerator) + gen._data_organization_config = None + gen.verbose_enabled = False + gen.should_use_compression = MagicMock(return_value=False) + gen._write_manifest = MagicMock() + gen._gather_generated_table_paths = MagicMock(return_value=({"customer": dirty_path}, set())) + gen.log_verbose = MagicMock() + + gen._finalize_generation(tmp_path) + + assert dirty_path.read_text(encoding="utf-8") == "1|Customer#1|BUILDING|comment.\n" From 275ed34d6a6bd18e21c44feb9ad488cb5d6b92f8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:03:33 +0000 Subject: [PATCH 03/12] fix(ci): pin release smoke test to the built version (#1072 review) test-installation's post-publish smoke test installed unqualified `benchbox`, so PyPI/Test PyPI propagation lag or a pre-existing Test PyPI version could install a different release than the one this workflow run just built and published, silently missing a broken current wheel. Add `build` to test-installation's needs and pin both install steps to benchbox==${{ needs.build.outputs.version }}. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- .github/workflows/release.yml | 10 +++++++--- tests/unit/test_release_infrastructure.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 581b612ba4..3ea05c6b3c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -189,7 +189,7 @@ jobs: # Verify installation works across platforms after publishing test-installation: - needs: publish + needs: [build, publish] runs-on: ${{ matrix.os }} strategy: fail-fast: false @@ -214,15 +214,19 @@ jobs: - name: Test installation from PyPI if: github.event.inputs.test_pypi != 'true' + env: + BENCHBOX_VERSION: ${{ needs.build.outputs.version }} run: | - pip install benchbox + pip install "benchbox==${BENCHBOX_VERSION}" python -c "import importlib.util; import benchbox; assert importlib.util.find_spec('pandas') is None, 'published core wheel must not pull pandas'; print(f'BenchBox {benchbox.__version__} installed successfully (minimal import surface)')" benchbox --help - name: Test installation from Test PyPI if: github.event.inputs.test_pypi == 'true' + env: + BENCHBOX_VERSION: ${{ needs.build.outputs.version }} run: | - pip install -i https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ benchbox + pip install -i https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ "benchbox==${BENCHBOX_VERSION}" python -c "import importlib.util; import benchbox; assert importlib.util.find_spec('pandas') is None, 'published core wheel must not pull pandas'; print(f'BenchBox {benchbox.__version__} installed successfully (minimal import surface)')" benchbox --help diff --git a/tests/unit/test_release_infrastructure.py b/tests/unit/test_release_infrastructure.py index 388d3350c1..cec0a3e96f 100644 --- a/tests/unit/test_release_infrastructure.py +++ b/tests/unit/test_release_infrastructure.py @@ -245,6 +245,24 @@ def test_release_workflow_configuration(self): assert "id-token" in publish_job["permissions"] assert publish_job["permissions"]["id-token"] == "write" + def test_release_smoke_test_pins_installed_version(self): + """#1072 review: the post-publish smoke test must install the exact + version this workflow run just built, not an unqualified `benchbox` + - PyPI/Test PyPI propagation lag or a pre-existing Test PyPI version + could otherwise mask a broken current wheel.""" + jobs = _workflow("release.yml")["jobs"] + test_installation_job = jobs["test-installation"] + + needs = test_installation_job["needs"] + needs = [needs] if isinstance(needs, str) else needs + assert "build" in needs, "test-installation must depend on build to reach needs.build.outputs.version" + + release_workflow_text = (REPO_ROOT / ".github" / "workflows" / "release.yml").read_text(encoding="utf-8") + assert "pip install benchbox" not in release_workflow_text, "smoke test must not install an unpinned benchbox" + assert release_workflow_text.count("needs.build.outputs.version") >= 3, ( + "the build/publish version pin, plus both PyPI and Test PyPI install steps, must reference it" + ) + def test_release_required_result_contract(self): """Test the main-PR release-required umbrella check shape.""" jobs = _workflow("test.yml")["jobs"] From 0017c7f7ddcf8bcd8b7d4e20bad1bea0f730ee22 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:04:54 +0000 Subject: [PATCH 04/12] fix: update load_testing rename in experimental enumerations (#1073 review) benchbox/experimental/__init__.py's subsystem list and the _EXPERIMENTAL_SUBSYSTEMS no-registry-leak guard in test_cli_package_exports.py still said "concurrency" after the concurrency -> load_testing rename, so the guard checked a name that no longer exists (vacuously passing) instead of the real module it's meant to protect. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- benchbox/experimental/__init__.py | 2 +- tests/unit/cli/test_cli_package_exports.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/benchbox/experimental/__init__.py b/benchbox/experimental/__init__.py index 96a3d4761e..40bfce03a3 100644 --- a/benchbox/experimental/__init__.py +++ b/benchbox/experimental/__init__.py @@ -11,5 +11,5 @@ - ``aiml_functions`` - AI/ML SQL function benchmarks - ``multiregion`` - Multi-region latency and orchestration - ``gpu`` - GPU detection, metrics, and benchmark primitives -- ``concurrency`` - Concurrent load testing patterns +- ``load_testing`` - Concurrent load testing patterns """ diff --git a/tests/unit/cli/test_cli_package_exports.py b/tests/unit/cli/test_cli_package_exports.py index c2c1260899..eed5fec16c 100644 --- a/tests/unit/cli/test_cli_package_exports.py +++ b/tests/unit/cli/test_cli_package_exports.py @@ -39,7 +39,7 @@ def test_public_reexports_accessible_from_benchbox(symbol: str, expected_type: s assert symbol in benchbox.__all__ -_EXPERIMENTAL_SUBSYSTEMS = {"nl2sql", "aiml_functions", "multiregion", "gpu", "concurrency"} +_EXPERIMENTAL_SUBSYSTEMS = {"nl2sql", "aiml_functions", "multiregion", "gpu", "load_testing"} def test_experimental_modules_not_in_benchmark_registry() -> None: From a32b35efe83547dc849ccb7002ebe82f8e551f12 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:06:47 +0000 Subject: [PATCH 05/12] fix: add Python 3.10 tomli fallback to build_joinorder_data.py (#1076 review) Unconditional import tomllib raises ModuleNotFoundError on Python 3.10 (stdlib tomllib is 3.11+), which the repo still advertises as supported (requires-python >=3.10). Falls back to the already-declared tomli backport, matching the pattern in benchbox/core/data_fetch/manifest.py and friends. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- _project/scripts/build_joinorder_data.py | 5 ++++- tests/unit/scripts/test_build_joinorder_data.py | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/_project/scripts/build_joinorder_data.py b/_project/scripts/build_joinorder_data.py index 6b511f42f6..100dc0fc49 100644 --- a/_project/scripts/build_joinorder_data.py +++ b/_project/scripts/build_joinorder_data.py @@ -37,7 +37,10 @@ from pathlib import Path from typing import Any -import tomllib +try: + import tomllib +except ModuleNotFoundError: # Python 3.10: stdlib tomllib is 3.11+ + import tomli as tomllib # type: ignore[no-redef] # Shared, single-source-of-truth logical-hash algorithm. The build script runs # under the repo-root uv environment, so it imports the same primitives the diff --git a/tests/unit/scripts/test_build_joinorder_data.py b/tests/unit/scripts/test_build_joinorder_data.py index ffa75be5b3..2fba6e7695 100644 --- a/tests/unit/scripts/test_build_joinorder_data.py +++ b/tests/unit/scripts/test_build_joinorder_data.py @@ -50,6 +50,19 @@ class FakeDatetimeModule: assert build_joinorder_data.utc_now_iso() == "2026-05-11T03:22:11Z" +def test_tomllib_import_has_python310_fallback() -> None: + """#1076 review: this script requires-python >=3.10 (repo-wide), but + stdlib tomllib is 3.11+. An unconditional `import tomllib` raises + ModuleNotFoundError on 3.10; it must fall back to the tomli backport + (already a declared dependency; python_version < '3.11'), matching the + pattern used by benchbox/core/data_fetch/manifest.py and friends.""" + script_path = REPO_ROOT / "_project" / "scripts" / "build_joinorder_data.py" + text = script_path.read_text(encoding="utf-8") + assert "import tomli as tomllib" in text, ( + "build_joinorder_data.py must fall back to tomli on Python 3.10 (stdlib tomllib is 3.11+)" + ) + + def test_container_cli_defaults_to_docker(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("BENCHBOX_CONTAINER_CLI", raising=False) assert build_joinorder_data.container_cli() == "docker" From 780a068116af9094300cc4dcf94a4206d071ebdd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:08:09 +0000 Subject: [PATCH 06/12] docs: correct runbook claim that flipping DEVELOP_REVIEW_RULE_ENFORCED needs no test changes (#1079 review) compare_ruleset's enforce_review_rule parameter defaults to the module constant (enforce_review_rule: bool = DEVELOP_REVIEW_RULE_ENFORCED), so flipping the constant also flips that default. test_missing_review_enforcement_is_a_non_blocking_warning_by_default calls compare_ruleset() without passing enforce_review_rule explicitly, relying on the current False default to exercise the non-blocking path -- after the flip it would exercise the blocking path instead and fail its own blocking_findings(findings) == [] assertion. Corrected the runbook to say the maintainer must update that test alongside the flip, rather than claiming no test changes are required. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- docs/operations/repo-admin-settings.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/operations/repo-admin-settings.md b/docs/operations/repo-admin-settings.md index 16a0e5dd6b..c2367b4789 100644 --- a/docs/operations/repo-admin-settings.md +++ b/docs/operations/repo-admin-settings.md @@ -199,10 +199,15 @@ Flipping the constant **before** step 1 turns the (still-live) missing-review ga `WARNING (non-blocking):` finding into a blocking one, which reddens **both** the daily `release-canary.yml` run **and** `validate-main-pr.yml`'s drift-check on every real develop PR until the PUT lands. Done in the correct order, the flip's own CI is green (the live -ruleset already satisfies the rule). No test changes are required: `compare_ruleset`'s -`enforce_review_rule` parameter is exercised in both `False`/`True` modes by -`tests/unit/release/test_ruleset_drift_review_coverage.py`, so the constant's value does -not itself make any unit test pass or fail. +ruleset already satisfies the rule). **One test must be updated alongside the flip:** +`compare_ruleset`'s `enforce_review_rule` parameter defaults to the module constant +(`enforce_review_rule: bool = DEVELOP_REVIEW_RULE_ENFORCED`), so flipping the constant also +flips that default. `tests/unit/release/test_ruleset_drift_review_coverage.py::test_missing_review_enforcement_is_a_non_blocking_warning_by_default` +calls `compare_ruleset(...)` without passing `enforce_review_rule` explicitly, relying on +that default to exercise the non-blocking/`False` path — after the flip it would exercise +the blocking/`True` path instead and fail its own `blocking_findings(findings) == []` +assertion. Pass `enforce_review_rule=False` explicitly in that one test call so it keeps +pinning the non-blocking-by-default behavior regardless of the live constant's value. History: From 1b7725ae9282f55ee02ce1de7c1dbeea8183c2ae Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:08:42 +0000 Subject: [PATCH 07/12] docs(todo): merge duplicate notes key on explorer-pipeline-frontend-handoff w2 (#1080 review) w2 had two notes: keys back to back. YAML loaders either silently drop one value or, in stricter loaders such as the ruamel-based structured TODO editor used by todo_cli.py done, reject duplicate mapping keys, so future updates to this item could fail or lose the original implementation note. Merged the completion text into the existing notes block. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- .../planning/explorer-pipeline-frontend-handoff.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/_project/TODO/results-labels-funding/planning/explorer-pipeline-frontend-handoff.yaml b/_project/TODO/results-labels-funding/planning/explorer-pipeline-frontend-handoff.yaml index 0fc0f9dc10..6c7032c8c6 100644 --- a/_project/TODO/results-labels-funding/planning/explorer-pipeline-frontend-handoff.yaml +++ b/_project/TODO/results-labels-funding/planning/explorer-pipeline-frontend-handoff.yaml @@ -61,7 +61,7 @@ work: (bare-column projection, G-11 rule), and add funding to the matching EXPECTED_VIEWS set in test_duckdb_browser_contract.py. No new base-table column, so no read_model_version bump. - notes: >- + DONE in PR #1080: r.funding added to result_detail_metrics in duckdb_builder.py + browser-duckdb-schema.sql; EXPECTED_VIEWS guards it. needs: [w1] From 1ca5862d90d862d42247879338bde27bf0b67727 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:11:56 +0000 Subject: [PATCH 08/12] test: assert exact TPC-DS query/variant multiset in throughput bleed test (#1074 review) test_no_cross_stream_result_bleed only checked len(query_ids) == 103 per stream. A stream that duplicates one query while omitting another, or collapses a variant pair like 14a/14b into the same id, keeps the same length but changes which ids are present -- the length check alone can't catch it. Compare against the exact expected query_id Counter (derived independently from query range 1-99 and MULTI_PART_QUERY_IDS, matching how throughput_test.py builds query_display_id). Verified against a real DuckDB run (SF 0.01) that the new assertion passes on genuine production output, and synthetically that a variant-collapse corruption keeping the same length is caught by the Counter comparison while the old length-only check would have missed it. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- .../test_throughput_real_duckdb.py | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_throughput_real_duckdb.py b/tests/integration/test_throughput_real_duckdb.py index 7fb03041ff..2b701accae 100644 --- a/tests/integration/test_throughput_real_duckdb.py +++ b/tests/integration/test_throughput_real_duckdb.py @@ -36,7 +36,7 @@ from __future__ import annotations -from collections import defaultdict +from collections import Counter, defaultdict from collections.abc import Generator from pathlib import Path from typing import Any @@ -44,6 +44,7 @@ import pytest from benchbox.core.tpcds.benchmark import TPCDSBenchmark +from benchbox.core.tpcds.streams import MULTI_PART_QUERY_IDS from benchbox.core.tpch.benchmark import TPCHBenchmark from benchbox.core.tpch.streams import TPCHStreams from benchbox.platforms.duckdb import DuckDBAdapter @@ -72,6 +73,20 @@ # from this test's own run output), so it can safely serve as an independent # expected-count oracle. _TPCDS_QUERIES_PER_STREAM = 103 +# The exact multiset of query_id strings every stream must produce, derived +# independently from the same seed-invariant properties as +# _TPCDS_QUERIES_PER_STREAM above (query range 1-99, MULTI_PART_QUERY_IDS +# expand into "a"/"b" instead of the plain "" - see +# benchbox/core/tpcds/throughput_test.py's query_display_id construction). +# A stream that duplicates one query while omitting another, or collapses a +# variant pair like "14a"/"14b" into the same id, keeps the same length but +# changes this multiset - which len(...) == _TPCDS_QUERIES_PER_STREAM alone +# cannot detect. +_TPCDS_EXPECTED_QUERY_IDS = Counter( + f"{query_id}{variant}" if variant else str(query_id) + for query_id in range(1, 100) + for variant in (["a", "b"] if query_id in MULTI_PART_QUERY_IDS else [None]) +) # Shared base seed for both throughput runs below. _BASE_SEED = 7 @@ -342,13 +357,20 @@ def test_no_cross_stream_result_bleed(self, tpcds_throughput_run: tuple[list[dic assert set(by_stream.keys()) == set(range(_TPCDS_NUM_STREAMS)) - # Each stream must have run the expected, seed-invariant TPC-DS query - # count - independent of what the flattened rows/stream_results - # themselves report. + # Each stream must have run the exact expected, seed-invariant TPC-DS + # query multiset - not just the right count. A stream that duplicates + # one query while omitting another, or collapses a variant pair like + # "14a"/"14b" into the same id, keeps len(query_ids) == 103 but + # changes which ids are present; Counter equality catches that. for stream_id, query_ids in by_stream.items(): assert len(query_ids) == _TPCDS_QUERIES_PER_STREAM, ( f"stream {stream_id} ran {len(query_ids)} queries, expected {_TPCDS_QUERIES_PER_STREAM}" ) + assert Counter(query_ids) == _TPCDS_EXPECTED_QUERY_IDS, ( + f"stream {stream_id} ran the wrong query/variant multiset: " + f"missing={_TPCDS_EXPECTED_QUERY_IDS - Counter(query_ids)}, " + f"unexpected={Counter(query_ids) - _TPCDS_EXPECTED_QUERY_IDS}" + ) # Structured stream_results must agree with the flattened adapter # rows on the expected per-stream count too. From 50d8957d2e19c19503b9dd736aee9f4002f846e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:16:38 +0000 Subject: [PATCH 09/12] fix(cli): reject run-official --streams 1 instead of silently flooring to 2 (#1077 review) _resolve_requested_stream_count() (benchbox/platforms/base/execution.py) floors any resolved stream count to the TPC throughput minimum of 2, because it has no wire-level way to distinguish an explicit user request of 1 from the schema's own default of 1 at that deep boundary -- that trade-off is intentional and documented there. But run-official --streams 1 is one of the two reachable paths (the other being the interactive configurator) where the request IS still known to be explicit at the point it's captured. Reject it there instead of letting it silently run a different stream count than what the user typed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- benchbox/cli/commands/run_official.py | 12 ++++++++++ .../test_throughput_stream_count.py | 23 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/benchbox/cli/commands/run_official.py b/benchbox/cli/commands/run_official.py index 37ad18a797..59db03a743 100644 --- a/benchbox/cli/commands/run_official.py +++ b/benchbox/cli/commands/run_official.py @@ -97,6 +97,18 @@ def run_official(ctx, benchmark, platform, scale, phases, streams, seed, output_ console.print(f"[red]Error: --streams must be a non-negative integer, got: {streams}[/red]") sys.exit(1) + # Reject an explicit --streams 1 outright rather than letting it silently + # become 2: _resolve_requested_stream_count() (benchbox/platforms/base/ + # execution.py) floors any resolved value to the TPC throughput minimum + # of 2 because there is no wire-level way to distinguish "explicitly + # requested 1" from the schema's own default of 1 at that boundary. A + # user who explicitly typed --streams 1 here would otherwise see their + # own value silently overridden with no error, so catch it at the one + # place we still know it was explicit. + if streams == 1: + console.print("[red]Error: --streams must be >= 2 (TPC throughput minimum); got: 1[/red]") + sys.exit(1) + console.print("[bold blue]TPC-Compliant Official Benchmark Run[/bold blue]") for label, value in [ ("Benchmark", f"TPC-{benchmark.upper()}"), diff --git a/tests/integration/test_throughput_stream_count.py b/tests/integration/test_throughput_stream_count.py index d5bddd95f4..56f6834fa7 100644 --- a/tests/integration/test_throughput_stream_count.py +++ b/tests/integration/test_throughput_stream_count.py @@ -290,4 +290,25 @@ def test_run_official_forward_requested_streams_noop_when_not_requested(): original = BenchmarkOrchestrator.execute_benchmark with _forward_requested_streams(None): assert BenchmarkOrchestrator.execute_benchmark is original - assert BenchmarkOrchestrator.execute_benchmark is original + + +def test_run_official_rejects_explicit_streams_one(): + """#1077 review: an explicit --streams 1 must error, not be silently + floored to 2. _resolve_requested_stream_count() floors any resolved + value to the TPC throughput minimum because it cannot distinguish an + explicit user request of 1 from the schema's own default of 1 at that + deep boundary -- so run-official (the one place the request is still + known to be explicit) must reject it up front instead of accepting it + and silently running a different stream count than requested.""" + from click.testing import CliRunner + + from benchbox.cli.commands.run_official import run_official + + runner = CliRunner() + result = runner.invoke( + run_official, + ["tpch", "--platform", "duckdb", "--scale", "1", "--phases", "throughput", "--streams", "1", "--seed", "42"], + ) + + assert result.exit_code != 0 + assert "streams must be >= 2" in result.output From fab0e4e5b7e321082b9aded9103c5b281855627e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:23:45 +0000 Subject: [PATCH 10/12] fix(ducklake): resolve nested platform options, force_recreate, and defer extension probe (#1082 review) Three findings from the late-landing bot review on merged PR #1082: 1. from_config() read metadata_path/data_path via flat config.get(...) only. The real `benchbox run --platform ducklake --platform-option metadata_path=... --platform-option data_path=...` path nests those values under config["options"] (DuckLake has no registered PlatformHookRegistry config builder to promote them to top-level flat keys - the same reason _resolve_option() already exists in this function for catalog/pg_*/s3_* options). User-specified catalog/data locations were silently ignored in favor of generated defaults. 2. force_recreate was read from a flat "force" key, but the real --force CLI flag threads through as force_recreate nested in config["options"] via the same default config builder. A real --force request was silently reset to False, so an existing catalog was reused instead of wiped for a clean rebuild. Now checks both the legacy flat "force" key (config_utils.py's build_config() path) and force_recreate (flat or nested). 3. tests/integration/test_ducklake_integration.py probed DuckDB extension availability (INSTALL ducklake / INSTALL sqlite) at module level, so `@pytest.mark.skipif` class decorators triggered a real, potentially networked INSTALL during test COLLECTION - which pytest always does regardless of which markers later deselect the tests from execution. Moved the probe into setup_method (only reached by a test pytest has already decided to run), so a fast/offline collection never pays for it. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- benchbox/platforms/ducklake.py | 12 +++-- .../integration/test_ducklake_integration.py | 38 +++++++------ tests/unit/platforms/test_ducklake_adapter.py | 54 +++++++++++++++++++ 3 files changed, 85 insertions(+), 19 deletions(-) diff --git a/benchbox/platforms/ducklake.py b/benchbox/platforms/ducklake.py index d60bfdb6ba..a0eaeb4ebb 100644 --- a/benchbox/platforms/ducklake.py +++ b/benchbox/platforms/ducklake.py @@ -246,8 +246,8 @@ def _resolve_option(key: str, default: Any = None) -> Any: adapter_config: dict[str, Any] = {} - metadata_path = config.get("ducklake_metadata_path") or config.get("metadata_path") - data_path = config.get("ducklake_data_path") or config.get("data_path") + metadata_path = config.get("ducklake_metadata_path") or _resolve_option("metadata_path") + data_path = config.get("ducklake_data_path") or _resolve_option("data_path") if not metadata_path or not data_path: if config.get("output_dir"): @@ -280,7 +280,13 @@ def _resolve_option(key: str, default: Any = None) -> Any: # Pass through DuckDB-family configuration (base DuckDB connection # settings for the "shell" connection the DuckLake catalog attaches to). adapter_config["memory_limit"] = config.get("memory_limit", "4GB") - adapter_config["force_recreate"] = config.get("force", False) + # "force" (flat) is the legacy config_utils.py build_config() shape; + # "force_recreate" (flat or nested in config["options"]) is what the + # real --force CLI flag actually threads through as via the modern + # PlatformHookRegistry default config builder (see _resolve_option's + # docstring above for why nested lookups are needed at all). Check + # both so a real --force request is never silently dropped to False. + adapter_config["force_recreate"] = bool(config.get("force") or _resolve_option("force_recreate", False)) for key in ["tuning_config", "verbose_enabled", "very_verbose"]: if key in config: adapter_config[key] = config[key] diff --git a/tests/integration/test_ducklake_integration.py b/tests/integration/test_ducklake_integration.py index 33c6f9f6a7..904f8edd02 100644 --- a/tests/integration/test_ducklake_integration.py +++ b/tests/integration/test_ducklake_integration.py @@ -61,15 +61,22 @@ # Extension-availability probes # ============================================================================= # Mirrors tests/integration/test_open_table_formats.py's TestDuckLakeFormatSmoke -# guard (and tests/unit/platforms/test_ducklake_adapter.py's DUCKLAKE_AVAILABLE -# probe): INSTALL/LOAD an extension so the tests below can skip cleanly - never +# guard: INSTALL/LOAD an extension so the tests below can skip cleanly - never # fail - when the environment cannot reach the DuckDB extension repository or # lacks a given extension. # -# Only the extensions the default lane actually needs (ducklake, sqlite) are -# probed at import time. The postgres/httpfs probes are deferred into their -# live_integration classes, which are deselected by default - probing them -# eagerly would spend a network INSTALL attempt on every collection. +# The probe must stay LAZY (called from setup_method, never at module level): +# pytest imports every collected module to discover markers/tests regardless +# of which markers are later deselected (module-level pytestmark = [slow] only +# excludes these tests from EXECUTION, not from collection/import). A +# module-level probe call, or a `@pytest.mark.skipif(not , ...)` +# class decorator whose condition captures that eager flag, would therefore +# still spend a real, potentially-networked INSTALL/LOAD attempt on every test +# collection - including a fast/offline lane that will never run any test in +# this file (#1082 review: this is exactly what test_ducklake_adapter.py's own +# hermetic unit module avoids by never touching a live DuckDB connection). +# setup_method only runs for a test pytest has already decided to execute, so +# deferring the probe there means an excluded/deselected run never pays for it. @cache @@ -94,25 +101,25 @@ def _skip_unless_extensions(*names: str) -> None: pytest.skip(f"DuckDB extension(s) not available: {', '.join(missing)}") -DUCKLAKE_AVAILABLE = _probe_extension("ducklake") -SQLITE_EXTENSION_AVAILABLE = _probe_extension("sqlite") - - # ============================================================================= # 1. Live connection - in-process DuckDB-file catalog ATTACH # ============================================================================= @pytest.mark.live_integration -@pytest.mark.skipif(not DUCKLAKE_AVAILABLE, reason="DuckLake extension not available (needs DuckDB>=1.3 + network)") class TestDuckLakeLiveConnection: """End-to-end tests exercising a real INSTALL/LOAD/ATTACH of the ducklake extension. Marked live_integration (deselected from the default/fast lane) because they perform a real, potentially-networked INSTALL ducklake; they also skip - entirely when the extension cannot be installed/loaded. + entirely when the extension cannot be installed/loaded (checked in + setup_method, not a collection-time skipif - see the probe docstring + above for why). """ + def setup_method(self) -> None: + _skip_unless_extensions("ducklake") + def test_cursor_defaults_to_lake_catalog(self, tmp_path): # Regression for the w8 validation bug: framework seams (e.g. # phase_tracking._validate_data_integrity) probe tables via @@ -291,13 +298,12 @@ def test_sqlite_catalog_end_to_end(self, tmp_path): # ============================================================================= -@pytest.mark.skipif( - not (DUCKLAKE_AVAILABLE and SQLITE_EXTENSION_AVAILABLE), - reason="ducklake/sqlite DuckDB extensions not available", -) class TestDuckLakeSqliteCatalogLive: """Real end-to-end TPC-H run through the DuckLake adapter with catalog=sqlite.""" + def setup_method(self) -> None: + _skip_unless_extensions("ducklake", "sqlite") + def test_tpch_sf001_sqlite_catalog_end_to_end(self, tmp_path: Path) -> None: """Run TPC-H SF 0.01 via `benchbox run` with --platform-option catalog=sqlite. diff --git a/tests/unit/platforms/test_ducklake_adapter.py b/tests/unit/platforms/test_ducklake_adapter.py index 737475c513..e59cd6e466 100644 --- a/tests/unit/platforms/test_ducklake_adapter.py +++ b/tests/unit/platforms/test_ducklake_adapter.py @@ -127,6 +127,26 @@ def test_argparse_style_key_takes_precedence_over_normalized(self, tmp_path): assert adapter.metadata_path == preferred + def test_honors_metadata_and_data_path_from_nested_options(self, tmp_path): + """#1082 review: benchbox run --platform ducklake --platform-option + metadata_path=... --platform-option data_path=... nests those values + under config["options"] (DuckLake has no registered PlatformHookRegistry + config builder to promote them to top-level flat keys), but the old + code only ever read the flat config.get("metadata_path")/ + ("data_path"), so a real CLI-supplied catalog/data location was + silently ignored and the generated-default paths were used instead.""" + metadata_path = tmp_path / "nested" / "catalog.ducklake" + data_path = tmp_path / "nested" / "data" + config = { + "benchmark": "tpch", + "scale_factor": 0.01, + "options": {"metadata_path": str(metadata_path), "data_path": str(data_path)}, + } + adapter = DuckLakeAdapter.from_config(config) + + assert adapter.metadata_path == metadata_path + assert adapter.data_path == data_path + class TestDuckLakeAdapterBasics: """Test basic adapter properties and construction.""" @@ -498,6 +518,40 @@ def test_resolves_s3_creds_from_nested_options(self, tmp_path): assert adapter.s3_secret == "shh" assert adapter.s3_region == "us-east-1" + def test_resolves_force_recreate_from_nested_options(self, tmp_path): + """#1082 review: the real --force CLI flag threads through as + force_recreate nested in config["options"] (via PlatformHookRegistry's + default config builder), not as a flat "force" key. The old code only + read config.get("force", False), so a real --force request was + silently reset to False and an existing catalog would be reused + instead of wiped for a clean rebuild.""" + config = { + "benchmark": "tpch", + "scale_factor": 0.01, + "output_dir": str(tmp_path), + "options": {"force_recreate": True}, + } + adapter = DuckLakeAdapter.from_config(config) + assert adapter.force_recreate is True + + def test_resolves_force_recreate_from_legacy_flat_force_key(self, tmp_path): + """Back-compat: the legacy config_utils.py build_config() path passes + a flat "force" key (not "force_recreate") - must keep working + alongside the nested force_recreate resolution above.""" + config = { + "benchmark": "tpch", + "scale_factor": 0.01, + "output_dir": str(tmp_path), + "force": True, + } + adapter = DuckLakeAdapter.from_config(config) + assert adapter.force_recreate is True + + def test_force_recreate_defaults_to_false(self, tmp_path): + config = {"benchmark": "tpch", "scale_factor": 0.01, "output_dir": str(tmp_path)} + adapter = DuckLakeAdapter.from_config(config) + assert adapter.force_recreate is False + class TestDuckLakeS3Routing: """Test S3/cloud DATA_PATH routing and secret-SQL construction (w3).""" From 80c00f164fed2aec3c2f7e7a3d6316e62356f533 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 18:40:51 +0000 Subject: [PATCH 11/12] fix(ci): pin release smoke test shell to bash for Windows matrix leg (#1116 review) test-installation's windows-latest leg used the default PowerShell shell for the two install steps, so `${BENCHBOX_VERSION}` (bash env syntax) resolved to empty and `pip install "benchbox=="` would have run instead of installing the pinned version. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- .github/workflows/release.yml | 2 ++ tests/unit/test_release_infrastructure.py | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 3ea05c6b3c..de63b7a6a3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -214,6 +214,7 @@ jobs: - name: Test installation from PyPI if: github.event.inputs.test_pypi != 'true' + shell: bash env: BENCHBOX_VERSION: ${{ needs.build.outputs.version }} run: | @@ -223,6 +224,7 @@ jobs: - name: Test installation from Test PyPI if: github.event.inputs.test_pypi == 'true' + shell: bash env: BENCHBOX_VERSION: ${{ needs.build.outputs.version }} run: | diff --git a/tests/unit/test_release_infrastructure.py b/tests/unit/test_release_infrastructure.py index cec0a3e96f..f22b7cbf64 100644 --- a/tests/unit/test_release_infrastructure.py +++ b/tests/unit/test_release_infrastructure.py @@ -263,6 +263,16 @@ def test_release_smoke_test_pins_installed_version(self): "the build/publish version pin, plus both PyPI and Test PyPI install steps, must reference it" ) + steps = test_installation_job["steps"] + install_steps = [s for s in steps if s.get("name", "").startswith("Test installation from")] + assert len(install_steps) == 2, "expected exactly the PyPI and Test PyPI install steps" + for step in install_steps: + assert step.get("shell") == "bash", ( + f"{step['name']!r} must pin shell: bash - the matrix includes windows-latest, whose " + "default runner shell is PowerShell and does not understand ${BENCHBOX_VERSION} bash " + "syntax, so the Windows leg would silently install an unversioned/empty package name" + ) + def test_release_required_result_contract(self): """Test the main-PR release-required umbrella check shape.""" jobs = _workflow("test.yml")["jobs"] From 7bed63db9aac44a713e0f9bac178415212fa0393 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 17:30:45 +0000 Subject: [PATCH 12/12] chore(ci): merge develop (drop moot #1079 runbook hunk); composed fast-lane bump 25190->25220 The #1079 finding's runbook correction is superseded by the 2026-07-18 require_code_owner_review retirement: the DEVELOP_REVIEW_RULE_ENFORCED constant, the flip-order paragraph it corrected, and the test it named were all removed on develop, so the hunk is dropped as moot. Fast-lane ceiling bumped once for the whole open-PR merge queue with byte-identical content across queued branches; this merge ref collects 25191. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018y41mPzRqiLqvZLZCxwS6j --- _project/config/fast_test_lane_policy.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/_project/config/fast_test_lane_policy.json b/_project/config/fast_test_lane_policy.json index e9d4c7ab71..de4d50f626 100644 --- a/_project/config/fast_test_lane_policy.json +++ b/_project/config/fast_test_lane_policy.json @@ -1,7 +1,7 @@ { "enabled": true, - "max_fast_tests": 25190, - "_ceiling_note": "Bumped 13200 -> 22000 on 2026-04-13 to reflect grown fast-lane (20,184 collected). Bumped 22000 -> 22100 on 2026-05-10 for joinorder-canonical-foundation Phase-1 (data_fetch + scale-factor + surface-field tests; 22038 collected). Bumped 22100 -> 22400 on 2026-05-11 for joinorder-canonical-cutover canonical SQL/DataFrame, MCP, publishing, and corpus-inventory guards; 22317 collected. Bumped 22400 -> 22500 on 2026-05-13 for UAT enabled-platform remediation coverage; pr-preflight collected 22419. Bumped 22500 -> 22510 on 2026-05-15 after rebasing onto develop at 36ab01058; pr-preflight collected 22501 before this branch's new public theme contract tests, which are medium-marked to avoid fast-lane growth. Bumped 22510 -> 22530 on 2026-05-14 for pr-review-followup batch coverage (joinorder dataframe family, pg_mooncake adapter, landing quickstart validation, explorer read-model retry); pr-preflight collected 22520. Bumped 22530 -> 22540 on 2026-05-15 for results-explorer-post-theme-reconcile count-aware Home/script coverage; CI collected 22537. Bumped 22540 -> 22543 on 2026-05-16 for pr-review-followups coverage on UAT CLI dispatch, UAT release-gate envelope scoping, and develop-post-merge orphan ordering; ci-lint collected 22543. Bumped 22543 -> 22550 on 2026-05-16 for prompts landing Phase 1 CLI hygiene validator/prompt-shape coverage; pr-preflight collected 22550. Bumped 22550 -> 22553 on 2026-05-16 for prompts landing Phase 2 cost-class and registry prompt-safety coverage; timing policy collected 22553. Bumped 22553 -> 22558 on 2026-05-16 for prompts landing Phase 3 platform-option and TPC-DS dsdgen validator coverage; timing_policy_check collected 22558. Bumped 22558 -> 22561 on 2026-05-16 for prompts landing Phase 4 provenance template and capture-plan footer coverage; timing_policy_check collected 22561. Bumped 22561 -> 22563 on 2026-05-16 for prompts landing review fixes covering MCP cost-gating parity and compare summary discipline; targeted lane count increased by two fast tests. Bumped 22563 -> 22568 on 2026-05-16 for prompts landing Phase 5 MCP parity prompt-surface coverage; timing_policy_check collected 22568. Bumped 22568 -> 22570 on 2026-05-16 for prompts landing MCP real-tool parity and platform-option gap follow-up coverage; timing_policy_check collected 22570. Bumped 22570 -> 22572 on 2026-05-19 for pr-review-followups prompt regressions covering credential-before-smoke ordering and smoke-scale dsdgen warning behavior; timing_policy_check collected 22572. Set 22572 -> 25000 on 2026-05-19 by maintainer direction during pr-review-followups cleanup. Ratchet down if the lane contracts; never raise without justification. Bumped 25000 -> 25050 on 2026-07-16 for tuning-mode-vocabulary-and-facet-implementation-20260712 (ADR-2 canonical_mode/tuned-fallback/official-refusal coverage, the PACKAGED_RESOURCE composition pin added when merging develop's #1188 packaged-template tier, and the physical_mechanisms unknown-vs-empty ingest-pipeline regression tests); develop alone collected 24995, this branch's merge collected 25022. Bumped 25050 -> 25060 on 2026-07-17 for tuning-capability-registry-coverage-20260716 (alias-resolution invariant tests and derived generator-coverage drift guards for the 9 newly-registered platforms); CI collected 25052 on the merge ref. Bumped 25060 -> 25080 on 2026-07-17 composing #1198's registry-coverage guards with #1176's provenance/hash export coverage (test_tuning_provenance_export.py + test_requested_config_hash.py); both branches independently bumped from 25050 (#1198: 25052 collected; #1176 pre-compose: 25062 collected); composed merge tree collects 25077. Bumped 25080 -> 25140 on 2026-07-17 for tuning-from-config-forwarding-sweep-20260716 (test_tuning_config_forwarding.py's registry-driven parametrized test, one case per registered platform adapter, verifying from_config forwards tuning_enabled/tuning_config/unified_tuning_configuration/tuning_source/tuning_source_file); timing_policy_check collected 25128. Bumped 25140 -> 25180 on 2026-07-18 for the todo-db-tracker local-SQLite spike (tests/unit/scripts/test_todo_db.py: 49 fast unit tests covering the enforced lifecycle invariants, archive import, and concurrency guards); develop collected 25128, this branch collects 25177. Bumped 25180 -> 25190 on 2026-07-18 composing the todo-db spike branch with develop's post-25128 growth: the branch alone collects 25177 (49 fast tracker tests; the 11 wrapper tests are medium-marked and collect 0 under -m fast), but the PR merge ref collects 25187.", + "max_fast_tests": 25220, + "_ceiling_note": "Bumped 13200 -> 22000 on 2026-04-13 to reflect grown fast-lane (20,184 collected). Bumped 22000 -> 22100 on 2026-05-10 for joinorder-canonical-foundation Phase-1 (data_fetch + scale-factor + surface-field tests; 22038 collected). Bumped 22100 -> 22400 on 2026-05-11 for joinorder-canonical-cutover canonical SQL/DataFrame, MCP, publishing, and corpus-inventory guards; 22317 collected. Bumped 22400 -> 22500 on 2026-05-13 for UAT enabled-platform remediation coverage; pr-preflight collected 22419. Bumped 22500 -> 22510 on 2026-05-15 after rebasing onto develop at 36ab01058; pr-preflight collected 22501 before this branch's new public theme contract tests, which are medium-marked to avoid fast-lane growth. Bumped 22510 -> 22530 on 2026-05-14 for pr-review-followup batch coverage (joinorder dataframe family, pg_mooncake adapter, landing quickstart validation, explorer read-model retry); pr-preflight collected 22520. Bumped 22530 -> 22540 on 2026-05-15 for results-explorer-post-theme-reconcile count-aware Home/script coverage; CI collected 22537. Bumped 22540 -> 22543 on 2026-05-16 for pr-review-followups coverage on UAT CLI dispatch, UAT release-gate envelope scoping, and develop-post-merge orphan ordering; ci-lint collected 22543. Bumped 22543 -> 22550 on 2026-05-16 for prompts landing Phase 1 CLI hygiene validator/prompt-shape coverage; pr-preflight collected 22550. Bumped 22550 -> 22553 on 2026-05-16 for prompts landing Phase 2 cost-class and registry prompt-safety coverage; timing policy collected 22553. Bumped 22553 -> 22558 on 2026-05-16 for prompts landing Phase 3 platform-option and TPC-DS dsdgen validator coverage; timing_policy_check collected 22558. Bumped 22558 -> 22561 on 2026-05-16 for prompts landing Phase 4 provenance template and capture-plan footer coverage; timing_policy_check collected 22561. Bumped 22561 -> 22563 on 2026-05-16 for prompts landing review fixes covering MCP cost-gating parity and compare summary discipline; targeted lane count increased by two fast tests. Bumped 22563 -> 22568 on 2026-05-16 for prompts landing Phase 5 MCP parity prompt-surface coverage; timing_policy_check collected 22568. Bumped 22568 -> 22570 on 2026-05-16 for prompts landing MCP real-tool parity and platform-option gap follow-up coverage; timing_policy_check collected 22570. Bumped 22570 -> 22572 on 2026-05-19 for pr-review-followups prompt regressions covering credential-before-smoke ordering and smoke-scale dsdgen warning behavior; timing_policy_check collected 22572. Set 22572 -> 25000 on 2026-05-19 by maintainer direction during pr-review-followups cleanup. Ratchet down if the lane contracts; never raise without justification. Bumped 25000 -> 25050 on 2026-07-16 for tuning-mode-vocabulary-and-facet-implementation-20260712 (ADR-2 canonical_mode/tuned-fallback/official-refusal coverage, the PACKAGED_RESOURCE composition pin added when merging develop's #1188 packaged-template tier, and the physical_mechanisms unknown-vs-empty ingest-pipeline regression tests); develop alone collected 24995, this branch's merge collected 25022. Bumped 25050 -> 25060 on 2026-07-17 for tuning-capability-registry-coverage-20260716 (alias-resolution invariant tests and derived generator-coverage drift guards for the 9 newly-registered platforms); CI collected 25052 on the merge ref. Bumped 25060 -> 25080 on 2026-07-17 composing #1198's registry-coverage guards with #1176's provenance/hash export coverage (test_tuning_provenance_export.py + test_requested_config_hash.py); both branches independently bumped from 25050 (#1198: 25052 collected; #1176 pre-compose: 25062 collected); composed merge tree collects 25077. Bumped 25080 -> 25140 on 2026-07-17 for tuning-from-config-forwarding-sweep-20260716 (test_tuning_config_forwarding.py's registry-driven parametrized test, one case per registered platform adapter, verifying from_config forwards tuning_enabled/tuning_config/unified_tuning_configuration/tuning_source/tuning_source_file); timing_policy_check collected 25128. Bumped 25140 -> 25180 on 2026-07-18 for the todo-db-tracker local-SQLite spike (tests/unit/scripts/test_todo_db.py: 49 fast unit tests covering the enforced lifecycle invariants, archive import, and concurrency guards); develop collected 25128, this branch collects 25177. Bumped 25180 -> 25190 on 2026-07-18 composing the todo-db spike branch with develop's post-25128 growth: the branch alone collects 25177 (49 fast tracker tests; the 11 wrapper tests are medium-marked and collect 0 under -m fast), but the PR merge ref collects 25187. Bumped 25190 -> 25220 on 2026-07-18 for the post-CODEOWNERS-retirement merge queue, composed once so the queued branches carry byte-identical policy content instead of five conflicting bumps (#1142 seed-validation exclusion coverage +10; #1116 bot-finding sweep regressions +4; #1202 capability-registry/report regressions ~+8; #1206 object-schema loader regressions ~+5; #1114 branch-rename test moves +0): develop alone collects 25187; the queue's final merge ref is expected to collect ~25214.", "forbidden_marker_expressions": [ "resource_heavy", "stress",