audit: harden Oanda execution lifecycle (close-fill, netting, races, fail-closed config)
2026-08-04 Fable 5 QA audit, Batch A. All fixes are execution-robustness only — no signal code touched (git diff scope: daemon/executor/tests). Findings verified by runnable repros before fixing (h3 harness): - close_trade: a 200 whose close order was created but NOT filled (e.g. MARKET_HALTED during the settlement pause) now raises instead of returning; the daemon's timeout path keeps tracking and retries. The old path popped state and orphaned a live position forever (reconciliation was boot-only). - place_market_order: positionFill=OPEN_ONLY — on a netting account an opposite-side order can no longer silently close an untracked position; the divergence surfaces as an order cancel. - _execute_trade: a fill without tradeOpened (netting close/reduce) and any placement OandaAPIError (server may have filled before a network timeout) now trigger _reconcile_positions instead of silent return. - _discover_existing_positions → lock-guarded _reconcile_positions, callable mid-run; adoption of a position with NO broker stop now screams (CRITICAL + per-bar warning) instead of computing sl=entry, risk=0 and silently never trailing. - _manage_trailing: snapshot deep-copies per-trade dicts (no mutation of live state outside the lock); write-back skips trades popped by _check_closed_trades mid-iteration (no ghost resurrection). - Fail-closed config: unknown OANDA_QUALITY_GATE refuses to start (previously a typo silently became the most permissive gate "C"); non-integer OANDA_LOT_SIZE/MAX_HOLD_BARS exit with a clear message. Suite: 302 → 312 tests (Module 21), all green. Claude-Session: https://claude.ai/code/session_01UnZkzeFcTu9agFg8BDh2tn
This commit is contained in:
Harwinder Sekhon
+128
-52
@@ -90,8 +90,17 @@ STATE_FILE = os.path.join(
|
||||
def _oanda_setting_int(env_key: str, json_key: str, default: int) -> int:
|
||||
env = os.environ.get(env_key)
|
||||
if env:
|
||||
return int(env)
|
||||
return int(get_app_config().oanda_settings.get(json_key, default))
|
||||
try:
|
||||
return int(env)
|
||||
except ValueError:
|
||||
raise SystemExit(
|
||||
f"{env_key}={env!r} is not an integer — refusing to start")
|
||||
try:
|
||||
return int(get_app_config().oanda_settings.get(json_key, default))
|
||||
except (TypeError, ValueError):
|
||||
raise SystemExit(
|
||||
f"config.json oanda.{json_key} is not an integer — "
|
||||
f"refusing to start")
|
||||
|
||||
|
||||
def _oanda_setting_str(env_key: str, json_key: str, default: str) -> str:
|
||||
@@ -165,7 +174,13 @@ class TradingDaemon:
|
||||
base_url = oanda_base_url()
|
||||
lot_size = _oanda_setting_int("OANDA_LOT_SIZE", "lot_size", 1)
|
||||
self._quality_gate = _oanda_setting_str(
|
||||
"OANDA_QUALITY_GATE", "quality_gate", "C")
|
||||
"OANDA_QUALITY_GATE", "quality_gate", "C").strip()
|
||||
if self._quality_gate not in QUALITY_RANK:
|
||||
# Fail CLOSED: an unrecognized gate must never silently fall
|
||||
# back to the most permissive rank (QUALITY_RANK.get default).
|
||||
raise SystemExit(
|
||||
f"OANDA_QUALITY_GATE/quality_gate {self._quality_gate!r} "
|
||||
f"is not one of {sorted(QUALITY_RANK)} — refusing to start")
|
||||
self._max_hold_bars = _oanda_setting_int(
|
||||
"OANDA_MAX_HOLD_BARS", "max_hold_bars", 300)
|
||||
self._price_precision = PAIR_INFO[SYMBOL]["price_precision"]
|
||||
@@ -275,60 +290,86 @@ class TradingDaemon:
|
||||
# ── Position discovery (restart recovery) ──────────────────────
|
||||
|
||||
def _discover_existing_positions(self) -> None:
|
||||
self._reconcile_positions("boot discovery")
|
||||
|
||||
def _reconcile_positions(self, reason: str) -> None:
|
||||
"""Re-sync tracked state with the broker's open trades. Safe to
|
||||
call mid-run (lock-guarded); used at boot and whenever a fill or
|
||||
placement error suggests state and broker may have diverged."""
|
||||
try:
|
||||
open_trades = self._executor.get_open_trades(INSTRUMENT)
|
||||
except OandaAPIError:
|
||||
logger.exception("Failed to query open trades at boot")
|
||||
logger.exception(
|
||||
"Failed to query open trades (%s) — reconcile skipped",
|
||||
reason)
|
||||
return
|
||||
|
||||
saved = self._state.get("trades", {})
|
||||
live_ids = {t["id"] for t in open_trades}
|
||||
|
||||
stale = [tid for tid in saved if tid not in live_ids]
|
||||
for tid in stale:
|
||||
logger.info(
|
||||
"Stale trade %s in state file (closed on Oanda), removing", tid)
|
||||
del saved[tid]
|
||||
with self._state_lock:
|
||||
saved = self._state.get("trades", {})
|
||||
|
||||
for trade in open_trades:
|
||||
tid = trade["id"]
|
||||
if tid in saved:
|
||||
stale = [tid for tid in saved if tid not in live_ids]
|
||||
for tid in stale:
|
||||
logger.info(
|
||||
"Recovered trade %s from state: %s %s @ %.2f, SL %.2f",
|
||||
tid, saved[tid].get("direction"), INSTRUMENT,
|
||||
saved[tid].get("entry"), saved[tid].get("current_sl"))
|
||||
continue
|
||||
"Stale trade %s in state file (closed on Oanda), "
|
||||
"removing", tid)
|
||||
del saved[tid]
|
||||
|
||||
direction = "BUY" if float(trade.get("currentUnits", 0)) > 0 else "SELL"
|
||||
entry = float(trade.get("price", 0))
|
||||
sl_order = trade.get("stopLossOrder", {})
|
||||
sl = float(sl_order.get("price", entry))
|
||||
risk = abs(entry - sl) if sl else 1.0
|
||||
for trade in open_trades:
|
||||
tid = trade["id"]
|
||||
if tid in saved:
|
||||
logger.info(
|
||||
"Recovered trade %s from state: %s %s @ %.2f, SL %.2f",
|
||||
tid, saved[tid].get("direction"), INSTRUMENT,
|
||||
saved[tid].get("entry"), saved[tid].get("current_sl"))
|
||||
continue
|
||||
|
||||
saved[tid] = {
|
||||
"tf": "H1",
|
||||
"direction": direction,
|
||||
"entry": entry,
|
||||
"sl_initial": sl,
|
||||
"current_sl": sl,
|
||||
"risk": risk,
|
||||
"peak": entry,
|
||||
"trail_armed": False,
|
||||
"trail_arm_r": 0.6,
|
||||
"trail_dist_r": 0.75,
|
||||
"bars_held": 0,
|
||||
"last_bar_ts": None,
|
||||
"signal_bar_ts": None,
|
||||
}
|
||||
logger.info(
|
||||
"Adopted unknown trade %s: %s @ %.2f, SL %.2f, risk %.2f "
|
||||
"(conservative defaults)",
|
||||
tid, direction, entry, sl, risk)
|
||||
direction = ("BUY" if float(trade.get("currentUnits", 0)) > 0
|
||||
else "SELL")
|
||||
entry = float(trade.get("price", 0))
|
||||
sl_order = trade.get("stopLossOrder") or {}
|
||||
sl_price = sl_order.get("price")
|
||||
if sl_price is None:
|
||||
# No broker-side stop at all: risk is undefined, so the
|
||||
# trailing model cannot manage this trade. Scream — a
|
||||
# naked position needs a human (or a policy) to set a
|
||||
# stop; fabricating one here would invent trading
|
||||
# behavior the books never validated.
|
||||
logger.critical(
|
||||
"Adopted trade %s (%s @ %.2f) has NO STOP LOSS on "
|
||||
"the broker — it will NOT be trailed. Set a stop "
|
||||
"manually or close it.", tid, direction, entry)
|
||||
sl = entry
|
||||
risk = 0.0
|
||||
else:
|
||||
sl = float(sl_price)
|
||||
risk = abs(entry - sl)
|
||||
|
||||
self._state["trades"] = saved
|
||||
_save_state(self._state)
|
||||
saved[tid] = {
|
||||
"tf": "H1",
|
||||
"direction": direction,
|
||||
"entry": entry,
|
||||
"sl_initial": sl,
|
||||
"current_sl": sl,
|
||||
"risk": risk,
|
||||
"peak": entry,
|
||||
"trail_armed": False,
|
||||
"trail_arm_r": 0.6,
|
||||
"trail_dist_r": 0.75,
|
||||
"bars_held": 0,
|
||||
"last_bar_ts": None,
|
||||
"signal_bar_ts": None,
|
||||
}
|
||||
logger.info(
|
||||
"Adopted unknown trade %s: %s @ %.2f, SL %.2f, risk %.2f "
|
||||
"(conservative defaults)",
|
||||
tid, direction, entry, sl, risk)
|
||||
|
||||
self._state["trades"] = saved
|
||||
_save_state(self._state)
|
||||
logger.info(
|
||||
"Position discovery: %d active trade(s)", len(saved))
|
||||
"Position reconcile (%s): %d active trade(s)", reason, len(saved))
|
||||
|
||||
# ── Candle-boundary loop ───────────────────────────────────────
|
||||
|
||||
@@ -521,9 +562,15 @@ class TradingDaemon:
|
||||
stop_loss=ts.stop_loss,
|
||||
)
|
||||
except OandaAPIError:
|
||||
# The order is never retried (a timeout after a server-side
|
||||
# fill would double-up on retry), but the server MAY have
|
||||
# filled before the error reached us — reconcile so a live
|
||||
# position can't stay untracked until the next restart.
|
||||
logger.exception(
|
||||
"Failed to place %s order for %s %s",
|
||||
"Failed to place %s order for %s %s — reconciling "
|
||||
"broker positions",
|
||||
ts.direction, SYMBOL, tf.value)
|
||||
self._reconcile_positions("order placement error")
|
||||
return
|
||||
|
||||
fill_tx = result.get("orderFillTransaction")
|
||||
@@ -535,7 +582,16 @@ class TradingDaemon:
|
||||
trade_opened = fill_tx.get("tradeOpened", {})
|
||||
trade_id = trade_opened.get("tradeID")
|
||||
if not trade_id:
|
||||
logger.warning("No trade ID in fill response")
|
||||
# A fill with no tradeOpened means the order interacted
|
||||
# with an existing broker position (tradesClosed /
|
||||
# tradeReduced on a netting account) — state and broker
|
||||
# have diverged. Surface it and re-sync instead of
|
||||
# silently forgetting a real fill.
|
||||
logger.critical(
|
||||
"Order FILLED but opened no trade (netting close/"
|
||||
"reduce?) — fill: closed=%s reduced=%s. Reconciling.",
|
||||
fill_tx.get("tradesClosed"), fill_tx.get("tradeReduced"))
|
||||
self._reconcile_positions("fill without tradeOpened")
|
||||
return
|
||||
|
||||
fill_price = float(fill_tx.get("price", ts.entry_price))
|
||||
@@ -587,8 +643,12 @@ class TradingDaemon:
|
||||
# ── Trailing stop management ───────────────────────────────────
|
||||
|
||||
def _manage_trailing(self, tf: Timeframe) -> None:
|
||||
# Copy the per-trade dicts too: the loop mutates info (peak,
|
||||
# bars_held, last_bar_ts) and must not touch live state outside
|
||||
# the lock — another thread may be serializing it in _save_state.
|
||||
with self._state_lock:
|
||||
trades = dict(self._state.get("trades", {}))
|
||||
trades = {tid: dict(info)
|
||||
for tid, info in self._state.get("trades", {}).items()}
|
||||
|
||||
for trade_id, info in trades.items():
|
||||
if info.get("tf") != tf.value:
|
||||
@@ -634,13 +694,16 @@ class TradingDaemon:
|
||||
self._executor.close_trade(trade_id)
|
||||
except OandaAPIError:
|
||||
# Keep tracking — the trade is still live on the
|
||||
# broker; we retry the close at the next bar.
|
||||
# broker (incl. a 200 whose close order was cancelled,
|
||||
# e.g. MARKET_HALTED — close_trade raises on no-fill);
|
||||
# we retry the close at the next bar.
|
||||
logger.exception(
|
||||
"Failed to close timed-out trade %s — will retry",
|
||||
trade_id)
|
||||
with self._state_lock:
|
||||
self._state["trades"][trade_id] = info
|
||||
_save_state(self._state)
|
||||
if trade_id in self._state["trades"]:
|
||||
self._state["trades"][trade_id] = info
|
||||
_save_state(self._state)
|
||||
continue
|
||||
with self._state_lock:
|
||||
self._state["trades"].pop(trade_id, None)
|
||||
@@ -651,6 +714,14 @@ class TradingDaemon:
|
||||
bar_high = float(last_bar["High"])
|
||||
bar_low = float(last_bar["Low"])
|
||||
|
||||
if info.get("risk", 0) <= 0:
|
||||
# Adopted naked position (no broker SL → risk undefined):
|
||||
# never manageable by the trailing model — keep it loud
|
||||
# instead of silently skipping forever.
|
||||
logger.warning(
|
||||
"Trade %s has risk<=0 (no stop loss) — trailing "
|
||||
"skipped; set a stop manually", trade_id)
|
||||
|
||||
new_sl = self._compute_trail(info, bar_high, bar_low)
|
||||
|
||||
if new_sl is not None:
|
||||
@@ -667,8 +738,13 @@ class TradingDaemon:
|
||||
trade_id)
|
||||
|
||||
with self._state_lock:
|
||||
self._state["trades"][trade_id] = info
|
||||
_save_state(self._state)
|
||||
# Write back ONLY if the trade is still tracked — another
|
||||
# loop's _check_closed_trades may have popped it while we
|
||||
# iterated (broker-side close). Re-inserting would create
|
||||
# a ghost that blocks new entries.
|
||||
if trade_id in self._state["trades"]:
|
||||
self._state["trades"][trade_id] = info
|
||||
_save_state(self._state)
|
||||
|
||||
def _compute_trail(
|
||||
self, info: Dict, bar_high: float, bar_low: float
|
||||
|
||||
Reference in New Issue
Block a user