feat(performance): executor performance as a first-class read surface, plus an 18-item backlog burndown

[cardosofede]

What this is

Two things on one branch: executor performance becomes a first-class read surface, and the improvements backlog is burned down to empty — 18 items closed against a test suite that, at the start, could not actually run.

---

Part 1 — Executor performance as a read surface

Before this branch, only controllers had a performance history. An executor's numbers lived in memory and were lost on restart: reap a live executor after a crash and its ExecutorRecord was terminated as SYSTEM_CLEANUP carrying creation-time zeros, so /executors/performance booked real PnL at 0. There was no way to chart an executor at all.

Now:

• Every live executor is snapshotted on an interval into its own table, and completion writes a terminal row in the same transaction as the ExecutorRecord update (behind a SAVEPOINT, so a failing snapshot INSERT can't lose the completion). A restart's reap now adopts the last snapshot's PnL, fees and filled amount instead of zeros.
• Both populations are served from one pair of routes — GET /performance/history and GET /performance/latest — in one normalized row shape. A consumer writes seriesFor(scope) and latestFor(scope) once instead of branching on controller-vs-executor; the branch is a subject= query parameter, so it stays a parameter to the client too.
• A closed executor's latest row is its terminal row, carrying is_terminal and close_type — "its final value" and "its current value" are the same query, with no join back to executors.
• Wire compatibility is absolute. /bot-orchestration/controller-performance-history and -latest are untouched and byte-identical, and the new controller subject goes through the exact same orchestrator methods, so old and new cannot drift. Nothing has to migrate before it wants to.

Two new settings, both defaulting to today's behaviour: PERFORMANCE_EXECUTOR_SNAPSHOT_INTERVAL (60s — finer than the controller dump because executors are short-lived; at a 5-minute grain a three-minute position executor gets one point) and PERFORMANCE_RETENTION_DAYS (0 = keep everything forever, so an upgrade never starts deleting an operator's history).

The design doc is in features/done/FEAT-001-executor-performance-snapshots.md, including the deviations from the original design and why.

---

Part 2 — The backlog burndown

Start here: the test suite was lying

pytest test/ reported 82 failed, 557 passed. 81 of those were not assertions — they were:

async def functions are not natively supported.

asyncio_mode was configured nowhere in the tree (no pytest.ini, no conftest.py, no [tool.pytest.ini_options]), and pytest/pytest-asyncio were not declared dependencies, so a fresh conda env create produced an environment that could not run test/ at all. Five test files were written expecting auto mode; every async test in them was collected and then refused.

The consequence is the part that matters: a genuine regression was sitting inside that wall of red, indistinguishable from the noise. CORR-118 — an untyped lp_provider silently resolving to a router pool — had a guard test that was already failing and nobody could see it.

So the first two commits fix the instrument before using it:

• ARCH-116 — declare asyncio_mode = "auto" and the test deps in-tree. 81 failures vanish.
• CORR-118 — fix the one real regression that was hiding behind them.

Suite: 82 failed / 557 passed → 798 passed, 0 failed. Net +241 tests, every one of them from the items below.

The 18 items

Each item was implemented in isolation, then verified against the integrated tree before the next one started — build, full suite, and flake8/isort on the touched files. Any item that broke the tree would have been reverted and marked blocked. None were: 18 closed, 0 blocked, 0 reverted.

Correctness

Item	Commit
CORR-118	459bf48	An untyped lp_provider is refused at config load instead of silently becoming a router pool
CORR-101	6e2e251	POST /backtesting/run reports failure as 500 and timeout as 504, instead of HTTP 200 with an error body
CORR-102	8f85852	A transient RuntimeError no longer permanently kills a market-data subscription — only the send_json is guarded, so a fetch blip is logged and retried
CORR-104	5657e38	gas_token repaired on CLMM liquidity rows left NULL/UNKNOWN by the old chain map, plus an idempotent backfill script
CORR-106	b4723cf	stop-and-archive's silent returns now mark the bot run errored, instead of leaving it neither archived nor errored
CORR-107	951b4e9	The in-flight book is no longer truncated at startup by a hardcoded limit(1000)
CORR-111	1d23303	A database outage in get_performance_report raises instead of reporting a zeroed report as if it were real
CORR-113	e8a8c3f	A non-numeric update_interval answers with a WebSocket error frame instead of crashing the handler

Performance

Item	Commit
PERF-105	bee217c	Closing a CLMM position no longer holds a pooled DB connection across its 2-second propagation sleep
PERF-108	5e75ce1	reconcile_active_orders: one batched SELECT and a single conditional flush, not one of each per order
PERF-110	bc02753	get_executor_stats: 7 statements → 2, return shape unchanged
PERF-114	bafaddc	The Gateway availability guard answers from a 2s-TTL ping cache, invalidated on connection failure; the 503 now carries Retry-After
PERF-112	d080507	Backtest workers share a bounded, range-exact on-disk candle cache instead of re-downloading per run

Architecture and readability

Item	Commit
ARCH-116	f532cf9	The suite is runnable from a fresh checkout
ARCH-109	8e7e841	AMM and archived-bots routes persist through a service — grep get_session_context routers/ is now empty
ARCH-103	6868769	The Gateway poller writes through the CLMM/swap services via one shared build_position_row()
READ-115	1477625	Container removal is gated on real API ownership rather than a name prefix
READ-117	ce19104	The .env template documents all five undocumented settings groups, with a test pinning it against config.py

Three findings worth a reviewer's attention

1. The docker prefix guard was inverted, not just vestigial (READ-115). create_hummingbot_instance passes name=instance_name verbatim, so the hummingbot- check rejected every container the API creates — while accepting exactly the infrastructure containers (hummingbot-api, hummingbot-postgres, hummingbot-broker, hummingbot-tailscale) and then trying to archive a bots/instances/ directory they never had. It is now an ownership check: a container is removable only if it owns the directory the endpoint archives. That also closed an incidental path-traversal hole in how that path was built.

2. The CLMM row shape was reconciled to the union, not the intersection (ARCH-103). The poller and the open route wrote different column sets for the same table. Dropping the poller's four extra columns would have been the smaller diff, but bin IDs and pending fees have no other reader — so both paths now write the union through one builder: NULL for what a path cannot know, 0 only where 0 is the fact. Two narrow value changes ride along: percentage is computed on Decimal on both paths, and a degenerate upper_price == 0 yields NULL rather than -1 on the discovery path.

3. The candle cache was deliberately built to not undo CORR-060 (PERF-112). The obvious implementation of "stop re-downloading candles" is to share the engine — which is exactly the shared-mutable-state bug CORR-060 closed. What crosses runs here is candle data only: no engine, no data provider, no controller, each reader unpickling its own copy. The cache is bounded three independent ways (LRU count, TTL, and exact-range keys so a wider or shifted window misses rather than serving short), with a per-key single flight so "download once" holds when BACKTESTING_MAX_CONCURRENT is raised. test_backtest_concurrency.py, test_backtest_offloading.py and test_backtest_storage.py pass unmodified.

