From 76ce1c49205faebcd11c770b51468108ee8d2d19 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Wed, 16 Sep 2026 15:58:42 +0300 Subject: [PATCH 01/13] (feat) restore rate oracle config endpoints, scoped to bots Replacing RateOracle with the in-house ticker pool (f067c37) also deleted the endpoints that set rate_oracle_source in conf_client.yml. The API no longer uses it, but every deployed bot still runs hummingbot's RateOracle from the conf_client.yml copied out of its credentials profile, so bots were stuck on the template's source. Adds GET /bot-orchestration/rate-oracle/sources and GET/PUT /bot-orchestration/rate-oracle/config, per account (account_name, default master_account). Updating master_account's global token also switches the API's own quote token. Co-Authored-By: Claude Opus 5 --- main.py | 2 + models/bot_rate_oracle.py | 59 +++++++++++++ routers/bot_rate_oracle.py | 128 ++++++++++++++++++++++++++++ services/market_data_service.py | 4 + test/test_bot_rate_oracle_config.py | 89 +++++++++++++++++++ 5 files changed, 282 insertions(+) create mode 100644 models/bot_rate_oracle.py create mode 100644 routers/bot_rate_oracle.py create mode 100644 test/test_bot_rate_oracle_config.py diff --git a/main.py b/main.py index c87d226a..e816b29e 100644 --- a/main.py +++ b/main.py @@ -45,6 +45,7 @@ def patched_save_to_yml(yml_path, cm): archived_bots, backtesting, bot_orchestration, + bot_rate_oracle, connectors, controllers, docker, @@ -476,6 +477,7 @@ def auth_user( app.include_router(controllers.router, dependencies=[Depends(auth_user)]) app.include_router(scripts.router, dependencies=[Depends(auth_user)]) app.include_router(market_data.router, dependencies=[Depends(auth_user)]) +app.include_router(bot_rate_oracle.router, dependencies=[Depends(auth_user)]) app.include_router(performance.router, dependencies=[Depends(auth_user)]) app.include_router(backtesting.router, dependencies=[Depends(auth_user)]) app.include_router(archived_bots.router, dependencies=[Depends(auth_user)]) diff --git a/models/bot_rate_oracle.py b/models/bot_rate_oracle.py new file mode 100644 index 00000000..6b332dd7 --- /dev/null +++ b/models/bot_rate_oracle.py @@ -0,0 +1,59 @@ +""" +Pydantic models for the bot rate oracle config endpoints. + +The API itself prices through the in-house ticker pool, but deployed bots still run +hummingbot's RateOracle, configured from the conf_client.yml copied out of their +credentials profile. These models describe that persisted configuration. +""" + +from typing import List, Optional + +from pydantic import BaseModel, Field + + +class GlobalTokenConfig(BaseModel): + """Global token configuration for displaying values.""" + global_token_name: Optional[str] = Field( + default=None, + description="The token to use as global quote (e.g., USDT, USD, BTC)" + ) + global_token_symbol: Optional[str] = Field( + default=None, + description="Symbol to display for the global token" + ) + + +class RateOracleSourceConfig(BaseModel): + """Rate oracle source configuration.""" + name: str = Field(description="The rate oracle source deployed bots use for price data") + + +class RateOracleConfig(BaseModel): + """Rate oracle configuration persisted in an account's conf_client.yml.""" + account_name: str = Field(description="Credentials profile the configuration belongs to") + rate_oracle_source: RateOracleSourceConfig + global_token: GlobalTokenConfig + + +class RateOracleConfigResponse(RateOracleConfig): + """Response for the rate oracle configuration GET endpoint.""" + available_sources: List[str] = Field(description="Rate oracle sources that can be configured") + + +class RateOracleConfigUpdateRequest(BaseModel): + """Request model for updating the rate oracle configuration.""" + rate_oracle_source: Optional[RateOracleSourceConfig] = Field( + default=None, + description="New rate oracle source configuration (optional)" + ) + global_token: Optional[GlobalTokenConfig] = Field( + default=None, + description="New global token configuration (optional)" + ) + + +class RateOracleConfigUpdateResponse(BaseModel): + """Response for a rate oracle configuration update.""" + success: bool = Field(description="Whether the update was successful") + message: str = Field(description="Status message") + config: RateOracleConfig = Field(description="Updated configuration") diff --git a/routers/bot_rate_oracle.py b/routers/bot_rate_oracle.py new file mode 100644 index 00000000..ead8d9d8 --- /dev/null +++ b/routers/bot_rate_oracle.py @@ -0,0 +1,128 @@ +""" +Bot rate oracle config router. + +Configures the rate oracle of deployed bots, not the API. The API prices through its own +ticker pool (see MarketDataService), but every deployed bot still runs hummingbot's +RateOracle, configured from the conf_client.yml copied out of its +credentials profile at deploy time. These endpoints read and persist that configuration so +bots are not stuck with whatever source the shipped template defaults to. +""" + +from fastapi import APIRouter, Depends, HTTPException, Query +from hummingbot.core.rate_oracle.rate_oracle import RATE_ORACLE_SOURCES + +from deps import get_market_data_service +from models.bot_rate_oracle import ( + GlobalTokenConfig, + RateOracleConfig, + RateOracleConfigResponse, + RateOracleConfigUpdateRequest, + RateOracleConfigUpdateResponse, + RateOracleSourceConfig, +) +from services.accounts_service import validate_safe_name +from services.market_data_service import MarketDataService +from utils.file_system import FileSystemUtil + +router = APIRouter(tags=["Bot Orchestration"], prefix="/bot-orchestration/rate-oracle") + +DEFAULT_ACCOUNT = "master_account" + + +def _conf_client_path(account_name: str) -> str: + """Path to an account's conf_client.yml, relative to the FileSystemUtil base path ("bots").""" + validate_safe_name(account_name, "account name") + return f"credentials/{account_name}/conf_client.yml" + + +def _read_conf_client(account_name: str) -> dict: + path = _conf_client_path(account_name) + try: + return FileSystemUtil().read_yaml_file(path) or {} + except FileNotFoundError: + raise HTTPException(status_code=404, detail=f"Configuration file not found: {path}") + + +def _to_config(account_name: str, config_data: dict) -> RateOracleConfig: + global_token = config_data.get("global_token") or {} + return RateOracleConfig( + account_name=account_name, + rate_oracle_source=RateOracleSourceConfig( + name=(config_data.get("rate_oracle_source") or {}).get("name", "gate_io") + ), + global_token=GlobalTokenConfig( + global_token_name=global_token.get("global_token_name", "USDT"), + global_token_symbol=global_token.get("global_token_symbol", "$"), + ), + ) + + +@router.get("/sources", response_model=list[str]) +async def get_bot_rate_oracle_sources(): + """List the rate oracle sources a bot can be configured with. Does not affect the API's own pricing.""" + return list(RATE_ORACLE_SOURCES.keys()) + + +@router.get("/config", response_model=RateOracleConfigResponse) +async def get_bot_rate_oracle_config(account_name: str = Query(DEFAULT_ACCOUNT)): + """ + Get the rate oracle configuration that bots deployed with this credentials profile will use. + + Args: + account_name: Credentials profile whose conf_client.yml to read (default master_account) + """ + config = _to_config(account_name, _read_conf_client(account_name)) + return RateOracleConfigResponse(**config.model_dump(), available_sources=list(RATE_ORACLE_SOURCES.keys())) + + +@router.put("/config", response_model=RateOracleConfigUpdateResponse) +async def update_bot_rate_oracle_config( + update_request: RateOracleConfigUpdateRequest, + account_name: str = Query(DEFAULT_ACCOUNT), + market_data_service: MarketDataService = Depends(get_market_data_service), +): + """ + Update the bot rate oracle source and/or global token in an account's conf_client.yml. + + Bots pick the change up on their next deploy (running bots keep their copied config). + Changing master_account's global token also switches the API's own valuation quote token. + + Args: + update_request: Configuration updates to apply + account_name: Credentials profile whose conf_client.yml to update (default master_account) + """ + config_data = _read_conf_client(account_name) + changes_made = [] + + if update_request.rate_oracle_source is not None: + source_name = update_request.rate_oracle_source.name + if source_name not in RATE_ORACLE_SOURCES: + raise HTTPException( + status_code=400, + detail=f"Invalid rate oracle source: {source_name}. " + f"Available sources: {list(RATE_ORACLE_SOURCES.keys())}" + ) + config_data.setdefault("rate_oracle_source", {})["name"] = source_name + changes_made.append(f"rate_oracle_source updated to {source_name}") + + if update_request.global_token is not None: + global_token = config_data.setdefault("global_token", {}) + token_name = update_request.global_token.global_token_name + if token_name is not None: + global_token["global_token_name"] = token_name + changes_made.append(f"global_token_name updated to {token_name}") + if account_name == DEFAULT_ACCOUNT: + market_data_service.quote_token = token_name + token_symbol = update_request.global_token.global_token_symbol + if token_symbol is not None: + global_token["global_token_symbol"] = token_symbol + changes_made.append(f"global_token_symbol updated to {token_symbol}") + + if changes_made: + FileSystemUtil().dump_dict_to_yaml(_conf_client_path(account_name), config_data) + + return RateOracleConfigUpdateResponse( + success=True, + message="; ".join(changes_made) if changes_made else "No changes made", + config=_to_config(account_name, config_data), + ) diff --git a/services/market_data_service.py b/services/market_data_service.py index 0ed839a6..8c20c769 100644 --- a/services/market_data_service.py +++ b/services/market_data_service.py @@ -1107,6 +1107,10 @@ def quote_token(self) -> str: """The global quote token everything is valued in.""" return self._quote_token + @quote_token.setter + def quote_token(self, value: str) -> None: + self._quote_token = value + @property def connector_service(self) -> "UnifiedConnectorService": """Get the connector service instance.""" diff --git a/test/test_bot_rate_oracle_config.py b/test/test_bot_rate_oracle_config.py new file mode 100644 index 00000000..3e1f82a7 --- /dev/null +++ b/test/test_bot_rate_oracle_config.py @@ -0,0 +1,89 @@ +""" +Tests for the bot rate oracle config endpoints. + +The API prices through its own ticker pool, but deployed bots still run hummingbot's +RateOracle from the conf_client.yml copied out of their credentials profile. Removing +these endpoints left every bot stuck on the template's source; pinned here: the config +is read and persisted per account, invalid sources and names are rejected, and changing +master_account's global token also moves the API's own quote token. + +Run with: pytest test/test_bot_rate_oracle_config.py -v +""" +from types import SimpleNamespace + +import pytest +import yaml +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from utils.file_system import FileSystemUtil + + +@pytest.fixture +def env(tmp_path, monkeypatch): + from deps import get_market_data_service + from routers import bot_rate_oracle + + for account in ("master_account", "other"): + (tmp_path / "credentials" / account).mkdir(parents=True) + (tmp_path / "credentials" / account / "conf_client.yml").write_text(yaml.safe_dump({ + "instance_id": "x", + "rate_oracle_source": {"name": "gate_io"}, + "global_token": {"global_token_name": "USDT", "global_token_symbol": "$"}, + })) + monkeypatch.setattr(FileSystemUtil(), "base_path", str(tmp_path)) + + service = SimpleNamespace(quote_token="USDT") + app = FastAPI() + app.include_router(bot_rate_oracle.router) + app.dependency_overrides[get_market_data_service] = lambda: service + return SimpleNamespace(client=TestClient(app), service=service, root=tmp_path) + + +def _conf(env, account): + return yaml.safe_load((env.root / "credentials" / account / "conf_client.yml").read_text()) + + +def test_get_config_reads_account_file(env): + resp = env.client.get("/bot-orchestration/rate-oracle/config") + assert resp.status_code == 200 + body = resp.json() + assert body["rate_oracle_source"]["name"] == "gate_io" + assert body["global_token"]["global_token_name"] == "USDT" + assert "binance" in body["available_sources"] + + +def test_update_source_persists_and_keeps_other_keys(env): + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"rate_oracle_source": {"name": "binance"}}) + assert resp.status_code == 200 + conf = _conf(env, "master_account") + assert conf["rate_oracle_source"]["name"] == "binance" + assert conf["instance_id"] == "x" + + +def test_update_other_account_does_not_touch_api_quote_token(env): + resp = env.client.put( + "/bot-orchestration/rate-oracle/config", + params={"account_name": "other"}, + json={"global_token": {"global_token_name": "USDC"}}, + ) + assert resp.status_code == 200 + assert _conf(env, "other")["global_token"] == {"global_token_name": "USDC", "global_token_symbol": "$"} + assert _conf(env, "master_account")["global_token"]["global_token_name"] == "USDT" + assert env.service.quote_token == "USDT" + + +def test_master_global_token_updates_api_quote_token(env): + env.client.put("/bot-orchestration/rate-oracle/config", json={"global_token": {"global_token_name": "USDC"}}) + assert env.service.quote_token == "USDC" + + +def test_invalid_source_rejected(env): + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"rate_oracle_source": {"name": "nope"}}) + assert resp.status_code == 400 + assert _conf(env, "master_account")["rate_oracle_source"]["name"] == "gate_io" + + +def test_unknown_or_unsafe_account(env): + assert env.client.get("/bot-orchestration/rate-oracle/config", params={"account_name": "missing"}).status_code == 404 + assert env.client.get("/bot-orchestration/rate-oracle/config", params={"account_name": "../x"}).status_code == 400 From 17e62b24d8f6954be992a22855eee56ed977b51a Mon Sep 17 00:00:00 2001 From: cardosofede Date: Wed, 16 Sep 2026 16:07:25 +0300 Subject: [PATCH 02/13] fix(bot-rate-oracle): reject blank quote tokens, switch live token only after persisting A blank global_token_name was persisted and installed as the API quote token, so every cross-rate lookup resolved to nothing (and kept doing so after a restart). And the live token was switched before conf_client.yml was written, so a failed write left the API on the new token while bots kept the old one. Co-Authored-By: Claude Opus 5 --- models/bot_rate_oracle.py | 12 +++++++++++- routers/bot_rate_oracle.py | 10 +++++++--- test/test_bot_rate_oracle_config.py | 18 ++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/models/bot_rate_oracle.py b/models/bot_rate_oracle.py index 6b332dd7..df99c1e8 100644 --- a/models/bot_rate_oracle.py +++ b/models/bot_rate_oracle.py @@ -8,7 +8,7 @@ from typing import List, Optional -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator class GlobalTokenConfig(BaseModel): @@ -22,6 +22,16 @@ class GlobalTokenConfig(BaseModel): description="Symbol to display for the global token" ) + @field_validator("global_token_name") + @classmethod + def _reject_blank_token_name(cls, v: Optional[str]) -> Optional[str]: + # A blank quote token would make every cross-rate lookup resolve to nothing. + if v is not None: + v = v.strip() + if not v: + raise ValueError("global_token_name must not be blank") + return v + class RateOracleSourceConfig(BaseModel): """Rate oracle source configuration.""" diff --git a/routers/bot_rate_oracle.py b/routers/bot_rate_oracle.py index ead8d9d8..22c25fc0 100644 --- a/routers/bot_rate_oracle.py +++ b/routers/bot_rate_oracle.py @@ -51,7 +51,7 @@ def _to_config(account_name: str, config_data: dict) -> RateOracleConfig: name=(config_data.get("rate_oracle_source") or {}).get("name", "gate_io") ), global_token=GlobalTokenConfig( - global_token_name=global_token.get("global_token_name", "USDT"), + global_token_name=global_token.get("global_token_name") or "USDT", global_token_symbol=global_token.get("global_token_symbol", "$"), ), ) @@ -111,8 +111,6 @@ async def update_bot_rate_oracle_config( if token_name is not None: global_token["global_token_name"] = token_name changes_made.append(f"global_token_name updated to {token_name}") - if account_name == DEFAULT_ACCOUNT: - market_data_service.quote_token = token_name token_symbol = update_request.global_token.global_token_symbol if token_symbol is not None: global_token["global_token_symbol"] = token_symbol @@ -121,6 +119,12 @@ async def update_bot_rate_oracle_config( if changes_made: FileSystemUtil().dump_dict_to_yaml(_conf_client_path(account_name), config_data) + # Only switch the API's live quote token once the file is written, so a failed write + # can't leave the API and future bots disagreeing on the token. + token_name = update_request.global_token and update_request.global_token.global_token_name + if token_name is not None and account_name == DEFAULT_ACCOUNT: + market_data_service.quote_token = token_name + return RateOracleConfigUpdateResponse( success=True, message="; ".join(changes_made) if changes_made else "No changes made", diff --git a/test/test_bot_rate_oracle_config.py b/test/test_bot_rate_oracle_config.py index 3e1f82a7..c7cc759a 100644 --- a/test/test_bot_rate_oracle_config.py +++ b/test/test_bot_rate_oracle_config.py @@ -87,3 +87,21 @@ def test_invalid_source_rejected(env): def test_unknown_or_unsafe_account(env): assert env.client.get("/bot-orchestration/rate-oracle/config", params={"account_name": "missing"}).status_code == 404 assert env.client.get("/bot-orchestration/rate-oracle/config", params={"account_name": "../x"}).status_code == 400 + + +def test_blank_global_token_name_rejected(env): + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"global_token": {"global_token_name": " "}}) + assert resp.status_code == 422 + assert _conf(env, "master_account")["global_token"]["global_token_name"] == "USDT" + assert env.service.quote_token == "USDT" + + +def test_failed_write_leaves_live_quote_token_untouched(env, monkeypatch): + def fail(*args, **kwargs): + raise PermissionError("read-only") + + monkeypatch.setattr(FileSystemUtil(), "dump_dict_to_yaml", fail) + client = TestClient(env.client.app, raise_server_exceptions=False) + resp = client.put("/bot-orchestration/rate-oracle/config", json={"global_token": {"global_token_name": "USDC"}}) + assert resp.status_code == 500 + assert env.service.quote_token == "USDT" From 31e671dbf08cd0fa3dbaaa64675ef7ba8df8df48 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Thu, 17 Sep 2026 20:55:15 +0300 Subject: [PATCH 03/13] feat(system): report API/hummingbot versions and image provenance at GET /system/info The Settings -> Server panel of a dashboard can say which server it is talking to but not what that server runs: the hummingbot-api version, the hummingbot library version behind it, and whether the image is the published one or something pinned on the box. That last one is the question asked after every incident ("is this host actually on the build we shipped?"), and today it is answered by ssh-ing in and reading `docker inspect`. /system/info answers it over the API. It reports the API version, the hummingbot version from importlib.metadata, and the API's own container as seen through the already-mounted docker.sock: id, name, image reference, image id, RepoDigests digest, and the compose project, working_dir and config_files labels. From those it derives `pinned`: an image with no registry digest was built locally, and a config_files entry whose name contains "override" means compose was brought up with a file that can repoint `image:` (the tailscale overlay does not count -- it changes networking, not the image). `pinned_reason` says which, and `override_file` names the file. The route is read-only and never raises. Without docker.sock, with an unreachable daemon, or when this process cannot place itself, it answers `docker_available: false` with a null container and a null (not false) `pinned` -- unknown is not "unpinned" -- and still reports both versions, which is the part that works from source too. The container is found the way a process finds itself from inside Docker: the hostname is the short container id, with a fallback scan for the com.docker.compose.service label for deployments that override `hostname:` or run the API from source. VERSION moves from main.py to a new version.py so the router can read it -- main.py imports every router, so importing it back would be circular. main.py re-exports it, and the Docker buildx workflow's grep now reads version.py. --- .github/workflows/docker_buildx_workflow.yml | 4 +- main.py | 2 +- routers/system.py | 157 +++++++++++++++- test/test_system_info.py | 178 +++++++++++++++++++ version.py | 9 + 5 files changed, 346 insertions(+), 4 deletions(-) create mode 100644 test/test_system_info.py create mode 100644 version.py diff --git a/.github/workflows/docker_buildx_workflow.yml b/.github/workflows/docker_buildx_workflow.yml index 682f832e..d1415565 100644 --- a/.github/workflows/docker_buildx_workflow.yml +++ b/.github/workflows/docker_buildx_workflow.yml @@ -14,10 +14,10 @@ jobs: - name: Checkout code uses: actions/checkout@v4.1.1 - - name: Extract version from main.py + - name: Extract version from version.py id: get_version run: | - VERSION=$(grep -E '^VERSION *= *' main.py | head -1 | sed -E 's/^VERSION *= *["\x27]?([^"\x27]*)["\x27]?/\1/') + VERSION=$(grep -E '^VERSION *= *' version.py | head -1 | sed -E 's/^VERSION *= *["\x27]?([^"\x27]*)["\x27]?/\1/') echo "VERSION=$VERSION" >> $GITHUB_OUTPUT - name: Set up QEMU diff --git a/main.py b/main.py index e816b29e..ecaee465 100644 --- a/main.py +++ b/main.py @@ -13,7 +13,7 @@ # Load environment variables early load_dotenv() -VERSION = "1.0.1" +from version import VERSION # noqa: E402 (kept in its own module so routers can read it) # Monkey patch save_to_yml to prevent writes to library directory diff --git a/routers/system.py b/routers/system.py index eb9dbae7..74a2f366 100644 --- a/routers/system.py +++ b/routers/system.py @@ -1,11 +1,29 @@ +import logging import os +import socket +from importlib import metadata import psutil +from fastapi import APIRouter, Depends -from fastapi import APIRouter +from deps import get_docker_service +from services.docker_service import DockerService +from version import VERSION + +logger = logging.getLogger(__name__) router = APIRouter(tags=["System"], prefix="/system") +# Compose service this API is deployed as, used to find our own container when the +# hostname lookup fails (see _own_container). +COMPOSE_SERVICE = "hummingbot-api" + +# Compose stamps these on every container it creates; they say which project the API +# belongs to and which files were used to bring it up. +LABEL_PROJECT = "com.docker.compose.project" +LABEL_WORKING_DIR = "com.docker.compose.project.working_dir" +LABEL_CONFIG_FILES = "com.docker.compose.project.config_files" + # When running inside Docker, mount the host's /proc and root filesystem into the # container and point these env vars at the mount locations so psutil reports the # HOST machine's metrics rather than the container's. They default to the local @@ -66,3 +84,140 @@ async def get_system_resources(): "percent": disk.percent, }, } + + +def _hummingbot_version(): + """The version of the hummingbot library this API runs against, or None if absent.""" + try: + return metadata.version("hummingbot") + except metadata.PackageNotFoundError: + return None + + +def _own_container(client): + """Find the container this API process runs in. + + Docker sets a container's hostname to its own short id, which is how a process + identifies itself from the inside. That fails when the deployment overrides + `hostname:` or when the API runs from source on the host, so fall back to the one + container carrying this project's compose service label. + + Returns: + A docker-py Container, or None when this process cannot be placed. + """ + try: + return client.containers.get(socket.gethostname()) + except Exception: + pass + try: + matches = client.containers.list(filters={"label": f"com.docker.compose.service={COMPOSE_SERVICE}"}) + except Exception: + return None + return matches[0] if matches else None + + +def _describe_container(container): + """Identity, image and compose provenance of a container. + + Returns: + Dictionary with the container's id and name, the image reference it was started + from, the image id, its registry digest (None for a locally built image) and the + three compose labels that say where the deployment lives. + """ + labels = container.labels or {} + image = container.image + # A locally built image has no RepoDigests: nothing published it, so there is no + # content address to compare against the tag it carries. + digests = (image.attrs.get("RepoDigests") or []) if image is not None else [] + return { + "id": container.id, + "name": container.name, + "image": container.attrs.get("Config", {}).get("Image"), + "image_id": image.id if image is not None else None, + "digest": digests[0] if digests else None, + "compose_project": labels.get(LABEL_PROJECT), + "compose_working_dir": labels.get(LABEL_WORKING_DIR), + "compose_config_files": labels.get(LABEL_CONFIG_FILES), + } + + +def _pinning(container): + """Whether this deployment runs something other than the published image. + + Two things make an image not the one `hummingbot/hummingbot-api:latest` currently + resolves to: it was built on the box (no registry digest), or compose was brought up + with an override file that can repoint `image:` at a pinned tag. Only files whose + name contains "override" count -- `docker-compose.tailscale.yml` is an overlay for + networking, not for the image. + + Returns: + Dictionary with pinned, a short pinned_reason and the override file name (None + when that is not the reason). + """ + files = [f.strip() for f in (container.get("compose_config_files") or "").split(",") if f.strip()] + override_file = next((os.path.basename(f) for f in files if "override" in os.path.basename(f).lower()), None) + + reasons = [] + if not container.get("digest"): + reasons.append("image was built locally (no registry digest)") + if override_file: + reasons.append(f"compose override file {override_file} can pin the image") + + return { + "pinned": bool(reasons), + "pinned_reason": "; ".join(reasons) or None, + "override_file": override_file, + } + + +@router.get("/info") +async def get_system_info(docker_service: DockerService = Depends(get_docker_service)): + """ + Get the versions this API server runs and how its own container was deployed. + + Read-only, and never raises: when docker.sock is not mounted, the daemon is + unreachable or this process cannot find its own container, the version fields are + still reported and the container block is None. + + Returns: + Dictionary with the API version, the hummingbot library version, whether the + Docker daemon could be reached, the API's own container (id, name, image, + digest and compose labels), and whether that image is pinned, with a short + reason and the override file name when it is. + """ + info = { + "api_version": VERSION, + "hummingbot_version": _hummingbot_version(), + "docker_available": False, + "container": None, + # None rather than False: without the daemon we have not established that the + # image is unpinned, only that we cannot tell. + "pinned": None, + "pinned_reason": None, + "override_file": None, + } + + # DockerService leaves `client` unset when docker.from_env() failed at startup. + client = getattr(docker_service, "client", None) + if client is None: + return info + try: + client.ping() + except Exception as e: + logger.warning(f"Docker daemon unreachable while reporting system info: {e}") + return info + info["docker_available"] = True + + container = _own_container(client) + if container is None: + logger.warning("Could not identify the API's own container while reporting system info") + return info + + try: + info["container"] = _describe_container(container) + except Exception as e: + logger.warning(f"Could not inspect the API's own container while reporting system info: {e}") + return info + + info.update(_pinning(info["container"])) + return info diff --git a/test/test_system_info.py b/test/test_system_info.py new file mode 100644 index 00000000..43070131 --- /dev/null +++ b/test/test_system_info.py @@ -0,0 +1,178 @@ +""" +Tests for the /system/info endpoint. + +The dashboard needs to tell which hummingbot-api and hummingbot version a server runs, +and whether its image is the published one or something pinned on the box. That answer +comes from the API's own container over the mounted docker.sock, which is exactly the +thing that can be missing -- so pinned here: the happy path reports the digest and the +compose labels, a locally built image and a compose override file each read as pinned, +and a daemon that cannot be reached still answers with both versions. + +Run with: pytest test/test_system_info.py -v +""" +from types import SimpleNamespace + +import pytest +from docker.errors import DockerException, NotFound +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from version import VERSION + +CONTAINER_ID = "3f2a1b0c9d8e7f6a5b4c3d2e1f0a9b8c7d6e5f4a3b2c1d0e9f8a7b6c5d4e3f2a" +WORKING_DIR = "/home/hbot/hummingbot-api" + + +def _container(digests=("hummingbot/hummingbot-api@sha256:abc123",), config_files=f"{WORKING_DIR}/docker-compose.yml"): + """A docker-py Container as this route reads it.""" + return SimpleNamespace( + id=CONTAINER_ID, + name="hummingbot-api", + labels={ + "com.docker.compose.project": "hummingbot-api", + "com.docker.compose.project.working_dir": WORKING_DIR, + "com.docker.compose.project.config_files": config_files, + "com.docker.compose.service": "hummingbot-api", + }, + attrs={"Config": {"Image": "hummingbot/hummingbot-api:latest"}}, + image=SimpleNamespace( + id="sha256:deadbeef", + tags=["hummingbot/hummingbot-api:latest"], + attrs={"RepoDigests": list(digests)}, + ), + ) + + +class _Containers: + def __init__(self, container, found_by_hostname=True): + self.container = container + self.found_by_hostname = found_by_hostname + self.list_filters = None + + def get(self, name): + if not self.found_by_hostname: + raise NotFound(f"no such container: {name}") + return self.container + + def list(self, filters=None): + self.list_filters = filters + return [self.container] if self.container is not None else [] + + +class _Client: + def __init__(self, container, found_by_hostname=True, reachable=True): + self.containers = _Containers(container, found_by_hostname) + self.reachable = reachable + + def ping(self): + if not self.reachable: + raise DockerException("Error while fetching server API version") + return True + + +@pytest.fixture +def make_client(): + """Build a TestClient whose /system/info talks to the given fake docker client.""" + from deps import get_docker_service + from routers import system + + def _make(docker_client): + app = FastAPI() + app.include_router(system.router) + app.dependency_overrides[get_docker_service] = lambda: SimpleNamespace(client=docker_client) + return TestClient(app) + + return _make + + +def test_reports_versions_and_container(make_client): + body = make_client(_Client(_container())).get("/system/info").json() + + assert body["api_version"] == VERSION + assert body["hummingbot_version"] # the library is installed in the test environment + assert body["docker_available"] is True + assert body["container"] == { + "id": CONTAINER_ID, + "name": "hummingbot-api", + "image": "hummingbot/hummingbot-api:latest", + "image_id": "sha256:deadbeef", + "digest": "hummingbot/hummingbot-api@sha256:abc123", + "compose_project": "hummingbot-api", + "compose_working_dir": WORKING_DIR, + "compose_config_files": f"{WORKING_DIR}/docker-compose.yml", + } + assert body["pinned"] is False + assert body["pinned_reason"] is None + assert body["override_file"] is None + + +def test_locally_built_image_is_pinned(make_client): + body = make_client(_Client(_container(digests=()))).get("/system/info").json() + + assert body["container"]["digest"] is None + assert body["pinned"] is True + assert "locally" in body["pinned_reason"] + assert body["override_file"] is None + + +def test_override_file_is_pinned_and_named(make_client): + container = _container( + config_files=f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.override.yml", + ) + body = make_client(_Client(container)).get("/system/info").json() + + assert body["pinned"] is True + assert body["override_file"] == "docker-compose.override.yml" + assert "docker-compose.override.yml" in body["pinned_reason"] + + +def test_extra_overlay_that_is_not_an_override_is_not_pinned(make_client): + # The Tailscale overlay repoints networking, not the image. + container = _container( + config_files=f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.tailscale.yml", + ) + assert make_client(_Client(container)).get("/system/info").json()["pinned"] is False + + +def test_falls_back_to_the_compose_service_label(make_client): + client = _Client(_container(), found_by_hostname=False) + body = make_client(client).get("/system/info").json() + + assert client.containers.list_filters == {"label": "com.docker.compose.service=hummingbot-api"} + assert body["container"]["name"] == "hummingbot-api" + + +def test_docker_unavailable_still_reports_versions(make_client): + body = make_client(_Client(_container(), reachable=False)).get("/system/info").json() + + assert body["docker_available"] is False + assert body["container"] is None + assert body["pinned"] is None + assert body["pinned_reason"] is None + assert body["api_version"] == VERSION + assert body["hummingbot_version"] + + +def test_container_not_identified_reports_no_container(make_client): + client = _Client(None, found_by_hostname=False) + body = make_client(client).get("/system/info").json() + + assert body["docker_available"] is True + assert body["container"] is None + assert body["pinned"] is None + assert body["api_version"] == VERSION + + +def test_docker_service_without_a_client_never_raises(): + """DockerService leaves `client` unset when the daemon was absent at startup.""" + from deps import get_docker_service + from routers import system + + app = FastAPI() + app.include_router(system.router) + app.dependency_overrides[get_docker_service] = lambda: SimpleNamespace() + + body = TestClient(app).get("/system/info").json() + assert body["docker_available"] is False + assert body["container"] is None + assert body["api_version"] == VERSION diff --git a/version.py b/version.py new file mode 100644 index 00000000..59ac9278 --- /dev/null +++ b/version.py @@ -0,0 +1,9 @@ +"""The API server's own version number. + +It lives in its own module, with no imports of its own, because main.py imports every +router: a router that needs the version -- /system/info reports it -- cannot import it +back from main.py without a circular import. main.py re-exports it, and the Docker +build workflow reads the assignment below with grep, so keep it a plain literal. +""" + +VERSION = "1.0.1" From d8e3f1fa9e14b6bfc0e4d2674700dd6b853e66bc Mon Sep 17 00:00:00 2001 From: cardosofede Date: Thu, 17 Sep 2026 21:00:00 +0300 Subject: [PATCH 04/13] fix(docker): copy version.py into the image 31e671d moved VERSION out of main.py so routers can read it without importing main, but the Dockerfile copies source files one by one and version.py was not on the list, so the built image would fail at startup on `from version import VERSION`. The build itself stays green -- nothing imports the app at build time -- which is why the miss is worth a line of its own here. --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 27e4b497..2bf04d4c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -33,7 +33,7 @@ COPY --from=builder /opt/conda/envs/hummingbot-api /opt/conda/envs/hummingbot-ap WORKDIR /hummingbot-api # Copy only necessary application files -COPY main.py config.py deps.py ./ +COPY main.py version.py config.py deps.py ./ COPY models ./models COPY routers ./routers COPY services ./services From 836f1a4f70fdc01653baffe272c4813274470735 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Thu, 17 Sep 2026 22:38:41 +0300 Subject: [PATCH 05/13] feat(system): report the market-data tunables at GET /system/info A dashboard's server panel can now say what a server runs, but not how it is tuned. The MARKET_DATA_* knobs are the ones an operator actually asks about after a slow ticker or a dropped subscription -- ticker_update_interval, ticker_max_age, the /ws/executors interval floor -- and today reading them means ssh-ing in and opening the .env. /system/info reports them alongside the versions, as this process resolved them, which is the honest answer: the file on disk may have been edited since the API started. They are read-only by construction and stay that way -- env_file is not mounted into the container, so the API cannot rewrite its own .env, and pretending otherwise would promise a change that silently disappears. Safe to report: MarketDataSettings holds intervals and timeouts and nothing else. A test pins that -- every value numeric, no key that reads like a credential -- so a field added later that does not belong in this payload fails here rather than leaking. Another pins that the tunables are still reported when the Docker daemon is unreachable, which is the same degrade path the versions already take. The mock container id in the test file gains a `noqa: mock` tag: it is 64 hex characters, which the wallet-private-key hook cannot tell from a key. --- routers/system.py | 16 ++++++++++++---- test/test_system_info.py | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/routers/system.py b/routers/system.py index 74a2f366..1ed3f666 100644 --- a/routers/system.py +++ b/routers/system.py @@ -6,6 +6,7 @@ import psutil from fastapi import APIRouter, Depends +from config import settings from deps import get_docker_service from services.docker_service import DockerService from version import VERSION @@ -180,14 +181,21 @@ async def get_system_info(docker_service: DockerService = Depends(get_docker_ser still reported and the container block is None. Returns: - Dictionary with the API version, the hummingbot library version, whether the - Docker daemon could be reached, the API's own container (id, name, image, - digest and compose labels), and whether that image is pinned, with a short - reason and the override file name when it is. + Dictionary with the API version, the hummingbot library version, the market-data + tunables this process is running with, whether the Docker daemon could be + reached, the API's own container (id, name, image, digest and compose labels), + and whether that image is pinned, with a short reason and the override file name + when it is. """ info = { "api_version": VERSION, "hummingbot_version": _hummingbot_version(), + # The MARKET_DATA_* knobs as this process resolved them. Reported rather than + # editable: they come from the env_file, which is not mounted into the + # container, so the API cannot write its own .env -- a dashboard shows them and + # says where to change them. Non-secret by construction (MarketDataSettings + # holds only intervals and timeouts). + "market_data": settings.market_data.model_dump(), "docker_available": False, "container": None, # None rather than False: without the daemon we have not established that the diff --git a/test/test_system_info.py b/test/test_system_info.py index 43070131..14491952 100644 --- a/test/test_system_info.py +++ b/test/test_system_info.py @@ -19,7 +19,7 @@ from version import VERSION -CONTAINER_ID = "3f2a1b0c9d8e7f6a5b4c3d2e1f0a9b8c7d6e5f4a3b2c1d0e9f8a7b6c5d4e3f2a" +CONTAINER_ID = "3f2a1b0c9d8e7f6a5b4c3d2e1f0a9b8c7d6e5f4a3b2c1d0e9f8a7b6c5d4e3f2a" # noqa: mock (a Docker container id, not a key) WORKING_DIR = "/home/hbot/hummingbot-api" @@ -176,3 +176,36 @@ def test_docker_service_without_a_client_never_raises(): assert body["docker_available"] is False assert body["container"] is None assert body["api_version"] == VERSION + + +def test_reports_the_market_data_tunables(make_client): + # The dashboard shows these read-only: they come from the server's .env, which is not + # mounted into the container, so the API can report them but never rewrite them. + from config import settings + + body = make_client(_Client(_container())).get("/system/info").json() + + assert body["market_data"] == settings.market_data.model_dump() + assert body["market_data"]["ticker_update_interval"] == settings.market_data.ticker_update_interval + + +def test_market_data_tunables_carry_no_secret(make_client): + # MarketDataSettings is intervals and timeouts by construction. Pinned so a later + # field that does not belong in an unauthenticated-looking payload fails here first. + body = make_client(_Client(_container())).get("/system/info").json() + + assert body["market_data"] + assert all(isinstance(v, (int, float)) for v in body["market_data"].values()) + assert not any( + word in key.lower() + for key in body["market_data"] + for word in ("password", "secret", "token", "key", "user") + ) + + +def test_market_data_is_reported_without_docker(make_client): + # The versions and the tunables are the part that works from source too. + body = make_client(_Client(_container(), reachable=False)).get("/system/info").json() + + assert body["docker_available"] is False + assert body["market_data"] From 549f4f52c1664439ef778bc9e07afeb62f1a3a42 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Thu, 17 Sep 2026 22:40:40 +0300 Subject: [PATCH 06/13] feat(bot-rate-oracle): rate_limits_share_pct with the rest of the bot client defaults The rate-oracle endpoints already read and write the slice of a credentials profile's conf_client.yml that deployed bots inherit. rate_limits_share_pct -- the share of each exchange's API rate limit one bot instance may spend -- is the other knob operators actually tune per profile, and it is the one they reach for when several bots share an account and start tripping the exchange's limiter. It was the last reason to open the file by hand. It goes here rather than behind an endpoint of its own on purpose. This router is the only writer of that file, and a second one would have to repeat the source validation and would silently skip the live-quote-token switch at the end of the PUT -- two writers of one file disagreeing about the side effects is the bug, not the duplication. The set written stays closed and small, so no caller can reach the mqtt_bridge or gateway credentials sharing the file. The bounds are hummingbot's own: gt=0, le=100, copied from ClientConfigMap so a value the bot would reject at startup cannot be persisted here -- a bad write breaks the next deploy, not the request that made it, which is the expensive way to find out. Reading degrades instead: the key is absent from every profile created before this commit, and a hand-edited file can hold anything, so an unreadable value answers with hummingbot's default rather than failing a GET that is only asking what bots will run with. --- models/bot_rate_oracle.py | 25 ++++++++++-- routers/bot_rate_oracle.py | 50 ++++++++++++++++++----- test/test_bot_rate_oracle_config.py | 63 +++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 13 deletions(-) diff --git a/models/bot_rate_oracle.py b/models/bot_rate_oracle.py index df99c1e8..be165b55 100644 --- a/models/bot_rate_oracle.py +++ b/models/bot_rate_oracle.py @@ -1,9 +1,11 @@ """ -Pydantic models for the bot rate oracle config endpoints. +Pydantic models for the bot client defaults endpoints. The API itself prices through the in-house ticker pool, but deployed bots still run hummingbot's RateOracle, configured from the conf_client.yml copied out of their -credentials profile. These models describe that persisted configuration. +credentials profile. These models describe that persisted configuration, plus the one +other conf_client.yml knob operators tune per profile -- rate_limits_share_pct, the share +of each exchange's API rate limit a bot instance is allowed to spend. """ from typing import List, Optional @@ -39,10 +41,19 @@ class RateOracleSourceConfig(BaseModel): class RateOracleConfig(BaseModel): - """Rate oracle configuration persisted in an account's conf_client.yml.""" + """Bot client defaults persisted in an account's conf_client.yml.""" account_name: str = Field(description="Credentials profile the configuration belongs to") rate_oracle_source: RateOracleSourceConfig global_token: GlobalTokenConfig + # Bounds mirror hummingbot's own ClientConfigMap.rate_limits_share_pct (gt=0, le=100): + # the file is read back by hummingbot inside the bot container, so a value it would + # reject must not be writable here. + rate_limits_share_pct: float = Field( + default=100.0, + gt=0, + le=100, + description="Percentage of each exchange's API rate limit allocated to a bot instance" + ) class RateOracleConfigResponse(RateOracleConfig): @@ -60,10 +71,16 @@ class RateOracleConfigUpdateRequest(BaseModel): default=None, description="New global token configuration (optional)" ) + rate_limits_share_pct: Optional[float] = Field( + default=None, + gt=0, + le=100, + description="New share of each exchange's API rate limit for a bot instance, 1-100 (optional)" + ) class RateOracleConfigUpdateResponse(BaseModel): - """Response for a rate oracle configuration update.""" + """Response for a bot client defaults update.""" success: bool = Field(description="Whether the update was successful") message: str = Field(description="Status message") config: RateOracleConfig = Field(description="Updated configuration") diff --git a/routers/bot_rate_oracle.py b/routers/bot_rate_oracle.py index 22c25fc0..92cae304 100644 --- a/routers/bot_rate_oracle.py +++ b/routers/bot_rate_oracle.py @@ -1,11 +1,18 @@ """ -Bot rate oracle config router. - -Configures the rate oracle of deployed bots, not the API. The API prices through its own -ticker pool (see MarketDataService), but every deployed bot still runs hummingbot's -RateOracle, configured from the conf_client.yml copied out of its -credentials profile at deploy time. These endpoints read and persist that configuration so -bots are not stuck with whatever source the shipped template defaults to. +Bot client defaults router. + +Configures deployed bots, not the API. The API prices through its own ticker pool (see +MarketDataService), but every deployed bot still runs hummingbot's RateOracle, configured +from the conf_client.yml copied out of its credentials profile at deploy time. These +endpoints read and persist that configuration so bots are not stuck with whatever source +the shipped template defaults to. + +This router is the only writer of a credentials profile's conf_client.yml, which is why +rate_limits_share_pct lives here rather than behind an endpoint of its own: a second +writer of the same file would have to repeat this validation and would silently skip the +live-quote-token switch below. The set it writes is closed and small -- rate_oracle_source, +global_token, rate_limits_share_pct -- so no caller can reach the mqtt_bridge or gateway +credentials that share the file. """ from fastapi import APIRouter, Depends, HTTPException, Query @@ -43,6 +50,21 @@ def _read_conf_client(account_name: str) -> dict: raise HTTPException(status_code=404, detail=f"Configuration file not found: {path}") +def _share_pct(config_data: dict) -> float: + """rate_limits_share_pct as a number, falling back to hummingbot's own default. + + The key is absent from older templates, and a hand-edited file can hold anything, so + an unreadable value reads as the default rather than failing the whole GET -- the + caller is asking what bots will run with, and hummingbot itself would fall back the + same way. + """ + try: + value = float(config_data.get("rate_limits_share_pct")) + except (TypeError, ValueError): + return 100.0 + return value if 0 < value <= 100 else 100.0 + + def _to_config(account_name: str, config_data: dict) -> RateOracleConfig: global_token = config_data.get("global_token") or {} return RateOracleConfig( @@ -54,6 +76,7 @@ def _to_config(account_name: str, config_data: dict) -> RateOracleConfig: global_token_name=global_token.get("global_token_name") or "USDT", global_token_symbol=global_token.get("global_token_symbol", "$"), ), + rate_limits_share_pct=_share_pct(config_data), ) @@ -66,7 +89,7 @@ async def get_bot_rate_oracle_sources(): @router.get("/config", response_model=RateOracleConfigResponse) async def get_bot_rate_oracle_config(account_name: str = Query(DEFAULT_ACCOUNT)): """ - Get the rate oracle configuration that bots deployed with this credentials profile will use. + Get the client defaults that bots deployed with this credentials profile will use. Args: account_name: Credentials profile whose conf_client.yml to read (default master_account) @@ -82,7 +105,8 @@ async def update_bot_rate_oracle_config( market_data_service: MarketDataService = Depends(get_market_data_service), ): """ - Update the bot rate oracle source and/or global token in an account's conf_client.yml. + Update the bot rate oracle source, global token and/or rate-limit share in an account's + conf_client.yml. Bots pick the change up on their next deploy (running bots keep their copied config). Changing master_account's global token also switches the API's own valuation quote token. @@ -116,6 +140,14 @@ async def update_bot_rate_oracle_config( global_token["global_token_symbol"] = token_symbol changes_made.append(f"global_token_symbol updated to {token_symbol}") + if update_request.rate_limits_share_pct is not None: + # Written as a float so the file keeps the shape hummingbot's own writer produces; + # the 0 < pct <= 100 bound is enforced by the request model, which mirrors + # ClientConfigMap's, so a value the bot would reject never reaches the file. + share_pct = float(update_request.rate_limits_share_pct) + config_data["rate_limits_share_pct"] = share_pct + changes_made.append(f"rate_limits_share_pct updated to {share_pct}") + if changes_made: FileSystemUtil().dump_dict_to_yaml(_conf_client_path(account_name), config_data) diff --git a/test/test_bot_rate_oracle_config.py b/test/test_bot_rate_oracle_config.py index c7cc759a..c9a5ddc5 100644 --- a/test/test_bot_rate_oracle_config.py +++ b/test/test_bot_rate_oracle_config.py @@ -105,3 +105,66 @@ def fail(*args, **kwargs): resp = client.put("/bot-orchestration/rate-oracle/config", json={"global_token": {"global_token_name": "USDC"}}) assert resp.status_code == 500 assert env.service.quote_token == "USDT" + + +def test_share_pct_defaults_when_the_template_has_no_key(env): + # The fixture's conf_client.yml predates the key, which is the state every existing + # credentials profile is in. The answer is hummingbot's own default, not an error. + body = env.client.get("/bot-orchestration/rate-oracle/config").json() + + assert body["rate_limits_share_pct"] == 100.0 + + +def test_update_share_pct_persists_and_leaves_the_oracle_alone(env): + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"rate_limits_share_pct": 40}) + + assert resp.status_code == 200 + assert resp.json()["config"]["rate_limits_share_pct"] == 40.0 + conf = _conf(env, "master_account") + assert conf["rate_limits_share_pct"] == 40.0 + # A partial update touches only what it names. + assert conf["rate_oracle_source"]["name"] == "gate_io" + assert conf["instance_id"] == "x" + assert env.client.get("/bot-orchestration/rate-oracle/config").json()["rate_limits_share_pct"] == 40.0 + + +@pytest.mark.parametrize("bad", [0, -5, 100.5, 1000]) +def test_share_pct_outside_hummingbots_own_bounds_is_rejected(env, bad): + # Anything the bot's ClientConfigMap would refuse must not reach the file: the bot + # reads it back at startup, so a bad write breaks the deploy, not this request. + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"rate_limits_share_pct": bad}) + + assert resp.status_code == 422 + assert "rate_limits_share_pct" not in _conf(env, "master_account") + + +def test_a_hand_edited_share_pct_reads_as_the_default(env): + path = env.root / "credentials" / "master_account" / "conf_client.yml" + conf = yaml.safe_load(path.read_text()) + conf["rate_limits_share_pct"] = "not a number" + path.write_text(yaml.safe_dump(conf)) + + body = env.client.get("/bot-orchestration/rate-oracle/config").json() + + assert body["rate_limits_share_pct"] == 100.0 + + +def test_share_pct_and_oracle_change_together_in_one_write(env): + resp = env.client.put( + "/bot-orchestration/rate-oracle/config", + json={"rate_oracle_source": {"name": "binance"}, "rate_limits_share_pct": 50}, + ) + + assert resp.status_code == 200 + conf = _conf(env, "master_account") + assert conf["rate_oracle_source"]["name"] == "binance" + assert conf["rate_limits_share_pct"] == 50.0 + assert "rate_limits_share_pct" in resp.json()["message"] + + +def test_share_pct_is_scoped_to_the_account_it_was_sent_for(env): + env.client.put("/bot-orchestration/rate-oracle/config", params={"account_name": "other"}, + json={"rate_limits_share_pct": 25}) + + assert _conf(env, "other")["rate_limits_share_pct"] == 25.0 + assert "rate_limits_share_pct" not in _conf(env, "master_account") From ea7f5a0c5ead511979c824c2212cb1f9a12b144f Mon Sep 17 00:00:00 2001 From: cardosofede Date: Fri, 18 Sep 2026 07:39:27 +0300 Subject: [PATCH 07/13] feat(system): let the API replace its own container with the published image Upgrading a hummingbot-api server means ssh-ing to the host and running `docker compose pull && up -d`. Condor's updater only knows how to do that in a local checkout, so every remote server is a manual job and servers drift -- which is how a box ends up months behind with nobody able to say so. Three routes, off the same /system prefix that already reports what a server runs: GET /system/upgrade/preflight can this server upgrade itself, and what would it cost POST /system/upgrade pull the image and hand the recreate to a helper GET /system/upgrade/status follow the run, including across the restart it causes A process cannot recreate its own container: `compose up` stops the old one, killing the process running compose halfway through, and the image has no compose plugin anyway. So the API pulls in-process -- what it already does for every other image -- and starts a one-shot docker:cli container with the socket and the compose working dir bound at the same host path, running the `docker compose -p -f up -d --no-deps hummingbot-api` the operator would have typed. That helper outlives the API it replaces, and is also the run record: its exit code and log tail are what the *new* container reads on boot, so there is no state file and no table. The working dir is bound at the identical path because that is what makes ./bots, .env and the override paths inside those compose files resolve to the same things they resolved to when the stack was brought up. Everything here defaults to refusing, because the failure mode is a trading API server that does not come back. preflight starts blocked and clears that in exactly one place, once every fact it needs has been established; a fact it could not establish is treated the same as a fact that says no. An unreachable daemon, a container it cannot place, a stack with no compose labels, a registry digest it could not read, an executor count it could not take, a helper listing the daemon refused -- all of them are can_upgrade false with a reason, never a 500 and never an optimistic default. start re-runs the whole preflight rather than trusting the caller's earlier read, because between the dialog opening and the button being pressed an executor can start or another operator can begin a run, and it claims the run under the same lock it checked, so two callers cannot both pass and race two `compose up`s at one container. A pinned deployment is refused outright and named: an image built on the box, or a compose override file that can repoint `image:`. Removing an override or swapping a hand-built tag is a decision about the operator's own host -- moneymaker's pr226 pin is exactly this -- and silently rolling such a server onto the published build is worse than not upgrading. Those stay an SSH job, and the reason says which file. The consent is explicit and withheld by default: a restart closes every RUNNING executor as SYSTEM_CLEANUP and they are not restored, so acknowledge_executor_loss defaults to False and a body-less POST is refused with a 409 rather than served. Bot containers are separate and keep running, which preflight reports so the operator can see it. Both images are pulled before anything is stopped, the helper's included -- leaving that one to containers.run's implicit pull would put the failure after the recreate was already committed. A pull that fails leaves the server untouched and running, and reports the image that failed rather than the daemon's own string, which carries the socket URL and the negotiated API version. utils/compose.py is the three container/pinning helpers lifted out of routers/system.py verbatim, because the panel's "is this pinned" and this module's "refuse if pinned" have to be the same answer. A panel saying not pinned over a path that refuses is a confusing afternoon; a panel saying not pinned over a path that *proceeds* on a hand-built image is a trading API server replaced by a build nobody asked for. Out of scope and stated as such: switching tags or images, editing override files, rebuilding from source, upgrading postgres/emqx/gateway, and rollback. --- deps.py | 6 + main.py | 14 + models/__init__.py | 5 + models/system.py | 21 ++ routers/system.py | 177 +++++------ services/self_upgrade.py | 535 +++++++++++++++++++++++++++++++ test/test_self_upgrade.py | 640 ++++++++++++++++++++++++++++++++++++++ utils/compose.py | 119 +++++++ 8 files changed, 1425 insertions(+), 92 deletions(-) create mode 100644 models/system.py create mode 100644 services/self_upgrade.py create mode 100644 test/test_self_upgrade.py create mode 100644 utils/compose.py diff --git a/deps.py b/deps.py index bb4dc640..9d753edd 100644 --- a/deps.py +++ b/deps.py @@ -15,6 +15,7 @@ from services.gateway_service import GatewayService from services.gateway_swap_service import GatewaySwapService from services.market_data_service import MarketDataService +from services.self_upgrade import SelfUpgradeService from services.trading_history_service import TradingHistoryService from services.trading_service import TradingService from services.unified_connector_service import UnifiedConnectorService @@ -61,6 +62,11 @@ def get_docker_service(request: Request) -> DockerService: return request.app.state.docker_service +def get_self_upgrade_service(request: Request) -> SelfUpgradeService: + """Get SelfUpgradeService from app state.""" + return request.app.state.self_upgrade_service + + def get_gateway_service(request: Request) -> GatewayService: """Get GatewayService from app state.""" return request.app.state.gateway_service diff --git a/main.py b/main.py index ecaee465..2d481e95 100644 --- a/main.py +++ b/main.py @@ -74,6 +74,7 @@ def patched_save_to_yml(yml_path, cm): from services.gateway_service import GatewayService # noqa: E402 from services.gateway_swap_service import GatewaySwapService # noqa: E402 from services.market_data_service import MarketDataService # noqa: E402 +from services.self_upgrade import SelfUpgradeService # noqa: E402 from services.trading_history_service import TradingHistoryService # noqa: E402 from services.trading_service import TradingService # noqa: E402 from services.unified_connector_service import UnifiedConnectorService # noqa: E402 @@ -306,6 +307,18 @@ async def lifespan(app: FastAPI): else: logging.info("Gateway container not running; status monitor deferred until it is started") + # An upgrade started by the *previous* API process outlives it: the helper container + # that ran `docker compose up` is still on the box with its exit code and logs, and + # this process is the new container it created. Collect that record now, before + # anything can ask for it, and remove the helper so the next preflight is not blocked + # by its own predecessor. Never raises (FEAT-122). + self_upgrade_service = SelfUpgradeService( + docker_service=docker_service, + executor_service=executor_service, + bots_orchestrator=bots_orchestrator, + ) + self_upgrade_service.collect_on_boot() + bot_archiver = BotArchiver( settings.aws.api_key, settings.aws.secret_key, @@ -355,6 +368,7 @@ async def lifespan(app: FastAPI): app.state.backtesting_service = backtesting_service app.state.bots_orchestrator = bots_orchestrator app.state.docker_service = docker_service + app.state.self_upgrade_service = self_upgrade_service app.state.gateway_service = gateway_service app.state.bot_archiver = bot_archiver diff --git a/models/__init__.py b/models/__init__.py index da3dae61..1b637590 100644 --- a/models/__init__.py +++ b/models/__init__.py @@ -181,6 +181,9 @@ # Script models from .scripts import Script, ScriptConfig, ScriptConfigResponse, ScriptResponse +# System models +from .system import SelfUpgradeRequest + # Trading models from .trading import ( AccountBalance, @@ -382,4 +385,6 @@ "ExecutorsSummaryResponse", "OrphanedPositionRecord", "OrphanedPositionsResponse", + # System models + "SelfUpgradeRequest", ] diff --git a/models/system.py b/models/system.py new file mode 100644 index 00000000..fffe4eb4 --- /dev/null +++ b/models/system.py @@ -0,0 +1,21 @@ +"""System router models (FEAT-122).""" + +from pydantic import BaseModel, Field + + +class SelfUpgradeRequest(BaseModel): + """Consent to what a self-upgrade destroys. + + Restarting the API closes every RUNNING executor as SYSTEM_CLEANUP and they are not + restored, so the one field here is the acknowledgement of that -- and it defaults to + False, which means a caller that sends an empty body is refused rather than served. + Bot containers are separate and keep running. + """ + + acknowledge_executor_loss: bool = Field( + default=False, + description=( + "Confirms that every running executor will be closed as SYSTEM_CLEANUP and not " + "restored. Required when the server has any running executor." + ), + ) diff --git a/routers/system.py b/routers/system.py index 1ed3f666..a0f16dc1 100644 --- a/routers/system.py +++ b/routers/system.py @@ -1,30 +1,23 @@ import logging import os -import socket from importlib import metadata +from typing import Optional import psutil -from fastapi import APIRouter, Depends +from fastapi import APIRouter, Depends, HTTPException from config import settings -from deps import get_docker_service +from deps import get_docker_service, get_self_upgrade_service +from models import SelfUpgradeRequest from services.docker_service import DockerService +from services.self_upgrade import SelfUpgradeRefused, SelfUpgradeService +from utils.compose import describe_container, own_container, pinning from version import VERSION logger = logging.getLogger(__name__) router = APIRouter(tags=["System"], prefix="/system") -# Compose service this API is deployed as, used to find our own container when the -# hostname lookup fails (see _own_container). -COMPOSE_SERVICE = "hummingbot-api" - -# Compose stamps these on every container it creates; they say which project the API -# belongs to and which files were used to bring it up. -LABEL_PROJECT = "com.docker.compose.project" -LABEL_WORKING_DIR = "com.docker.compose.project.working_dir" -LABEL_CONFIG_FILES = "com.docker.compose.project.config_files" - # When running inside Docker, mount the host's /proc and root filesystem into the # container and point these env vars at the mount locations so psutil reports the # HOST machine's metrics rather than the container's. They default to the local @@ -95,82 +88,6 @@ def _hummingbot_version(): return None -def _own_container(client): - """Find the container this API process runs in. - - Docker sets a container's hostname to its own short id, which is how a process - identifies itself from the inside. That fails when the deployment overrides - `hostname:` or when the API runs from source on the host, so fall back to the one - container carrying this project's compose service label. - - Returns: - A docker-py Container, or None when this process cannot be placed. - """ - try: - return client.containers.get(socket.gethostname()) - except Exception: - pass - try: - matches = client.containers.list(filters={"label": f"com.docker.compose.service={COMPOSE_SERVICE}"}) - except Exception: - return None - return matches[0] if matches else None - - -def _describe_container(container): - """Identity, image and compose provenance of a container. - - Returns: - Dictionary with the container's id and name, the image reference it was started - from, the image id, its registry digest (None for a locally built image) and the - three compose labels that say where the deployment lives. - """ - labels = container.labels or {} - image = container.image - # A locally built image has no RepoDigests: nothing published it, so there is no - # content address to compare against the tag it carries. - digests = (image.attrs.get("RepoDigests") or []) if image is not None else [] - return { - "id": container.id, - "name": container.name, - "image": container.attrs.get("Config", {}).get("Image"), - "image_id": image.id if image is not None else None, - "digest": digests[0] if digests else None, - "compose_project": labels.get(LABEL_PROJECT), - "compose_working_dir": labels.get(LABEL_WORKING_DIR), - "compose_config_files": labels.get(LABEL_CONFIG_FILES), - } - - -def _pinning(container): - """Whether this deployment runs something other than the published image. - - Two things make an image not the one `hummingbot/hummingbot-api:latest` currently - resolves to: it was built on the box (no registry digest), or compose was brought up - with an override file that can repoint `image:` at a pinned tag. Only files whose - name contains "override" count -- `docker-compose.tailscale.yml` is an overlay for - networking, not for the image. - - Returns: - Dictionary with pinned, a short pinned_reason and the override file name (None - when that is not the reason). - """ - files = [f.strip() for f in (container.get("compose_config_files") or "").split(",") if f.strip()] - override_file = next((os.path.basename(f) for f in files if "override" in os.path.basename(f).lower()), None) - - reasons = [] - if not container.get("digest"): - reasons.append("image was built locally (no registry digest)") - if override_file: - reasons.append(f"compose override file {override_file} can pin the image") - - return { - "pinned": bool(reasons), - "pinned_reason": "; ".join(reasons) or None, - "override_file": override_file, - } - - @router.get("/info") async def get_system_info(docker_service: DockerService = Depends(get_docker_service)): """ @@ -216,16 +133,92 @@ async def get_system_info(docker_service: DockerService = Depends(get_docker_ser return info info["docker_available"] = True - container = _own_container(client) + container = own_container(client) if container is None: logger.warning("Could not identify the API's own container while reporting system info") return info try: - info["container"] = _describe_container(container) + info["container"] = describe_container(container) except Exception as e: logger.warning(f"Could not inspect the API's own container while reporting system info: {e}") return info - info.update(_pinning(info["container"])) + info.update(pinning(info["container"])) return info + + +# ── Self-upgrade (FEAT-122) ────────────────────────────────────────────────────── +# +# Replacing this API's own container with the published image, so a remote server can +# be upgraded from a dashboard instead of over SSH. See services/self_upgrade.py for +# why a helper container does the recreate, and for the rule these three routes exist +# to enforce: anything not established is a refusal. + + +@router.get("/upgrade/preflight") +async def get_upgrade_preflight(upgrade_service: SelfUpgradeService = Depends(get_self_upgrade_service)): + """ + Report whether this API can replace its own container, and what it would cost. + + Read-only and never raises: a daemon that cannot be reached, a container that cannot + be placed and a registry that cannot be read are all reported as can_upgrade false + with a blocked_reason, because the caller of a destructive action needs a reason far + more than it needs a 500. + + Returns: + Dictionary with image_ref, current_digest, available_digest, up_to_date, the + pinning verdict (pinned / pinned_reason / override_file), the compose project, + working dir and config files, running_executors and running_bots, and + can_upgrade with its blocked_reason (None only when the upgrade may be started). + """ + return await upgrade_service.preflight() + + +@router.post("/upgrade", status_code=202) +async def start_upgrade( + request: Optional[SelfUpgradeRequest] = None, + upgrade_service: SelfUpgradeService = Depends(get_self_upgrade_service), +): + """ + Pull the published image and hand the container recreate to a helper container. + + The preflight is re-run here, so a server that became unupgradable since the caller + last looked -- an executor started, another upgrade began, the daemon went away -- + is refused rather than upgraded on a stale reading. + + Args: + request: acknowledge_executor_loss, the caller's consent to every running + executor being closed as SYSTEM_CLEANUP. Required when any is running. + + Returns: + 202 with the run id and the initial phase. The API is replaced part-way through, + so the result is read back from /system/upgrade/status after it restarts. + + Raises: + HTTPException: 409 when the upgrade is refused. Nothing was pulled, and nothing + on the server was changed. + """ + # An absent body is a caller who acknowledged nothing, which is the refusing default. + request = request or SelfUpgradeRequest() + try: + return await upgrade_service.start(acknowledge_executor_loss=request.acknowledge_executor_loss) + except SelfUpgradeRefused as e: + raise HTTPException(status_code=409, detail=str(e)) + + +@router.get("/upgrade/status") +async def get_upgrade_status(upgrade_service: SelfUpgradeService = Depends(get_self_upgrade_service)): + """ + Report the current or last self-upgrade. + + Phases: idle (there has been none), pulling, recreating (this API is about to be + replaced, so the next answer comes from the new one), done, failed. After a + successful recreate the record is the one this API collected from the helper + container on boot, including its exit code and the tail of its output. + + Returns: + Dictionary with run_id, phase, detail, previous_digest, new_digest, exit_code + and log_tail. + """ + return upgrade_service.status() diff --git a/services/self_upgrade.py b/services/self_upgrade.py new file mode 100644 index 00000000..71237ae4 --- /dev/null +++ b/services/self_upgrade.py @@ -0,0 +1,535 @@ +"""Upgrade this API's own container to the published image (FEAT-122). + +Today upgrading a hummingbot-api server means ssh-ing to the host and running +``docker compose pull && docker compose up -d``. Condor's own updater only knows how to +do that in a *local* checkout, so every remote server is a manual job and servers drift. + +**Why a helper container.** A process cannot recreate its own container: ``compose up`` +stops the old container, which kills the process running compose halfway through. So the +API pulls the image in-process -- the thing it already does for every other image -- and +then hands the recreate to a one-shot ``docker:cli`` container that survives the API's +restart. That helper is also the run record: its exit code and logs say what happened, +and the *new* API collects them on boot. There is no state file and no DB table. + +**Everything here defaults to refusing.** The failure mode of this module is a trading +API server that does not come back, so ``preflight`` starts from "blocked" and only +clears that at the very end, once every fact it needs has actually been established. +Not established is the same as blocked: an unreachable Docker daemon, a container it +cannot place, missing compose labels, a registry it cannot read, an executor count it +cannot take. None of those are "probably fine". + +**What it will not do.** It never upgrades a *pinned* deployment -- an image built on the +box, or a compose override file that can repoint ``image:``. Removing an override or +swapping a hand-built tag is an operator's decision about their own host, and silently +rolling such a server onto the published build is the one outcome worse than not +upgrading at all. Those servers stay an SSH job and the reason names the override file. + +Out of scope, deliberately: switching tags or images, editing override files, rebuilding +from source, upgrading postgres/emqx/gateway, and rollback. +""" + +import logging +import threading +import time +import uuid +from typing import Any, Dict, Optional + +from utils.compose import config_files, describe_container, own_container, pinning + +logger = logging.getLogger(__name__) + +# Ships the compose plugin. Pinned to a major tag rather than :latest so the helper that +# recreates this API is not itself a moving target. +HELPER_IMAGE = "docker:27-cli" + +# Carries the run id, and is how a helper -- running or exited -- is found again after +# the API that started it has been replaced. +HELPER_LABEL = "hummingbot-api.self-upgrade" +HELPER_PREVIOUS_DIGEST_LABEL = "hummingbot-api.self-upgrade.previous-digest" + +# The compose service to recreate. Only this one: postgres, emqx, any tailscale sidecar +# and every bot container are left exactly as they are. +SERVICE_NAME = "hummingbot-api" + +# Lines of the helper's output kept as the run record. +LOG_TAIL_LINES = 40 + + +class SelfUpgradeRefused(Exception): + """This upgrade will not be started, and nothing has been changed. + + Raised before any pull and before any container is touched, so a caller that sees it + knows the server is exactly as it was. The router answers 409. + """ + + +def _sha(digest: Optional[str]) -> Optional[str]: + """The ``sha256:...`` part of a digest, however it was spelled. + + A container's ``RepoDigests`` entry is ``repo@sha256:...`` while the registry answers + with the bare ``sha256:...``; comparing the two as written would make every server + look out of date forever. + """ + if not digest: + return None + return digest.split("@")[-1].strip() or None + + +class SelfUpgradeService: + """Preflight, start and report on replacing this API's container. + + Args: + docker_service: The API's ``DockerService``; its ``client`` is the daemon handle + and its ``pull_image_sync`` is the pull already used everywhere else. + executor_service: Source of the running-executor count, the number that decides + whether this upgrade destroys anything. + bots_orchestrator: Source of the running-bot count, which is reported so the + operator can see that bots are *not* affected. + """ + + def __init__(self, docker_service, executor_service=None, bots_orchestrator=None): + self.docker_service = docker_service + self.executor_service = executor_service + self.bots_orchestrator = bots_orchestrator + self._lock = threading.Lock() + self._run: Optional[Dict[str, Any]] = None + + # ── Facts ──────────────────────────────────────────────────────────────────── + + @property + def client(self): + """The Docker daemon handle, or None when it was never established.""" + return getattr(self.docker_service, "client", None) + + def _daemon(self): + """The daemon handle if it answers a ping, else None. Never raises.""" + client = self.client + if client is None: + return None + try: + client.ping() + except Exception as e: + logger.warning(f"Docker daemon unreachable while preparing a self-upgrade: {e}") + return None + return client + + def _registry_digest(self, image_ref: str) -> Optional[str]: + """The digest the registry currently serves for ``image_ref``, or None. + + None is a *blocking* answer, not an optimistic one: without it there is no + evidence that a newer image exists, and pulling a tag on that basis is how a + server gets recreated for nothing. + """ + client = self.client + try: + return _sha(client.images.get_registry_data(image_ref).id) + except Exception as e: + logger.warning(f"Could not read the registry digest for {image_ref}: {e}") + return None + + def _existing_helper(self, client): + """``(established, helper)`` for a helper container already on this box. + + ``established`` is False when the daemon would not answer the question at all, + which blocks: an upgrade started while another one is mid-recreate is two + ``compose up`` runs racing for the same container, and "I could not check" is not + a reason to start the second one. + """ + try: + found = client.containers.list(all=True, filters={"label": HELPER_LABEL}) + except Exception as e: + logger.warning(f"Could not list self-upgrade helper containers: {e}") + return False, None + return True, (found[0] if found else None) + + async def _running_executors(self) -> Optional[int]: + """How many executors are live, or None when that could not be established.""" + if self.executor_service is None: + return None + try: + return int(self.executor_service.get_summary().get("total_active", 0)) + except Exception as e: + logger.warning(f"Could not count running executors before a self-upgrade: {e}") + return None + + async def _running_bots(self) -> Optional[int]: + """How many bot containers are running, or None when that is unknown.""" + if self.bots_orchestrator is None: + return None + try: + return len(await self.bots_orchestrator.get_active_containers()) + except Exception as e: + logger.warning(f"Could not count running bots before a self-upgrade: {e}") + return None + + # ── Preflight ──────────────────────────────────────────────────────────────── + + async def preflight(self) -> Dict[str, Any]: + """Everything the caller needs to decide, and whether it may proceed at all. + + Returns: + Dictionary with the image reference and its current/available digests, + ``up_to_date``, the pinning verdict, the compose coordinates, the counts of + running executors and bots, and ``can_upgrade`` with a ``blocked_reason`` + that is None only when the upgrade may actually be started. + + Never raises: this is what a dashboard polls, and a daemon hiccup has to render + as a refusal with a reason, not as a 500. + """ + info: Dict[str, Any] = { + "service": SERVICE_NAME, + "helper_image": HELPER_IMAGE, + "image_ref": None, + "current_digest": None, + "available_digest": None, + "up_to_date": None, + "pinned": None, + "pinned_reason": None, + "override_file": None, + "compose": None, + "running_executors": None, + "running_bots": None, + "can_upgrade": False, + # Refusing is the default. Every branch below either replaces this reason + # with a more specific one or falls through to the single place that clears + # it, so a path that forgets to decide still refuses. + "blocked_reason": "This server has not established that it can upgrade itself.", + } + + client = self._daemon() + if client is None: + info["blocked_reason"] = ( + "The Docker daemon is not reachable from this API, so it cannot upgrade itself. " + "Check that /var/run/docker.sock is mounted into the container." + ) + return info + + container = own_container(client) + if container is None: + info["blocked_reason"] = ( + "This API could not identify its own container, so it does not know what to recreate. " + "Upgrade this server over SSH." + ) + return info + + try: + described = describe_container(container) + except Exception as e: + logger.warning(f"Could not inspect the API's own container before a self-upgrade: {e}") + info["blocked_reason"] = ( + "This API could not inspect its own container, so it does not know what to recreate. " + "Upgrade this server over SSH." + ) + return info + + info["image_ref"] = described.get("image") + info["current_digest"] = described.get("digest") + info.update(pinning(described)) + + project = described.get("compose_project") + working_dir = described.get("compose_working_dir") + files = config_files(described) + if project and working_dir and files: + info["compose"] = {"project": project, "working_dir": working_dir, "config_files": files} + + # Counts first: they are reported even on a blocked preflight, because "you + # cannot upgrade here" and "17 executors are running" are both things the + # operator came to find out. + info["running_executors"] = await self._running_executors() + info["running_bots"] = await self._running_bots() + + if info["compose"] is None: + info["blocked_reason"] = ( + "This API was not started by Docker Compose (no compose labels on its container), " + "so there is no compose project to recreate it from. Upgrade this server over SSH." + ) + return info + + if info["pinned"]: + info["blocked_reason"] = ( + f"This server's image is pinned: {info['pinned_reason']}. " + "Pulling the published image would undo that, so it is refused. " + "Upgrade this server over SSH." + ) + return info + + if not info["image_ref"]: + info["blocked_reason"] = ( + "This API's container does not report the image it was started from, " + "so there is nothing to compare against the registry." + ) + return info + + available = self._registry_digest(info["image_ref"]) + info["available_digest"] = available + if available is None: + info["blocked_reason"] = ( + f"Could not read the published digest for {info['image_ref']} from the registry, " + "so there is no evidence that a newer image exists. Nothing was pulled." + ) + return info + + info["up_to_date"] = _sha(info["current_digest"]) == available + if info["up_to_date"]: + info["blocked_reason"] = f"Already running the published {info['image_ref']}." + return info + + if info["running_executors"] is None: + info["blocked_reason"] = ( + "Could not determine how many executors are running, and a restart closes every " + "running executor as SYSTEM_CLEANUP. Refusing rather than guessing." + ) + return info + + established, helper = self._existing_helper(client) + if not established: + info["blocked_reason"] = ( + "Could not check whether an upgrade is already running on this server. " + "Refusing rather than risking two recreates of the same container." + ) + return info + if helper is not None: + info["blocked_reason"] = ( + f"An upgrade helper container is already present ({helper.name}). " + "Wait for it to finish; it is cleared when this API next starts." + ) + return info + + with self._lock: + phase = (self._run or {}).get("phase") + if phase in ("pulling", "recreating"): + info["blocked_reason"] = "An upgrade is already running on this server." + return info + + # The one place the refusal is lifted. + info["can_upgrade"] = True + info["blocked_reason"] = None + return info + + # ── Start ──────────────────────────────────────────────────────────────────── + + async def start(self, acknowledge_executor_loss: bool = False) -> Dict[str, Any]: + """Pull the published image and hand the recreate to a helper container. + + The preflight is re-run here rather than trusted from the caller's earlier read: + between the dialog opening and the button being pressed, an executor can start, + another operator can begin an upgrade, or the daemon can go away. + + Args: + acknowledge_executor_loss: The caller has been told that every running + executor will be closed as ``SYSTEM_CLEANUP`` and is not coming back. + + Returns: + ``{"run_id": ..., "phase": "pulling"}``. + + Raises: + SelfUpgradeRefused: Nothing was pulled and nothing was touched. + """ + pre = await self.preflight() + if pre["blocked_reason"]: + raise SelfUpgradeRefused(pre["blocked_reason"]) + + running = pre["running_executors"] or 0 + if running > 0 and not acknowledge_executor_loss: + raise SelfUpgradeRefused( + f"{running} executor(s) are running. Restarting this API closes every one of them as " + "SYSTEM_CLEANUP and they are not restored. Confirm that loss to proceed." + ) + + run_id = uuid.uuid4().hex[:12] + compose = pre["compose"] + with self._lock: + # Re-checked under the lock: two callers can both clear the preflight before + # either has claimed the run, and the loser would start a second `compose up` + # against the container the first is already replacing. + in_flight = (self._run or {}).get("phase") + if in_flight in ("pulling", "recreating"): + raise SelfUpgradeRefused("An upgrade is already running on this server.") + self._run = { + "run_id": run_id, + "phase": "pulling", + "detail": f"Pulling {pre['image_ref']}", + "image_ref": pre["image_ref"], + "previous_digest": pre["current_digest"], + "new_digest": None, + "exit_code": None, + "log_tail": [], + "started_at": time.time(), + } + + threading.Thread( + target=self._pull_then_recreate, + args=(run_id, pre["image_ref"], compose, pre["current_digest"]), + daemon=True, + name=f"self-upgrade-{run_id}", + ).start() + + return {"run_id": run_id, "phase": "pulling"} + + def _set(self, run_id: str, **fields) -> None: + """Update the current run, ignoring a write for a run that has been replaced.""" + with self._lock: + if not self._run or self._run.get("run_id") != run_id: + return + self._run.update(fields) + + def _pull_then_recreate(self, run_id, image_ref, compose, previous_digest) -> None: + """The whole upgrade, off the event loop. Never raises out of the thread. + + Order matters and is the safety property: both images are on the box *before* + anything is stopped. A pull that fails leaves the server untouched and running, + which is why the helper image is pulled here too rather than being left to + ``containers.run``'s implicit pull -- that one would happen with the recreate + already committed. + """ + for ref in (image_ref, HELPER_IMAGE): + self._set(run_id, phase="pulling", detail=f"Pulling {ref}") + result = self.docker_service.pull_image_sync(ref) + if not (isinstance(result, dict) and result.get("success")): + # The daemon's own string carries the socket URL and API version, so it + # goes to the log; the caller gets the image that failed. + logger.error(f"Self-upgrade {run_id}: pull of {ref} failed: {result}") + self._set( + run_id, + phase="failed", + detail=f"Could not pull {ref}. Nothing was changed; see the API log for the daemon's error.", + finished_at=time.time(), + ) + return + + command = ["docker", "compose", "-p", compose["project"]] + for path in compose["config_files"]: + command += ["-f", path] + command += ["up", "-d", "--no-deps", SERVICE_NAME] + + working_dir = compose["working_dir"] + try: + helper = self.client.containers.run( + HELPER_IMAGE, + command=command, + detach=True, + labels={ + HELPER_LABEL: run_id, + HELPER_PREVIOUS_DIGEST_LABEL: previous_digest or "", + }, + volumes={ + "/var/run/docker.sock": {"bind": "/var/run/docker.sock", "mode": "rw"}, + # Bound at the *same* host path so the relative paths inside the + # compose files -- ./bots, .env, the override file -- resolve to the + # same things they resolved to when the operator brought the stack up. + working_dir: {"bind": working_dir, "mode": "rw"}, + }, + working_dir=working_dir, + # auto_remove would delete the run record along with the container, and + # the record is the only thing that survives this API being replaced. + auto_remove=False, + ) + except Exception as e: + logger.error(f"Self-upgrade {run_id}: could not start the helper container: {e}") + self._set( + run_id, + phase="failed", + detail="Could not start the upgrade helper container. Nothing was changed.", + finished_at=time.time(), + ) + return + + logger.info(f"Self-upgrade {run_id}: helper {helper.name} running `{' '.join(command)}`") + self._set( + run_id, + phase="recreating", + detail=( + "Recreating the hummingbot-api container. This API is about to be replaced; " + "the new one reports the result." + ), + helper=getattr(helper, "name", None), + ) + + # ── Report ─────────────────────────────────────────────────────────────────── + + def status(self) -> Dict[str, Any]: + """The current or last run, or an idle record when there has been none.""" + with self._lock: + if self._run is None: + return {"run_id": None, "phase": "idle", "detail": None, "log_tail": []} + return dict(self._run) + + def collect_on_boot(self) -> None: + """Adopt the outcome of a helper that outlived the API that started it. + + This is where an upgrade actually finishes: the process that started it is gone, + and the record of what happened is the exited helper container. Its exit code and + log tail become this API's last run, then it is removed so the next preflight is + not blocked by its own predecessor. + + A helper still running is left alone -- it may be mid-recreate, and preflight + blocks on it. Never raises: a failure to tidy up must not stop the API booting. + """ + client = self._daemon() + if client is None: + return + try: + helpers = client.containers.list(all=True, filters={"label": HELPER_LABEL}) + except Exception as e: + logger.warning(f"Could not look for self-upgrade helper containers on boot: {e}") + return + + for helper in helpers: + # A helper that has not exited is very likely running `docker compose up` + # right now. Removing it would kill the recreate half-way, which is the one + # thing this whole module is built to avoid, so it is left alone -- and + # preflight blocks on it until it finishes. + if getattr(helper, "status", None) not in ("exited", "dead"): + logger.info(f"Self-upgrade helper {helper.name} is still {helper.status}; leaving it.") + continue + try: + labels = helper.labels or {} + run_id = labels.get(HELPER_LABEL) + previous_digest = labels.get(HELPER_PREVIOUS_DIGEST_LABEL) or None + exit_code = (helper.attrs.get("State") or {}).get("ExitCode") + try: + raw = helper.logs(tail=LOG_TAIL_LINES) + text = raw.decode("utf-8", "replace") if isinstance(raw, bytes) else str(raw) + log_tail = [line for line in text.splitlines() if line.strip()] + except Exception as e: + logger.warning(f"Could not read logs of self-upgrade helper {helper.name}: {e}") + log_tail = [] + + succeeded = exit_code == 0 + with self._lock: + self._run = { + "run_id": run_id, + "phase": "done" if succeeded else "failed", + "detail": ( + "Upgrade finished; this API is the new container." + if succeeded + else f"The recreate failed (exit code {exit_code}). " + "The previous container may still be running; check " + "`docker compose logs hummingbot-api` on the host." + ), + "image_ref": None, + "previous_digest": previous_digest, + "new_digest": self._current_digest(client), + "exit_code": exit_code, + "log_tail": log_tail, + "finished_at": time.time(), + } + logger.info(f"Collected self-upgrade helper {helper.name} (exit {exit_code})") + except Exception as e: + logger.warning(f"Could not read self-upgrade helper {getattr(helper, 'name', '?')}: {e}") + finally: + try: + helper.remove(force=True) + except Exception as e: + logger.warning(f"Could not remove self-upgrade helper {getattr(helper, 'name', '?')}: {e}") + + def _current_digest(self, client) -> Optional[str]: + """The digest this API is running right now, or None when it cannot be read.""" + try: + container = own_container(client) + if container is None: + return None + return describe_container(container).get("digest") + except Exception as e: + logger.warning(f"Could not read this API's own digest after an upgrade: {e}") + return None diff --git a/test/test_self_upgrade.py b/test/test_self_upgrade.py new file mode 100644 index 00000000..efc25b2b --- /dev/null +++ b/test/test_self_upgrade.py @@ -0,0 +1,640 @@ +"""The API replacing its own container (FEAT-122). + +The failure mode of this feature is a trading API server that does not come back, so +almost everything here is a refusal. `preflight` starts blocked and only clears that once +every fact it needs is established, and `start` re-runs it, so the tests worth having are +the ones that prove nothing happens: + +* an unreachable daemon, a container it cannot place, a stack with no compose labels +* a *pinned* image -- built on the box, or an override file that can repoint `image:` -- + which is the moneymaker case and must name the override file +* a registry it cannot read, which is "no evidence a newer image exists", not "go ahead" +* an executor count it cannot take, because a restart closes every running executor +* running executors without the caller acknowledging that loss +* a pull that fails, which must leave the server untouched and never start the helper + +The happy path is here too, and what it asserts is the exact `docker compose` the helper +is given: the project, every config file in the order compose was given them, `--no-deps` +so postgres/emqx/bots are not touched, and the working dir bound at the *same* host path +so `./bots`, `.env` and the override file resolve to what the operator actually has. + +Run with: pytest test/test_self_upgrade.py -v +""" +import threading +from types import SimpleNamespace + +import pytest +from docker.errors import DockerException, NotFound +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from services.self_upgrade import ( + HELPER_IMAGE, + HELPER_LABEL, + HELPER_PREVIOUS_DIGEST_LABEL, + SERVICE_NAME, + SelfUpgradeRefused, + SelfUpgradeService, +) + +CONTAINER_ID = "3f2a1b0c9d8e7f6a5b4c3d2e1f0a9b8c7d6e5f4a3b2c1d0e9f8a7b6c5d4e3f2a" # noqa: mock (a Docker container id, not a key) +WORKING_DIR = "/home/hbot/hummingbot-api" +IMAGE_REF = "hummingbot/hummingbot-api:latest" +CURRENT_SHA = "sha256:1111111111111111111111111111111111111111111111111111111111111111" # noqa: mock +NEWER_SHA = "sha256:2222222222222222222222222222222222222222222222222222222222222222" # noqa: mock +COMPOSE_FILES = f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.tailscale.yml" + + +# ── Doubles ────────────────────────────────────────────────────────────────────── + + +def _container(digests=(f"hummingbot/hummingbot-api@{CURRENT_SHA}",), config_files=COMPOSE_FILES): + """A docker-py Container as utils/compose.py reads it.""" + labels = { + "com.docker.compose.project": "hummingbot-api", + "com.docker.compose.project.working_dir": WORKING_DIR, + "com.docker.compose.service": SERVICE_NAME, + } + if config_files is not None: + labels["com.docker.compose.project.config_files"] = config_files + return SimpleNamespace( + id=CONTAINER_ID, + name="hummingbot-api", + labels=labels, + attrs={"Config": {"Image": IMAGE_REF}}, + image=SimpleNamespace( + id="sha256:deadbeef", # noqa: mock + tags=[IMAGE_REF], + attrs={"RepoDigests": list(digests)}, + ), + ) + + +def _helper(status="exited", exit_code=0, run_id="run123", logs=b"Container hummingbot-api Started\n"): + removed = [] + return SimpleNamespace( + name=f"helper-{run_id}", + status=status, + labels={HELPER_LABEL: run_id, HELPER_PREVIOUS_DIGEST_LABEL: f"hummingbot/hummingbot-api@{CURRENT_SHA}"}, + attrs={"State": {"ExitCode": exit_code}}, + logs=lambda tail=None: logs, + remove=lambda force=False: removed.append(force), + removed=removed, + ) + + +class _Containers: + def __init__(self, container, helpers=()): + self.container = container + self.helpers = list(helpers) + self.runs = [] + + def get(self, name): + if self.container is None: + raise NotFound(f"no such container: {name}") + return self.container + + def list(self, all=False, filters=None): + label = (filters or {}).get("label", "") + if label.startswith(HELPER_LABEL): + return list(self.helpers) + # The compose-service fallback own_container() uses. + return [self.container] if self.container is not None else [] + + def run(self, image, **kwargs): + self.runs.append({"image": image, **kwargs}) + return SimpleNamespace(name="helper-new") + + +class _Images: + def __init__(self, registry_sha=NEWER_SHA): + self.registry_sha = registry_sha + + def get_registry_data(self, ref): + if self.registry_sha is None: + raise DockerException(f"error while looking up {ref}") + return SimpleNamespace(id=self.registry_sha) + + +class _Client: + def __init__(self, container=None, helpers=(), registry_sha=NEWER_SHA, reachable=True): + self.containers = _Containers(container, helpers) + self.images = _Images(registry_sha) + self.reachable = reachable + + def ping(self): + if not self.reachable: + raise DockerException("Error while fetching server API version") + return True + + +class _DockerService: + """Just the two things SelfUpgradeService uses off the real one.""" + + def __init__(self, client, pull_fails=()): + self.client = client + self.pull_fails = set(pull_fails) + self.pulled = [] + + def pull_image_sync(self, image_name): + self.pulled.append(image_name) + if image_name in self.pull_fails: + return {"success": False, "error": f"500 Server Error for http+docker://localhost/v1.55/images/create: {image_name}"} + return {"success": True, "image": image_name} + + +class _Executors: + def __init__(self, active=0, raises=False): + self.active = active + self.raises = raises + + def get_summary(self): + if self.raises: + raise RuntimeError("executor service is not up") + return {"total_active": self.active} + + +class _Bots: + def __init__(self, names=("bot-a", "bot-b")): + self.names = list(names) + + async def get_active_containers(self): + return list(self.names) + + +def _service(client=None, pull_fails=(), active=0, executor_raises=False, bots=None): + if client is None: + client = _Client(_container()) + return SelfUpgradeService( + docker_service=_DockerService(client, pull_fails), + executor_service=_Executors(active, executor_raises), + bots_orchestrator=_Bots() if bots is None else bots, + ) + + +@pytest.fixture +def run_threads_inline(monkeypatch): + """Run the upgrade's background thread on the calling thread, so tests are ordered.""" + started = [] + + class _Inline: + def __init__(self, target=None, args=(), daemon=None, name=None): + self.target, self.args = target, args + + def start(self): + started.append(self) + self.target(*self.args) + + monkeypatch.setattr(threading, "Thread", _Inline) + return started + + +# ── Preflight refuses ──────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_refuses_when_the_docker_daemon_is_unreachable(): + info = await _service(_Client(_container(), reachable=False)).preflight() + + assert info["can_upgrade"] is False + assert "docker.sock" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_when_the_docker_client_was_never_built(): + service = SelfUpgradeService(docker_service=SimpleNamespace(), executor_service=_Executors()) + + info = await service.preflight() + + assert info["can_upgrade"] is False + assert "Docker daemon is not reachable" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_when_it_cannot_identify_its_own_container(): + info = await _service(_Client(container=None)).preflight() + + assert info["can_upgrade"] is False + assert "could not identify its own container" in info["blocked_reason"] + assert "SSH" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_when_there_are_no_compose_labels(): + """Not a compose deployment: there is no `docker compose up` to replay.""" + info = await _service(_Client(_container(config_files=None))).preflight() + + assert info["can_upgrade"] is False + assert info["compose"] is None + assert "not started by Docker Compose" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_a_pinned_image_and_names_the_override_file(): + """The moneymaker case: an override file can repoint `image:` at a pinned tag.""" + files = f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.override.yml" + + info = await _service(_Client(_container(config_files=files))).preflight() + + assert info["can_upgrade"] is False + assert info["pinned"] is True + assert info["override_file"] == "docker-compose.override.yml" + assert "docker-compose.override.yml" in info["blocked_reason"] + assert "SSH" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_a_locally_built_image(): + """No RepoDigests: nothing published this image, so the published one is not it.""" + info = await _service(_Client(_container(digests=()))).preflight() + + assert info["can_upgrade"] is False + assert info["pinned"] is True + assert "built locally" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_when_the_registry_digest_cannot_be_read(): + """No evidence a newer image exists is not the same as "go ahead".""" + service = _service(_Client(_container(), registry_sha=None)) + + info = await service.preflight() + + assert info["can_upgrade"] is False + assert info["available_digest"] is None + assert "no evidence that a newer image exists" in info["blocked_reason"] + assert service.docker_service.pulled == [] + + +@pytest.mark.asyncio +async def test_refuses_when_already_running_the_published_image(): + info = await _service(_Client(_container(), registry_sha=CURRENT_SHA)).preflight() + + assert info["can_upgrade"] is False + assert info["up_to_date"] is True + assert "Already running" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_when_the_executor_count_cannot_be_taken(): + """A restart closes every running executor; not knowing how many is not "none".""" + info = await _service(executor_raises=True).preflight() + + assert info["can_upgrade"] is False + assert info["running_executors"] is None + assert "Refusing rather than guessing" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_while_a_helper_container_is_present(): + info = await _service(_Client(_container(), helpers=[_helper(status="running")])).preflight() + + assert info["can_upgrade"] is False + assert "already present" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_refuses_when_it_cannot_check_for_an_existing_helper(): + """Two `compose up` runs racing for the same container is the worst outcome here.""" + service = _service() + + def _boom(all=False, filters=None): + raise DockerException("daemon said no") + + service.docker_service.client.containers.list = _boom + + info = await service.preflight() + + assert info["can_upgrade"] is False + assert "Refusing rather than risking two recreates" in info["blocked_reason"] + + +@pytest.mark.asyncio +async def test_a_second_start_is_refused_while_the_first_is_in_flight(run_threads_inline): + service = _service() + + await service.start() + with pytest.raises(SelfUpgradeRefused) as excinfo: + await service.start() + + assert "already running" in str(excinfo.value) + # Still exactly one helper: the second call started nothing. + assert len(service.docker_service.client.containers.runs) == 1 + + +@pytest.mark.asyncio +async def test_refuses_while_a_run_is_in_flight(): + service = _service() + service._run = {"run_id": "abc", "phase": "recreating"} + + info = await service.preflight() + + assert info["can_upgrade"] is False + assert "already running" in info["blocked_reason"] + + +# ── Preflight allows ───────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_allows_an_unpinned_compose_install_with_a_newer_image(): + info = await _service(active=3).preflight() + + assert info["can_upgrade"] is True + assert info["blocked_reason"] is None + assert info["image_ref"] == IMAGE_REF + assert info["current_digest"].endswith(CURRENT_SHA) + assert info["available_digest"] == NEWER_SHA + assert info["up_to_date"] is False + assert info["pinned"] is False + assert info["running_executors"] == 3 + assert info["running_bots"] == 2 + assert info["compose"] == { + "project": "hummingbot-api", + "working_dir": WORKING_DIR, + "config_files": [f"{WORKING_DIR}/docker-compose.yml", f"{WORKING_DIR}/docker-compose.tailscale.yml"], + } + + +@pytest.mark.asyncio +async def test_a_tailscale_overlay_is_not_a_pin(): + """Only a file named *override* can repoint `image:`; an overlay is networking.""" + info = await _service().preflight() + + assert info["pinned"] is False + assert info["override_file"] is None + assert info["can_upgrade"] is True + + +@pytest.mark.asyncio +async def test_counts_are_reported_even_on_a_blocked_preflight(): + """"You cannot upgrade here" and "17 executors are running" are both the answer.""" + files = f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.override.yml" + + info = await _service(_Client(_container(config_files=files)), active=17).preflight() + + assert info["can_upgrade"] is False + assert info["running_executors"] == 17 + assert info["running_bots"] == 2 + + +# ── Start refuses ──────────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_start_refuses_a_blocked_preflight_without_touching_anything(): + service = _service(_Client(_container(), registry_sha=CURRENT_SHA)) + + with pytest.raises(SelfUpgradeRefused) as excinfo: + await service.start(acknowledge_executor_loss=True) + + assert "Already running" in str(excinfo.value) + assert service.docker_service.pulled == [] + assert service.docker_service.client.containers.runs == [] + assert service.status()["phase"] == "idle" + + +@pytest.mark.asyncio +async def test_start_refuses_running_executors_without_acknowledgement(): + service = _service(active=4) + + with pytest.raises(SelfUpgradeRefused) as excinfo: + await service.start() + + assert "4 executor(s) are running" in str(excinfo.value) + assert "SYSTEM_CLEANUP" in str(excinfo.value) + assert service.docker_service.pulled == [] + assert service.docker_service.client.containers.runs == [] + + +@pytest.mark.asyncio +async def test_start_refuses_a_pinned_server_even_when_the_loss_is_acknowledged(): + """Acknowledging the executor loss does not buy past the pin.""" + files = f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.override.yml" + service = _service(_Client(_container(config_files=files)), active=1) + + with pytest.raises(SelfUpgradeRefused) as excinfo: + await service.start(acknowledge_executor_loss=True) + + assert "docker-compose.override.yml" in str(excinfo.value) + assert service.docker_service.client.containers.runs == [] + + +# ── Start proceeds ─────────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_start_pulls_both_images_then_runs_the_helper(run_threads_inline): + service = _service() + + started = await service.start() + + assert started["phase"] == "pulling" + assert started["run_id"] + # Both images on the box before anything is stopped. + assert service.docker_service.pulled == [IMAGE_REF, HELPER_IMAGE] + + run = service.docker_service.client.containers.runs[0] + assert run["image"] == HELPER_IMAGE + assert run["detach"] is True + assert run["command"] == [ + "docker", "compose", "-p", "hummingbot-api", + "-f", f"{WORKING_DIR}/docker-compose.yml", + "-f", f"{WORKING_DIR}/docker-compose.tailscale.yml", + "up", "-d", "--no-deps", SERVICE_NAME, + ] + # Same host path on both sides, or ./bots, .env and the override file resolve to + # nothing inside the helper. + assert run["volumes"][WORKING_DIR]["bind"] == WORKING_DIR + assert run["volumes"]["/var/run/docker.sock"]["bind"] == "/var/run/docker.sock" + assert run["working_dir"] == WORKING_DIR + assert run["labels"][HELPER_LABEL] == started["run_id"] + assert run["labels"][HELPER_PREVIOUS_DIGEST_LABEL].endswith(CURRENT_SHA) + # The record has to outlive the helper; auto_remove would delete it. + assert run["auto_remove"] is False + + status = service.status() + assert status["phase"] == "recreating" + assert status["previous_digest"].endswith(CURRENT_SHA) + + +@pytest.mark.asyncio +async def test_start_proceeds_when_the_executor_loss_is_acknowledged(run_threads_inline): + service = _service(active=2) + + await service.start(acknowledge_executor_loss=True) + + assert len(service.docker_service.client.containers.runs) == 1 + + +@pytest.mark.asyncio +async def test_no_remove_orphans_and_only_the_api_service_is_recreated(run_threads_inline): + """postgres, emqx, any sidecar and every bot container must be left alone.""" + service = _service() + + await service.start() + + command = service.docker_service.client.containers.runs[0]["command"] + assert "--remove-orphans" not in command + assert "--no-deps" in command + assert command[-1] == SERVICE_NAME + + +# ── A pull that fails changes nothing ──────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_a_failed_api_image_pull_never_starts_the_helper(run_threads_inline): + service = _service(pull_fails=[IMAGE_REF]) + + await service.start() + + assert service.docker_service.client.containers.runs == [] + status = service.status() + assert status["phase"] == "failed" + assert "Nothing was changed" in status["detail"] + # The daemon's own string names the socket and API version; it stays in the log. + assert "http+docker" not in status["detail"] + + +@pytest.mark.asyncio +async def test_a_failed_helper_image_pull_never_starts_the_helper(run_threads_inline): + """The helper image is pulled up front precisely so this failure is harmless.""" + service = _service(pull_fails=[HELPER_IMAGE]) + + await service.start() + + assert service.docker_service.client.containers.runs == [] + assert service.status()["phase"] == "failed" + + +@pytest.mark.asyncio +async def test_a_helper_that_cannot_be_started_is_reported_not_raised(run_threads_inline): + service = _service() + + def _boom(image, **kwargs): + raise DockerException("no such image") + + service.docker_service.client.containers.run = _boom + + await service.start() + + status = service.status() + assert status["phase"] == "failed" + assert "Nothing was changed" in status["detail"] + + +# ── Collecting the run on boot ─────────────────────────────────────────────────── + + +def test_collect_on_boot_reports_a_successful_recreate_and_removes_the_helper(): + helper = _helper(exit_code=0) + client = _Client(_container(digests=(f"hummingbot/hummingbot-api@{NEWER_SHA}",)), helpers=[helper]) + service = _service(client) + + service.collect_on_boot() + + status = service.status() + assert status["phase"] == "done" + assert status["run_id"] == "run123" + assert status["exit_code"] == 0 + assert status["previous_digest"].endswith(CURRENT_SHA) + assert status["new_digest"].endswith(NEWER_SHA) + assert helper.removed == [True] + + +def test_collect_on_boot_reports_a_failed_recreate_with_the_log_tail(): + helper = _helper(exit_code=1, logs=b"service hummingbot-api: error\nvolume not found\n") + service = _service(_Client(_container(), helpers=[helper])) + + service.collect_on_boot() + + status = service.status() + assert status["phase"] == "failed" + assert status["exit_code"] == 1 + assert status["log_tail"] == ["service hummingbot-api: error", "volume not found"] + assert "docker compose logs hummingbot-api" in status["detail"] + # Still removed: a failed run must not block the next preflight forever. + assert helper.removed == [True] + + +def test_collect_on_boot_leaves_a_helper_that_is_still_running(): + helper = _helper(status="running") + service = _service(_Client(_container(), helpers=[helper])) + + service.collect_on_boot() + + assert helper.removed == [] + assert service.status()["phase"] == "idle" + + +def test_collect_on_boot_survives_an_unreachable_daemon(): + service = _service(_Client(_container(), reachable=False)) + + service.collect_on_boot() + + assert service.status()["phase"] == "idle" + + +def test_status_is_idle_before_anything_has_run(): + assert _service().status() == {"run_id": None, "phase": "idle", "detail": None, "log_tail": []} + + +# ── The routes ─────────────────────────────────────────────────────────────────── + + +@pytest.fixture +def make_client(): + """A TestClient whose /system/upgrade/* talks to the given SelfUpgradeService.""" + from deps import get_self_upgrade_service + from routers import system + + def _make(service): + app = FastAPI() + app.include_router(system.router) + app.dependency_overrides[get_self_upgrade_service] = lambda: service + return TestClient(app) + + return _make + + +def test_preflight_route_reports_the_refusal_rather_than_failing(make_client): + body = make_client(_service(_Client(_container(), reachable=False))).get("/system/upgrade/preflight") + + assert body.status_code == 200 + assert body.json()["can_upgrade"] is False + assert body.json()["blocked_reason"] + + +def test_upgrade_route_answers_409_when_refused(make_client): + response = make_client(_service(active=2)).post("/system/upgrade", json={}) + + assert response.status_code == 409 + assert "2 executor(s) are running" in response.json()["detail"] + + +def test_upgrade_route_with_no_body_acknowledges_nothing(make_client): + """An empty body must be read as consent withheld, not consent given.""" + response = make_client(_service(active=2)).post("/system/upgrade") + + assert response.status_code == 409 + + +def test_upgrade_route_answers_202_when_it_starts(make_client, run_threads_inline): + service = _service(active=2) + + response = make_client(service).post("/system/upgrade", json={"acknowledge_executor_loss": True}) + + assert response.status_code == 202 + assert response.json()["phase"] == "pulling" + assert response.json()["run_id"] + + +def test_status_route_serves_the_collected_run(make_client): + service = _service(_Client(_container(), helpers=[_helper(exit_code=0)])) + service.collect_on_boot() + + body = make_client(service).get("/system/upgrade/status").json() + + assert body["phase"] == "done" + assert body["run_id"] == "run123" diff --git a/utils/compose.py b/utils/compose.py new file mode 100644 index 00000000..904c23d4 --- /dev/null +++ b/utils/compose.py @@ -0,0 +1,119 @@ +"""How this API's own container was deployed, read from Docker Compose's own labels. + +Compose stamps every container it creates with the project name, the directory it was +run from and the config files it was run with. Those labels are the only record on the +box of how the deployment is put together -- there is nothing to configure and nothing +to guess -- so both things that need to know read them from here: + +* ``GET /system/info`` (FEAT-121) reports them, and reports whether the running image is + the published one or something pinned on the box. +* ``services/self_upgrade.py`` (FEAT-122) *refuses to upgrade* when it is pinned, and + rebuilds the exact ``docker compose -f ... up`` the operator would have typed. + +They share this module rather than each reading the labels themselves because the two +answers have to agree. A panel that says "not pinned" over an upgrade path that refuses +as pinned is a confusing afternoon; a panel that says "not pinned" over an upgrade path +that *proceeds* on a hand-built image is a trading API server replaced by a published +build nobody asked for. +""" + +import os + +# Compose service this API is deployed as, used to find our own container when the +# hostname lookup fails (see own_container). +COMPOSE_SERVICE = "hummingbot-api" + +# Compose stamps these on every container it creates; they say which project the API +# belongs to and which files were used to bring it up. +LABEL_PROJECT = "com.docker.compose.project" +LABEL_WORKING_DIR = "com.docker.compose.project.working_dir" +LABEL_CONFIG_FILES = "com.docker.compose.project.config_files" + + +def own_container(client): + """Find the container this API process runs in. + + Docker sets a container's hostname to its own short id, which is how a process + identifies itself from the inside. That fails when the deployment overrides + `hostname:` or when the API runs from source on the host, so fall back to the one + container carrying this project's compose service label. + + Returns: + A docker-py Container, or None when this process cannot be placed. + """ + import socket + + try: + return client.containers.get(socket.gethostname()) + except Exception: + pass + try: + matches = client.containers.list(filters={"label": f"com.docker.compose.service={COMPOSE_SERVICE}"}) + except Exception: + return None + return matches[0] if matches else None + + +def describe_container(container): + """Identity, image and compose provenance of a container. + + Returns: + Dictionary with the container's id and name, the image reference it was started + from, the image id, its registry digest (None for a locally built image) and the + three compose labels that say where the deployment lives. + """ + labels = container.labels or {} + image = container.image + # A locally built image has no RepoDigests: nothing published it, so there is no + # content address to compare against the tag it carries. + digests = (image.attrs.get("RepoDigests") or []) if image is not None else [] + return { + "id": container.id, + "name": container.name, + "image": container.attrs.get("Config", {}).get("Image"), + "image_id": image.id if image is not None else None, + "digest": digests[0] if digests else None, + "compose_project": labels.get(LABEL_PROJECT), + "compose_working_dir": labels.get(LABEL_WORKING_DIR), + "compose_config_files": labels.get(LABEL_CONFIG_FILES), + } + + +def config_files(described): + """The compose config files of a described container, as a list of host paths. + + The label holds them comma-separated in the order they were passed to compose, and + that order is load-bearing: later files override earlier ones, so replaying it as + `-f a -f b` is the only way to recreate the container the operator actually has. + """ + raw = (described or {}).get("compose_config_files") or "" + return [f.strip() for f in raw.split(",") if f.strip()] + + +def pinning(described): + """Whether this deployment runs something other than the published image. + + Two things make an image not the one `hummingbot/hummingbot-api:latest` currently + resolves to: it was built on the box (no registry digest), or compose was brought up + with an override file that can repoint `image:` at a pinned tag. Only files whose + name contains "override" count -- `docker-compose.tailscale.yml` is an overlay for + networking, not for the image. + + Returns: + Dictionary with pinned, a short pinned_reason and the override file name (None + when that is not the reason). + """ + files = config_files(described) + override_file = next((os.path.basename(f) for f in files if "override" in os.path.basename(f).lower()), None) + + reasons = [] + if not (described or {}).get("digest"): + reasons.append("image was built locally (no registry digest)") + if override_file: + reasons.append(f"compose override file {override_file} can pin the image") + + return { + "pinned": bool(reasons), + "pinned_reason": "; ".join(reasons) or None, + "override_file": override_file, + } From 7e27faef0e218a6417b614b0ebf1b537242b5bcb Mon Sep 17 00:00:00 2001 From: cardosofede Date: Fri, 18 Sep 2026 13:14:25 +0300 Subject: [PATCH 08/13] fix(api): render validation errors from ValueError validators as 422, not 500 The app's RequestValidationError handler passed exc.errors() straight to JSONResponse; a validator raising ValueError puts the exception object in the error ctx, which is not JSON-serializable, so a blank global_token_name came back as a 500. Encode with jsonable_encoder, and move the handler to utils/validation_errors.py so the rate-oracle tests mount the real one. Co-Authored-By: Claude Opus 5 (1M context) --- main.py | 27 +++----------------- test/test_bot_rate_oracle_config.py | 5 ++++ utils/validation_errors.py | 38 +++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 24 deletions(-) create mode 100644 utils/validation_errors.py diff --git a/main.py b/main.py index 2d481e95..a7d93ab2 100644 --- a/main.py +++ b/main.py @@ -28,11 +28,10 @@ def patched_save_to_yml(yml_path, cm): config_helpers.save_to_yml = patched_save_to_yml -from fastapi import Depends, FastAPI, HTTPException, Request, status # noqa: E402 +from fastapi import Depends, FastAPI, HTTPException, status # noqa: E402 from fastapi.exceptions import RequestValidationError # noqa: E402 from fastapi.middleware.cors import CORSMiddleware # noqa: E402 from fastapi.middleware.gzip import GZipMiddleware # noqa: E402 -from fastapi.responses import JSONResponse # noqa: E402 from fastapi.security import HTTPBasic, HTTPBasicCredentials # noqa: E402 from hummingbot.client.config.client_config_map import GatewayConfigMap # noqa: E402 from hummingbot.client.config.config_crypt import ETHKeyFileSecretManger # noqa: E402 @@ -82,6 +81,7 @@ def patched_save_to_yml(yml_path, cm): from utils.bot_archiver import BotArchiver # noqa: E402 from utils.core_compatibility import require_core_surface # noqa: E402 from utils.security import BackendAPISecurity # noqa: E402 +from utils.validation_errors import validation_exception_handler # noqa: E402 # Set up logging configuration logging.basicConfig( @@ -425,28 +425,7 @@ async def lifespan(app: FastAPI): app.add_middleware(GZipMiddleware, minimum_size=1000) -@app.exception_handler(RequestValidationError) -async def validation_exception_handler(request: Request, exc: RequestValidationError): - """ - Custom handler for validation errors to log detailed error messages. - """ - # Build a readable error message from validation errors - error_messages = [] - for error in exc.errors(): - loc = " -> ".join(str(part) for part in error.get("loc", [])) - msg = error.get("msg", "Validation error") - error_messages.append(f"{loc}: {msg}") - - # Log the validation error with details - logging.warning( - f"Validation error on {request.method} {request.url.path}: {'; '.join(error_messages)}" - ) - - # Return standard FastAPI validation error response - return JSONResponse( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - content={"detail": exc.errors()}, - ) +app.add_exception_handler(RequestValidationError, validation_exception_handler) logfire.configure(send_to_logfire="if-token-present", environment=settings.app.logfire_environment, service_name="hummingbot-api") diff --git a/test/test_bot_rate_oracle_config.py b/test/test_bot_rate_oracle_config.py index c9a5ddc5..f8359879 100644 --- a/test/test_bot_rate_oracle_config.py +++ b/test/test_bot_rate_oracle_config.py @@ -14,9 +14,11 @@ import pytest import yaml from fastapi import FastAPI +from fastapi.exceptions import RequestValidationError from fastapi.testclient import TestClient from utils.file_system import FileSystemUtil +from utils.validation_errors import validation_exception_handler @pytest.fixture @@ -36,6 +38,9 @@ def env(tmp_path, monkeypatch): service = SimpleNamespace(quote_token="USDT") app = FastAPI() app.include_router(bot_rate_oracle.router) + # The real app's handler, not FastAPI's default: a blank token name must come back as + # a 422 through the handler main.py installs, which is where it once became a 500. + app.add_exception_handler(RequestValidationError, validation_exception_handler) app.dependency_overrides[get_market_data_service] = lambda: service return SimpleNamespace(client=TestClient(app), service=service, root=tmp_path) diff --git a/utils/validation_errors.py b/utils/validation_errors.py new file mode 100644 index 00000000..6cf5871f --- /dev/null +++ b/utils/validation_errors.py @@ -0,0 +1,38 @@ +"""The app-wide handler for request validation errors. + +Lives outside main.py so tests can mount the handler the real app uses: a router tested +on a bare FastAPI() gets FastAPI's default handler instead, which is how a 422 that the +real app could not render went unnoticed. +""" + +import logging + +from fastapi import Request +from fastapi.encoders import jsonable_encoder +from fastapi.exceptions import RequestValidationError +from fastapi.responses import JSONResponse + + +async def validation_exception_handler(request: Request, exc: RequestValidationError): + """ + Custom handler for validation errors to log detailed error messages. + """ + # Build a readable error message from validation errors + error_messages = [] + for error in exc.errors(): + loc = " -> ".join(str(part) for part in error.get("loc", [])) + msg = error.get("msg", "Validation error") + error_messages.append(f"{loc}: {msg}") + + # Log the validation error with details + logging.warning( + f"Validation error on {request.method} {request.url.path}: {'; '.join(error_messages)}" + ) + + # Return standard FastAPI validation error response. jsonable_encoder is what makes a + # validator that raises ValueError renderable: pydantic puts the exception object + # itself in the error's ctx, and JSONResponse cannot serialize it. + return JSONResponse( + status_code=422, + content={"detail": jsonable_encoder(exc.errors())}, + ) From 238ecb7d8a873ba01ef8dfc6b0f7ee89623beb04 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Fri, 18 Sep 2026 13:14:26 +0300 Subject: [PATCH 09/13] fix(system): never report another container or a local build as the published image - own_container: also find itself by the container id in /proc/self/mountinfo (survives a custom hostname:); use the compose-service label fallback only from inside a container and only when exactly one container matches. - describe_container: only a digest in hummingbot/hummingbot-api counts, since the containerd image store gives local builds a RepoDigest of their own. - pinning: a tag other than latest, or a digest-pinned reference, is pinned. Co-Authored-By: Claude Opus 5 (1M context) --- test/test_system_info.py | 115 +++++++++++++++++++++++++++++++++++---- utils/compose.py | 112 +++++++++++++++++++++++++++++++------- 2 files changed, 194 insertions(+), 33 deletions(-) diff --git a/test/test_system_info.py b/test/test_system_info.py index 14491952..83017101 100644 --- a/test/test_system_info.py +++ b/test/test_system_info.py @@ -23,45 +23,53 @@ WORKING_DIR = "/home/hbot/hummingbot-api" -def _container(digests=("hummingbot/hummingbot-api@sha256:abc123",), config_files=f"{WORKING_DIR}/docker-compose.yml"): +def _container( + digests=("hummingbot/hummingbot-api@sha256:abc123",), + config_files=f"{WORKING_DIR}/docker-compose.yml", + image="hummingbot/hummingbot-api:latest", + name="hummingbot-api", + project="hummingbot-api", +): """A docker-py Container as this route reads it.""" return SimpleNamespace( id=CONTAINER_ID, - name="hummingbot-api", + name=name, labels={ - "com.docker.compose.project": "hummingbot-api", + "com.docker.compose.project": project, "com.docker.compose.project.working_dir": WORKING_DIR, "com.docker.compose.project.config_files": config_files, "com.docker.compose.service": "hummingbot-api", }, - attrs={"Config": {"Image": "hummingbot/hummingbot-api:latest"}}, + attrs={"Config": {"Image": image}}, image=SimpleNamespace( id="sha256:deadbeef", - tags=["hummingbot/hummingbot-api:latest"], + tags=[image], attrs={"RepoDigests": list(digests)}, ), ) class _Containers: - def __init__(self, container, found_by_hostname=True): + def __init__(self, container, found_by_hostname=True, others=(), found_by_id=None): self.container = container self.found_by_hostname = found_by_hostname + self.found_by_id = found_by_id + self.others = list(others) self.list_filters = None def get(self, name): - if not self.found_by_hostname: - raise NotFound(f"no such container: {name}") - return self.container + if self.found_by_hostname or (self.found_by_id and name == self.found_by_id): + return self.container + raise NotFound(f"no such container: {name}") def list(self, filters=None): self.list_filters = filters - return [self.container] if self.container is not None else [] + return ([self.container] if self.container is not None else []) + self.others class _Client: - def __init__(self, container, found_by_hostname=True, reachable=True): - self.containers = _Containers(container, found_by_hostname) + def __init__(self, container, found_by_hostname=True, reachable=True, others=(), found_by_id=None): + self.containers = _Containers(container, found_by_hostname, others, found_by_id) self.reachable = reachable def ping(self): @@ -70,6 +78,16 @@ def ping(self): return True +@pytest.fixture(autouse=True) +def inside_a_container(monkeypatch): + """Run as the containerised API by default, with no container id in the mount table.""" + from utils import compose + + monkeypatch.setattr(compose, "_container_id_from_mountinfo", lambda: None) + monkeypatch.setattr(compose, "_in_container", lambda: True) + return monkeypatch + + @pytest.fixture def make_client(): """Build a TestClient whose /system/info talks to the given fake docker client.""" @@ -115,6 +133,42 @@ def test_locally_built_image_is_pinned(make_client): assert body["override_file"] is None +def test_locally_built_image_on_the_containerd_store_is_pinned(make_client): + # The containerd image store gives a local build a RepoDigest of its own; it is not a + # digest in the published repository, so it does not make the image the published one. + sha = "sha256:f84fd1ab6802c4434a8ea709c63a7a99c30b4aaa80e828fc508d1ebb2e65f806" # noqa: mock + container = _container(digests=(f"hbapi@{sha}",), image="hbapi:pr235") + body = make_client(_Client(container)).get("/system/info").json() + + assert body["container"]["digest"] is None + assert body["pinned"] is True + assert "locally" in body["pinned_reason"] + + +def test_a_fully_qualified_published_digest_is_the_published_image(make_client): + container = _container(digests=("docker.io/hummingbot/hummingbot-api@sha256:abc123",)) + body = make_client(_Client(container)).get("/system/info").json() + + assert body["container"]["digest"] == "docker.io/hummingbot/hummingbot-api@sha256:abc123" + assert body["pinned"] is False + + +def test_a_version_tag_in_the_main_compose_file_is_pinned(make_client): + # Pulled from the registry, so it has a published digest -- but upgrading it would + # pull the same tag back, not latest. + container = _container(image="hummingbot/hummingbot-api:1.0.1") + body = make_client(_Client(container)).get("/system/info").json() + + assert body["pinned"] is True + assert "1.0.1" in body["pinned_reason"] + assert body["override_file"] is None + + +def test_an_image_pinned_by_digest_is_pinned(make_client): + container = _container(image="hummingbot/hummingbot-api@sha256:abc123") + assert make_client(_Client(container)).get("/system/info").json()["pinned"] is True + + def test_override_file_is_pinned_and_named(make_client): container = _container( config_files=f"{WORKING_DIR}/docker-compose.yml,{WORKING_DIR}/docker-compose.override.yml", @@ -142,6 +196,43 @@ def test_falls_back_to_the_compose_service_label(make_client): assert body["container"]["name"] == "hummingbot-api" +def test_finds_itself_by_container_id_when_the_hostname_is_overridden(make_client, inside_a_container): + from utils import compose + + inside_a_container.setattr(compose, "_container_id_from_mountinfo", lambda: CONTAINER_ID) + decoy = _container(name="decoy-api", project="some-other-stack", image="alpine:latest") + client = _Client(_container(), found_by_hostname=False, found_by_id=CONTAINER_ID, others=[decoy]) + body = make_client(client).get("/system/info").json() + + assert body["container"]["name"] == "hummingbot-api" + assert client.containers.list_filters is None # never needed the label fallback + + +def test_two_candidates_for_the_service_label_report_no_container(make_client): + # Another stack on the same host runs a hummingbot-api service too. Picking one would + # report its image and its pinning verdict as this server's. + decoy = _container(name="decoy-api", project="some-other-stack", image="alpine:latest") + body = make_client(_Client(_container(), found_by_hostname=False, others=[decoy])).get("/system/info").json() + + assert body["docker_available"] is True + assert body["container"] is None + assert body["pinned"] is None + + +def test_running_from_source_does_not_claim_a_container(make_client, inside_a_container): + # From source this process is no container at all; the containerised API on the same + # host carries the service label, but it is not us. + from utils import compose + + inside_a_container.setattr(compose, "_in_container", lambda: False) + client = _Client(_container(), found_by_hostname=False) + body = make_client(client).get("/system/info").json() + + assert body["container"] is None + assert body["pinned"] is None + assert client.containers.list_filters is None + + def test_docker_unavailable_still_reports_versions(make_client): body = make_client(_Client(_container(), reachable=False)).get("/system/info").json() diff --git a/utils/compose.py b/utils/compose.py index 904c23d4..475e9931 100644 --- a/utils/compose.py +++ b/utils/compose.py @@ -18,6 +18,13 @@ """ import os +import re +import socket + +# The image this API is published as. An image from any other repository -- a local +# `docker build -t hbapi .`, a fork -- is by definition not the published one. +PUBLISHED_REPO = "hummingbot/hummingbot-api" +PUBLISHED_TAG = "latest" # Compose service this API is deployed as, used to find our own container when the # hostname lookup fails (see own_container). @@ -30,28 +37,74 @@ LABEL_CONFIG_FILES = "com.docker.compose.project.config_files" +def _container_id_from_mountinfo(): + """This process's own container id, read from the files Docker bind-mounts into it. + + Docker mounts /var/lib/docker/containers//{hostname,hosts,resolv.conf} into every + container, so the id is in our own mount table whatever `hostname:` was set to. + """ + try: + with open("/proc/self/mountinfo") as f: + match = re.search(r"/containers/([0-9a-f]{64})/", f.read()) + except OSError: + return None + return match.group(1) if match else None + + +def _in_container(): + return os.path.exists("/.dockerenv") or _container_id_from_mountinfo() is not None + + def own_container(client): """Find the container this API process runs in. Docker sets a container's hostname to its own short id, which is how a process - identifies itself from the inside. That fails when the deployment overrides - `hostname:` or when the API runs from source on the host, so fall back to the one - container carrying this project's compose service label. + identifies itself from the inside; when the deployment overrides `hostname:` the id + is still in the mount table. Only when both miss, and only from inside a container, + fall back to the compose service label -- and only when exactly one container carries + it. A process running from source on the host is not any container, and with two + candidates picking one would report another deployment's image and pinning as ours: + None is the honest answer in both cases. Returns: A docker-py Container, or None when this process cannot be placed. """ - import socket - - try: - return client.containers.get(socket.gethostname()) - except Exception: - pass + for ref in (socket.gethostname(), _container_id_from_mountinfo()): + if not ref: + continue + try: + return client.containers.get(ref) + except Exception: + pass + if not _in_container(): + return None try: matches = client.containers.list(filters={"label": f"com.docker.compose.service={COMPOSE_SERVICE}"}) except Exception: return None - return matches[0] if matches else None + return matches[0] if len(matches) == 1 else None + + +def _repository(ref): + """The repository of an image reference, without tag, digest or the Docker Hub host. + + ``docker.io/hummingbot/hummingbot-api:latest`` and ``hummingbot/hummingbot-api@sha256:..`` + are both ``hummingbot/hummingbot-api``. A colon after the last slash is a tag; one + before it is a registry port. + """ + repo = (ref or "").split("@")[0] + if ":" in repo.rsplit("/", 1)[-1]: + repo = repo.rsplit(":", 1)[0] + for host in ("docker.io/", "index.docker.io/"): + if repo.startswith(host): + repo = repo[len(host):] + return repo + + +def _tag(ref): + """The tag of an image reference; an untagged reference means ``latest``.""" + name = (ref or "").split("@")[0].rsplit("/", 1)[-1] + return name.rsplit(":", 1)[1] if ":" in name else "latest" def describe_container(container): @@ -59,14 +112,21 @@ def describe_container(container): Returns: Dictionary with the container's id and name, the image reference it was started - from, the image id, its registry digest (None for a locally built image) and the - three compose labels that say where the deployment lives. + from, the image id, its digest in the published repository (None when it has + none, e.g. a locally built image) and the three compose labels that say where the + deployment lives. """ labels = container.labels or {} image = container.image - # A locally built image has no RepoDigests: nothing published it, so there is no - # content address to compare against the tag it carries. - digests = (image.attrs.get("RepoDigests") or []) if image is not None else [] + # Only a digest in the published repository says the image came from it. With the + # legacy image store a local build has no RepoDigests at all, but with the containerd + # store (the default on current Docker Desktop) `docker build -t hbapi .` gets one too + # -- `hbapi@sha256:...` -- and taking that as a registry digest reads a hand-built + # image as the published one. + digests = [ + d for d in ((image.attrs.get("RepoDigests") or []) if image is not None else []) + if _repository(d) == PUBLISHED_REPO + ] return { "id": container.id, "name": container.name, @@ -93,11 +153,16 @@ def config_files(described): def pinning(described): """Whether this deployment runs something other than the published image. - Two things make an image not the one `hummingbot/hummingbot-api:latest` currently - resolves to: it was built on the box (no registry digest), or compose was brought up - with an override file that can repoint `image:` at a pinned tag. Only files whose - name contains "override" count -- `docker-compose.tailscale.yml` is an overlay for - networking, not for the image. + Three things make an image not the one `hummingbot/hummingbot-api:latest` currently + resolves to: it was built on the box or comes from another repository (no digest in + the published one), it runs a tag other than `latest` (a published version pinned in + docker-compose.yml itself), or compose was brought up with an override file that can + repoint `image:` at a pinned tag. Only files whose name contains "override" count -- + `docker-compose.tailscale.yml` is an overlay for networking, not for the image. + + The one case this cannot see is an image built locally *as* + `hummingbot/hummingbot-api:latest` on the containerd store, which carries a digest in + the published repository like a pulled image does; nothing local tells them apart. Returns: Dictionary with pinned, a short pinned_reason and the override file name (None @@ -106,9 +171,14 @@ def pinning(described): files = config_files(described) override_file = next((os.path.basename(f) for f in files if "override" in os.path.basename(f).lower()), None) + image_ref = (described or {}).get("image") reasons = [] if not (described or {}).get("digest"): - reasons.append("image was built locally (no registry digest)") + reasons.append(f"image was built locally or is not from {PUBLISHED_REPO} (no published digest)") + elif "@" in (image_ref or ""): + reasons.append("image is pinned to a digest") + elif _tag(image_ref) != PUBLISHED_TAG: + reasons.append(f"image is pinned to tag {_tag(image_ref)}") if override_file: reasons.append(f"compose override file {override_file} can pin the image") From ab29ce9ef8b64c3038a8542abc22939a7efb7964 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Fri, 18 Sep 2026 16:55:40 +0300 Subject: [PATCH 10/13] fix(system): collect a failed upgrade helper without waiting for a restart A helper that exited without replacing the API was only collected by collect_on_boot(), which never runs because the API was not replaced: the run stayed 'recreating' forever and every later preflight refused on the exited helper. preflight() and status() now collect exited helpers too; collected from a live process the run is 'failed' whatever the exit code, since this process is the container the helper was meant to replace. Co-Authored-By: Claude Opus 5 (1M context) --- services/self_upgrade.py | 70 ++++++++++++++++++++++++++++----------- test/test_self_upgrade.py | 64 +++++++++++++++++++++++++++++++++-- 2 files changed, 113 insertions(+), 21 deletions(-) diff --git a/services/self_upgrade.py b/services/self_upgrade.py index 71237ae4..afba924b 100644 --- a/services/self_upgrade.py +++ b/services/self_upgrade.py @@ -204,6 +204,11 @@ async def preflight(self) -> Dict[str, Any]: ) return info + # A helper that exited while this process is still alive is a recreate that did + # not replace us. Record it and clear it here, before any other check can return, + # so neither the run status nor the helper check below is stuck on it. + self._collect_finished_helpers(client, on_boot=False) + container = own_container(client) if container is None: info["blocked_reason"] = ( @@ -290,8 +295,8 @@ async def preflight(self) -> Dict[str, Any]: return info if helper is not None: info["blocked_reason"] = ( - f"An upgrade helper container is already present ({helper.name}). " - "Wait for it to finish; it is cleared when this API next starts." + f"An upgrade helper container is still running ({helper.name}). " + "Wait for it to finish." ) return info @@ -448,7 +453,18 @@ def _pull_then_recreate(self, run_id, image_ref, compose, previous_digest) -> No # ── Report ─────────────────────────────────────────────────────────────────── def status(self) -> Dict[str, Any]: - """The current or last run, or an idle record when there has been none.""" + """The current or last run, or an idle record when there has been none. + + A run this process left at ``recreating`` is checked against its helper: if the + helper has exited and we are still here, the recreate did not replace us, and + without collecting it here the run would read ``recreating`` forever. + """ + with self._lock: + phase = (self._run or {}).get("phase") + if phase == "recreating": + client = self._daemon() + if client is not None: + self._collect_finished_helpers(client, on_boot=False) with self._lock: if self._run is None: return {"run_id": None, "phase": "idle", "detail": None, "log_tail": []} @@ -457,21 +473,30 @@ def status(self) -> Dict[str, Any]: def collect_on_boot(self) -> None: """Adopt the outcome of a helper that outlived the API that started it. - This is where an upgrade actually finishes: the process that started it is gone, - and the record of what happened is the exited helper container. Its exit code and - log tail become this API's last run, then it is removed so the next preflight is - not blocked by its own predecessor. - - A helper still running is left alone -- it may be mid-recreate, and preflight - blocks on it. Never raises: a failure to tidy up must not stop the API booting. + This is where a successful upgrade finishes: the process that started it is gone, + and the record of what happened is the exited helper container. Never raises: a + failure to tidy up must not stop the API booting. """ client = self._daemon() if client is None: return + self._collect_finished_helpers(client, on_boot=True) + + def _collect_finished_helpers(self, client, on_boot: bool) -> None: + """Turn every exited helper into this API's last run, then remove it. + + On boot the helper is our predecessor's, and exit 0 means we are the new + container. Anywhere else this process is the one the helper was meant to replace + and it is still running, so the recreate did not happen whatever the exit code + says -- recorded as failed, so the operator sees it and can retry. + + A helper still running is left alone -- it may be mid-recreate, and preflight + blocks on it. Never raises. + """ try: helpers = client.containers.list(all=True, filters={"label": HELPER_LABEL}) except Exception as e: - logger.warning(f"Could not look for self-upgrade helper containers on boot: {e}") + logger.warning(f"Could not look for self-upgrade helper containers: {e}") return for helper in helpers: @@ -495,18 +520,25 @@ def collect_on_boot(self) -> None: logger.warning(f"Could not read logs of self-upgrade helper {helper.name}: {e}") log_tail = [] - succeeded = exit_code == 0 + succeeded = on_boot and exit_code == 0 + if succeeded: + detail = "Upgrade finished; this API is the new container." + elif on_boot: + detail = ( + f"The recreate failed (exit code {exit_code}). " + "The previous container may still be running; check " + "`docker compose logs hummingbot-api` on the host." + ) + else: + detail = ( + f"The recreate did not replace this API (exit code {exit_code}); it is still " + "running the previous image. See log_tail for compose's output, then retry." + ) with self._lock: self._run = { "run_id": run_id, "phase": "done" if succeeded else "failed", - "detail": ( - "Upgrade finished; this API is the new container." - if succeeded - else f"The recreate failed (exit code {exit_code}). " - "The previous container may still be running; check " - "`docker compose logs hummingbot-api` on the host." - ), + "detail": detail, "image_ref": None, "previous_digest": previous_digest, "new_digest": self._current_digest(client), diff --git a/test/test_self_upgrade.py b/test/test_self_upgrade.py index efc25b2b..7da9f63e 100644 --- a/test/test_self_upgrade.py +++ b/test/test_self_upgrade.py @@ -97,7 +97,7 @@ def get(self, name): def list(self, all=False, filters=None): label = (filters or {}).get("label", "") if label.startswith(HELPER_LABEL): - return list(self.helpers) + return [h for h in self.helpers if not getattr(h, "removed", None)] # The compose-service fallback own_container() uses. return [self.container] if self.container is not None else [] @@ -290,7 +290,7 @@ async def test_refuses_while_a_helper_container_is_present(): info = await _service(_Client(_container(), helpers=[_helper(status="running")])).preflight() assert info["can_upgrade"] is False - assert "already present" in info["blocked_reason"] + assert "still running" in info["blocked_reason"] @pytest.mark.asyncio @@ -524,6 +524,66 @@ def _boom(image, **kwargs): assert "Nothing was changed" in status["detail"] +# ── Collecting a helper that did not replace this API ──────────────────────────── + + +@pytest.mark.asyncio +async def test_preflight_collects_a_failed_helper_instead_of_blocking_on_it(): + # The helper exited non-zero and this API was never replaced, so no boot will ever + # collect it. Blocking on it would need SSH to clear -- what this endpoint replaces. + helper = _helper(exit_code=1, logs=b"service bogus-service depends on undefined service\n") + service = _service(_Client(_container(), helpers=[helper])) + + info = await service.preflight() + + assert info["can_upgrade"] is True + assert helper.removed == [True] + status = service.status() + assert status["phase"] == "failed" + assert status["exit_code"] == 1 + assert status["log_tail"] == ["service bogus-service depends on undefined service"] + + +@pytest.mark.asyncio +async def test_status_does_not_stay_recreating_after_the_helper_fails(run_threads_inline): + client = _Client(_container()) + service = _service(client) + run_id = (await service.start())["run_id"] + assert service.status()["phase"] == "recreating" + + client.containers.helpers.append(_helper(exit_code=1, run_id=run_id)) + + status = service.status() + assert status["phase"] == "failed" + assert status["run_id"] == run_id + assert status["exit_code"] == 1 + assert "did not replace this API" in status["detail"] + + +@pytest.mark.asyncio +async def test_a_helper_that_exits_zero_without_replacing_this_api_is_not_done(): + # We are still here, so the recreate did not happen whatever compose's exit code. + helper = _helper(exit_code=0) + service = _service(_Client(_container(), helpers=[helper])) + + await service.preflight() + + assert service.status()["phase"] == "failed" + assert helper.removed == [True] + + +@pytest.mark.asyncio +async def test_status_leaves_a_helper_that_is_still_running(run_threads_inline): + client = _Client(_container()) + service = _service(client) + run_id = (await service.start())["run_id"] + helper = _helper(status="running", run_id=run_id) + client.containers.helpers.append(helper) + + assert service.status()["phase"] == "recreating" + assert helper.removed == [] + + # ── Collecting the run on boot ─────────────────────────────────────────────────── From d5bbe5060826bf2aa6d335c0ffae2fb8947dceb1 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Fri, 18 Sep 2026 16:55:41 +0300 Subject: [PATCH 11/13] fix(bot-rate-oracle): reject a blank global_token_symbol like its sibling Co-Authored-By: Claude Opus 5 (1M context) --- models/bot_rate_oracle.py | 10 ++++++---- test/test_bot_rate_oracle_config.py | 6 ++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/models/bot_rate_oracle.py b/models/bot_rate_oracle.py index be165b55..e65c3432 100644 --- a/models/bot_rate_oracle.py +++ b/models/bot_rate_oracle.py @@ -24,14 +24,16 @@ class GlobalTokenConfig(BaseModel): description="Symbol to display for the global token" ) - @field_validator("global_token_name") + @field_validator("global_token_name", "global_token_symbol") @classmethod - def _reject_blank_token_name(cls, v: Optional[str]) -> Optional[str]: - # A blank quote token would make every cross-rate lookup resolve to nothing. + def _reject_blank(cls, v: Optional[str], info) -> Optional[str]: + # A blank quote token would make every cross-rate lookup resolve to nothing, and a + # blank symbol renders every value in the bot's status without a unit. Omit the + # field to leave it unchanged. if v is not None: v = v.strip() if not v: - raise ValueError("global_token_name must not be blank") + raise ValueError(f"{info.field_name} must not be blank") return v diff --git a/test/test_bot_rate_oracle_config.py b/test/test_bot_rate_oracle_config.py index f8359879..60d3cf78 100644 --- a/test/test_bot_rate_oracle_config.py +++ b/test/test_bot_rate_oracle_config.py @@ -101,6 +101,12 @@ def test_blank_global_token_name_rejected(env): assert env.service.quote_token == "USDT" +def test_blank_global_token_symbol_rejected(env): + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"global_token": {"global_token_symbol": " "}}) + assert resp.status_code == 422 + assert _conf(env, "master_account")["global_token"]["global_token_symbol"] == "$" + + def test_failed_write_leaves_live_quote_token_untouched(env, monkeypatch): def fail(*args, **kwargs): raise PermissionError("read-only") From 9292dec15638b86929ca71cfae1c0748e4d14d57 Mon Sep 17 00:00:00 2001 From: cardosofede Date: Mon, 21 Sep 2026 16:17:34 +0300 Subject: [PATCH 12/13] fix(system): never adopt or remove a foreign stack's self-upgrade helper --- services/self_upgrade.py | 64 +++++++++++++++++++++++++++++++++++---- test/test_self_upgrade.py | 60 ++++++++++++++++++++++++++++++++++-- 2 files changed, 116 insertions(+), 8 deletions(-) diff --git a/services/self_upgrade.py b/services/self_upgrade.py index afba924b..18993d30 100644 --- a/services/self_upgrade.py +++ b/services/self_upgrade.py @@ -46,6 +46,11 @@ # the API that started it has been replaced. HELPER_LABEL = "hummingbot-api.self-upgrade" HELPER_PREVIOUS_DIGEST_LABEL = "hummingbot-api.self-upgrade.previous-digest" +# The compose project the helper was started for. HELPER_LABEL alone is not scoped to a +# deployment -- on a host running more than one hummingbot-api stack, every stack's helper +# carries it -- so collection is only ever allowed to touch a helper carrying this API's +# own project. +HELPER_PROJECT_LABEL = "hummingbot-api.self-upgrade.project" # The compose service to recreate. Only this one: postgres, emqx, any tailscale sidecar # and every bot container are left exactly as they are. @@ -128,19 +133,31 @@ def _registry_digest(self, image_ref: str) -> Optional[str]: return None def _existing_helper(self, client): - """``(established, helper)`` for a helper container already on this box. + """``(established, helper)`` for a helper of this deployment still in flight. ``established`` is False when the daemon would not answer the question at all, which blocks: an upgrade started while another one is mid-recreate is two ``compose up`` runs racing for the same container, and "I could not check" is not a reason to start the second one. + + An exited helper is not in flight -- it is either this deployment's, which + ``_collect_finished_helpers`` already turns into a run and removes before this is + reached, or another stack's on the same host, which is inert as far as this + deployment's own recreate is concerned and must not block it. """ try: found = client.containers.list(all=True, filters={"label": HELPER_LABEL}) except Exception as e: logger.warning(f"Could not list self-upgrade helper containers: {e}") return False, None - return True, (found[0] if found else None) + own_project = self._own_compose_project(client) + in_flight = [ + h for h in found + if getattr(h, "status", None) not in ("exited", "dead") + and own_project is not None + and (h.labels or {}).get(HELPER_PROJECT_LABEL) == own_project + ] + return True, (in_flight[0] if in_flight else None) async def _running_executors(self) -> Optional[int]: """How many executors are live, or None when that could not be established.""" @@ -416,6 +433,7 @@ def _pull_then_recreate(self, run_id, image_ref, compose, previous_digest) -> No labels={ HELPER_LABEL: run_id, HELPER_PREVIOUS_DIGEST_LABEL: previous_digest or "", + HELPER_PROJECT_LABEL: compose["project"], }, volumes={ "/var/run/docker.sock": {"bind": "/var/run/docker.sock", "mode": "rw"}, @@ -482,8 +500,24 @@ def collect_on_boot(self) -> None: return self._collect_finished_helpers(client, on_boot=True) + def _own_compose_project(self, client) -> Optional[str]: + """This container's own compose project, or None when it cannot be determined. + + A helper is only ever this deployment's if it was labelled with this project at + creation time (see _pull_then_recreate). Without knowing our own project there is + nothing to compare it against, so collection is skipped rather than guessed. + """ + try: + container = own_container(client) + if container is None: + return None + return describe_container(container).get("compose_project") + except Exception as e: + logger.warning(f"Could not read this API's own compose project: {e}") + return None + def _collect_finished_helpers(self, client, on_boot: bool) -> None: - """Turn every exited helper into this API's last run, then remove it. + """Turn every exited helper of this deployment into its last run, then remove it. On boot the helper is our predecessor's, and exit 0 means we are the new container. Anywhere else this process is the one the helper was meant to replace @@ -491,7 +525,12 @@ def _collect_finished_helpers(self, client, on_boot: bool) -> None: says -- recorded as failed, so the operator sees it and can retry. A helper still running is left alone -- it may be mid-recreate, and preflight - blocks on it. Never raises. + blocks on it. A helper that does not carry this API's own compose project is + another stack's, whatever its label and exit code say, and touching it -- let + alone removing it -- would take away the only record that stack has of its own + run. When a run of ours is already tracked (self._run has a run_id), a helper for + some other run under our own project is left alone too, on the same reasoning. + Never raises. """ try: helpers = client.containers.list(all=True, filters={"label": HELPER_LABEL}) @@ -499,6 +538,10 @@ def _collect_finished_helpers(self, client, on_boot: bool) -> None: logger.warning(f"Could not look for self-upgrade helper containers: {e}") return + own_project = self._own_compose_project(client) + with self._lock: + expected_run_id = (self._run or {}).get("run_id") + for helper in helpers: # A helper that has not exited is very likely running `docker compose up` # right now. Removing it would kill the recreate half-way, which is the one @@ -507,9 +550,18 @@ def _collect_finished_helpers(self, client, on_boot: bool) -> None: if getattr(helper, "status", None) not in ("exited", "dead"): logger.info(f"Self-upgrade helper {helper.name} is still {helper.status}; leaving it.") continue + + labels = helper.labels or {} + run_id = labels.get(HELPER_LABEL) + helper_project = labels.get(HELPER_PROJECT_LABEL) + if own_project is None or helper_project != own_project: + logger.info(f"Self-upgrade helper {helper.name} belongs to another compose project; leaving it.") + continue + if expected_run_id is not None and run_id != expected_run_id: + logger.info(f"Self-upgrade helper {helper.name} is not the run this API is tracking; leaving it.") + continue + try: - labels = helper.labels or {} - run_id = labels.get(HELPER_LABEL) previous_digest = labels.get(HELPER_PREVIOUS_DIGEST_LABEL) or None exit_code = (helper.attrs.get("State") or {}).get("ExitCode") try: diff --git a/test/test_self_upgrade.py b/test/test_self_upgrade.py index 7da9f63e..1985f4d5 100644 --- a/test/test_self_upgrade.py +++ b/test/test_self_upgrade.py @@ -32,6 +32,7 @@ HELPER_IMAGE, HELPER_LABEL, HELPER_PREVIOUS_DIGEST_LABEL, + HELPER_PROJECT_LABEL, SERVICE_NAME, SelfUpgradeRefused, SelfUpgradeService, @@ -70,12 +71,20 @@ def _container(digests=(f"hummingbot/hummingbot-api@{CURRENT_SHA}",), config_fil ) -def _helper(status="exited", exit_code=0, run_id="run123", logs=b"Container hummingbot-api Started\n"): +def _helper(status="exited", exit_code=0, run_id="run123", logs=b"Container hummingbot-api Started\n", + project="hummingbot-api"): + """A helper container as _pull_then_recreate creates one, for this test file's own + stack by default (see _container(), whose compose project is also "hummingbot-api"). + Pass a different `project` (or None) to simulate another stack's helper. + """ removed = [] + labels = {HELPER_LABEL: run_id, HELPER_PREVIOUS_DIGEST_LABEL: f"hummingbot/hummingbot-api@{CURRENT_SHA}"} + if project is not None: + labels[HELPER_PROJECT_LABEL] = project return SimpleNamespace( name=f"helper-{run_id}", status=status, - labels={HELPER_LABEL: run_id, HELPER_PREVIOUS_DIGEST_LABEL: f"hummingbot/hummingbot-api@{CURRENT_SHA}"}, + labels=labels, attrs={"State": {"ExitCode": exit_code}}, logs=lambda tail=None: logs, remove=lambda force=False: removed.append(force), @@ -450,6 +459,7 @@ async def test_start_pulls_both_images_then_runs_the_helper(run_threads_inline): assert run["working_dir"] == WORKING_DIR assert run["labels"][HELPER_LABEL] == started["run_id"] assert run["labels"][HELPER_PREVIOUS_DIGEST_LABEL].endswith(CURRENT_SHA) + assert run["labels"][HELPER_PROJECT_LABEL] == "hummingbot-api" # The record has to outlive the helper; auto_remove would delete it. assert run["auto_remove"] is False @@ -584,6 +594,38 @@ async def test_status_leaves_a_helper_that_is_still_running(run_threads_inline): assert helper.removed == [] +@pytest.mark.asyncio +async def test_an_idle_api_does_not_adopt_or_remove_another_stacks_helper(): + """A host running more than one hummingbot-api stack: an idle API -- one that has + never started an upgrade -- must not adopt a foreign helper's record as its own, nor + remove it and rob the stack that actually ran it of its result. + """ + foreign_helper = _helper(exit_code=7, run_id="FOREIGN_RUN_9999", project="some-other-stack") + service = _service(_Client(_container(), helpers=[foreign_helper])) + + info = await service.preflight() + + assert info["can_upgrade"] is True + assert foreign_helper.removed == [] + assert service.status() == {"run_id": None, "phase": "idle", "detail": None, "log_tail": []} + + +@pytest.mark.asyncio +async def test_a_tracked_run_does_not_adopt_a_helper_from_a_different_run_in_the_same_project(run_threads_inline): + """A helper carrying this API's own compose project but a different run id is not the + run this process is tracking -- e.g. a leftover from a run this process never started. + """ + client = _Client(_container()) + service = _service(client) + await service.start() + + other_helper = _helper(exit_code=0, run_id="some-other-run") + client.containers.helpers.append(other_helper) + + assert service.status()["phase"] == "recreating" + assert other_helper.removed == [] + + # ── Collecting the run on boot ─────────────────────────────────────────────────── @@ -628,6 +670,20 @@ def test_collect_on_boot_leaves_a_helper_that_is_still_running(): assert service.status()["phase"] == "idle" +def test_collect_on_boot_leaves_a_helper_from_another_compose_project(): + """The exited helper belongs to a different hummingbot-api deployment on the same + host; adopting it here would report the wrong stack's upgrade as this one's, and + removing it would erase the only record the other stack has of its own run. + """ + helper = _helper(exit_code=0, run_id="FOREIGN_RUN_9999", project="some-other-stack") + service = _service(_Client(_container(), helpers=[helper])) + + service.collect_on_boot() + + assert helper.removed == [] + assert service.status()["phase"] == "idle" + + def test_collect_on_boot_survives_an_unreachable_daemon(): service = _service(_Client(_container(), reachable=False)) From 96dabd64976b4230c3e8603c9c4077e745b7c63f Mon Sep 17 00:00:00 2001 From: cardosofede Date: Mon, 21 Sep 2026 16:17:49 +0300 Subject: [PATCH 13/13] fix(bot-rate-oracle): tolerate a hand-edited or malformed conf_client.yml --- routers/bot_rate_oracle.py | 39 ++++++++++--- test/test_bot_rate_oracle_config.py | 85 +++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/routers/bot_rate_oracle.py b/routers/bot_rate_oracle.py index 92cae304..bf0237d9 100644 --- a/routers/bot_rate_oracle.py +++ b/routers/bot_rate_oracle.py @@ -15,6 +15,7 @@ credentials that share the file. """ +import yaml from fastapi import APIRouter, Depends, HTTPException, Query from hummingbot.core.rate_oracle.rate_oracle import RATE_ORACLE_SOURCES @@ -45,9 +46,24 @@ def _conf_client_path(account_name: str) -> str: def _read_conf_client(account_name: str) -> dict: path = _conf_client_path(account_name) try: - return FileSystemUtil().read_yaml_file(path) or {} + data = FileSystemUtil().read_yaml_file(path) except FileNotFoundError: raise HTTPException(status_code=404, detail=f"Configuration file not found: {path}") + except yaml.YAMLError: + raise HTTPException(status_code=500, detail=f"Configuration file is not valid YAML: {path}") + # A hand-edited file can hold anything at the top level, including a YAML list or a + # bare scalar; read as empty rather than handing callers something they can't `.get()`. + return data if isinstance(data, dict) else {} + + +def _clean_str(value, default: str) -> str: + """``value`` as a stripped string, or ``default`` for anything else. + + conf_client.yml is user-editable and this only reads it back, so a blank, wrong-typed + or missing value must read as the default rather than failing the whole GET -- same + principle as _share_pct below. + """ + return value.strip() if isinstance(value, str) and value.strip() else default def _share_pct(config_data: dict) -> float: @@ -66,15 +82,20 @@ def _share_pct(config_data: dict) -> float: def _to_config(account_name: str, config_data: dict) -> RateOracleConfig: - global_token = config_data.get("global_token") or {} + rate_oracle_source = config_data.get("rate_oracle_source") + if not isinstance(rate_oracle_source, dict): + rate_oracle_source = {} + global_token = config_data.get("global_token") + if not isinstance(global_token, dict): + global_token = {} return RateOracleConfig( account_name=account_name, rate_oracle_source=RateOracleSourceConfig( - name=(config_data.get("rate_oracle_source") or {}).get("name", "gate_io") + name=_clean_str(rate_oracle_source.get("name"), "gate_io") ), global_token=GlobalTokenConfig( - global_token_name=global_token.get("global_token_name") or "USDT", - global_token_symbol=global_token.get("global_token_symbol", "$"), + global_token_name=_clean_str(global_token.get("global_token_name"), "USDT"), + global_token_symbol=_clean_str(global_token.get("global_token_symbol"), "$"), ), rate_limits_share_pct=_share_pct(config_data), ) @@ -126,11 +147,15 @@ async def update_bot_rate_oracle_config( detail=f"Invalid rate oracle source: {source_name}. " f"Available sources: {list(RATE_ORACLE_SOURCES.keys())}" ) - config_data.setdefault("rate_oracle_source", {})["name"] = source_name + if not isinstance(config_data.get("rate_oracle_source"), dict): + config_data["rate_oracle_source"] = {} + config_data["rate_oracle_source"]["name"] = source_name changes_made.append(f"rate_oracle_source updated to {source_name}") if update_request.global_token is not None: - global_token = config_data.setdefault("global_token", {}) + if not isinstance(config_data.get("global_token"), dict): + config_data["global_token"] = {} + global_token = config_data["global_token"] token_name = update_request.global_token.global_token_name if token_name is not None: global_token["global_token_name"] = token_name diff --git a/test/test_bot_rate_oracle_config.py b/test/test_bot_rate_oracle_config.py index 60d3cf78..9b3246bc 100644 --- a/test/test_bot_rate_oracle_config.py +++ b/test/test_bot_rate_oracle_config.py @@ -179,3 +179,88 @@ def test_share_pct_is_scoped_to_the_account_it_was_sent_for(env): assert _conf(env, "other")["rate_limits_share_pct"] == 25.0 assert "rate_limits_share_pct" not in _conf(env, "master_account") + + +# ── A hand-edited or legacy file can hold anything ────────────────────────────── + + +def _rewrite(env, account, conf): + path = env.root / "credentials" / account / "conf_client.yml" + path.write_text(yaml.safe_dump(conf)) + + +@pytest.mark.parametrize("bad_global_token", ["nonsense", ["a", "b"]]) +def test_a_global_token_of_the_wrong_shape_reads_as_the_default(env, bad_global_token): + conf = _conf(env, "master_account") + conf["global_token"] = bad_global_token + _rewrite(env, "master_account", conf) + + body = env.client.get("/bot-orchestration/rate-oracle/config").json() + + assert body["global_token"] == {"global_token_name": "USDT", "global_token_symbol": "$"} + + +@pytest.mark.parametrize("bad_global_token", ["nonsense", ["a", "b"]]) +def test_updating_a_global_token_of_the_wrong_shape_replaces_it_instead_of_crashing(env, bad_global_token): + conf = _conf(env, "master_account") + conf["global_token"] = bad_global_token + _rewrite(env, "master_account", conf) + + resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"global_token": {"global_token_name": "USDC"}}) + + assert resp.status_code == 200 + assert _conf(env, "master_account")["global_token"] == {"global_token_name": "USDC"} + + +def test_a_rate_oracle_source_of_the_wrong_shape_reads_as_the_default(env): + conf = _conf(env, "master_account") + conf["rate_oracle_source"] = "nonsense" + _rewrite(env, "master_account", conf) + + body = env.client.get("/bot-orchestration/rate-oracle/config").json() + + assert body["rate_oracle_source"]["name"] == "gate_io" + + +def test_a_null_rate_oracle_source_name_reads_as_the_default(env): + conf = _conf(env, "master_account") + conf["rate_oracle_source"]["name"] = None + _rewrite(env, "master_account", conf) + + body = env.client.get("/bot-orchestration/rate-oracle/config").json() + + assert body["rate_oracle_source"]["name"] == "gate_io" + + +@pytest.mark.parametrize("field", ["global_token_name", "global_token_symbol"]) +def test_a_blank_or_numeric_global_token_field_reads_as_the_default(env, field): + conf = _conf(env, "master_account") + conf["global_token"][field] = 12345 + _rewrite(env, "master_account", conf) + + body = env.client.get("/bot-orchestration/rate-oracle/config").json() + + assert body["global_token"][field] == ({"global_token_name": "USDT", "global_token_symbol": "$"})[field] + + +def test_invalid_yaml_is_a_clean_500_not_a_crash(env): + path = env.root / "credentials" / "master_account" / "conf_client.yml" + path.write_text("global_token: [unterminated\n") + + resp = env.client.get("/bot-orchestration/rate-oracle/config") + + assert resp.status_code == 500 + assert "not valid YAML" in resp.json()["detail"] + + +def test_a_top_level_yaml_list_reads_as_empty_and_can_be_repaired(env): + path = env.root / "credentials" / "master_account" / "conf_client.yml" + path.write_text(yaml.safe_dump(["not", "a", "mapping"])) + + get_resp = env.client.get("/bot-orchestration/rate-oracle/config") + assert get_resp.status_code == 200 + assert get_resp.json()["rate_oracle_source"]["name"] == "gate_io" + + put_resp = env.client.put("/bot-orchestration/rate-oracle/config", json={"rate_oracle_source": {"name": "binance"}}) + assert put_resp.status_code == 200 + assert _conf(env, "master_account")["rate_oracle_source"]["name"] == "binance"