Harden order/risk path (execution+broker audit): 3 HIGH + 2 MED + 1 LOW
HIGH: - Circuit breaker fails CLOSED: a repeated unreadable equity (API 401/429/5xx or missing field) now halts new entries after 3 consecutive failures instead of silently reporting 'no breach' and continuing to trade through a drawdown. - execute(): map order statuses. Terminal-fail (rejected/expired/...) -> ok=False (never armed as a resting entry); record ANY fill (partial too), not only an exact 'filled'; require a real broker order id. approve_order only arms genuinely OPEN statuses. - sync: account/positions/orders failures now emit a BrokerHealthChangedEvent (degraded) and step_sync surfaces 'DEGRADED', instead of a success-shaped None that left risk gates reading stale snapshots as current. MEDIUM: - _flatten_all records a position closed only on a CONFIRMED close; the breaker event reports 'halted_flatten_incomplete' + the unclosed symbols when a close fails, instead of claiming the book is flat. - submit_exit_order prefers close_position (handles short sign + fractional qty; a plain sell of abs(qty) would double a short). LOW: - get_orders(nested=true) so bracket child legs aren't dropped from the snapshot. + tests/unit/test_execution_safety.py (4 regressions). 75/75 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
c146341bb4
commit
660cd494d5
6 files changed
+200
-21
No files matched your search
@@ -79,9 +79,14 @@ class AlpacaPaperClient:
|
||||
def get_positions(self) -> list[dict]:
|
||||
return self._trading("GET", "/positions")
|
||||
|
||||
def get_orders(self, status: str = "all", limit: int = 100) -> list[dict]:
|
||||
def get_orders(
|
||||
self, status: str = "all", limit: int = 100, nested: bool = True
|
||||
) -> list[dict]:
|
||||
# nested=true includes bracket child legs (stop/target) so working risk
|
||||
# isn't under-reported in the orders snapshot.
|
||||
return self._trading(
|
||||
"GET", "/orders", params={"status": status, "limit": limit}
|
||||
"GET", "/orders",
|
||||
params={"status": status, "limit": limit, "nested": str(nested).lower()},
|
||||
)
|
||||
|
||||
def submit_order(
|
||||
|
||||
@@ -130,6 +130,16 @@ class AlpacaPaperExecutor:
|
||||
|
||||
broker_order_id = raw.get("id")
|
||||
status = raw.get("status")
|
||||
|
||||
# An empty/odd 200 body with no order id is not a real submission — don't
|
||||
# return ok=True and let it be armed as an untrackable resting order.
|
||||
if not broker_order_id:
|
||||
self._emit_cancelled(request, f"broker returned no order id (status={status})")
|
||||
return ExecutionResult(
|
||||
ok=False, order_id=request.order_id, status="error",
|
||||
error="no broker order id", raw=raw,
|
||||
)
|
||||
|
||||
self.store.emit(
|
||||
OrderSubmittedEvent(
|
||||
timestamp=datetime.now(),
|
||||
@@ -156,27 +166,40 @@ class AlpacaPaperExecutor:
|
||||
)
|
||||
)
|
||||
|
||||
if status == "filled":
|
||||
# Record ANY fill (partial or full), not only an exact 'filled' status,
|
||||
# so positions / P&L never under-report real shares.
|
||||
filled_qty = int(float(raw.get("filled_qty") or 0))
|
||||
if filled_qty > 0:
|
||||
fill_price = float(raw.get("filled_avg_price") or 0) or None
|
||||
self.store.emit(
|
||||
OrderFilledEvent(
|
||||
timestamp=datetime.now(),
|
||||
mode=self.mode,
|
||||
correlation_id=self.session_id,
|
||||
message=f"Filled {request.symbol} @ {fill_price}",
|
||||
message=f"Filled {request.symbol} {filled_qty} @ {fill_price}",
|
||||
symbol=request.symbol,
|
||||
order_id=request.order_id,
|
||||
fill_price=fill_price or 0.0,
|
||||
fill_quantity=int(float(raw.get("filled_qty") or 0)),
|
||||
fill_quantity=filled_qty,
|
||||
payload={
|
||||
"symbol": request.symbol,
|
||||
"order_id": request.order_id,
|
||||
"fill_price": fill_price,
|
||||
"fill_quantity": int(float(raw.get("filled_qty") or 0)),
|
||||
"fill_quantity": filled_qty,
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
# Terminal-failure statuses are NOT live orders: report ok=False so the
|
||||
# caller never arms a rejected/expired order as a resting entry.
|
||||
if status in {"rejected", "canceled", "cancelled", "expired",
|
||||
"done_for_day", "suspended", "stopped"}:
|
||||
self._emit_cancelled(request, f"broker {status}")
|
||||
return ExecutionResult(
|
||||
ok=False, order_id=request.order_id,
|
||||
broker_order_id=broker_order_id, status=status, raw=raw,
|
||||
)
|
||||
|
||||
return ExecutionResult(
|
||||
ok=True,
|
||||
order_id=request.order_id,
|
||||
|
||||
+22
-3
@@ -14,6 +14,7 @@ from storage.event_schema import (
|
||||
AccountOrdersUpdatedEvent,
|
||||
AccountPositionsUpdatedEvent,
|
||||
AccountSummaryUpdatedEvent,
|
||||
BrokerHealthChangedEvent,
|
||||
EventMode,
|
||||
)
|
||||
from storage.event_store import EventStore
|
||||
@@ -36,11 +37,27 @@ class AlpacaPaperSync:
|
||||
self.session_id = session_id
|
||||
self.mode = mode
|
||||
|
||||
def _emit_health(self, reason: str) -> None:
|
||||
"""Make a broker sync failure VISIBLE (else stale snapshots look current)."""
|
||||
self.store.emit(
|
||||
BrokerHealthChangedEvent(
|
||||
timestamp=datetime.now(),
|
||||
mode=self.mode,
|
||||
correlation_id=self.session_id,
|
||||
message=f"alpaca_paper sync degraded: {reason}",
|
||||
broker_name=BROKER_NAME,
|
||||
previous_health="healthy",
|
||||
new_health="degraded",
|
||||
health_reason=reason,
|
||||
)
|
||||
)
|
||||
|
||||
def sync_account(self) -> dict | None:
|
||||
try:
|
||||
account = self.client.get_account()
|
||||
except Exception:
|
||||
except Exception as exc:
|
||||
logger.exception("alpaca account sync failed")
|
||||
self._emit_health(f"account: {exc}")
|
||||
return None
|
||||
account_id = str(account.get("account_number") or account.get("id") or "paper")
|
||||
self.store.emit(
|
||||
@@ -63,8 +80,9 @@ class AlpacaPaperSync:
|
||||
def sync_positions(self) -> list[dict] | None:
|
||||
try:
|
||||
raw = self.client.get_positions()
|
||||
except Exception:
|
||||
except Exception as exc:
|
||||
logger.exception("alpaca positions sync failed")
|
||||
self._emit_health(f"positions: {exc}")
|
||||
return None
|
||||
positions = [
|
||||
{
|
||||
@@ -95,8 +113,9 @@ class AlpacaPaperSync:
|
||||
def sync_orders(self, status: str = "all", limit: int = 50) -> list[dict] | None:
|
||||
try:
|
||||
raw = self.client.get_orders(status=status, limit=limit)
|
||||
except Exception:
|
||||
except Exception as exc:
|
||||
logger.exception("alpaca orders sync failed")
|
||||
self._emit_health(f"orders: {exc}")
|
||||
return None
|
||||
orders = [
|
||||
{
|
||||
|
||||
+3
-2
@@ -335,8 +335,9 @@ def main(argv: list[str] | None = None) -> int:
|
||||
def step_sync():
|
||||
if not rt["sync"]:
|
||||
return "skipped (no keys)"
|
||||
rt["sync"].sync_all()
|
||||
return "ok"
|
||||
res = rt["sync"].sync_all()
|
||||
failed = [k for k, v in res.items() if v is None]
|
||||
return "ok" if not failed else f"DEGRADED ({', '.join(failed)})"
|
||||
|
||||
def step_execute():
|
||||
if not rt["execution"]:
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
"""Execution/broker safety regressions (from the silent-failure audit).
|
||||
|
||||
Locks in: a rejected or id-less order is never reported ok; partial fills are
|
||||
recorded; and the daily-loss circuit breaker fails CLOSED when equity can't be
|
||||
read (rather than silently disabling itself).
|
||||
"""
|
||||
|
||||
from alpaca_paper.execution import AlpacaPaperExecutor, ExecutionRequest
|
||||
from storage.event_store import EventStore
|
||||
from trading_execution import ExecutionSettings, TradingExecutionService
|
||||
|
||||
|
||||
class _FakeClient:
|
||||
def __init__(self, resp=None, account=None, raise_account=False):
|
||||
self.resp = resp or {}
|
||||
self.account = account or {"equity": "100000", "last_equity": "100000"}
|
||||
self.raise_account = raise_account
|
||||
|
||||
def submit_order(self, **kw):
|
||||
return self.resp
|
||||
|
||||
def cancel_order(self, order_id):
|
||||
pass
|
||||
|
||||
def get_account(self):
|
||||
if self.raise_account:
|
||||
raise RuntimeError("API down")
|
||||
return self.account
|
||||
|
||||
|
||||
def _req(symbol="X"):
|
||||
return ExecutionRequest(
|
||||
symbol=symbol, side="buy", quantity=10, entry_price=1.0,
|
||||
stop_loss_price=0.9, take_profit_price=1.2, order_type="limit",
|
||||
)
|
||||
|
||||
|
||||
def test_rejected_order_is_not_ok():
|
||||
ex = AlpacaPaperExecutor(
|
||||
EventStore(":memory:"),
|
||||
client=_FakeClient({"id": "o1", "status": "rejected", "filled_qty": "0"}),
|
||||
)
|
||||
r = ex.execute(_req())
|
||||
assert r.ok is False and r.status == "rejected"
|
||||
|
||||
|
||||
def test_order_with_no_broker_id_is_not_ok():
|
||||
ex = AlpacaPaperExecutor(EventStore(":memory:"), client=_FakeClient({"status": "new"}))
|
||||
assert ex.execute(_req()).ok is False
|
||||
|
||||
|
||||
def test_partial_fill_is_recorded():
|
||||
store = EventStore(":memory:")
|
||||
ex = AlpacaPaperExecutor(store, client=_FakeClient(
|
||||
{"id": "o2", "status": "partially_filled", "filled_qty": "5", "filled_avg_price": "1.01"}))
|
||||
r = ex.execute(_req("Y"))
|
||||
assert r.ok is True
|
||||
assert len(store.query_events(event_type="order_filled", limit=None)) == 1
|
||||
|
||||
|
||||
def test_breaker_fails_closed_when_equity_unreadable():
|
||||
store = EventStore(":memory:")
|
||||
svc = TradingExecutionService(
|
||||
store,
|
||||
executor=AlpacaPaperExecutor(store, client=_FakeClient(raise_account=True)),
|
||||
settings=ExecutionSettings(max_daily_loss_pct=0.03),
|
||||
session_id="cb",
|
||||
)
|
||||
assert svc._daily_loss_breach() is False # 1 failure
|
||||
assert svc._daily_loss_breach() is False # 2 failures
|
||||
assert svc._daily_loss_breach() is True # 3 -> fail CLOSED
|
||||
assert svc._halted is True
|
||||
+69
-10
@@ -142,6 +142,8 @@ class TradingExecutionService:
|
||||
self._armed: dict[str, dict] = {}
|
||||
# daily-loss circuit breaker: once tripped, no new entries this session
|
||||
self._halted = False
|
||||
# consecutive equity-read failures; after a few we fail CLOSED (halt)
|
||||
self._equity_read_failures = 0
|
||||
|
||||
# -- pipeline -----------------------------------------------------------
|
||||
|
||||
@@ -189,10 +191,12 @@ class TradingExecutionService:
|
||||
# 2) market-close open positions
|
||||
client = getattr(self.executor, "client", None)
|
||||
for symbol in sorted(self._held_symbols()):
|
||||
if client is None or not hasattr(client, "close_position"):
|
||||
result["errors"].append(f"close {symbol}: no broker client to flatten")
|
||||
continue
|
||||
try:
|
||||
if client is not None and hasattr(client, "close_position"):
|
||||
client.close_position(symbol)
|
||||
result["closed_positions"].append(symbol)
|
||||
client.close_position(symbol)
|
||||
result["closed_positions"].append(symbol) # only on a confirmed close
|
||||
except Exception as exc: # noqa: BLE001
|
||||
result["errors"].append(f"close {symbol}: {exc}")
|
||||
return result
|
||||
@@ -215,10 +219,38 @@ class TradingExecutionService:
|
||||
return False
|
||||
try:
|
||||
acct = client.get_account()
|
||||
equity = float(acct.get("equity"))
|
||||
equity_raw = acct.get("equity")
|
||||
if equity_raw is None:
|
||||
raise ValueError("account 'equity' field missing")
|
||||
equity = float(equity_raw)
|
||||
baseline = float(acct.get("last_equity") or equity)
|
||||
except Exception: # noqa: BLE001 - don't halt trading on a data hiccup
|
||||
return False
|
||||
self._equity_read_failures = 0
|
||||
except Exception as exc: # noqa: BLE001
|
||||
self._equity_read_failures += 1
|
||||
logger.warning(
|
||||
"equity read failed (%d consecutive): %s",
|
||||
self._equity_read_failures, exc,
|
||||
)
|
||||
# FAIL CLOSED: if equity is unreadable repeatedly, stop opening risk
|
||||
# rather than silently disabling the most important safety control.
|
||||
if self._equity_read_failures >= 3 and not self._halted:
|
||||
self._halted = True
|
||||
self.store.emit(
|
||||
RiskRuleTriggeredEvent(
|
||||
timestamp=datetime.now(),
|
||||
mode=self.mode,
|
||||
correlation_id=self.session_id,
|
||||
message=(
|
||||
f"Equity unreadable {self._equity_read_failures}x — "
|
||||
"failing CLOSED, halting new entries"
|
||||
),
|
||||
rule_type="equity_unreadable",
|
||||
rule_value=float(self._equity_read_failures),
|
||||
current_state={"failures": self._equity_read_failures},
|
||||
action_taken="halted_new_entries",
|
||||
)
|
||||
)
|
||||
return self._halted
|
||||
if baseline <= 0:
|
||||
return False
|
||||
pnl_pct = (equity - baseline) / baseline
|
||||
@@ -230,13 +262,18 @@ class TradingExecutionService:
|
||||
else {"closed_positions": [], "cancelled_entries": 0}
|
||||
)
|
||||
extra = ""
|
||||
action = "halted_new_entries"
|
||||
if self.settings.flatten_on_breach:
|
||||
n_closed = len(flat["closed_positions"])
|
||||
n_cancel = flat["cancelled_entries"]
|
||||
flat_errs = flat.get("errors") or []
|
||||
extra = (
|
||||
f" — flattened {n_closed} position(s), "
|
||||
f"cancelled {n_cancel} unfilled entr{'y' if n_cancel == 1 else 'ies'}"
|
||||
)
|
||||
if flat_errs:
|
||||
extra += f"; FLATTEN INCOMPLETE: {flat_errs}"
|
||||
action = "halted_flatten_incomplete" if flat_errs else "halted_and_flattened"
|
||||
self.store.emit(
|
||||
RiskRuleTriggeredEvent(
|
||||
timestamp=datetime.now(),
|
||||
@@ -254,9 +291,7 @@ class TradingExecutionService:
|
||||
"pnl_pct": round(pnl_pct, 4),
|
||||
"flatten": flat,
|
||||
},
|
||||
action_taken="halted_and_flattened"
|
||||
if self.settings.flatten_on_breach
|
||||
else "halted_new_entries",
|
||||
action_taken=action,
|
||||
)
|
||||
)
|
||||
return self._halted
|
||||
@@ -374,7 +409,14 @@ class TradingExecutionService:
|
||||
# If the entry didn't fill immediately (a resting limit at the trigger),
|
||||
# arm it so expire_stale_entries() can back out on timeout or a break
|
||||
# below the entry. Market orders that fill on submit are never armed.
|
||||
if result.ok and result.status not in {"filled", "cancelled"} and request.entry_price:
|
||||
# Only arm genuinely OPEN resting entries — never a filled, partial, or
|
||||
# rejected order (a rejected entry would otherwise occupy a position slot
|
||||
# forever and a partial would be backed out while real shares are held).
|
||||
if (
|
||||
result.ok
|
||||
and result.status in {"new", "accepted", "pending_new", "held", "accepted_for_bidding"}
|
||||
and request.entry_price
|
||||
):
|
||||
self._armed[order_id] = {
|
||||
"symbol": request.symbol,
|
||||
"entry_price": float(request.entry_price),
|
||||
@@ -435,6 +477,23 @@ class TradingExecutionService:
|
||||
quantity = int(abs(float(p.get("quantity") or 0)))
|
||||
if symbol not in held or not quantity:
|
||||
return {"ok": False, "error": f"no open position for {symbol}"}
|
||||
# Prefer close_position: closes the FULL position regardless of long/short
|
||||
# sign and fractional qty. A plain 'sell' of abs(qty) would DOUBLE a short
|
||||
# position, and int-truncation would leave residual shares un-flat.
|
||||
client = getattr(self.executor, "client", None)
|
||||
if client is not None and hasattr(client, "close_position"):
|
||||
try:
|
||||
raw = client.close_position(symbol) or {}
|
||||
return {
|
||||
"ok": True,
|
||||
"order_id": str(raw.get("id") or f"close-{symbol}"),
|
||||
"broker_order_id": raw.get("id"),
|
||||
"status": raw.get("status") or "closing",
|
||||
"error": None,
|
||||
"reason": reason,
|
||||
}
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return {"ok": False, "error": str(exc), "reason": reason}
|
||||
request = ExecutionRequest(symbol=symbol, side="sell", quantity=quantity)
|
||||
result = self.executor.execute(request)
|
||||
return {
|
||||
|
||||
Reference in new issue
Block a user