---

Earlier on the branch

Correctness and safety: stop accepting API credentials in the WebSocket query string (they end up in access logs and proxy history); run each backtest in its own killable worker process; stop concurrent callers orphaning duplicate candle feeds; persist a completion whose creation row is not there yet; validate controller config names read back from the script YAML; read the EVM transaction id everywhere from one parser.

Performance: Sharpe ratio from aggregates rather than every PnL row; the in-flight order book in one SELECT when syncing; a bounded MQTT log dedup cache that is not rescanned per message.

Architecture: the CLMM and swap routes get a service to persist through; the gateway availability 503 becomes a route dependency rather than body code; the executor push loops are driven by one polling loop with configurable interval bounds; stop-and-archive addresses the bot by one name; the config-map endpoint exposes config field prompts.

---

Known follow-ups (found during the sweep, deliberately not fixed here)

1. BotRunRepository.update_bot_run_stopped matches only RUNNING/CREATED rows, but stop-and-archive has already set the row to STOPPED by the time it runs — so every error-message update in that function, including the ones CORR-106 adds here, can silently no-op against a real database. This partly undercuts CORR-106 and is the most urgent of the three.
2. setup.sh writes GATEWAY_URL=http://localhost:15888 while config.py defaults to https:// per SEC-048. The new drift test does not catch it — it only checks defaults on commented lines.
3. Nothing runs this suite in CI. .github/workflows/ contains only the Docker buildx job, which is precisely why 82 failures went unnoticed. Now that the suite is green and runnable from a clean checkout, a CI job is finally meaningful — and is the single highest-value follow-up on this list.

Testing

798 passed, 0 failed, verified on the integrated tree after every single item — not once at the end. flake8 --max-line-length=130 and isort --check-only clean on every touched file; the repo's pre-commit gate passed on every commit with no --no-verify.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UV33Qr3Z56GwkD4Wo8jjne

[greptile-apps]

Confidence Score: 3/5

The PR is not yet safe to merge because sampled history can terminate early and non-positive limits can bypass or inconsistently apply result caps.

Summary

• Adds periodic and terminal executor snapshots, restart recovery, retention, and normalized history/latest APIs.
• Optimizes executor statistics, backtesting, order reconciliation, Gateway access, and WebSocket polling.
• Expands configuration documentation and enables the asynchronous test suite with substantial regression coverage.

Diagram

sequenceDiagram
    participant E as ExecutorService
    participant DB as Performance repositories
    participant API as Performance routes
    participant C as Client
    E->>DB: Persist periodic or terminal snapshot
    C->>API: "GET /performance/history?subject=executor"
    API->>DB: Query filtered snapshot window
    DB-->>API: Sampled rows, cursor, has_more
    API-->>C: Normalized performance response
    C->>API: "GET /performance/latest?subject=controller|executor"
    API->>DB: Query latest rows
    DB-->>API: Latest normalized values
    API-->>C: Subject-independent response shape

Loading

[greptile-apps]

Sampling breaks pagination termination

When snapshots are missed, delayed, or unevenly spaced, the fixed over-fetch multiplier can yield at most limit sampled rows even though additional matching rows remain, causing has_more=false and next_cursor=null to silently truncate performance history.

Knowledge Base Used: Database models and repositories

[greptile-apps]

Invalid limits bypass result caps

When a client supplies zero or a negative limit, the value reaches implementations that handle it inconsistently: zero disables the executor latest SQL cap and makes executor history fetch a default-sized page, while controller latest returns no rows; negative values can also produce invalid or backend-dependent SQL limits.

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Try greploops.

[david-hummingbot]

Two findings from verifying this branch against a live Condor client

Both found while exercising the new /performance surface end to end. The first should be fixed before merge; the second needs a decision about which side owns it.

---

1. The executor sampler drops whole executors, not just points

GET /performance/history?subject=executor thins its series with one global timestamp cursor and no grouping by executor. Asked for any interval coarser than the 60-second write grain, it silently returns a subset of the fleet — with HTTP 200 and nothing indicating anything is missing.

Site: database/repositories/executor_performance_repository.py:42
Introduced: 1c09a4b — the route is new in this branch, so it ships with the defect unless fixed here.

Reproduction

Three executors, five snapshot rows, all inside a five-minute span:

INSERT INTO executor_performance_snapshots
  (timestamp, executor_id, executor_type, account_name, connector_name,
   trading_pair, controller_id, status, close_type, is_terminal,
   net_pnl_quote, net_pnl_pct, cum_fees_quote, filled_amount_quote)
VALUES
  (now() - interval '5 min', 'exec-A', 'position_executor', 'acct', 'binance', 'BTC-USDT', 'ctrl-1', 'RUNNING',    NULL,            false,  1.50,  0.0015, 0.20, 1000),
  (now() - interval '4 min', 'exec-A', 'position_executor', 'acct', 'binance', 'BTC-USDT', 'ctrl-1', 'RUNNING',    NULL,            false,  3.25,  0.0032, 0.40, 2000),
  (now() - interval '3 min', 'exec-A', 'position_executor', 'acct', 'binance', 'BTC-USDT', 'ctrl-1', 'TERMINATED', 'TAKE_PROFIT',   true,   4.75,  0.0047, 0.55, 2500),
  (now() - interval '2 min', 'exec-B', 'position_executor', 'acct', 'binance', 'ETH-USDT', 'ctrl-1', 'RUNNING',    NULL,            false, -0.80, -0.0008, 0.15,  900),
  (now() - interval '1 min', 'exec-C', 'lp_executor',       'acct', 'meteora', 'SOL-USDC', 'ctrl-2', 'TERMINATED', 'POSITION_HOLD', true,   2.00,  0.0020, 0.30, 1500);

Then read the route at each interval:

interval	HTTP	rows	executors	pairs represented
1m	200	5	3 of 3	BTC-USDT, ETH-USDT, SOL-USDC
5m — the default	200	1	1 of 3	SOL-USDC only
15m	200	1	1 of 3	SOL-USDC only
1h	200	1	1 of 3	SOL-USDC only

At the default interval, two executors and two trading pairs are absent from a 200 response. pagination.has_more is false, and the echoed interval reports what was asked for.

Cause

