Skip to content

POST /connection/login bricks the API: old-client shutdown closes the shared process-global MT5 connection #45

Description

@dceoy

Problem

POST /connection/login reports success (connected: true) but leaves the process with a closed MT5 connection: after installing the new client, replace_mt5_client shuts down the old client, and all pdmt5 client instances share the single process-global MetaTrader5 module connection.

Evidence

  • mt5api/dependencies.py:106-125 (replace_mt5_client): builds and initializes new_client, assigns _mt5_client = new_client, then calls old_client.shutdown().
  • In pdmt5 (>= 1.0.1, the pinned minimum), every client wraps the same module object: Mt5Client.mt5 uses default_factory=lambda: importlib.import_module("MetaTrader5") (pdmt5/mt5.py), and importlib.import_module returns the cached sys.modules entry. The MetaTrader5 Python API holds one IPC connection per process, so "old client" and "new client" are never two separate connections.
  • Mt5Client.shutdown() delegates to self.mt5.shutdown() (pdmt5/mt5.py), which closes the connection new_client.initialize_and_login_mt5() just established.
  • The new client will not recover: Mt5DataClient._initialize_if_needed() (pdmt5/dataframe.py) only re-initializes when _is_initialized is False, and new_client._is_initialized is still True — only old_client's flag was cleared. Subsequent MT5 module calls return None/errors, so every endpoint fails with 503 (Mt5RuntimeError) until the process is restarted or another /connection/login succeeds.
  • Tests do not catch this because each mocked client has an independent fake connection; tests/test_connection.py:377 (old_client.shutdown.assert_called_once_with()) actually pins the broken behavior.

Two secondary defects in the same flow:

  1. The docstring guarantee in replace_mt5_client ("a failed reconnect leaves the existing client in place instead of disconnecting the operator from a working terminal") cannot hold: new_client.initialize_and_login_mt5() calls mt5.initialize(login=…, password=…, server=…) on the shared module, so a failed login attempt has already torn down / re-pointed the existing session even though the old Python object stays installed.
  2. mt5api/routers/connection.py:58-60 releases all market-book subscriptions before attempting the reconnect, so a failed login (503) still silently drops every active DOM subscription.

Impact

Any use of the documented reconnect endpoint (README, docs/api/rest-api.md, skills/mt5api/SKILL.md) bricks the API on a real Windows/MT5 host: login returns 200/connected: true, then all data endpoints return 503. This is the primary operational endpoint for switching accounts.

Suggested fix

Model the MT5 connection as process-global instead of pretending clients own independent connections:

  • On successful reconnect, drop the old client reference without calling shutdown() (or make reconnect operate on a single long-lived client via initialize_and_login_mt5(login=…, password=…, server=…, timeout=…) overrides instead of swapping instances).
  • Update the replace_mt5_client docstring/tests to stop promising the old session survives a failed attempt, or explicitly attempt re-login with the previous config on failure.
  • Release market-book subscriptions only after the new connection is established (or re-subscribe on failure).
  • Fix tests/test_connection.py assertions that currently require old_client.shutdown().

Validation

  • Unit: update tests/test_connection.py so the success path asserts the shared connection is not shut down; keep 100% coverage (uv run pytest).
  • Manual (Windows + MT5 terminal): POST /connection/login with valid credentials, then GET /account — it must return 200, not 503. Also verify a failed login (bad password) leaves prior behavior in a documented state.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions