From 4a8b0132303f41f5a472eba0e5704d4f1ffdc9df Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 1 Sep 2026 07:18:54 -0400 Subject: [PATCH] fix(execution): cancel a resting SELL whose position has left the account (#668) `_clear_resting_bracket` fails closed before an exit. `reconcile_unbracketed_ positions` heals a position that has no bracket. Both are POSITION-DRIVEN: they start from a tranche and ask what protects it. A protective order whose position has left the account has no tranche to be walked from, so neither can reach it, and nothing else in the engine looked. That is the structural reason the gap existed rather than an oversight in either sweep. `sweep_orphan_brackets` runs the other way -- from keel's own resting SELLs to the account -- and cancels any whose product the venue no longer holds. Three ways one appears: the operator sells or transfers the asset at the venue and keel's bracket keeps resting against inventory that left without telling it; `scale_out` and `_roll_stop` both cancel before they place, and a process dying inside that window can leave a replacement resting over a resolved position; and a venue without a native trigger-bracket needs two legs, where a filled target leaves a live stop behind -- not reachable on the venue that trades live today, where `BracketGTC` is one order, and reachable the moment one is not. Left alone the market reverses through the stop and the venue is asked to sell an asset that is not there. On a cash account that is a rejection; on a margin-enabled one (#666) it is a short produced entirely by a missing cancel. THE VENUE DECIDES, NEVER THE LEDGER, and that asymmetry is a safety property rather than a preference. Cancelling a protective order over a position that really exists strips a live holding of its only stop, so the trigger has to be the account's own statement about itself. keel's ledger can be stale in exactly the dangerous direction -- saying zero while the venue holds the position -- and a sweep driven from it would cancel precisely the brackets that were working. An unreadable balance therefore cancels NOTHING. `Balance.total`, not `Balance.available`, for the same reason as #667's clamp: a resting SELL holds the base it commits, so `available` reads ~0 for exactly the products this sweep looks at. Reading it would cancel every protective order keel has ever placed, on the first cycle, and call the result a fix. Dust strictly BELOW the venue's `base_increment` counts as nothing held -- a residue the venue cannot express as a size is not a position. Exactly one increment is the smallest real position there is and keeps its bracket. With no increment known there is no threshold and none is invented: `_base_increment_for` returns None for a venue error as well as an unknown product, and a guessed floor would cancel protective orders over a number nobody supplied. Runs LAST of the three reconciliation passes, and the order is load-bearing: `reconcile_unbracketed_positions` will PLACE a bracket for a tranche whose position left out of band, so sweeping afterwards makes the cancel the cycle's final word and nothing re-creates within one cycle what it just removed. #667's rail 21 stops that re-place at source, which makes the churn rare rather than the ordering optional -- the rail refuses to PLACE into an empty holding, this cancels what already rests, and neither subsumes the other. The tranche is deliberately left open. Closing it would book a realized outcome at a price nobody observed; `doctor`'s new `bracket.orphan` and the existing `ledger.unbooked_exit` both surface it for a human instead. 11 mutants, 11 killed -- including the wiring (a sweep nothing calls reports nothing) and its POSITION in the cycle, which a presence-only test would miss. Stacked on #667: the sweep reads `executor._held_base`, and a second reader of the same venue fact would be two things to keep in agreement. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NzuKAe2RVrPt9acVAWjRyL --- docs/fiqh-basis.md | 8 + keel/agent.py | 10 + keel/commands/doctor.py | 46 ++++ keel/execution/reconcile.py | 151 +++++++++++ tests/commands/test_doctor.py | 49 ++++ tests/execution/test_orphan_bracket.py | 352 +++++++++++++++++++++++++ tests/test_agent.py | 39 +++ 7 files changed, 655 insertions(+) create mode 100644 tests/execution/test_orphan_bracket.py diff --git a/docs/fiqh-basis.md b/docs/fiqh-basis.md index 44d6288f..b9324454 100644 --- a/docs/fiqh-basis.md +++ b/docs/fiqh-basis.md @@ -182,6 +182,14 @@ refused BUY costs nothing; a refused SELL strands a position that wanted out. An balance is not evidence the position is gone, and this is the one place in the engine where "unknown is a rejection" would do more harm than the hole it closes. +Beside the rail, one sweep with the same warrant and no number: `sweep_orphan_brackets` +(#668) cancels a resting SELL whose position has left the account. It is not a rail because it +runs on the reconciliation pass rather than before an order, and it is order-driven where every +other sweep is position-driven -- a protective order whose position is gone has no tranche to be +found from. The trigger is the venue's own statement, never keel's ledger: cancelling a stop +over a position that really exists strips a live holding of its only protection, and the ledger +can be stale in exactly that direction. + What remains open at the venue boundary is #666: on a cash account every case above is a rejected order rather than a short, and keel has no cash-account posture check on Coinbase — `verify_cash_account` exists only on the Alpaca adapter. diff --git a/keel/agent.py b/keel/agent.py index 00bb1e12..875ed8eb 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -1648,6 +1648,16 @@ def run_once( # row, so the pass above cannot see it (issue #195). reconcile.reconcile_unbracketed_positions(broker, repo, config, now_ts) + # Then cancel any resting SELL the account can no longer honour (#668). LAST of the + # three, and the order is load-bearing: the pass above heals a tranche that has no + # bracket, and for a tranche whose position left the account out of band it will place + # one -- against nothing. Sweeping afterwards makes this the cycle's final word, so + # nothing re-creates within the same cycle what it just cancelled. (#667's rail 21 stops + # that re-place at source, which makes the churn rare rather than making the order + # optional: the rail refuses to PLACE into an empty holding, and this cancels what is + # already resting. Neither subsumes the other.) + reconcile.sweep_orphan_brackets(broker, repo, now_ts) + # Rail 11's inputs, refreshed BEFORE any entry this cycle. `poll_once` above has already # written the candles, so every product's latest price is readable here -- and it has to # be done now, not after the loop: `guards.check` reads these scalars from inside diff --git a/keel/commands/doctor.py b/keel/commands/doctor.py index 480c136e..64438d9a 100644 --- a/keel/commands/doctor.py +++ b/keel/commands/doctor.py @@ -813,6 +813,45 @@ def balance_drift_findings(records: dict[str, Any]) -> list[Finding]: ] +def orphan_bracket_findings(records: dict[str, Any]) -> list[Finding]: + """Resting SELLs the orphan sweep cancelled because the account no longer held them (#668). + + The cancel already resolved the ORDER -- there is nothing left at the venue to trigger + against nothing. What it did not resolve is why keel was protecting a position the account + says is gone, and that question outlives the order: the tranche is still open in the ledger, + deliberately, because closing it would book a realized outcome at a price nobody observed. + + WARN. The sweep did the safe thing and did it automatically; this is the record that it had + to, which is a state a human should look at once rather than an ongoing fault. + """ + orphans = sorted((p, r) for p, r in records.items() if isinstance(r, dict)) + if not orphans: + return [ + Finding( + "bracket.orphan", + OK, + "no orphaned protective orders", + "every resting SELL stands against a position the venue confirms", + "-", + ) + ] + described = ", ".join( + f"{product}: order {record.get('order_id')} cancelled, venue held {record.get('held')}" + for product, record in orphans + ) + return [ + Finding( + "bracket.orphan", + WARN, + f"{len(orphans)} product(s) had a protective order with nothing behind it", + f"{described} -- cancelled before the market could trigger it; the tranche is " + "still open in the ledger", + "reconcile the position: an out-of-band sale or transfer, or an exit that " + "already executed at the venue", + ) + ] + + def unbooked_exit_findings( open_positions: list[dict[str, Any]], orders: list[dict[str, Any]] ) -> list[Finding]: @@ -985,6 +1024,7 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in from keel.commands._products import _default_sim_products from keel.execution import executor as executor_mod from keel.execution import guards + from keel.execution import reconcile as reconcile_mod findings: list[Finding] = [] @@ -1037,6 +1077,12 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in for key in repo.get_state_keys(executor_mod.BALANCE_DRIFT_PREFIX) } ) + findings += orphan_bracket_findings( + { + key[len(reconcile_mod.ORPHAN_BRACKET_PREFIX) :]: repo.get_state(key) + for key in repo.get_state_keys(reconcile_mod.ORPHAN_BRACKET_PREFIX) + } + ) # #639: modes are POOLED here, unlike the partial-fill sweep above -- the ledger # invariant belongs to `agent._open_tranche`, which writes it for paper and live alike. findings += unbooked_exit_findings(repo.get_open_positions(), repo.get_orders()) diff --git a/keel/execution/reconcile.py b/keel/execution/reconcile.py index 6053dadc..7ae6c29a 100644 --- a/keel/execution/reconcile.py +++ b/keel/execution/reconcile.py @@ -29,6 +29,14 @@ recorded before this change, which is the gap itself. That rarity changes the priority of auto-remediation, not the validity of recognizing the state. +ORPHANED PROTECTIVE ORDERS (#668) are swept here too, and by a different kind of pass. Every +other reconciliation in this module and in `executor` is POSITION-DRIVEN: it starts from a +tranche and asks what protects it. `sweep_orphan_brackets` runs the other way -- it starts from +keel's own resting SELLs and asks whether the account still holds what each one stands over -- +because a protective order whose position has left the account has no tranche to be walked from, +and nothing else in the engine can reach it. Left alone, the market reverses through the stop and +the venue is asked to sell an asset that is not there. + What is deliberately NOT done here: resizing or amending the bracket when a partially-filled entry leaves it oversized for what is held. The port migration is done and the cancel-and-replace is expressible (`BracketGTC` since #569; `executor._roll_stop` performs @@ -316,6 +324,149 @@ def reconcile_unbracketed_positions( return healed +#: `agent_state` key prefix for a resting SELL this sweep cancelled because the account no +#: longer held the product (#668). Read by `keel doctor`: the cancel resolves the ORDER, but the +#: divergence that produced it -- keel holding a protective order for something the venue says is +#: gone -- is a fact about the deployment that outlives the order and nobody else reports. +ORPHAN_BRACKET_PREFIX = "orphan_bracket:" + + +def sweep_orphan_brackets(broker: Any, repo: Repository, now_ts: int) -> list[int]: + """Cancel every resting SELL for a product the venue says the account no longer holds (#668). + + **The gap this closes is structural, not incidental.** `_clear_resting_bracket` fails closed + before an exit, and `reconcile_unbracketed_positions` heals a position with no bracket. Both + are POSITION-DRIVEN -- they start from a tranche and ask what protects it. A resting SELL + whose position is gone has no tranche to be walked from, so neither can reach it, and nothing + else looks. + + How one appears, since the shape is not obvious on a venue with native brackets: + + * the operator sells or transfers the asset at the venue themselves -- keel's bracket keeps + resting against inventory that left without telling it; + * `scale_out` and `_roll_stop` both cancel before they place, and a process that dies inside + that window can leave a replacement resting with the position already resolved; + * a venue without a native trigger-bracket needs two legs, and a filled target then leaves a + live stop behind (the classic spot-OCO collision -- not reachable on the venue that trades + live today, where `BracketGTC` is one order, and reachable the moment one is not). + + The market then reverses through the stop and the venue sells an asset that is not held. On a + cash account that is a rejection; on a margin-enabled one (#666) it is a short, produced + entirely by a missing cancel -- *bay' ma la yamlik* with nobody in the loop. + + **The VENUE decides, never the ledger, and that asymmetry is a safety property.** Cancelling + a protective order on a position that really exists strips a live holding of its stop, so the + trigger must be the account's own statement about itself. keel's ledger can be stale in the + dangerous direction -- it says zero while the venue holds the position -- and a sweep driven + from it would cancel exactly the brackets that were doing their job. An unreadable balance + therefore cancels NOTHING; the sweep simply did not run for that product this cycle. + + `Balance.total`, not `Balance.available`, for the same reason as #667's clamp: a resting SELL + holds the base it commits, so `available` reads ~0 for precisely the products this sweep is + looking at, and cancelling on it would cancel every bracket keel has ever placed. + + Dust below the venue's own `base_increment` counts as nothing held -- a residue the venue + cannot even express as a size is not a position, and a bracket protecting it is an orphan + with a rounding error attached. + + Returns the local ids it cancelled. **Never raises into the cycle**: an unreachable venue + means the sweep did not run, which is the honest state, and the next cycle retries. The + tranche is deliberately LEFT ALONE -- closing it would book a realized outcome at a price + nobody observed, and this function's warrant covers the order, not the ledger. + """ + cancelled: list[int] = [] + rows = [r for r in _polled_rows(repo) if str(r["side"]).upper() == Side.SELL.value.upper()] + if not rows: + return cancelled + + # One balance read per PRODUCT, not per row: a product with two resting legs asks the same + # question twice otherwise, and this runs on every cycle. + held_by_product: dict[str, Decimal | None] = {} + for row in rows: + product_id = row["product_id"] + if product_id not in held_by_product: + held_by_product[product_id] = _orphan_threshold_held(broker, repo, product_id, now_ts) + held = held_by_product[product_id] + if held is None: + continue + + try: + executor._cancel_at_exchange(broker, repo, row) + except executor.CancelPending: + # The venue took it and settles asynchronously. Not a failure and not a retry: + # `reconcile_open_orders` reads the terminal state next cycle, and this sweep will + # see the row gone from `_polled_rows` when it does. + log_event( + logger, + logging.INFO, + "reconcile.orphan_cancel_pending", + product=product_id, + order_id=row["id"], + ) + continue + except Exception: + # Includes `CancelUnavailable` -- an order the venue refuses to cancel may be one it + # already filled, which is a fact the next status poll will settle. Per-row + # isolation, like every other venue call in this module: one refusal must not + # abandon the remaining orphans. + log_exception( + logger, + "reconcile.orphan_cancel_failed", + product=product_id, + order_id=row["id"], + ) + continue + + repo.update_order(row["id"], status="canceled", updated_at=now_ts) + repo.set_state( + f"{ORPHAN_BRACKET_PREFIX}{product_id}", + {"order_id": row["id"], "held": str(held), "cancelled_at": now_ts}, + ) + cancelled.append(row["id"]) + log_event( + logger, + logging.WARNING, + "reconcile.orphan_bracket_cancelled", + product=product_id, + order_id=row["id"], + held=str(held), + detail=( + "a resting SELL was protecting a position the venue reports the account no " + "longer holds -- cancelled before the market could trigger it against nothing. " + "The tranche is left open deliberately: closing it would book an outcome at a " + "price nobody observed" + ), + ) + + return cancelled + + +def _orphan_threshold_held( + broker: Any, repo: Repository, product_id: str, now_ts: int +) -> Decimal | None: + """The venue's holding when it is small enough to orphan a bracket, else `None`. + + `None` is the DO-NOTHING answer and covers two different situations on purpose: the balance + could not be read (so nothing may be cancelled), and the account holds a real position (so + nothing should be). Collapsing them is safe precisely because the action is the same and the + dangerous mistake -- cancelling a live position's only stop -- is impossible from either. + """ + held = executor._held_base(broker, product_id) + if held is None: + return None + increment = executor._base_increment_for(broker, repo, product_id, now_ts) + if increment is not None and increment > 0: + # Dust the venue cannot express as a size is not a position. Strictly BELOW the + # increment: a holding of exactly one increment is the smallest real position there is, + # and a bracket over it is doing its job. + return held if held < increment else None + # No increment known, so there is no dust threshold to apply and none may be invented -- + # only an affirmative nothing counts. `_base_increment_for` returns None for a venue error + # as well as for a genuinely unknown product, and guessing a floor from either would cancel + # protective orders over a number nobody supplied. + return held if held <= 0 else None + + def _has_resting_bracket(repo: Repository, position: dict[str, Any]) -> bool: """Whether this tranche still has a bracket working at the exchange.""" bracket_id = position.get("bracket_order_id") diff --git a/tests/commands/test_doctor.py b/tests/commands/test_doctor.py index b78f2646..c583578c 100644 --- a/tests/commands/test_doctor.py +++ b/tests/commands/test_doctor.py @@ -29,6 +29,7 @@ doctor_exit_code, doctor_lines, gather_findings, + orphan_bracket_findings, partial_fill_findings, rail_state_findings, render_json, @@ -582,6 +583,7 @@ def test_gather_findings_covers_every_check_over_a_seeded_db(tmp_path, valid_con "veto.recent", "fill.partial", "balance.drift", + "bracket.orphan", "ledger.unbooked_exit", "data.missing", "data.stale", @@ -834,3 +836,50 @@ def test_gather_findings_surfaces_a_recorded_drift(tmp_path, valid_config_path) (drift,) = [f for f in findings if f.name == "balance.drift"] assert drift.status == "warn" assert "BTC-USD" in drift.detail + + +# -- orphaned protective orders (#668) ----------------------------------------------------------- + + +def test_orphan_bracket_findings_is_ok_when_none_were_swept() -> None: + (finding,) = orphan_bracket_findings({}) + + assert finding.name == "bracket.orphan" + assert finding.status == "ok" + + +def test_orphan_bracket_findings_warns_and_says_the_tranche_is_still_open() -> None: + """The cancel resolved the order. The ledger row it stood over is deliberately untouched. + + An operator reading only "cancelled" would assume the position was closed out; it was not, + and the detail has to say so or the finding is misleading in the direction that matters. + """ + (finding,) = orphan_bracket_findings( + {"BTC-USD": {"order_id": 7, "held": "0", "cancelled_at": NOW}} + ) + + assert finding.status == "warn" + assert "still open in the ledger" in finding.detail + + +def test_orphan_bracket_findings_ignores_a_malformed_record() -> None: + (finding,) = orphan_bracket_findings({"BTC-USD": "not a record"}) + + assert finding.status == "ok" + + +def test_gather_findings_surfaces_a_swept_orphan(tmp_path, valid_config_path) -> None: + """The wiring, keyed on the prefix the sweep actually writes.""" + from keel.execution.reconcile import ORPHAN_BRACKET_PREFIX + + repo = _seeded_repo(tmp_path / "keel.db") + repo.set_state( + f"{ORPHAN_BRACKET_PREFIX}BTC-USD", {"order_id": 7, "held": "0", "cancelled_at": NOW} + ) + config = load_config(valid_config_path) + + findings = gather_findings(repo, config, [], NOW) + + (orphan,) = [f for f in findings if f.name == "bracket.orphan"] + assert orphan.status == "warn" + assert "BTC-USD" in orphan.detail diff --git a/tests/execution/test_orphan_bracket.py b/tests/execution/test_orphan_bracket.py new file mode 100644 index 00000000..84dc8bac --- /dev/null +++ b/tests/execution/test_orphan_bracket.py @@ -0,0 +1,352 @@ +"""#668 -- a resting SELL whose position is gone is cancelled before the market can trigger it. + +`_clear_resting_bracket` fails closed before an exit. `reconcile_unbracketed_positions` heals a +position that has no bracket. Both are POSITION-DRIVEN: they start from a tranche and ask what +protects it. A protective order whose position has left the account has no tranche to be walked +from, so neither can reach it, and before this sweep nothing looked. + +The market then reverses through the stop and the venue is asked to sell an asset that is not +there. On a cash account that is a rejection; on a margin-enabled one (#666) it is a short -- +*bay' ma la yamlik* produced entirely by a missing cancel, with nobody in the loop. + +The trigger is the VENUE's own statement about the account, never keel's ledger, and that +asymmetry is the safety property these tests exist to hold: cancelling a protective order over a +position that really exists strips a live holding of its only stop, and keel's ledger can be +stale in exactly that direction. +""" + +from __future__ import annotations + +from decimal import Decimal + +from keel_broker_api.results import Balance, CancelOutcome, Instrument + +from keel.data.repository import Repository +from keel.execution.reconcile import ORPHAN_BRACKET_PREFIX, sweep_orphan_brackets +from keel.types import Side +from tests.execution.test_reconcile import ( + NOW, + PRODUCT, + repo, # noqa: F401 -- the shared in-memory Repository fixture +) + + +class SweepBroker: + """The three reads the sweep makes, and a cancel that records what it was asked to kill. + + `base` is the account's holding of the base leg as `Balance.total`; `available` is set + independently so a test can model the venue's real answer for a product with a resting + order against it -- `available` zero, the whole position on `hold`. + """ + + def __init__( + self, + *, + base: Decimal | None = Decimal("0"), + available: Decimal | None = None, + increment: Decimal | None = Decimal("0.00000001"), + cancel: object = CancelOutcome.CONFIRMED, + raise_on_balances: bool = False, + ) -> None: + self._base = base + self._available = base if available is None else available + self._increment = increment + self._cancel = cancel + self._raise_on_balances = raise_on_balances + self.cancelled: list[str] = [] + self.get_balances_calls = 0 + + def get_balances(self) -> list[Balance]: + self.get_balances_calls += 1 + if self._raise_on_balances: + raise ConnectionError("simulated balance outage") + balances = [Balance(currency="USD", available=Decimal("1000"), total=Decimal("1000"))] + if self._base is not None: + balances.append( + Balance(currency="BTC", available=self._available, total=self._base) + ) + return balances + + def get_instrument(self, product_id: str) -> Instrument | None: + if self._increment is None: + return None + return Instrument(product_id=product_id, base_increment=self._increment) + + def cancel_order(self, order_id: str) -> object: + self.cancelled.append(order_id) + if isinstance(self._cancel, Exception): + raise self._cancel + return self._cancel + + +def _resting_sell( + repo: Repository, # noqa: F811 + *, + native_id: str = "cb-orphan-1", + status: str = "pending", + product_id: str = PRODUCT, + side: str = Side.SELL.value, +) -> int: + """One resting protective order, as `place_bracket` leaves it.""" + return repo.insert_order( + dict( + mode="live", + product_id=product_id, + side=side, + order_type="market", + qty=Decimal("0.01"), + limit_price=None, + status=status, + fee=None, + expected_fill=Decimal("49000"), + actual_fill=None, + raw_response=f'{{"order_id": "{native_id}"}}', + created_at=NOW - 1000, + updated_at=NOW - 1000, + ) + ) + + +# -- what the sweep cancels ------------------------------------------------------------------- + + +def test_a_resting_sell_over_an_empty_account_is_cancelled(repo): # noqa: F811 + """The whole point: the venue holds nothing, so the protective order protects nothing.""" + order_id = _resting_sell(repo) + broker = SweepBroker(base=Decimal("0")) + + cancelled = sweep_orphan_brackets(broker, repo, NOW) + + assert cancelled == [order_id] + assert broker.cancelled == ["cb-orphan-1"] + assert repo.get_order(order_id)["status"] == "canceled" + + +def test_dust_below_the_base_increment_counts_as_nothing_held(repo): # noqa: F811 + """A residue the venue cannot express as a size is not a position. + + A bracket over it is an orphan with a rounding error attached: the venue would refuse the + order for being under its own minimum, so nothing is being protected either way. + """ + order_id = _resting_sell(repo) + broker = SweepBroker(base=Decimal("0.000000005"), increment=Decimal("0.00000001")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [order_id] + + +def test_exactly_one_increment_is_a_real_position(repo): # noqa: F811 + """The boundary, and it belongs on the side of NOT cancelling. + + One increment is the smallest position the venue can express. A bracket over it is doing its + job, and the comparison must be strictly-below rather than at-or-below. + """ + _resting_sell(repo) + broker = SweepBroker(base=Decimal("0.00000001"), increment=Decimal("0.00000001")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + assert broker.cancelled == [] + + +def test_a_partially_filled_bracket_is_swept_too(repo): # noqa: F811 + """`partially_filled` is RESTING (#446) -- its unfilled remainder still works at the venue. + + Sweeping only `pending` would leave exactly the leg that has already begun executing against + a position that is gone. + """ + order_id = _resting_sell(repo, status="partially_filled") + broker = SweepBroker(base=Decimal("0")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [order_id] + + +# -- what the sweep must NOT cancel ----------------------------------------------------------- + + +def test_a_real_position_keeps_its_bracket(repo): # noqa: F811 + """The dangerous mistake, and the one this suite exists to make impossible.""" + _resting_sell(repo) + broker = SweepBroker(base=Decimal("0.01")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + assert broker.cancelled == [] + + +def test_a_bracket_holding_its_own_base_keeps_it(repo): # noqa: F811 + """`Balance.total`, not `Balance.available` -- the same trap as #667's clamp. + + A resting SELL HOLDS the base it commits, so `available` reads zero for precisely the + products this sweep looks at. Reading it would cancel every protective order keel has ever + placed, on its first cycle, and call the result a fix. + + Modelled as the venue reports it: `available=0`, `hold=0.01`, `total=0.01`. + """ + _resting_sell(repo) + broker = SweepBroker(base=Decimal("0.01"), available=Decimal("0")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [], ( + "a resting bracket's own hold was read as an empty account -- the sweep is reading " + "`Balance.available`" + ) + + +def test_an_unreadable_balance_cancels_nothing(repo): # noqa: F811 + """Unknown is the do-nothing answer here, not a licence to cancel. + + An unreachable venue means the sweep did not run, which is the honest state; the next cycle + retries. Failing closed in the other direction would strip protection over a network blip. + """ + _resting_sell(repo) + broker = SweepBroker(raise_on_balances=True) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + assert broker.cancelled == [] + + +def test_a_venue_with_no_account_row_cancels_nothing(repo): # noqa: F811 + """A venue that omits empty accounts is saying nothing, not saying zero. + + Only an affirmative report of a zero holding may cancel. Treating a sparse response as an + empty account would cancel brackets on every venue whose only fault is a terse balance list. + """ + _resting_sell(repo) + broker = SweepBroker(base=None) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + + +def test_an_unknown_increment_still_cancels_on_an_affirmative_zero(repo): # noqa: F811 + """No increment means no dust threshold -- and none may be invented. + + `_base_increment_for` returns None for a venue error as much as for an unknown product, so a + guessed floor would cancel protective orders over a number nobody supplied. An affirmative + zero needs no threshold to be unambiguous. + """ + order_id = _resting_sell(repo) + broker = SweepBroker(base=Decimal("0"), increment=None) + + assert sweep_orphan_brackets(broker, repo, NOW) == [order_id] + + +def test_an_unknown_increment_does_not_cancel_over_dust(repo): # noqa: F811 + """The other half of the same rule: without an increment, only zero counts.""" + _resting_sell(repo) + broker = SweepBroker(base=Decimal("0.000000005"), increment=None) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + + +def test_a_resting_buy_is_never_swept(repo): # noqa: F811 + """A resting BUY is not a protective order and holds no base to be orphaned from.""" + _resting_sell(repo, side=Side.BUY.value) + broker = SweepBroker(base=Decimal("0")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + + +def test_a_terminal_order_is_never_swept(repo): # noqa: F811 + """Only orders still working at the venue can be cancelled; the rest are history.""" + _resting_sell(repo, status="filled") + broker = SweepBroker(base=Decimal("0")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + assert broker.cancelled == [] + + +# -- failure behaviour -------------------------------------------------------------------------- + + +def test_a_refused_cancel_leaves_the_row_alone_and_continues(repo): # noqa: F811 + """A venue that refuses a cancel may have already filled the order. Local state must not lie. + + And the sweep keeps going: one refusal must not abandon the remaining orphans -- the same + per-item isolation every other venue call in this module has. + """ + first = _resting_sell(repo, native_id="cb-refused") + second = _resting_sell(repo, native_id="cb-ok") + + class PartlyRefusing(SweepBroker): + def cancel_order(self, order_id: str) -> object: + self.cancelled.append(order_id) + return CancelOutcome.REFUSED if order_id == "cb-refused" else CancelOutcome.CONFIRMED + + broker = PartlyRefusing(base=Decimal("0")) + cancelled = sweep_orphan_brackets(broker, repo, NOW) + + assert cancelled == [second] + assert repo.get_order(first)["status"] == "pending", ( + "a refused cancel was recorded locally as canceled while it may still be live" + ) + assert broker.cancelled == ["cb-refused", "cb-ok"] + + +def test_an_accepted_but_unsettled_cancel_is_not_recorded_as_done(repo): # noqa: F811 + """The venue took it and settles asynchronously. `reconcile_open_orders` reads the terminal + state next cycle; claiming it here would retire an order that is still working.""" + order_id = _resting_sell(repo) + broker = SweepBroker(base=Decimal("0"), cancel=CancelOutcome.ACCEPTED) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + assert repo.get_order(order_id)["status"] == "pending" + + +def test_a_raising_cancel_does_not_abort_the_sweep(repo): # noqa: F811 + """Never raises into the cycle. This runs at the top of every run and owns no failure.""" + _resting_sell(repo) + broker = SweepBroker(base=Decimal("0"), cancel=RuntimeError("venue exploded")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + + +def test_the_balance_is_read_once_per_product_not_once_per_order(repo): # noqa: F811 + """Two legs on one product ask the venue one question. This runs on every cycle.""" + _resting_sell(repo, native_id="cb-a") + _resting_sell(repo, native_id="cb-b") + broker = SweepBroker(base=Decimal("0")) + + assert len(sweep_orphan_brackets(broker, repo, NOW)) == 2 + assert broker.get_balances_calls == 1 + + +def test_an_empty_book_touches_no_venue_at_all(repo): # noqa: F811 + """No resting SELLs, no balance read. The common case must cost nothing.""" + broker = SweepBroker(base=Decimal("0")) + + assert sweep_orphan_brackets(broker, repo, NOW) == [] + assert broker.get_balances_calls == 0 + + +# -- what it records ---------------------------------------------------------------------------- + + +def test_a_cancelled_orphan_is_recorded_for_doctor(repo): # noqa: F811 + """The cancel resolves the ORDER; it does not resolve why keel was protecting a ghost.""" + order_id = _resting_sell(repo) + + sweep_orphan_brackets(SweepBroker(base=Decimal("0")), repo, NOW) + + record = repo.get_state(f"{ORPHAN_BRACKET_PREFIX}{PRODUCT}") + assert record is not None + assert record["order_id"] == order_id + assert record["held"] == "0" + + +def test_the_tranche_is_left_open_on_purpose(repo): # noqa: F811 + """Closing it would book a realized outcome at a price nobody observed. + + The sweep's warrant covers the ORDER. What the position was worth when it left the account + is a question only an operator can answer, and inventing an answer is worse than the open + row -- `doctor`'s `ledger.unbooked_exit` and `bracket.orphan` both surface it. + """ + _resting_sell(repo) + position_id = repo.open_position( + product_id=PRODUCT, + rule_name="turtle_breakout", + opened_at=NOW - 1000, + qty=Decimal("0.01"), + entry_fill=Decimal("50000"), + entry_fee=Decimal("3"), + ) + + sweep_orphan_brackets(SweepBroker(base=Decimal("0")), repo, NOW) + + assert [p["id"] for p in repo.get_open_positions()] == [position_id] diff --git a/tests/test_agent.py b/tests/test_agent.py index bcd801c6..51f2291a 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -4550,3 +4550,42 @@ def test_a_fully_filled_exit_still_closes_everything_and_clears_the_state( assert len(repo.get_trade_outcomes()) == 1 assert repo.get_state(f"position_rule:{PRODUCT}") is None assert repo.get_state(f"open_stop:{PRODUCT}") is None + + +# -- the orphan-bracket sweep runs, and runs LAST (#668) ---------------------------------------- + + +def test_run_once_sweeps_orphan_brackets_after_the_rebracket_pass(repo, monkeypatch): + """The sweep is wired into the cycle, and its POSITION in the cycle is the point. + + A function nothing calls reports nothing, and no test above this one would notice: every + sweep test drives `sweep_orphan_brackets` directly. This asserts the wiring. + + The order matters as much as the presence. `reconcile_unbracketed_positions` heals a tranche + that has no bracket, and for a tranche whose position left the account out of band it will + PLACE one -- against nothing. Sweeping afterwards makes the cancel the cycle's final word, + so nothing re-creates within the same cycle what the sweep just removed. Running it first + would leave a fresh orphan resting for a full day, and this deployment cycles once per UTC + day. + """ + from keel.execution import reconcile + + calls: list[str] = [] + + def record(name): + def _fn(*args, **kwargs): + calls.append(name) + return [] + + return _fn + + monkeypatch.setattr(reconcile, "reconcile_open_orders", record("open_orders")) + monkeypatch.setattr(reconcile, "reconcile_unbracketed_positions", record("unbracketed")) + monkeypatch.setattr(reconcile, "sweep_orphan_brackets", record("orphan_sweep")) + repo.set_state("kill_switch", False) + + run_once(FakeBroker(), repo, _config(), now_ts=90_000) + + assert calls == ["open_orders", "unbracketed", "orphan_sweep"], ( + f"expected the three reconciliation passes in order, got {calls}" + )