From 964a89600130b0bcb46bcfec6860e6762c4e332f Mon Sep 17 00:00:00 2001 From: "liyi.ly" Date: Sun, 19 Jul 2026 11:41:22 +0800 Subject: [PATCH] fix(reliability): bound three unbounded waits that could hang the CLI Three status/connection waits could block the CLI indefinitely when a backend stalls or silently dies: - cr.py `_create_instance`: `while True` status poll had no deadline; a CR instance stuck in "Creating" hung the CLI forever. Add a monotonic-clock deadline that raises TimeoutError. - ve_agentkit.py `_wait_for_runtime_status_multiple`: `timeout=None` made `if timeout and elapsed > timeout` dead code, so the poll never timed out. Fall back to a bounded poll timeout when None. - cli_exec.py sandbox terminal: `websocket run_forever()` had no ping_interval/ping_timeout, so a dead peer left the session wedged with only Ctrl-C as an exit. Enable keepalive pings. All three bounds are env-tunable via a single source of truth in http_defaults.py (AGENTKIT_POLL_TIMEOUT / AGENTKIT_WS_PING_INTERVAL / AGENTKIT_WS_PING_TIMEOUT), matching the existing http_timeout idiom. Adds offline regression tests covering each bound. --- agentkit/toolkit/cli/sandbox/cli_exec.py | 9 +- agentkit/toolkit/runners/ve_agentkit.py | 16 +- agentkit/toolkit/volcengine/cr.py | 7 + agentkit/utils/http_defaults.py | 43 ++++ .../toolkit/test_reliability_bounded_waits.py | 184 ++++++++++++++++++ 5 files changed, 254 insertions(+), 5 deletions(-) create mode 100644 tests/toolkit/test_reliability_bounded_waits.py diff --git a/agentkit/toolkit/cli/sandbox/cli_exec.py b/agentkit/toolkit/cli/sandbox/cli_exec.py index 19652b41..3eb21b39 100644 --- a/agentkit/toolkit/cli/sandbox/cli_exec.py +++ b/agentkit/toolkit/cli/sandbox/cli_exec.py @@ -57,6 +57,7 @@ normalize_model_base_url, validate_model_provider_base_url, ) +from agentkit.utils.http_defaults import ws_ping_interval, ws_ping_timeout from agentkit.toolkit.cli.sandbox.tool_resolve import ( SandboxToolType, find_tool_model_provider, @@ -393,7 +394,13 @@ def on_resize(_signum, _frame) -> None: err=True, ) with _raw_terminal_mode(): - websocket_app.run_forever() + # Keepalive: without ping_interval/ping_timeout a silently dead + # server leaves run_forever() blocked indefinitely with no way out + # but Ctrl-C. Pings detect the dead peer and unblock the session. + websocket_app.run_forever( + ping_interval=ws_ping_interval(), + ping_timeout=ws_ping_timeout(), + ) except KeyboardInterrupt: websocket_app.close() finally: diff --git a/agentkit/toolkit/runners/ve_agentkit.py b/agentkit/toolkit/runners/ve_agentkit.py index 7fdb3537..b54723dc 100644 --- a/agentkit/toolkit/runners/ve_agentkit.py +++ b/agentkit/toolkit/runners/ve_agentkit.py @@ -33,6 +33,7 @@ from agentkit.toolkit.models import DeployResult, InvokeResult, StatusResult from agentkit.toolkit.reporter import Reporter from agentkit.toolkit.errors import ErrorCode +from agentkit.utils.http_defaults import poll_timeout from agentkit.utils.misc import ( generate_runtime_name, generate_runtime_role_name, @@ -983,7 +984,10 @@ def _wait_for_runtime_status_multiple( """ last_status = None start_time = time.time() - total_time = timeout if timeout else 300 # For progress bar display + # None means "no caller-supplied timeout" — fall back to the bounded + # poll timeout so a stuck Runtime can never wedge the CLI forever. + effective_timeout = timeout if timeout else poll_timeout() + total_time = effective_timeout # For progress bar display expected_time = ( 30 # Controls progress curve speed (smaller = faster initial progress) ) @@ -1014,10 +1018,14 @@ def _wait_for_runtime_status_multiple( # Calculate elapsed time elapsed_time = time.time() - start_time - # Check timeout - if timeout and elapsed_time > timeout: + # Check timeout (always bounded — effective_timeout is never None) + if elapsed_time > effective_timeout: task.update(description="Wait timeout") - return False, runtime, f"{error_message} (timeout after {timeout}s)" + return ( + False, + runtime, + f"{error_message} (timeout after {effective_timeout:.0f}s)", + ) # Update progress description on status change if runtime.status != last_status: diff --git a/agentkit/toolkit/volcengine/cr.py b/agentkit/toolkit/volcengine/cr.py index 70ebbb71..3297d385 100644 --- a/agentkit/toolkit/volcengine/cr.py +++ b/agentkit/toolkit/volcengine/cr.py @@ -15,6 +15,7 @@ import time from agentkit.platform import resolve_endpoint, VolcConfiguration +from agentkit.utils.http_defaults import poll_timeout from agentkit.utils.logging_config import get_logger from agentkit.utils.ve_sign import ve_request @@ -116,12 +117,18 @@ def _create_instance( f"Error create cr instance {instance_name}: {error_code} {error_message}" ) + deadline = time.monotonic() + poll_timeout() while True: status = self._check_instance(instance_name) if status == "Running": break elif status == "Failed": raise ValueError(f"cr instance {instance_name} create failed") + elif time.monotonic() >= deadline: + raise TimeoutError( + f"cr instance {instance_name} did not reach Running within " + f"{poll_timeout():.0f}s (last status: {status})" + ) else: logger.debug(f"cr instance status: {status}") time.sleep(30) diff --git a/agentkit/utils/http_defaults.py b/agentkit/utils/http_defaults.py index 37171f7e..5aa49935 100644 --- a/agentkit/utils/http_defaults.py +++ b/agentkit/utils/http_defaults.py @@ -24,10 +24,21 @@ ENV_HTTP_TIMEOUT = "AGENTKIT_HTTP_TIMEOUT" ENV_HTTP_RETRIES = "AGENTKIT_HTTP_RETRIES" ENV_STREAM_TIMEOUT = "AGENTKIT_STREAM_TIMEOUT" +ENV_POLL_TIMEOUT = "AGENTKIT_POLL_TIMEOUT" +ENV_WS_PING_INTERVAL = "AGENTKIT_WS_PING_INTERVAL" +ENV_WS_PING_TIMEOUT = "AGENTKIT_WS_PING_TIMEOUT" DEFAULT_HTTP_TIMEOUT = 30.0 DEFAULT_HTTP_RETRIES = 2 DEFAULT_STREAM_TIMEOUT = 300.0 +# Upper bound for status-polling loops (resource create/status waits) so a +# stuck backend can never hang the CLI forever. 30 min covers slow provisioning. +DEFAULT_POLL_TIMEOUT = 1800.0 +# WebSocket keepalive: send a ping every interval and drop the connection if no +# pong arrives within the timeout, so a silently dead server can't wedge the +# interactive session indefinitely. +DEFAULT_WS_PING_INTERVAL = 20.0 +DEFAULT_WS_PING_TIMEOUT = 10.0 def http_timeout() -> float: @@ -52,3 +63,35 @@ def stream_timeout() -> float: return max(1.0, float(os.getenv(ENV_STREAM_TIMEOUT, "300"))) except ValueError: return DEFAULT_STREAM_TIMEOUT + + +def poll_timeout() -> float: + """Return the upper bound in seconds for status-polling loops (>= 1.0).""" + try: + return max(1.0, float(os.getenv(ENV_POLL_TIMEOUT, "1800"))) + except ValueError: + return DEFAULT_POLL_TIMEOUT + + +def ws_ping_interval() -> float: + """Return the WebSocket keepalive ping interval in seconds (>= 1.0).""" + try: + return max(1.0, float(os.getenv(ENV_WS_PING_INTERVAL, "20"))) + except ValueError: + return DEFAULT_WS_PING_INTERVAL + + +def ws_ping_timeout() -> float: + """Return the WebSocket pong-wait timeout in seconds (>= 1.0). + + Clamped to strictly less than the ping interval, as required by + ``websocket-client``'s ``run_forever``. + """ + try: + value = max(1.0, float(os.getenv(ENV_WS_PING_TIMEOUT, "10"))) + except ValueError: + value = DEFAULT_WS_PING_TIMEOUT + interval = ws_ping_interval() + if value >= interval: + value = max(1.0, interval - 1.0) + return value diff --git a/tests/toolkit/test_reliability_bounded_waits.py b/tests/toolkit/test_reliability_bounded_waits.py new file mode 100644 index 00000000..5bc3b225 --- /dev/null +++ b/tests/toolkit/test_reliability_bounded_waits.py @@ -0,0 +1,184 @@ +# Copyright (c) 2026 Beijing Volcano Engine Technology Co., Ltd. and/or its affiliates. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Regression tests for the three unbounded-wait reliability fixes. + +Each of these sites could previously hang the CLI forever: + +- ``VeCR.create_instance``: ``while True`` status poll with no deadline. +- ``VeAgentkitRuntimeRunner._wait_for_runtime_status_multiple``: ``timeout=None`` + made the deadline check dead code, so the poll could run forever. +- ``agentkit.utils.http_defaults``: new env-tunable bounds shared by the above + and by the interactive websocket keepalive. + +The tests stub ``time`` and the network layer so they run offline and fast. +""" + +from __future__ import annotations + +import pytest + +from agentkit.utils import http_defaults + + +# --------------------------------------------------------------------------- # +# http_defaults: new bounded-wait knobs +# --------------------------------------------------------------------------- # +class TestHttpDefaults: + def test_poll_timeout_default(self, monkeypatch): + monkeypatch.delenv(http_defaults.ENV_POLL_TIMEOUT, raising=False) + assert http_defaults.poll_timeout() == http_defaults.DEFAULT_POLL_TIMEOUT + + def test_poll_timeout_env_override(self, monkeypatch): + monkeypatch.setenv(http_defaults.ENV_POLL_TIMEOUT, "42") + assert http_defaults.poll_timeout() == 42.0 + + def test_poll_timeout_clamps_floor_and_ignores_garbage(self, monkeypatch): + monkeypatch.setenv(http_defaults.ENV_POLL_TIMEOUT, "0") + assert http_defaults.poll_timeout() == 1.0 + monkeypatch.setenv(http_defaults.ENV_POLL_TIMEOUT, "not-a-number") + assert http_defaults.poll_timeout() == http_defaults.DEFAULT_POLL_TIMEOUT + + def test_ws_ping_timeout_forced_below_interval(self, monkeypatch): + # A pong-wait >= ping interval is illegal for websocket-client; it must + # be clamped strictly below the interval. + monkeypatch.setenv(http_defaults.ENV_WS_PING_INTERVAL, "20") + monkeypatch.setenv(http_defaults.ENV_WS_PING_TIMEOUT, "30") + interval = http_defaults.ws_ping_interval() + timeout = http_defaults.ws_ping_timeout() + assert timeout < interval + assert timeout == 19.0 + + def test_ws_ping_defaults(self, monkeypatch): + monkeypatch.delenv(http_defaults.ENV_WS_PING_INTERVAL, raising=False) + monkeypatch.delenv(http_defaults.ENV_WS_PING_TIMEOUT, raising=False) + assert ( + http_defaults.ws_ping_interval() == http_defaults.DEFAULT_WS_PING_INTERVAL + ) + assert http_defaults.ws_ping_timeout() == http_defaults.DEFAULT_WS_PING_TIMEOUT + assert http_defaults.ws_ping_timeout() < http_defaults.ws_ping_interval() + + +# --------------------------------------------------------------------------- # +# cr.py: create_instance polling is now deadline-bounded +# --------------------------------------------------------------------------- # +class TestCrCreateInstanceDeadline: + def _make_cr(self, monkeypatch): + from agentkit.toolkit.volcengine import cr as cr_mod + + monkeypatch.setattr( + cr_mod, "resolve_endpoint", lambda *a, **k: "https://cr.example" + ) + instance = cr_mod.VeCR.__new__(cr_mod.VeCR) + instance.config = object() + instance.region = "cn-beijing" + instance._endpoint = "https://cr.example" + return cr_mod, instance + + def test_create_instance_times_out_instead_of_hanging(self, monkeypatch): + cr_mod, cr = self._make_cr(monkeypatch) + + # Fresh instance path: _check_instance says NONEXIST first, create + # succeeds, then status is stuck at "Creating" forever. + checks = ["NONEXIST"] + + def fake_check(_name): + return checks.pop(0) if checks else "Creating" + + monkeypatch.setattr(cr, "_check_instance", fake_check) + monkeypatch.setattr( + cr, + "_ve_request", + lambda **kw: {"ResponseMetadata": {}}, + ) + + # Deterministic clock: each monotonic() call advances 60s so the + # deadline is crossed quickly; sleep is a no-op. + clock = {"t": 0.0} + + def fake_monotonic(): + clock["t"] += 60.0 + return clock["t"] + + monkeypatch.setattr(cr_mod.time, "monotonic", fake_monotonic) + monkeypatch.setattr(cr_mod.time, "sleep", lambda *_a: None) + monkeypatch.setenv(http_defaults.ENV_POLL_TIMEOUT, "120") + + with pytest.raises(TimeoutError) as exc: + cr._create_instance("stuck-instance", instance_type="Micro") + assert "did not reach Running" in str(exc.value) + + def test_create_instance_returns_on_running(self, monkeypatch): + cr_mod, cr = self._make_cr(monkeypatch) + statuses = iter(["NONEXIST", "Creating", "Running"]) + monkeypatch.setattr(cr, "_check_instance", lambda _n: next(statuses)) + monkeypatch.setattr(cr, "_ve_request", lambda **kw: {"ResponseMetadata": {}}) + monkeypatch.setattr(cr_mod.time, "sleep", lambda *_a: None) + assert ( + cr._create_instance("ok-instance", instance_type="Micro") == "ok-instance" + ) + + +# --------------------------------------------------------------------------- # +# ve_agentkit.py: wait loop is bounded even when timeout=None +# --------------------------------------------------------------------------- # +class TestRuntimeWaitBounded: + def test_none_timeout_falls_back_to_poll_bound(self, monkeypatch): + from agentkit.toolkit.runners import ve_agentkit as va + + runner = va.VeAgentkitRuntimeRunner.__new__(va.VeAgentkitRuntimeRunner) + + # Runtime never reaches target; with the old code (timeout=None) this + # loop would spin forever. Now it must exit via the poll-timeout bound. + class _Runtime: + status = "Creating" + + monkeypatch.setattr(va, "retry", lambda fn: _Runtime()) + monkeypatch.setattr(runner, "_get_runtime_client", lambda region: object()) + + # Progress reporter context manager stub. + class _Task: + def update(self, *a, **k): + pass + + class _LongTask: + def __enter__(self): + return _Task() + + def __exit__(self, *a): + return False + + class _Reporter: + def long_task(self, *a, **k): + return _LongTask() + + def success(self, *a, **k): + pass + + runner.reporter = _Reporter() + + # Deterministic clock advancing 500s per reading; sleep no-op. + ticks = iter([0.0] + [500.0 * i for i in range(1, 100)]) + monkeypatch.setattr(va.time, "time", lambda: next(ticks)) + monkeypatch.setattr(va.time, "sleep", lambda *_a: None) + monkeypatch.setenv(http_defaults.ENV_POLL_TIMEOUT, "300") + + ok, runtime, err = runner._wait_for_runtime_status_multiple( + runtime_id="rt-1", + target_statuses=["Running"], + task_description="waiting", + timeout=None, + ) + assert ok is False + assert err is not None and "timeout" in err.lower()