[wippa-studios]

Fixes #743

The defect

execute_place branches on the venue's orderStatus:

if instruction_report.order_status == "PENDING":
    pass
elif instruction_report.order_status == "EXPIRED":
    order.execution_complete()
else:
    order.executable()  # let process.py pick it up

Betfair's orderStatus enum is PENDING | EXECUTABLE | EXECUTION_COMPLETE | EXPIRED. A limit order that crosses the book on placement returns EXECUTION_COMPLETE, which matches neither branch and falls through to the else — publishing a fully-matched order as EXECUTABLE.

Why it only shows up sometimes

The else is annotated "let process.py pick it up", and it does: process_current_order has an elif order.status == OrderStatus.EXECUTABLE arm that transitions to EXECUTION_COMPLETE once the stream delivers a CurrentOrder reporting that status.

So the order self-heals — after the HTTP response, when the next order-stream update lands. The window between the place response and that update is exactly when the order advertises a live status with size_remaining == 0. Anything reading order.status in that window acts on a phantom position.

That also matches the follow-up in #743: it reproduces in both sync and async mode, because this is about report handling rather than transport.

The fix

-elif instruction_report.order_status == "EXPIRED":
+elif instruction_report.order_status in ["EXECUTION_COMPLETE", "EXPIRED"]:
     order.execution_complete()

The place report is already authoritative — no stream round-trip is needed to know the order is terminal. This makes terminality a function of the venue's answer rather than of which branch happened to run.

Tests

New file tests/test_execution_order_status.py, four tests:

Test	Purpose
test_immediately_matched_is_execution_complete	the regression
test_resting_order_is_executable	control — resting orders still go EXECUTABLE
test_expired_is_execution_complete	control — pre-existing EXPIRED path
test_pending_leaves_status_alone	control — async PENDING publishes nothing

The tests drive a real BetfairOrder and mock only _execution_helper (the HTTP boundary). The defect is in the state the order ends up in; a Mock holds no state, so a mock-based assertion cannot fail on this — and the existing test_execute_place_success leaves order_status as an auto-Mock, which is why it passes today and effectively pins the else branch as correct.

Verified in both directions:

# without this patch
FAILED (failures=1)
AssertionError: <OrderStatus.EXECUTABLE> != <OrderStatus.EXECUTION_COMPLETE>

# with this patch
Ran 4 tests ... OK

Full suite: 1059 tests, all passing. No existing test is modified.

Scope

Only #743. The same else-branch pattern exists in execute_replace, where replacement_order.executable() at betfairexecution.py:209 is unconditional — but a replacement order's status has to come off the placeInstructionReport, and I'd rather not bundle two behavioural changes into one fix. Happy to follow up if you want it.

#509 (stale size_remaining/size_cancelled after a partial cancel) is diagnosed but not addressed here. execute_cancel reads instruction_report.size_cancelled to decide the status and then discards it, leaving the order publishing pre-cancel sizes until the stream catches up. A naive fix double-counts when the stream has already delivered the update, so it needs a deliberate idempotency decision rather than a patch. I'll put the failing test in that thread rather than land a red test here.

Happy to rework either if the approach is wrong — the analysis is in #743 and #509.

[liampauling]

High chance of a race condition if you do this

[wippa-studios]

You're right, and I missed the mechanism when I wrote that line.

The change isn't status-only, and that's my error. I traced it after your comment:

order.execution_complete()
  -> _update_status()
     -> if self.complete and self.trade.complete: self.trade.complete_trade()
        -> runner_context.reset(self.id)

runner_context.reset() mutates shared strategy state. Previously that only ever fired from the stream-processing path in process.py, on one thread. My patch adds a second, earlier call site from the execution thread pool — so trade completion and a runner-context reset can now land concurrently with strategy logic running on the stream thread. That's a new race I introduced, not one I avoided.

My claim that "the place report is already authoritative" holds for the status. It does not hold for when the trade is considered complete. I asserted terminality where I should have been asserting it without the cascade.

Where that leaves the options, as I see them

1. Close it. process.py already transitions EXECUTABLE -> EXECUTION_COMPLETE off the stream. The defect is real but the window may be acceptable, or better solved where the stream owns it.
2. Guard it — only take the terminal transition from the response if the order hasn't already completed:

   elif instruction_report.order_status in ["EXECUTION_COMPLETE", "EXPIRED"]:
       if not order.complete:
           order.execution_complete()
   

   This doesn't remove the cross-thread call, it only stops the double-fire. It does not address what you actually raised, so I'd only do it if you think that's the whole risk.
3. Split the concern — set the status without the trade cascade, and let the stream drive complete_trade() as it does today. That's a new code path and I'm not convinced it's better than (1).

I lean toward (1) — the status is genuinely wrong but I can't demonstrate that fixing it here is safe, and you know the threading model better than I do.

Tests in the PR stay useful either way: test_immediately_matched_is_execution_complete fails on current master, and the three controls pass. They document the defect without requiring the fix to land.

Happy to close it, or to rework to whichever option you prefer. Not going to push back on the race — I couldn't rule it out from the code and I've only now read complete_trade().

[liampauling]

i am not reading all of that

[wippa-studios]

Closing this one.

You're right about the race — execution_complete() cascades into trade.complete_trade() and runner_context.reset(), and my patch moved that off the stream thread. I should have read complete_trade() before proposing the change rather than after.

Closing rather than reworking since you've already told me where it lands. The analysis and the failing test are in #743 if it's useful later.

Not pushing back.