@staticmethod
def _sample_by_interval(history, interval_minutes, grain_minutes):
    if not history or interval_minutes <= grain_minutes:
        return history

    sampled = []
    last_sampled_time = None          # one cursor for the whole result set

    for item in history:
        item_time = datetime.fromisoformat(item["timestamp"].replace('Z','+00:00'))
        if last_sampled_time is None:
            sampled.append(item)
            last_sampled_time = item_time
        else:
            time_diff = (last_sampled_time - item_time).total_seconds() / 60
            if time_diff >= interval_minutes:    # no executor_id in this decision
                sampled.append(item)
                last_sampled_time = item_time

    return sampled

Because the cursor is global, the interval acts as a rate limit on the merged series rather than on each executor's own series. The executor that happens to own the newest row in a window survives; the rest become indistinguishable from executors that never reported.

The collision rate follows from the write cadence: executors are snapshotted every 60s (PERFORMANCE_EXECUTOR_SNAPSHOT_INTERVAL) against a 5m default, so at steady state roughly five rows per executor compete for each retained slot — across the whole fleet. The more executors are live, the smaller the fraction returned.

Who is affected

Condor's dashboard is not affected: PerfBrowser.tsx:1341 always sends a single executor_id, gated on enabled: !!execScopeId, so each window holds only that executor's rows. Re-verified after 34 commits that rewrote 240 lines of that file and added an Executors view — still exactly one call site in the whole frontend, still narrowed. But the protection is incidental: nothing in the code or its tests says the unnarrowed query is unsafe, so a future caller has no warning.

Condor's HTTP API is affected. GET /api/v1/servers/{name}/performance/history?subject=executor accepts the request with no executor_id and forwards it verbatim, so any caller of Condor's own API — an agent, an MCP tool, a script — gets the partial fleet, as does anything calling this API directly.

Worth noting the route's docstring warns that interval is "a floor, not a guarantee". That is a claim about resolution; a reader has no reason to expect scopes to disappear.

The same shape exists in the controller sampler and is already documented as upstream behaviour in Condor's condor/fetchers/run_history.py — "the same 12-controller fleet over the same window answers with 12 of 12 controllers at 5m and 11 of 12 at 1h."

---

2. A POSITION_HOLD executor's PnL is realized in one surface and unrealized in the other

For one terminated LP executor, read at the same moment in the same view:

source	realized	unrealized	net
API /performance/history row	0.00	2.00	2.00
Condor scope panel header	+$2.00	+$0.00	+$2.00

Both readings are defensible. models/performance.py treats POSITION_HOLD as unsettled on purpose — the close hands the position on to position_holds, so counting it as realized would double-count it, the same exclusion ExecutorRepository.get_performance_report applies. Condor's panel reads the executor record, where a TERMINATED executor's PnL is realized by definition.

The reason this belongs on this PR: before FEAT-087 there was no API-side realized/unrealized split for an executor, so there was nothing for the panel to contradict. They now sit one above the other on the same screen — the header above, and the chart it feeds below.

Someone has to own which one is the fact. The API's reasoning looks the sounder of the two, which would make the fix a Condor-side change — but it needs deciding rather than leaving both on screen.

Caveat on this one: it was observed against seeded rows (is_terminal = true, close_type = 'POSITION_HOLD', status TERMINATED). A real held LP position does terminate that way, so I believe the fixture is faithful, but it is worth confirming against a real one before acting.

[cardosofede]

@david-hummingbot thanks — both confirmed, fixed in 1c9ca97.

1. The sampler dropped whole executors

Fixed as diagnosed: _sample_by_interval now keeps one cursor per executor_id instead of one for the merged series. The interval now means what the parameter says — each executor's own series is thinned, no scope leaves the answer. Input order is preserved, so the series stays descending by timestamp and pagination is untouched.

Your repro, replayed against the fixed sampler:

interval	rows	executors	pairs represented
1m	5	3 of 3	BTC-USDT, ETH-USDT, SOL-USDC
5m — the default	3	3 of 3	BTC-USDT, ETH-USDT, SOL-USDC
15m	3	3 of 3	BTC-USDT, ETH-USDT, SOL-USDC
1h	3	3 of 3	BTC-USDT, ETH-USDT, SOL-USDC

I took the controller sampler too, rather than leaving it as documented upstream behaviour. It is the same defect in the same shape, subject=controller now serves it, and the row shape does not change — so /bot-orchestration/controller-performance-history stays wire-compatible, it just stops hiding controllers. Keyed on (bot_name, controller_id); your 12-controller fleet is a test now, 12 of 12 at 1h.

Five tests added, since your point that nothing in the code or its tests said the unnarrowed query was unsafe was the part worth fixing permanently: a fleet survives 5m and 1h, each scope's own series still obeys the interval (grouping must not quietly disable the thinning), the merge order is still newest-first, and the controller equivalent.

One residual I documented rather than fixed: per-scope cursors do not survive a page boundary, because next_cursor is a timestamp. So an executor's first row on page two can sit closer than interval to its last row on page one. That over-samples a scope by at most one row per page; it never drops one. Carrying per-scope state would mean an opaque cursor, which did not seem worth the wire change for that.

Your note about the docstring landed too. "A floor, not a guarantee" was indeed a claim about resolution only, and a reader had no reason to read scope-completeness into it. It now says explicitly that sampling is per scope and that an unnarrowed query over a fleet returns the whole fleet at every interval, at a lower resolution.

2. POSITION_HOLD realized in one surface, unrealized in the other

Agreed the API side is the sounder of the two, so I've kept it and made the stance explicit instead of leaving it as reasoning only visible in the source. The rule is now in the realized_pnl_quote / unrealized_pnl_quote field descriptions — so it reaches a consumer through the OpenAPI schema — and the route docstring spells out the surprising combination directly: a held executor is is_terminal: true, status: TERMINATED, and its PnL is still in unrealized_pnl_quote, because the position carries on in position_holds and counting it realized double-counts it against /executors/performance.

That leaves the actual change on the Condor side: the panel derives realized from TERMINATED on the executor record, and there is nothing this API can do to force the two to agree — it can only say which one is the fact, which it now does. Happy to file that against Condor if you'd rather it be tracked there.

On your caveat: the fixture looks faithful to me. POSITION_HOLD is exactly how a held LP position terminates (services/executor_service.py closes with it on keep_position=True, and again on an involuntary hold with hold_reason set), and ExecutorRepository.get_performance_report excludes that same close_type from realized PnL for the same double-count reason. Still worth a look against a real one before the Condor change goes in.

Full suite green — 803 passed.

[david-hummingbot]

Describe the bug

After an API crash/restart, an executor that was live at the time of the crash gets correctly reaped: ExecutorRecord.status becomes TERMINATED, close_type becomes SYSTEM_CLEANUP, and net_pnl_quote / cum_fees_quote / filled_amount_quote are adopted from its last performance snapshot instead of being zeroed (this is the fix in database/repositories/executor_repository.py::cleanup_orphaned_executors).

