fix(engine): require authoritative account reconciliation - #60
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds per-position realized PnL propagation, duplicate-order and client-event safeguards, recent-fill reconciliation with runtime truth publication, OMS account-aware order reconciliation, and expanded engine integrity metrics. ChangesRuntime integrity and accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionControlHandle
participant ExecutionWorker
participant Engine
ExecutionControlHandle->>ExecutionWorker: request reconciliation snapshot
ExecutionWorker-->>ExecutionControlHandle: return orders, positions, and recent fills
ExecutionControlHandle->>Engine: publish reconciliation progress
ExecutionControlHandle->>Engine: publish final runtime truth status
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d44f218dda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )), | ||
| Some(Ok(fills)) => { | ||
| for fill in fills { | ||
| let identity = (fill.order_id.clone(), fill.fill_id.clone()); |
There was a problem hiding this comment.
When full reconciliation runs with multiple execution clients, venue order IDs and fill IDs are not globally unique across accounts/venues (the open-order path in this change already treats client identity as part of the match). This unscoped (order_id, fill_id) key causes a legitimate fill from a second client with the same IDs to be reported as a duplicate, and the subsequent local lookup can also treat another account’s processed fill as matched. That can either block live startup incorrectly or hide an unaccounted authoritative fill; include the client/venue/account identity in the fill key used for duplicate detection and local matching.
Useful? React with 👍 / 👎.
| if self.latch_duplicate_order_id(&order_id, client_idx) { | ||
| self.stats.orders_failed += 1; | ||
| continue; |
There was a problem hiding this comment.
When a second execution client successfully returns an order_id that is already tracked for another client, this branch runs after the venue accepted the order. Continuing here drops the second account's order without emitting OrderNew or retaining client-specific metadata, so emergency cancellation and later reconciliation can only route that order_id to the first client and the duplicate live order is left unmanaged. Record or explicitly cancel the accepted duplicate on client_idx before discarding it.
Useful? React with 👍 / 👎.
| order.symbol == exchange_order.symbol | ||
| && order.venue == client.venue | ||
| && self.order_account_map.get(&order.order_id) == client.account_id.as_ref() |
There was a problem hiding this comment.
In a same-venue multi-account runtime, venue_to_client is a HashMap<VenueId, usize>, so only one of two clients for the same venue can round-trip a client.venue in the worker snapshot while the other still has the correct account_id. This exact venue equality then classifies otherwise matching orders for that second account as identity conflicts/local-only and keeps startup reconciliation unhealthy. Treat an absent snapshot venue as non-conflicting when the account identity matches, or carry per-client venue identity separately.
Useful? React with 👍 / 👎.
| pub runtime_truth_observed_at_us: u64, | ||
| pub reconciliation_complete: bool, | ||
| pub reconciliation_healthy: bool, | ||
| pub risk_halted: bool, | ||
| pub data_integrity_gaps: u64, |
There was a problem hiding this comment.
When reconciliation is incomplete/unhealthy or the runtime is risk-halted, these new fields are populated by the engine but update_engine_statistics still drops them and assess_readiness still returns ready using only idle time and queue utilization. In operator-control or degraded-account starts this can expose /readiness as ready while the authoritative-account facts are false, so persist these values in the registry and include them in the readiness decision or exported health detail.
Useful? React with 👍 / 👎.
Change contract
Require reconciliation truth to come from a complete, account- and venue-scoped authoritative snapshot. Export the typed account-truth facts from the engine through its metrics handoff; readiness policy remains downstream.
Out of scope
No readiness/liveness policy or endpoint behavior (#62), batch exposure policy (#61), LiveSmall promotion policy (#64), backtest, or market-data adapter change.
Dependencies and merge order
Independent; merge to
mainbefore #61 and #62.Focused validation
cargo test -p hft-enginecargo test -p hft-portfolio-corecargo test -p hft-infra-metricscargo check -p hft-live --features clickhouse,redis,grpc --lockedRollout / rollback
Fail closed: incomplete or identity-ambiguous account data cannot establish reconciliation truth. Revert this PR to remove the producer-side truth contract.
Scope exception
None.
Summary by CodeRabbit
New Features
Bug Fixes