fix: Race-safe active_orders cleanup in update_order_status#126
Conversation
…atus Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
keitaj
left a comment
There was a problem hiding this comment.
Review: Race-safe active_orders cleanup
Defensive fix with clear root-cause analysis. The original bug log (`Error updating order status: 398309313821`) is now fully explained: `KeyError` is in `API_ERRORS`, so a stale `del` would re-enter the broader `except` block and log only the missing key as the exception arg.
1. Status assignment before pop (informational)
`order_manager.py` L500-505 and `hip3/multi_dex_order_manager.py` L257-260:
`order.status` is assigned BEFORE `pop()`. This is correct — even if the dict entry is already gone, the in-list snapshot of the `Order` object still gets its status updated. The two operations don't depend on each other. ✓
2. Error log enrichment (informational)
Including `type(e).name` makes future bare-arg exceptions (like `KeyError(123)`) immediately diagnosable. Before:
```
Error updating order status: 398309313821
```
After:
```
Error updating order status (KeyError, 5 active orders): 398309313821
```
3. Race semantics (informational)
The fix is universally correct regardless of whether the race is true cross-thread or just same-thread re-entry (e.g., main-loop calling `cancel_order` between the disappeared snapshot and the cleanup loop). `pop(key, None)` adds no measurable overhead. ✓
4. Test coverage (informational)
6 tests cover:
- Race during fills fetch (concurrent removal mocked)
- Normal path unchanged
- Error path with stale orders in except cleanup
- Exception type appears in log message
- Both `OrderManager` and `MultiDexOrderManager` paths
Real assertions (`order_a.status == OrderStatus.CANCELLED`, `100 not in mdm.active_orders`), no formality. ✓
Verdict: LGTM
- No blockers or should-fix items
- Defensive change, no behavior regression on the happy path
- Diagnostics meaningfully improved
- Test scenarios precisely model the production bug
Merging after CI check.
Summary
del self.active_orders[order_id]withself.active_orders.pop(order_id, None)inupdate_order_statusof bothOrderManagerandMultiDexOrderManager. PreventsKeyErrorwhen an order has already been removed concurrently (e.g. bycancel_order/bulk_cancelbetween the disappeared snapshot and the cleanup loop).type(e).__name__and the active order count, so cryptic logs likeError updating order status: 398309313821(which was just a bareKeyErrorarg) are no longer ambiguous.KeyErroralso re-raised through the broaderexcept API_ERRORSbecauseKeyErroris inAPI_ERRORS, so a single race could trigger both inner and outer error logs.Changes
order_manager.pyupdate_order_status: usepop(default=None); richer error loghip3/multi_dex_order_manager.pytests/test_active_orders_race.pyTest plan
flake8cleantest_update_order_status.py🤖 Generated with Claude Code