However, that reap path never inserts a corresponding terminal row into executor_performance_snapshots. The normal (non-crash) completion path does write a terminal row in the same transaction as the ExecutorRecord update (services/executor_service.py, around the _persist_executor_completed / SAVEPOINT block), but cleanup_orphaned_executors only mutates the executors table.

The effect: GET /performance/latest?subject=executor and GET /performance/history?subject=executor keep serving the executor's last live snapshot forever — status: RUNNING, is_terminal: false, close_type: null — even though GET /executors/{executor_id} correctly reports TERMINATED / SYSTEM_CLEANUP with the adopted PnL. The two surfaces permanently disagree about whether the executor is done.

This contradicts the documented design of the new performance routes (routers/performance.py): "A closed executor's latest row is its terminal row... 'its final value' and 'its current value' are the same query, with no join back to executors."

Reproduced twice on feat/burndown-improvements (commit 8f3f056) — identical result both times.

Steps to reproduce bug

1. Create a live in-process executor directly:

   curl -s -u admin:admin -X POST "http://localhost:8000/executors/" \
     -H "Content-Type: application/json" \
     -d '{
       "account_name": "master_account",
       "controller_id": "repro",
       "executor_config": {
         "type": "position_executor",
         "connector_name": "binance_perpetual_testnet",
         "trading_pair": "BTC-USDT",
         "side": 1,
         "amount": "0.001",
         "leverage": 5,
         "triple_barrier_config": {
           "stop_loss": "0.05",
           "take_profit": "0.05",
           "time_limit": 900,
           "open_order_type": 1
         }
       }
     }'

2. Wait at least one PERFORMANCE_EXECUTOR_SNAPSHOT_INTERVAL (default 60s) so a live snapshot is written. Confirm with:

   curl -s -u admin:admin "http://localhost:8000/performance/history?subject=executor&executor_id=<executor_id>"

3. Simulate a crash: hard-kill the API worker process (not a graceful shutdown/SIGTERM — that runs the normal stop() path and won't reproduce this). Then let the process supervisor (or --reload) bring up a fresh worker, which runs cleanup_orphaned_executors() on startup.
4. Compare the two surfaces:

   curl -s -u admin:admin "http://localhost:8000/executors/<executor_id>"
   curl -s -u admin:admin "http://localhost:8000/performance/latest?subject=executor"

5. Query the snapshot table directly to confirm no terminal row was ever inserted:

   docker exec hummingbot-postgres psql -U hbot -d hummingbot_api -t -c \
     "SELECT executor_id, status, close_type, net_pnl_quote, cum_fees_quote, timestamp FROM executor_performance_snapshots WHERE executor_id = '<executor_id>' ORDER BY timestamp;"

Example output

GET /executors/<executor_id> (correct):

{
  "status": "TERMINATED",
  "close_type": "SYSTEM_CLEANUP",
  "net_pnl_quote": -0.0332054,
  "cum_fees_quote": 0.0314054,
  "filled_amount_quote": 78.5135
}

GET /performance/latest?subject=executor for the same executor_id (stale):

{
  "timestamp": "2026-09-08T15:04:02.974401+00:00",
  "status": "RUNNING",
  "is_terminal": false,
  "close_type": null,
  "unrealized_pnl_quote": -0.0332054,
  "cum_fees_quote": 0.0314054
}

Raw executor_performance_snapshots rows for the same executor — only the pre-crash live snapshots, no terminal row:

 executor_id  | status  | close_type | net_pnl_quote | cum_fees_quote |          timestamp
--------------+---------+------------+----------------+----------------+-------------------------------
 <executor_id>| RUNNING |            | -0.0332054     | 0.0314054      | 2026-09-08 15:03:02.951977+00
 <executor_id>| RUNNING |            | -0.0332054     | 0.0314054      | 2026-09-08 15:04:02.974401+00

Note there is no third row with status = TERMINATED / close_type = SYSTEM_CLEANUP, so /performance/latest has no terminal row to serve and keeps returning the last RUNNING one indefinitely.

[david-hummingbot]

Describe the bug

BotRunRepository.update_bot_run_stopped (database/repositories/bot_run_repository.py) sets bot_run.stopped_at = datetime.utcnow() — a timezone-naive datetime — on a column declared TIMESTAMP(timezone=True). When this naive value is written, it is stored as if it were already in the database session's local timezone rather than UTC, so the persisted stopped_at ends up offset from the real UTC stop time by the server's local UTC offset (8 hours in this environment).

By contrast, BotRunRepository.update_bot_run_archived (same file) sets stopped_at = datetime.now(timezone.utc) — a timezone-aware UTC value — which stores and reads back correctly.

Because stop-and-archive-bot's workflow calls update_bot_run_stopped first and then update_bot_run_archived at the end, the second call overwrites the wrong value with a correct one, masking the bug for that path. The bug is permanent, however, for any bot that is stopped via the plain POST /bot-orchestration/stop-bot route and never archived (or not archived immediately) — its stopped_at stays wrong indefinitely.

deployed_at on the same row is set correctly (from a different code path) and is not affected — only stopped_at set via update_bot_run_stopped is wrong.

This matters beyond the raw field: condor (the companion Telegram/web client) reads stopped_at for run-duration display and for attributing performance-history windows to a bot run (frontend/src/lib/run-attribution.ts, frontend/src/lib/perf-tree.ts), so a wrong stopped_at on a stopped-not-archived bot skews those calculations by the server's UTC offset.

Reproduced twice on feat/burndown-improvements (commit 8f3f056) — identical 8-hour offset both times.

Steps to reproduce bug

1. Deploy any V2 controller bot, e.g.:

   curl -s -u admin:admin -X POST "http://localhost:8000/bot-orchestration/deploy-v2-controllers" \
     -H "Content-Type: application/json" \
     -d '{
       "instance_name": "tz_repro",
       "credentials_profile": "master_account",
       "controllers_config": ["<any saved controller config>"],
       "image": "hummingbot/hummingbot:latest",
       "headless": true
     }'

2. Wait for it to report running via GET /bot-orchestration/status.
3. Note the current wall-clock UTC time, then stop it with the plain (non-archiving) stop route:

   date -u
   curl -s -u admin:admin -X POST "http://localhost:8000/bot-orchestration/stop-bot" \
     -H "Content-Type: application/json" \
     -d '{"bot_name": "<unique_instance_name>"}'

4. Immediately compare the recorded stopped_at against the wall-clock time from step 3:

   curl -s -u admin:admin "http://localhost:8000/bot-orchestration/bot-runs" | python3 -m json.tool

Example output

Actual stop time (step 3): Tue Sep 8 15:32:29 UTC 2026

Bot-run record returned in step 4:

