From 966aa9496a4998f57f463a7abfa3745ad8b15d2d Mon Sep 17 00:00:00 2001 From: Tsai Yuan <136825049+nothingyuancando@users.noreply.github.com> Date: Wed, 28 Oct 2026 20:47:59 +0800 Subject: [PATCH] fix(benchmark): validate sandbox timing options (#7651) * fix(benchmark): validate sandbox timing options * fix(benchmark): guard infinite idle reaping --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 9 ++ CHANGELOG_zh.md | 1 + backend/AGENTS.md | 6 +- backend/scripts/benchmark/sandbox/README.md | 21 ++-- .../benchmark/sandbox/bench_provider.py | 12 ++- backend/tests/AGENTS.md | 6 ++ backend/tests/test_bench_opensandbox.py | 99 ++++++++++++++++++- 7 files changed, 138 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc063f206c..52d30d4de0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1402,6 +1402,14 @@ As of 2026-10-21, the milestone includes **829 merged pull requests**. ### Fixed +- **benchmarks:** Reject negative and NaN sandbox idle-timeout values before + constructing any provider, and apply the same validation to BoxLite/AIO + health-check skip windows. Valid zero, positive, and positive-infinity values + retain their existing behavior, OpenSandbox keeps rejecting nonzero skip + windows, and AIO benchmark output now records accepted values without implying + that its provider consumes them. The idle-reaping scenario rejects infinity + before provisioning because it must sleep past the configured timeout. ([#7651]) + - **tui:** The `/threads` and `/model` pickers show thread titles and model names exactly as written. Labels were parsed as Textual markup, so a title such as `Fix [/bold] in parser` exited the TUI with `MarkupError` whenever the @@ -13237,3 +13245,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#7597]: https://github.com/bytedance/deer-flow/pull/7597 [#7606]: https://github.com/bytedance/deer-flow/pull/7606 [#7631]: https://github.com/bytedance/deer-flow/pull/7631 +[#7651]: https://github.com/bytedance/deer-flow/pull/7651 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 54e829b302..26a57c66cd 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -10555,3 +10555,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#7597]: https://github.com/bytedance/deer-flow/pull/7597 [#7606]: https://github.com/bytedance/deer-flow/pull/7606 [#7631]: https://github.com/bytedance/deer-flow/pull/7631 +[#7651]: https://github.com/bytedance/deer-flow/pull/7651 diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 96d7a413cc..aad9490f2b 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -46,9 +46,9 @@ vars; `summarize` and pytest are offline. See its README for the protocol. Benchmarks in `scripts/benchmark/` must be standalone and reproducible. Import production functions; never duplicate them or introduce an alternative runtime. -OpenSandbox benchmarks reuse the production lifecycle with scoped config overrides. -Export only non-secret dimensions; final cleanup failures must fail both the runner -and JSONL summary. See the sandbox benchmark README for supported options and tests. +Sandbox benchmarks reuse production lifecycle. Reject negative/NaN timing values +before provider creation, export no secrets, and fail on final cleanup errors. +See the sandbox benchmark README for supported options and tests. - Pin every external dataset by immutable revision and SHA-256. Callers provide the local dataset path; evaluation commands must not silently download data. diff --git a/backend/scripts/benchmark/sandbox/README.md b/backend/scripts/benchmark/sandbox/README.md index 456775d7db..898b9ec944 100644 --- a/backend/scripts/benchmark/sandbox/README.md +++ b/backend/scripts/benchmark/sandbox/README.md @@ -75,13 +75,20 @@ Python and POSIX utilities required by the selected workload and output sync. The normal warm-up creates and destroys a separate remote before measurement. CLI `--replicas` and `--idle-timeout` override their YAML values, including their -CLI defaults (3 and 600). All providers reject negative `--idle-timeout` values -before provider creation. Zero disables idle reaping, except that the -`idle_timeout` scenario requires a positive value. `--image` overrides YAML only -when supplied. Connection settings, including HTTPS and `use_server_proxy`, are retained. The exported -configuration dimensions contain only image, replicas and idle timeout; credentials, -endpoint and environment are not copied into result rows. Record non-secret SDK, -server and deployment versions separately for reproducibility. +CLI defaults (3 and 600). All providers reject negative and NaN +`--idle-timeout` values before provider creation. BoxLite and AIO likewise reject +negative and NaN `--health-check-skip-seconds`; zero, positive finite values, +and positive infinity remain accepted. OpenSandbox still rejects every nonzero +health-check skip value as unsupported. Zero idle timeout disables idle reaping, +except that the `idle_timeout` scenario requires a finite positive value because +it sleeps past the timeout. `--image` overrides YAML only when supplied. +Connection settings, including HTTPS and `use_server_proxy`, are retained. The +exported configuration dimensions contain only image, replicas, idle timeout, +and the health-check skip window; credentials, endpoint and environment are not +copied into result rows. BoxLite consumes the skip window during warm reclaim; +AIO accepts and records it for comparison but does not currently alter provider +behavior. Record non-secret SDK, server and deployment versions separately for +reproducibility. `release_ms` includes production best-effort output synchronization. Successful command/release timing does not prove that every artifact was mirrored. Warm hits diff --git a/backend/scripts/benchmark/sandbox/bench_provider.py b/backend/scripts/benchmark/sandbox/bench_provider.py index 8e1a55b491..2d71fff61b 100755 --- a/backend/scripts/benchmark/sandbox/bench_provider.py +++ b/backend/scripts/benchmark/sandbox/bench_provider.py @@ -50,6 +50,7 @@ import argparse import importlib import importlib.metadata import json +import math import os import re import statistics @@ -241,6 +242,7 @@ def _make_aio_provider(config: dict[str, Any]) -> tuple[Any, dict[str, Any]]: "container_prefix": config.get("container_prefix"), "replicas": config.get("replicas", 3), "idle_timeout": config.get("idle_timeout", 600), + "health_check_skip_seconds": config.get("health_check_skip_seconds", 0.0), "mounts": config.get("mounts", []), "environment": config.get("environment", {}), "provisioner_url": config.get("provisioner_url", ""), @@ -751,18 +753,20 @@ def _parse_args(argv: list[str] | None = None) -> argparse.Namespace: def main(argv: list[str] | None = None) -> int: args = _parse_args(argv) + if math.isnan(args.idle_timeout) or args.idle_timeout < 0: + raise SystemExit("idle timeout must be nonnegative and not NaN") + if math.isnan(args.health_check_skip_seconds) or args.health_check_skip_seconds < 0: + raise SystemExit("health-check skip seconds must be nonnegative and not NaN") if args.provider == "opensandbox" and (args.fault_inject is not None or args.health_check_skip_seconds != 0): raise SystemExit("opensandbox benchmarks do not support --fault-inject or nonzero --health-check-skip-seconds") if min(args.concurrency, args.users, args.replicas) < 1 or min(args.iterations, args.warmup_iterations) < 0: raise SystemExit("concurrency, users and replicas must be positive; iteration counts must be nonnegative") - if args.idle_timeout < 0: - raise SystemExit("idle timeout must be nonnegative") if args.fault_inject is not None and (args.concurrency != 1 or args.no_warmpool or args.scenario != "warm_same_thread"): raise SystemExit("fault injection requires warm_same_thread, concurrency=1 and an enabled warm pool") if args.fault_inject is not None and not 0 <= args.fault_inject < args.iterations: raise SystemExit("fault injection iteration must be inside the measured batch") - if args.scenario == "idle_timeout" and (args.concurrency != 1 or args.idle_timeout <= 0): - raise SystemExit("idle_timeout requires concurrency=1 and a positive idle timeout") + if args.scenario == "idle_timeout" and (args.concurrency != 1 or not math.isfinite(args.idle_timeout) or args.idle_timeout <= 0): + raise SystemExit("idle_timeout requires concurrency=1 and a finite positive idle timeout") if args.workload == "state_reuse" and (args.scenario != "warm_same_thread" or args.concurrency != 1): raise SystemExit("state_reuse requires --scenario warm_same_thread --concurrency 1") args.run_id = args.run_id or uuid.uuid4().hex[:12] diff --git a/backend/tests/AGENTS.md b/backend/tests/AGENTS.md index 393b5a1cec..c5095e9731 100644 --- a/backend/tests/AGENTS.md +++ b/backend/tests/AGENTS.md @@ -31,6 +31,12 @@ event waiters during teardown. benchmark module's clock binding. Keep asyncio's clock real, initialize `Trace.started` from the controlled clock, and advance ticks at event gates. +`test_bench_opensandbox.py` owns provider-independent sandbox benchmark CLI +validation. Invalid timing values must be rejected through real `main()` before +fail-loud factories run; accepted controls must preserve factory inputs and JSONL +dimensions. Keep live providers stubbed and OpenSandbox's unsupported-option +checks distinct from shared timing validation. + `test_doctor_script.py` runs a copied doctor from a temporary checkout with an empty-model config. Drop inherited `DEER_FLOW_*`, `PYTHONPATH` and `PYTHONHOME` from child processes; cover help, invalid flags, normal reporting and an diff --git a/backend/tests/test_bench_opensandbox.py b/backend/tests/test_bench_opensandbox.py index 2f882ea616..94603d75d9 100644 --- a/backend/tests/test_bench_opensandbox.py +++ b/backend/tests/test_bench_opensandbox.py @@ -291,7 +291,8 @@ def test_unsupported_options_fail_before_creating_resources(transport, extra): @pytest.mark.parametrize("provider", ["opensandbox", "boxlite", "aio-docker"]) -def test_negative_idle_timeout_fails_before_provider_creation(monkeypatch, provider): +@pytest.mark.parametrize("value", ["-5", "nan", "NaN", "-inf"]) +def test_invalid_idle_timeout_fails_before_provider_creation(monkeypatch, provider, value): assert provider in bench.PROVIDER_FACTORIES def unexpected_provider(*args, **kwargs): @@ -299,4 +300,98 @@ def test_negative_idle_timeout_fails_before_provider_creation(monkeypatch, provi monkeypatch.setitem(bench.PROVIDER_FACTORIES, provider, unexpected_provider) with pytest.raises(SystemExit, match="idle timeout must be nonnegative"): - bench.main(["--provider", provider, "--scenario", "warm_same_thread", "--idle-timeout", "-5"]) + bench.main(["--provider", provider, "--scenario", "warm_same_thread", f"--idle-timeout={value}"]) + + +@pytest.mark.parametrize("provider", ["boxlite", "aio-docker"]) +@pytest.mark.parametrize("value", ["-5", "nan", "NaN", "-inf"]) +def test_invalid_health_check_skip_fails_before_provider_creation(monkeypatch, provider, value): + assert provider in bench.PROVIDER_FACTORIES + + def unexpected_provider(*args, **kwargs): + pytest.fail("invalid health-check skip must be rejected before provisioning") + + monkeypatch.setitem(bench.PROVIDER_FACTORIES, provider, unexpected_provider) + with pytest.raises(SystemExit, match="health-check skip seconds must be nonnegative"): + bench.main(["--provider", provider, f"--health-check-skip-seconds={value}"]) + + +@pytest.mark.parametrize("provider", ["opensandbox", "boxlite", "aio-docker"]) +def test_idle_timeout_scenario_rejects_infinite_timeout_before_provider_creation(monkeypatch, provider): + def unexpected_provider(*args, **kwargs): + pytest.fail("an infinite idle_timeout scenario must be rejected before provisioning") + + monkeypatch.setitem(bench.PROVIDER_FACTORIES, provider, unexpected_provider) + + with pytest.raises(SystemExit, match="finite positive idle timeout"): + bench.main(["--provider", provider, "--scenario", "idle_timeout", "--idle-timeout=inf"]) + + +@pytest.mark.parametrize("provider", ["boxlite", "aio-docker"]) +@pytest.mark.parametrize("option", ["--idle-timeout", "--health-check-skip-seconds"]) +@pytest.mark.parametrize("value", [None, "0", "2.5", "inf"]) +def test_nonnegative_timing_values_reach_provider_factory(monkeypatch, tmp_path, provider, option, value): + captured = [] + + class FakeSandbox: + def execute_command(self, command, timeout=None): + return "ok" + + class FakeProvider: + def __init__(self): + self._lock = threading.Lock() + self._warm_pool = {} + + def acquire(self, thread_id, *, user_id=None): + return "sandbox-id" + + def get(self, sandbox_id): + return FakeSandbox() + + def release(self, sandbox_id): + return None + + def shutdown(self): + return None + + def factory(config): + captured.append(config) + return FakeProvider(), { + "image": config["image"], + "replicas": config["replicas"], + "idle_timeout": config["idle_timeout"], + "health_check_skip_seconds": config["health_check_skip_seconds"], + } + + monkeypatch.setitem(bench.PROVIDER_FACTORIES, provider, factory) + output = tmp_path / f"{provider}-{option[2:]}-{value or 'default'}.jsonl" + args = ["--provider", provider, "--iterations", "1", "--warmup-iterations", "0", "--output", str(output)] + if value is not None: + args.append(f"{option}={value}") + + assert bench.main(args) == 0 + key = option[2:].replace("-", "_") + expected = float(value) if value is not None else {"idle_timeout": 600.0, "health_check_skip_seconds": 0.0}[key] + assert captured[0][key] == expected + assert json.loads(output.read_text(encoding="utf-8"))[key] == expected + + +@pytest.mark.parametrize("value", [0.0, 7.5, float("inf")]) +def test_aio_factory_forwards_health_check_skip_to_config_and_snapshot(monkeypatch, value): + from deerflow.community.aio_sandbox import aio_sandbox_provider as aio_module + + captured = {} + + class FakeAioProvider: + def __init__(self): + captured.update(vars(aio_module.get_app_config().sandbox)) + self._config = {"image": "resolved-aio-image"} + + monkeypatch.setattr(aio_module, "AioSandboxProvider", FakeAioProvider) + + provider, snapshot = bench._make_aio_provider({"health_check_skip_seconds": value}) + + assert isinstance(provider, FakeAioProvider) + assert captured["health_check_skip_seconds"] == value + assert snapshot["health_check_skip_seconds"] == value + assert snapshot["image"] == "resolved-aio-image"