docs: production safety audit design — 38 fixes across 5 layers
4-domain code audit found 13 CRITICAL, 14 HIGH, 10 MEDIUM issues. Organized as layer-by-layer remediation: - Layer 0: Data integrity (positions, prices, market data) - Layer 1: Risk enforcement (make checks actually block) - Layer 2: ML pipeline (real weights, bounded predictions) - Layer 3: Broker safety (connection handling, volumes) - Layer 4: Auth & ops (credentials, rate limiting, monitoring) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
jgrusewski
245
docs/plans/2026-02-23-production-safety-audit-design.md
Normal file
245
docs/plans/2026-02-23-production-safety-audit-design.md
Normal file
@@ -0,0 +1,245 @@
|
||||
# Production Safety Audit — Remediation Design
|
||||
|
||||
**Goal:** Fix all 37 safety-critical issues found by 4-domain code audit before Foxhunt trades real money.
|
||||
|
||||
**Architecture:** Layer-by-layer remediation — fix data foundations first, then risk enforcement, ML pipeline, broker gateway, and finally auth/ops. Each layer is a checkpoint where correctness can be verified before proceeding.
|
||||
|
||||
**Scope:** 13 CRITICAL + 14 HIGH + 10 MEDIUM issues across trading_engine, risk, ml, broker_gateway_service, trading_service, adaptive-strategy, web-gateway, and common crates.
|
||||
|
||||
---
|
||||
|
||||
## Layer 0: Data Integrity Foundation (7 fixes)
|
||||
|
||||
Positions, prices, and market data must be correct before anything downstream can be trusted.
|
||||
|
||||
### C11 — Position update_with_execution race condition
|
||||
- **File:** `services/trading_service/src/core/position_manager.rs:100-195`
|
||||
- **Problem:** `update_with_execution` reads `quantity`/`avg_price` with Acquire loads, does 85 lines of calculation, then stores with Release — not atomic. Two concurrent fills for the same position race, and last store wins, discarding the other fill.
|
||||
- **Fix:** Hold the `positions` write-lock across the entire `update_with_execution` call (the outer `PositionManager::update_position` currently drops it before calling). Alternatively, use `Mutex<PositionState>` instead of individual atomics.
|
||||
|
||||
### C13 — Market data price NaN/zero validation
|
||||
- **File:** `services/trading_service/src/core/market_data_ingestion.rs:462-486`
|
||||
- **Problem:** Binary market data parsed to `f64` without finiteness check. Zeroed packet produces `price=0.0`, broadcast as valid tick. Position manager computes `unrealized_pnl = qty * 0.0` → 100% loss shown on all positions.
|
||||
- **Fix:** Add `!price.is_finite() || price <= 0.0 || !quantity.is_finite() || quantity < 0.0` guard. Drop invalid ticks, increment `drop_count`, log warning.
|
||||
|
||||
### H1 — get_orders status filter is a no-op
|
||||
- **File:** `trading_engine/src/trading/order_manager.rs:132-147`
|
||||
- **Problem:** `matches!(order.status, _status)` creates irrefutable binding — always matches. `get_orders(Some(Filled))` returns ALL orders. Any risk check counting open orders gets inflated exposure.
|
||||
- **Fix:** Replace with `order.status == *status`.
|
||||
|
||||
### H2 — Account never deducts trade value
|
||||
- **File:** `trading_engine/src/trading/account_manager.rs:104-133`
|
||||
- **Problem:** `_execution_value = quantity * price` is computed but discarded. Only commission deducted. Buying power never reduced after buy fills → allows unlimited buy orders.
|
||||
- **Fix:** Deduct `execution_value` from `cash_balance` on buy fills, add back on sell fills. Recalculate `buying_power = cash_balance + margin_available`.
|
||||
|
||||
### H4 — Position direction always treated as buy
|
||||
- **File:** `trading_engine/src/trading/position_manager.rs:64-67`
|
||||
- **Problem:** `is_buy = execution.executed_quantity > 0` but `executed_quantity` is always positive magnitude. All fills treated as buys. Short positions never opened via this path.
|
||||
- **Fix:** Add `OrderSide` field to `ExecutionResult` struct. Look up from `OrderManager` using `execution.order_id` before calling `position_manager.update_position`.
|
||||
|
||||
### M6 — price_to_fixed truncates negative prices
|
||||
- **File:** `services/trading_service/src/core/position_manager.rs:254-256`
|
||||
- **Problem:** `(price * 10000.0) as u64` — casting negative f64 to u64 saturates to 0. Bad fill report or instrument with negative prices (oil spreads) → avg_price=0, all PnL wrong.
|
||||
- **Fix:** Guard: `if !price.is_finite() || price <= 0.0 { return Err(PositionError::InvalidPrice) }`.
|
||||
|
||||
### M7 — clone_for_async creates independent counters
|
||||
- **File:** `services/trading_service/src/core/market_data_ingestion.rs:307-329`
|
||||
- **Problem:** `clone_for_async` creates new `AtomicU64` instances (not shared). Spawned connection task increments its own counters. `get_stats()` reads original struct's counters → always shows 0. Heartbeat monitor watches counter that's never updated → always fires "connection appears dead".
|
||||
- **Fix:** Wrap `message_count`, `drop_count`, `last_heartbeat`, `reconnect_attempts` in `Arc<AtomicU64>`. Clone the `Arc` in `clone_for_async`. Also fix heartbeat initialization to `HardwareTimestamp::now().as_nanos()` instead of `0`.
|
||||
|
||||
---
|
||||
|
||||
## Layer 1: Risk Enforcement (10 fixes)
|
||||
|
||||
Make the risk system actually block orders instead of just logging warnings.
|
||||
|
||||
### C1 — Order validate/add race condition
|
||||
- **File:** `trading_engine/src/trading/engine.rs:104-113`
|
||||
- **Problem:** `validate_order` releases read lock, then `add_order` acquires write lock. Gap allows two concurrent submissions with same OrderId to both pass duplicate check and both submit to broker.
|
||||
- **Fix:** Combine into single write-locked `validate_and_add_order()` method that does duplicate check and insertion atomically.
|
||||
|
||||
### C2 — No overfill protection
|
||||
- **File:** `trading_engine/src/trading/order_manager.rs:99-129`
|
||||
- **Problem:** `fill_quantity += execution.executed_quantity` with no cap. Duplicate execution report doubles fill quantity and corrupts average price. Position will be 2x reality.
|
||||
- **Fix:** Check `fill_quantity + execution.quantity <= order.quantity` before accumulating. Return error on overfill. Add fill ID idempotency check. Reject fills for already-Filled orders.
|
||||
|
||||
### C9 — Position/leverage checks bypass when broker_account_service is None
|
||||
- **File:** `risk/src/risk_engine.rs:1167, 1261, 1321`
|
||||
- **Problem:** `check_position_limits` and `check_leverage_limits` skip all checks and return `Approved` when `broker_account_service` is `None`. Comment: "approve by default".
|
||||
- **Fix:** Return `Err(RiskError::Config("broker_account_service not configured"))` instead. Fail safe, don't approve by default.
|
||||
|
||||
### C10 — check_order() hardcoded "default" account
|
||||
- **File:** `risk/src/risk_engine.rs:1885-1888`
|
||||
- **Problem:** `check_order()` always passes `"default"` as `account_id`. Per-account circuit breaker state is never checked for real accounts.
|
||||
- **Fix:** Extract `account_id` from `OrderInfo` field (add if needed) and pass to `check_pre_trade_risk`.
|
||||
|
||||
### H5 — Risk limit breach only logs warning
|
||||
- **File:** `adaptive-strategy/src/risk/mod.rs:904-930`
|
||||
- **Problem:** `check_risk_limits` checks VaR, drawdown, leverage — all only `warn!()` and return `Ok(())`. Never blocks orders.
|
||||
- **Fix:** Return `Err(anyhow!("Risk limit breached: ..."))` when any limit is exceeded. Callers propagate to block order generation. Consider tripping circuit breaker or kill switch for severe breaches.
|
||||
|
||||
### H10 — Drawdown HWM caller-supplied
|
||||
- **File:** `risk/src/drawdown_monitor.rs:134-143`
|
||||
- **Problem:** `check_drawdown_thresholds` reads `metrics.high_water_mark` from caller. If stale/wrong, drawdown understated, thresholds don't fire.
|
||||
- **Fix:** `DrawdownMonitor` maintains internal `high_water_mark: HashMap<String, Price>` per portfolio. Updated as running maximum of all `total_pnl` values seen. Caller-supplied value ignored.
|
||||
|
||||
### H11 — Circuit breaker HalfOpen allows unlimited probes
|
||||
- **File:** `common/src/resilience/circuit_breaker.rs:241`
|
||||
- **Problem:** In `HalfOpen` state, `can_execute()` returns `true` unconditionally. N concurrent requests all probe simultaneously. For order submission, N orders sent instead of 1.
|
||||
- **Fix:** Add `probe_in_flight: AtomicBool` flag. In HalfOpen, only allow one request (compare_exchange). Others return `false` until probe result is recorded.
|
||||
|
||||
### H13 — Kill switch engage() returns Err after setting local state
|
||||
- **File:** `risk/src/safety/kill_switch.rs:67-124`
|
||||
- **Problem:** Sets `AtomicBool` and `scoped_triggers` locally, then returns `Err` if Redis publish fails. Caller thinks kill switch didn't engage, but local state is already set.
|
||||
- **Fix:** After local state is set, return `Ok(())` regardless. Log Redis failure as warning. Redis is for distribution; local state is authoritative.
|
||||
|
||||
### H14 — validate_order gRPC skips most risk checks
|
||||
- **File:** `services/trading_service/src/services/risk.rs:565-648`
|
||||
- **Problem:** Only checks order quantity and VaR. Skips position limits, leverage, circuit breaker. Orders exceeding position limits pass validation.
|
||||
- **Fix:** Call `risk_engine.check_pre_trade_risk()` with full `OrderInfo` converted from gRPC request, using actual `account_id`.
|
||||
|
||||
### M10 — VaR z-score extrapolation wrong above 99%
|
||||
- **File:** `risk/src/var_calculator/parametric.rs:150-167`
|
||||
- **Problem:** Linear extrapolation above 0.99 confidence gives z=2.477 for 99.9% (correct: 3.09). VaR understated by ~25% at high confidence.
|
||||
- **Fix:** Implement Abramowitz-Stegun rational approximation for inverse normal CDF. Or extend lookup table to cover 99.5%, 99.9%, 99.95%.
|
||||
|
||||
---
|
||||
|
||||
## Layer 2: ML Pipeline Correctness (10 fixes)
|
||||
|
||||
Ensure predictions are real before they become trade signals.
|
||||
|
||||
### C5 — TFT/Mamba2 load random weights
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:1686-1702, 1794-1807`
|
||||
- **Problem:** `from_checkpoint` creates fresh model with random weights, sets `is_trained=true`. Trades on noise.
|
||||
- **Fix:** Implement safetensors loading via `VarBuilder::from_mmaped_safetensors` (same as DQN pattern). If checkpoint missing/corrupted, return `Err` — don't fake `is_trained`.
|
||||
|
||||
### C6 — DQN uses epsilon-greedy in production inference
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:1412-1415`
|
||||
- **Problem:** `select_action` uses epsilon-greedy with `epsilon_start=0.1` → 10% random actions in production.
|
||||
- **Fix:** Set `epsilon = 0.0` on loaded DQN agent before inference, or use dedicated `select_greedy_action()` / `act_greedy()` method that always picks highest Q-value.
|
||||
|
||||
### C7 — Ensemble predictions not validated for NaN/Inf
|
||||
- **File:** `ml/src/ensemble/coordinator.rs:236-251`
|
||||
- **Problem:** Adapter can return NaN/Inf direction or confidence. NaN propagates through `weighted_sum`, `from_signal`, and `consensus_confidence`. Behavior relies on implicit NaN semantics.
|
||||
- **Fix:** After each adapter prediction, check `direction.is_finite() && confidence.is_finite()`. After `weighted_sum`, check before `from_signal`. Log and skip invalid predictions.
|
||||
|
||||
### C8 — Confidence threshold never enforced in get_ensemble_prediction
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:529-621`
|
||||
- **Problem:** `EnsembleConfig::confidence_threshold=0.7` and `RuntimeModelInfo::confidence_threshold=0.7` are set but never checked. `get_ensemble_prediction` returns any confidence level.
|
||||
- **Fix:** After computing `consensus_confidence`, check `>= config.confidence_threshold`. Return error/Hold if below threshold.
|
||||
|
||||
### H6 — get_model_performance returns fake metrics
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:1168-1199`
|
||||
- **Problem:** Hardcoded `accuracy=0.85, sharpe=1.45, win_rate=0.62` for all models. Dashboard/monitoring sees fake health.
|
||||
- **Fix:** Wire to `ml_performance_monitor.get_model_stats()`. Return `Status::not_found` if no stats available.
|
||||
|
||||
### H7 — retrain_model RPC stub always returns success
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:1146-1166`
|
||||
- **Problem:** Returns `success: true` with fake `job_id` without training. Any corrective action system that calls this is deceived.
|
||||
- **Fix:** Return `Err(Status::unimplemented("Model retraining not yet connected to training service"))`.
|
||||
|
||||
### H8 — DST-broken trading session detection
|
||||
- **File:** `ml/src/ensemble/coordinator.rs:381-393`
|
||||
- **Problem:** Hardcoded `UTC-5` (EST only). During EDT (March-November), all session boundaries off by 1 hour.
|
||||
- **Fix:** Add `chrono-tz` dependency, use `America/New_York` timezone for correct DST-aware conversion.
|
||||
|
||||
### H9 — Ensemble weights not normalized after dynamic update
|
||||
- **File:** `ml/src/ensemble/decision.rs:189-204`
|
||||
- **Problem:** `effective_weight() = static_weight * dynamic_weight`. Dynamic ranges 0.5-1.5. Sum of weights can be 0.5-1.5, not 1.0. `model_votes` entries store un-normalized weights.
|
||||
- **Fix:** After `update_model_weights()`, re-normalize: compute sum of all `effective_weight()`, divide each by sum.
|
||||
|
||||
### M3 — Mamba2 inference silently returns 0.5 on error
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:1837-1839`
|
||||
- **Problem:** `unwrap_or_else(|_| 0.5)` swallows error. Not logged, not counted. Fallback manager never degrades model health.
|
||||
- **Fix:** Propagate with `?`. Let `get_single_model_prediction` call `record_model_error`.
|
||||
|
||||
### M4 — MarketStateTracker NaN initialization
|
||||
- **File:** `adaptive-strategy/src/risk/ppo_position_sizer.rs:1143-1148`
|
||||
- **Problem:** Feature vectors initialized with `f64::NAN`. If `update()` fails before `get_current_state()`, NaN propagates through PPO network → NaN position size.
|
||||
- **Fix:** Initialize with `0.0`. Add `values.iter().all(|v| v.is_finite())` check in `get_current_state()`.
|
||||
|
||||
---
|
||||
|
||||
## Layer 3: Broker Safety (5 fixes)
|
||||
|
||||
The final gate before real money.
|
||||
|
||||
### C3 — set_broker_client silently drops connection
|
||||
- **File:** `services/broker_gateway_service/src/service.rs:44-49`
|
||||
- **Problem:** `try_write()` is non-blocking. If lock contended, `CTraderClient` dropped silently. Service runs without broker, orders queued but never sent.
|
||||
- **Fix:** Use `self.broker_client.write().await` (blocking). This is initialization code, should not be non-blocking.
|
||||
|
||||
### C4 — cTrader volume truncation
|
||||
- **File:** `services/broker_gateway_service/src/service.rs:222`
|
||||
- **Problem:** `(req.quantity * 100_000.0) as i64` truncates fractional lots and can overflow.
|
||||
- **Fix:** `let volume_f = req.quantity * 100_000.0; if volume_f < 0.0 || volume_f > i64::MAX as f64 { return Err(InvalidArgument) }; let volume = volume_f.round() as i64;`
|
||||
|
||||
### H3 — DB update failure after submission silently discarded
|
||||
- **File:** `services/broker_gateway_service/src/service.rs:247-253`
|
||||
- **Problem:** `let _ = sqlx::query(...).execute(...).await;` — if DB update fails, order is live at broker but DB shows PENDING_SUBMIT. Recovery will re-submit → duplicate live order.
|
||||
- **Fix:** Don't discard. On failure: log CRITICAL, attempt compensating cancel of broker order, emit alert for manual reconciliation.
|
||||
|
||||
### M1 — reconnect() simulates success with sleep
|
||||
- **File:** `services/broker_gateway_service/src/recovery/mod.rs:129-170`
|
||||
- **Problem:** `sleep(500ms)` then unconditionally sets `SessionState::Active`. Reports healthy during real outage.
|
||||
- **Fix:** Gate state transition on actual broker health check (e.g., `CTraderClient::health_check()` or `get_account_info()`).
|
||||
|
||||
### M9 — SessionState unused in order routing
|
||||
- **File:** `services/broker_gateway_service/src/service.rs:24, 36`
|
||||
- **Problem:** `session_state` initialized to Active but never checked before `route_order` or `cancel_order`. Orders accepted even when connection is down.
|
||||
- **Fix:** Check `session_state != Active` and `broker_client.is_some()` at start of `route_order`. Return `Unavailable` if not connected. Wire `ErrorHandler` circuit breaker into submission path.
|
||||
|
||||
---
|
||||
|
||||
## Layer 4: Auth & Operational Hardening (6 fixes)
|
||||
|
||||
### C12 — Auth stub accepts any password
|
||||
- **File:** `web-gateway/src/routes/auth.rs:27-61`
|
||||
- **Problem:** Any non-empty username+password → valid 24hr JWT with full trading permissions. No `DEVELOPMENT_MODE` gate.
|
||||
- **Fix:** Gate behind `FOXHUNT_DEV_AUTH=true` env var. Without it, refuse to start and log error: "Production auth provider not configured". In dev mode, log prominent warning on every auth request.
|
||||
|
||||
### C-new — cTrader Live/Demo single env var
|
||||
- **File:** `services/broker_gateway_service/src/main.rs:72-79`
|
||||
- **Problem:** `CTRADER_LIVE=true` → real money with no confirmation.
|
||||
- **Fix:** Require additional `CTRADER_LIVE_CONFIRMED=I_UNDERSTAND_REAL_MONEY`. Print multi-line warning at startup. Add 5-second delay before connecting to live so warning appears in logs.
|
||||
|
||||
### H12 — VaR uses static hardcoded volatility
|
||||
- **File:** `risk/src/risk_engine.rs:333-338`
|
||||
- **Problem:** 80% crypto, 15% FX, 25% blue-chip, 35% default. Not real market data. Understates risk in high-vol regimes.
|
||||
- **Fix:** Wire `MarketDataService` for live implied/realized vol. Interim: add `VolatilityOverrides` config map, log `warn!("Using static volatility for {symbol}")` on every VaR calculation.
|
||||
|
||||
### M2 — Kelly sizing uses fake return history
|
||||
- **File:** `adaptive-strategy/src/risk/mod.rs:558-567`
|
||||
- **Problem:** Hardcoded 20-value return vector for all symbols. Kelly fraction wrong for every instrument.
|
||||
- **Fix:** Wire to market data service for historical returns. Until then, return `Err("Historical return data not available")` instead of fake returns. Callers fall back to fixed-fraction sizing.
|
||||
|
||||
### M5 — avg_latency_us overwrites instead of averaging
|
||||
- **File:** `services/trading_service/src/services/enhanced_ml.rs:817-819`
|
||||
- **Problem:** `avg_latency_us = latency_us` overwrites on each call. Not an average.
|
||||
- **Fix:** `if inference_count == 1 { avg_latency_us = latency_us } else { avg_latency_us = 0.9 * avg_latency_us + 0.1 * latency_us }`
|
||||
|
||||
### M8 — Rate limiter keyed by spoofable X-Forwarded-For
|
||||
- **File:** `web-gateway/src/rate_limit.rs:68-83`
|
||||
- **Problem:** First value from `X-Forwarded-For` is client-controlled. Attacker cycles fake IPs to bypass 200 req/min trading limit.
|
||||
- **Fix:** Use TCP `ConnectInfo` remote address as primary key. Only trust `X-Forwarded-For` from configured trusted proxy CIDR ranges (e.g., Cloudflare, internal LB).
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Layer | Fixes | CRITICALs | HIGHs | MEDIUMs |
|
||||
|-------|-------|-----------|-------|---------|
|
||||
| 0: Data Integrity | 7 | 2 | 3 | 2 |
|
||||
| 1: Risk Enforcement | 10 | 3 | 5 | 1 |
|
||||
| 2: ML Pipeline | 10 | 4 | 4 | 2 |
|
||||
| 3: Broker Safety | 5 | 2 | 1 | 2 |
|
||||
| 4: Auth & Ops | 6 | 2 | 1 | 3 |
|
||||
| **Total** | **38** | **13** | **14** | **10** |
|
||||
|
||||
Each layer should be implemented and tested before moving to the next. After all layers, a full integration test should verify:
|
||||
1. Positions are accurate under concurrent fills
|
||||
2. Risk checks block orders when they should
|
||||
3. ML models load real weights and produce bounded, finite predictions
|
||||
4. Broker gateway handles all failure modes gracefully
|
||||
5. Auth requires real credentials in production mode
|
||||
Reference in New Issue
Block a user