{
  "bot_name": "burndown_retest1-20260908-233145",
  "deployed_at": "2026-09-08T15:31:46.221226+00:00",
  "stopped_at": "2026-09-08T07:32:29.817197+00:00",
  "deployment_status": "DEPLOYED",
  "run_status": "STOPPED"
}

stopped_at (07:32:29 UTC) is exactly 8 hours behind the real stop time (15:32:29 UTC), while deployed_at (15:31:46 UTC) is correct. This was reproduced identically in two separate runs (06:56:56 vs actual 14:56:58, and 06:58:49 vs actual 14:58:49, and again 07:32:29 vs actual 15:32:29) — always exactly an 8-hour offset, matching this server's local UTC offset.

For comparison, when the same bot is stopped via stop-and-archive-bot instead, stopped_at ends up correct (14:56:00+00:00 for a bot deployed at 14:55:11+00:00) because the later update_bot_run_archived call overwrites the wrong value.

[david-hummingbot]

Describe the bug

GET /archived-bots/{db_path}/performance and GET /archived-bots/{db_path}/summary both return 500 Internal Server Error for an archived bot whose TradeFill table is empty (i.e., every order it placed failed before ever filling — no trades at all).

Both routes go through utils/hummingbot_database_reader.py. get_trade_fills() reads the TradeFill table into a pandas DataFrame via pd.read_sql_query(...) and then calls:

trade_fills["cum_fees_in_quote"] = trade_fills.groupby(groupers)["trade_fee_in_quote"].cumsum()

When the table is empty, the resulting DataFrame's numeric columns come back as object dtype instead of a numeric dtype (pandas can't infer a numeric dtype from zero rows), and .cumsum() raises TypeError: cumsum is not supported for object dtype for an object-dtype column, which the route surfaces as a generic 500.

This is a plausible real-world case, not just a synthetic one: a bot with a misconfigured order size (e.g., below the exchange's minimum notional) will have every order rejected and end up with exactly this all-failed, zero-fill state once archived.

Steps to reproduce bug

1. Deploy a controller whose orders will all be rejected by the exchange, e.g. a pmm_simple config with the per-level order notional below the trading pair's minimum notional (for BTC-USDT on binance_perpetual_testnet, minimum notional is 50):

   curl -s -u admin:admin -X POST "http://localhost:8000/controllers/configs/repro_zero_fill" \
     -H "Content-Type: application/json" \
     -d '{
       "id": "repro_zero_fill",
       "controller_name": "pmm_simple",
       "controller_type": "market_making",
       "connector_name": "binance_perpetual_testnet",
       "trading_pair": "BTC-USDT",
       "total_amount_quote": 100,
       "buy_spreads": [0.001, 0.002],
       "sell_spreads": [0.001, 0.002],
       "buy_amounts_pct": [0.5, 0.5],
       "sell_amounts_pct": [0.5, 0.5],
       "leverage": 5
     }'
   curl -s -u admin:admin -X POST "http://localhost:8000/bot-orchestration/deploy-v2-controllers" \
     -H "Content-Type: application/json" \
     -d '{
       "instance_name": "zero_fill_repro",
       "credentials_profile": "master_account",
       "controllers_config": ["repro_zero_fill"],
       "image": "hummingbot/hummingbot:latest",
       "headless": true
     }'

   (Each order's notional works out to ~$23.5, below the $50 minimum, so every order is rejected with Order notional 23.5... is less than the minimum notional.)
2. Let it run briefly, then stop and archive it:

   curl -s -u admin:admin -X POST "http://localhost:8000/bot-orchestration/stop-and-archive-bot/zero_fill_repro-<timestamp>"

3. Once archived, call the archived-bot performance/summary routes:

   DB="bots/archived/zero_fill_repro-<timestamp>/data/zero_fill_repro-<timestamp>.sqlite"
   ENC=$(python3 -c "import urllib.parse; print(urllib.parse.quote('$DB', safe=''))")
   curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin "http://localhost:8000/archived-bots/$ENC/performance"
   curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin "http://localhost:8000/archived-bots/$ENC/summary"

Example output

$ curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin "http://localhost:8000/archived-bots/<encoded_db_path>/performance"
{"detail":"Error calculating performance: cumsum is not supported for object dtype"}
HTTP:500

$ curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin "http://localhost:8000/archived-bots/<encoded_db_path>/summary"
{"detail":"Error analyzing database: cumsum is not supported for object dtype"}
HTTP:500

Confirmed the underlying TradeFill table is empty for this archived bot (every executor's close_type was 8 / FAILED, and GET /archived-bots/<db>/orders returns "orders": []), while GET /archived-bots/<db>/executors correctly returns the failed executor rows. So the executors/orders surfaces handle the zero-fill case fine; only /performance and /summary crash on it.

Reproduced twice on feat/burndown-improvements (commit 8f3f056) — identical error both times.

[cardosofede]

@david-hummingbot all three reproduced and fixed — aaba4b8, 6d2b723, e57e119. Full suite green, 816 passed.

1. The reap never wrote a terminal row (aaba4b8)

Exactly as diagnosed. cleanup_orphaned_executors only ever mutated the executors table, which made it the one path to TERMINATED with no terminal row behind it — so the invariant the /performance routes are built on held for every executor except the ones a crash caught, which is the least convenient possible exception.

The reap now writes the terminal row the completion path writes, in the same transaction as the record update, so the two surfaces cannot disagree about whether an executor is done. It reuses the completion path's SAVEPOINT discipline for the same reason that one has it: the reap is the accounting, the snapshot is a point on a chart, and a failing INSERT must roll back only itself rather than abort the reap and leave the whole fleet RUNNING.

Two details worth stating because they are decisions rather than mechanics:

• The identity comes off the record, not off the last snapshot. An executor orphaned inside its first snapshot interval has no snapshot to read executor_type / connector_name / trading_pair from, and those columns are constant across an executor's life anyway.
• An executor with no snapshot still gets a terminal row, carrying the zeros the record itself books. They are not fabricated figures — they are what /executors/{id} reports for that executor — and close_type: SYSTEM_CLEANUP marks the close as approximated on both surfaces, the same signal the record carries. The alternative was a gap, which is the state you just reported.

Four tests: the row is written with the record's post-adoption figures at closed_at, the identity comes from the record when there is no snapshot, the no-snapshot case still ends its series, and a failing snapshot insert leaves the reap committed.

Both route docstrings now say that a reaped executor's terminal row is a SYSTEM_CLEANUP approximated close, so the is_terminal: true / TERMINATED / last-observed-figures combination is documented rather than inferred.

2. stopped_at written naive against a TIMESTAMP(timezone=True) column (6d2b723)

datetime.utcnow() → datetime.now(timezone.utc). Your reading of the mechanism is right, including why stop-and-archive masks it: update_bot_run_archived overwrites the skewed value with a correct one, so only a stopped-and-not-archived run keeps it.

One more instance of the same class while I was in there: PositionHold.last_updated also used utcnow(). It is an in-memory field rather than a column, but the API serializes it with isoformat() in the same responses as values read back from the database — which are aware — so the same field reached a consumer labelled UTC on one path and unlabelled on the other. Both now use aware UTC.

Since nothing in the code or its tests said the naive call was unsafe, there is now a test that walks every repository's AST for utcnow() calls and fails on any of them. Every timestamp column in database/models.py is TIMESTAMP(timezone=True), so that rule has no exceptions to carve out.

3. Zero-fill archive 500s (e57e119)

Confirmed, and your read of the cause is exactly it: a zero-row read_sql_query yields object columns because pandas has nothing to infer a dtype from, and cumsum refuses object dtype however empty the frame is.

Fixed by coercing the scaled columns to numeric before any arithmetic — in all three readers that divide by 1e6 (get_trade_fills, get_orders, get_positions), because the trap is identical in each and a dtype that does not depend on row count is what every caller was already assuming. Coercion is per column rather than via DataFrame.apply, since apply never calls its function on a frame with no rows: precisely the case it would need to handle.

Tests build a real sqlite archive in your reported shape — rows in Order, none in TradeFill — and assert both routes answer instead of raising: /summary counts 1 order and 0 trades, /performance returns its "No trades found in database" body. Plus a with-rows case pinning that the scaling and the cumulative fees are unchanged, since a coercion that quietly altered a normal archive's numbers would be a worse bug than the one it fixes.

On the earlier POSITION_HOLD item

Still standing where we left it: the API keeps POSITION_HOLD unrealized and now says so in the schema and the route docstring, so the remaining change is Condor-side. Say the word and I'll file it there.

[david-hummingbot]

Describe the bug

POST /backtesting/run fails 100% of the time in this environment with a generic, uninformative error:

{"detail":"Backtest worker exited with code 1 without producing a result"}

This happens even for a fully valid config against a real connector — the backtest computation itself completes successfully every time. The failure is in a completely separate step: writing the result to disk.

Root cause, confirmed via a full traceback: bots/data/backtests/ (the on-disk candle-cache/results directory this branch's PERF-112 introduces) is owned by root:root, while the API process runs as the host user (madcatz in this environment) — a very plausible outcome of this project's documented workflow of running infrastructure via docker-compose (where the API container itself runs as root and shares the bind-mounted ./bots:/hummingbot-api/bots volume) while also running the API directly on the host for local development. Every other subdirectory under bots/ (instances, archived, controllers, credentials, scripts, conf) is host-user-owned; only bots/data and bots/gateway-files are root:root.

Because of this, _worker_main's attempt to write its result envelope fails:

File "services/backtesting_service.py", line 242, in _worker_main
    with open(out_path, "wb") as fh:
PermissionError: [Errno 13] Permission denied: 'bots/data/backtests/.worker-<uuid>.pkl'

Compounding bug: that open(out_path, "wb") call sits outside _worker_main's try/except block:

def _worker_main(config, controllers_path, controllers_module, out_path) -> None:
    try:
        result = _run_backtest_blocking(config, controllers_path, controllers_module)
        blob = pickle.dumps({"ok": True, "result": result}, ...)
    except Exception as e:
        blob = pickle.dumps({"ok": False, "error": ..., "traceback": ...})
    with open(out_path, "wb") as fh:   # <-- not covered by the try/except above
        fh.write(blob)

So instead of the worker producing a {"ok": False, "error": ..., "traceback": ...} envelope that the API could surface as a specific, actionable 500 (which is exactly what CORR-101 in this same PR added the machinery for), the worker process crashes with an uncaught exception, exits with code 1, writes nothing, and the parent has zero information beyond "exited with code 1" — the real traceback only exists in the worker's stderr, which is not captured or logged anywhere the API or its caller can see (confirmed by redirecting stdout/stderr manually — see repro).

The candle-cache feature in the same PERF-112 commit already hits the identical permission error on bots/data/backtests/candles/ but handles it gracefully (falls back to running without cache, logged as a WARNING) — the result-file write is the one place this class of failure isn't guarded.

Steps to reproduce bug

1. Confirm the ownership mismatch:

   ls -la bots/data/backtests/   # root:root
   ls -la bots/instances/        # host-user:host-user

2. Run any backtest with a valid config:

   curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin -X POST "http://localhost:8000/backtesting/run" \
     -H "Content-Type: application/json" \
     -d '{
       "start_time": 1735689600, "end_time": 1735776000,
       "backtesting_resolution": "1h", "trade_cost": 0.0006,
       "config": {
         "id": "bt_test", "controller_name": "pmm_simple", "controller_type": "market_making",
         "connector_name": "binance_perpetual", "trading_pair": "BTC-USDT", "total_amount_quote": 100,
         "buy_spreads": [0.01, 0.02], "sell_spreads": [0.01, 0.02],
         "buy_amounts_pct": [0.5, 0.5], "sell_amounts_pct": [0.5, 0.5]
       }
     }'

3. To see the real traceback (invisible through the API), run the server with stdout/stderr captured to a file (e.g. uvicorn main:app --port 8001 > server.log 2>&1 &) and repeat step 2, then check server.log.

Example output

API response, every time, regardless of config validity:

{"detail":"Backtest worker exited with code 1 without producing a result"}
HTTP:500

Captured server log showing the real cause:

2026-09-09 00:57:33,945 - services.candles_cache - WARNING - Candle cache at bots/data/backtests/candles is unusable, running without it: [Errno 13] Permission denied: 'bots/data/backtests/candles'
Process SpawnProcess-1:
Traceback (most recent call last):
  File ".../multiprocessing/process.py", line 314, in _bootstrap
    self.run()
  File ".../multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "services/backtesting_service.py", line 242, in _worker_main
    with open(out_path, "wb") as fh:
         ^^^^^^^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: 'bots/data/backtests/.worker-fd7a12603ac044f09709cc19bd921ea9.pkl'

Calling _run_backtest_blocking directly (bypassing the worker subprocess entirely) with the identical config succeeds and returns a real result — proving the backtest logic itself is correct and the failure is isolated to the result-file write.

Provenance

Introduced by PR #226. git diff origin/main -- services/backtesting_service.py shows _worker_main — including the unguarded open(out_path, "wb") write that's the actual bug — as entirely new code (every line of the function appears as a + addition; no prior version existed on main). This is part of this branch's "run each backtest in its own killable worker process" work (PERF-112 and related). The trigger for the crash (the root-owned bots/data/ directory) is an environment/deployment artifact rather than code in any repo, but the fact that hitting it produces a silent, uninformative 500 instead of a clear error is new code this PR added.

Does this reproduce when hummingbot-api itself runs in Docker?

No — not this specific trigger. The Dockerfile has no USER directive, so the hummingbot-api container runs as root, and root bypasses host file-ownership checks entirely. Confirmed empirically:

$ docker run --rm -v $(pwd)/bots:/hummingbot-api/bots continuumio/miniconda3:latest \
    bash -c "whoami && touch /hummingbot-api/bots/data/backtests/.write-test && echo WRITE_SUCCEEDED"
root
WRITE_SUCCEEDED

So this exact symptom is specific to the environment it was found in: hummingbot-api run directly on the host (conda run uvicorn, as a non-root user) against a bots/ tree where bots/data/ had already been created root-owned by something else sharing that bind mount (almost certainly a hummingbot bot-instance container, which always runs as root). That's a plausible, and arguably common, situation — infra started via docker-compose while the API itself is run locally from source for hot-reload during development — but it will not appear for an all-Docker deployment.

That doesn't make the underlying code issue moot, though: _worker_main's result-write sitting outside its own try/except means any write failure there — disk full, a quota, a permissions quirk under some other orchestrator, an unusual mount — will still silently kill the worker with zero diagnostics ever reaching the API or its caller, regardless of deployment mode. The root-owned-directory case is simply the one that happened to be reproducible here.

[david-hummingbot]

Describe the bug

POST /accounts/add-credential/{account_name}/{connector_name} leaks a raw Python AttributeError as the HTTP error detail whenever the request body omits a required credential field, instead of a clean validation message naming the missing field(s).

Root cause: UnifiedConnectorService.update_connector_keys (services/unified_connector_service.py) builds the connector config purely from whatever keys the caller supplied —

connector_config = HummingbotAPIConfigAdapter(
    AllConnectorSettings.get_connector_config_keys(connector_name)
)
for key, value in keys.items():
    setattr(connector_config, key, value)

— with no check that every field GET /connectors/{connector_name}/config-map marks "required": true was actually supplied. When a required field is missing, something downstream (BackendAPISecurity.update_connector_keys / decrypt_all) reads that never-set attribute and raises AttributeError, which the route's generic exception handler surfaces verbatim, including the internal Pydantic model class name (e.g. BinancePerpetualTestnetConfigMap).

Reproduces identically across every connector tried (binance_perpetual_testnet, binance, xrpl), so it's a generic gap in the credential-save path, not connector-specific.

Steps to reproduce bug

1. Check the required fields for any connector:

   curl -s -u admin:admin "http://localhost:8000/connectors/binance_perpetual_testnet/config-map"

2. Submit an empty (or partially filled) credentials body for that connector:

   curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin -X POST \
     "http://localhost:8000/accounts/add-credential/master_account/binance_perpetual_testnet" \
     -H "Content-Type: application/json" -d '{}'

Example output

$ curl ... -d '{}'
{"detail":"'BinancePerpetualTestnetConfigMap' object has no attribute 'binance_perpetual_testnet_api_key'"}
HTTP:400

$ curl ... -d '{"binance_perpetual_testnet_api_key": "dummy"}'   # secret still missing
{"detail":"'BinancePerpetualTestnetConfigMap' object has no attribute 'binance_perpetual_testnet_api_secret'"}
HTTP:400

$ curl .../add-credential/master_account/binance -d '{}'
{"detail":"'BinanceConfigMap' object has no attribute 'binance_api_key'"}
HTTP:400

$ curl .../add-credential/master_account/xrpl -d '{}'
{"detail":"'XRPLConfigMap' object has no attribute 'xrpl_secret_key'"}
HTTP:400

Provenance

Pre-existing — not introduced by PR #226. git diff origin/main -- services/unified_connector_service.py shows no changes to update_connector_keys; git log -L on that function's line range shows its last modification was commit fc6eaf6 ("prevent double initialization"), unrelated to and predating this branch.

[david-hummingbot]

Describe the bug

POST /controllers/{type}/{name}/config/validate (and, by the same code path, saving a config via POST /controllers/configs/{name} and deploying it) only checks that the submitted JSON satisfies the controller's Pydantic field shapes — it does not check that referenced connectors exist, and it does not enforce the same strict enum coercion the actual runtime uses. The result: a config that will deterministically crash the moment it is deployed is reported as "Configuration is valid", saved successfully, and deployed with "success": true, and only fails once the Docker container is already up and the strategy tries to start.

Two independent ways to trigger this, both reproduced:

1. Nonexistent connector. connector_name is validated as a plain string, never checked against the actual connector registry:

curl -s -X POST ".../controllers/market_making/pmm_simple/config/validate" -d '{"id":"t","controller_name":"pmm_simple","controller_type":"market_making","connector_name":"not_a_connector","trading_pair":"BTC-USDT","total_amount_quote":100}'
# -> {"message":"Configuration is valid"}

Saving and deploying this config succeeds at the API level (deploy-v2-controllers returns "success": true, a container is created), but the container crashes and exits within seconds:

connector_manager - Failed to create connector not_a_connector: 'not_a_connector'
trading_core - Failed to start strategy v2_with_controllers: 'not_a_connector'
root - Failed to load strategy. Exiting.

The resulting bot_runs row is left permanently stuck at deployment_status: "DEPLOYED", run_status: "CREATED", error_message: null — nothing ever reconciles it once the container is gone (see repro below).

2. Human-readable enum string instead of the numeric enum. The same gap applies to any TradeType-typed field (e.g. grid_strike's side): "side": "BUY" passes /validate and save, but the actual strategy load inside the container requires the raw enum int:

trading_core - Failed to start strategy v2_with_controllers: 1 validation error for GridStrikeConfig
side
  Input should be 1, 2 or 3 [type=enum, input_value='BUY', input_type=str]

(The same "BUY"-string-vs-numeric-enum mismatch also affects POST /executors/ directly — see the side field there — suggesting this is a systemic pattern anywhere a TradeType/similar enum is user-facing.)

Steps to reproduce bug (nonexistent connector → phantom bot_runs row)

1. Validate and save a config with a bogus connector:

   curl -s -X POST "http://localhost:8000/controllers/market_making/pmm_simple/config/validate" \
     -H "Content-Type: application/json" \
     -d '{"id":"t","controller_name":"pmm_simple","controller_type":"market_making","connector_name":"not_a_connector","trading_pair":"BTC-USDT","total_amount_quote":100}'
   curl -s -X POST "http://localhost:8000/controllers/configs/qa_bogus_connector_test" \
     -H "Content-Type: application/json" \
     -d '{"id":"qa_bogus_connector_test","controller_name":"pmm_simple","controller_type":"market_making","connector_name":"not_a_connector","trading_pair":"BTC-USDT","total_amount_quote":100}'

2. Deploy it:

   curl -s -X POST "http://localhost:8000/bot-orchestration/deploy-v2-controllers" \
     -H "Content-Type: application/json" \
     -d '{"instance_name":"qa_bogus_connector_bot","credentials_profile":"master_account","controllers_config":["qa_bogus_connector_test"],"image":"hummingbot/hummingbot:latest","headless":true}'

3. Wait ~15s, then check the container and the bot_runs row:

   curl -s -u admin:admin "http://localhost:8000/docker/exited-containers"
   curl -s -u admin:admin "http://localhost:8000/bot-orchestration/bot-runs"

Example output

Deploy response (falsely reports success):

{"success":true,"message":"Instance qa_bogus_connector_bot-20260909-004931 created successfully.", ...}

Container state seconds later:

[{"name": "qa_bogus_connector_bot-20260909-004931", "status": "exited", "image": "hummingbot/hummingbot:latest"}]

The permanently-orphaned bot_runs row (unchanged no matter how long you wait, or even after manually removing the exited container):

{
  "id": 7,
  "bot_name": "qa_bogus_connector_bot-20260909-004931",
  "deployed_at": "2026-09-08T16:49:31.297332+00:00",
  "stopped_at": null,
  "deployment_status": "DEPLOYED",
  "run_status": "CREATED",
  "error_message": null
}

GET /bot-orchestration/bot-runs/stats also carries this row forever under status_counts.CREATED, permanently showing one bot "stuck" in a state that never resolves.

Related: config template misreports a required field

GET /controllers/{type}/{name}/config/template (the introspection endpoint meant to help a caller build a valid config) reports the id field as optional with a placeholder default, when /validate actually rejects a config that omits it:

$ curl -s .../controllers/market_making/pmm_simple/config/template | python3 -c "import json,sys; print(json.load(sys.stdin)['id'])"
{"default": "PydanticUndefined", "type": "<class 'str'>", "required": false}

$ curl -s -X POST .../controllers/market_making/pmm_simple/config/validate -d '{"controller_name":"pmm_simple","controller_type":"market_making","connector_name":"binance_perpetual_testnet","trading_pair":"BTC-USDT","total_amount_quote":100}'
{"detail":"1 validation error for PMMSimpleConfig\nid\n  Field required [type=missing, ...]"}

The "default": "PydanticUndefined" value is Pydantic's internal missing-default sentinel, stringified directly into the JSON response rather than being translated into "required": true, "default": null — so a client following the template literally would omit id and get rejected.

Provenance

Pre-existing — not introduced by PR #226. git diff origin/main -- routers/controllers.py (which owns /validate and /config/template) shows zero changes — the file is byte-identical to main. git diff origin/main -- routers/bot_orchestration.py does show changes, but every hunk is scoped to stop_and_archive_bot (the READ-115 ownership-check fix); deploy_v2_controllers and deploy_v2_script fall outside every changed hunk and are untouched.

[david-hummingbot]

Describe the bug

POST /docker/stop-container/{container_name} returns HTTP 200 even when the container does not exist, with a raw docker-py exception string as the response body instead of a clean 404/400 error.

{"detail": "Not found"}   <- expected, something like this

vs. what actually comes back:

"404 Client Error for http+docker://localhost/v1.55/containers/<name>/json: Not Found (\"No such container: <name>\")"

on HTTP 200. This leaks the internal docker socket URL and API version, and — more importantly — a caller checking only the status code (the normal way to detect failure) will see 200 and assume the stop succeeded.

Steps to reproduce bug

curl -s -w "\nHTTP:%{http_code}\n" -u admin:admin -X POST \
  "http://localhost:8000/docker/stop-container/this-container-does-not-exist"

Example output

"404 Client Error for http+docker://localhost/v1.55/containers/this-container-does-not-exist/json: Not Found (\"No such container: this-container-does-not-exist\")"
HTTP:200

Provenance

Pre-existing — not introduced by PR #226. git diff origin/main -- routers/docker.py is scoped entirely to remove_container (the READ-115 ownership-check fix) plus incidental whitespace/docstring cleanup elsewhere in the file; the stop_container route body is unchanged. git diff origin/main -- services/docker_service.py shows two changed hunks, both well after (line 174+) DockerService.stop_container (defined at line 129) — that method itself is untouched.

[cardosofede]

@david-hummingbot the stop-container one is fixed in 1977383. Full suite green, 829 passed.

Both halves of it confirmed. A missing container now answers 404 with No such container: <name>, and a daemon that refuses or cannot be reached answers 502 — the raw docker-py string, socket URL and API version included, goes to the log where it belongs. A successful stop now returns {"success": true, "message": ...} rather than null, which was itself indistinguishable from an error body.

I took the whole shape rather than the one route. Every container operation in DockerService was except DockerException as e: return str(e), and every route returned it verbatim, so start-container, clean-exited-containers and both listing routes gave the same 200-with-an-error-string answer — the listings while documenting a list as their return type. DockerService now separates NotFound from the rest and reports the kind of failure; routers/docker.py maps kind to status code in one place.

Two decisions worth stating, since both are cases where the obvious fix would have been wrong:

Failures stay in-band rather than becoming exceptions. stop_and_archive_bot calls stop_container in a ten-attempt retry loop and decides whether it worked by reading the container's status afterwards — it is built to tolerate a stop that did not take. Raising would abort that workflow at the first attempt, so the service returns the failure and the HTTP layer is what turns it into a status code.

remove-container deliberately keeps its 200, called out at the call site. That route also archives, and a container that is already gone is a bot whose data still needs archiving — a removal that finds nothing to remove has reached the end state the caller asked for. Its response still carries success for a caller that wants to know which of the two happened. Happy to make it 404 if you would rather it be strict, but that would mean a bot whose container was removed out from under the API can no longer be archived through this route.

13 tests, driven by the exception docker-py actually raises (socket URL and v1.55 in the message), including an explicit assertion that neither string can reach a response body, and one pinning the in-band contract the retry loop depends on.

The other three

Not touched yet — say which you want first and I'll take them in order:

• Backtest worker's unguarded result write. The environment trigger is yours to keep or fix, but you are right that the open(out_path, "wb") outside _worker_main's try/except is this PR's code and is what turns any write failure into exited with code 1 with no diagnostics. That one is squarely mine.
• add-credential leaking AttributeError. Pre-existing, as you found, but the fix is small: check the supplied keys against the connector's own required fields before building the config.
• /config/validate accepting a config that cannot run. The largest of the three — connector-registry validation plus strict enum coercion, and the phantom bot_runs row left behind by a container that dies on startup is arguably a separate reconciliation fix.