From 474996f3fe147c44457da8501586362f1b2ead7c Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 17 Sep 2026 10:48:12 -0400 Subject: [PATCH] fix(execution): a cache record predating quote_increment is a MISS, not an "unknown" #802 shipped inert on the deployment it was cut for. `base_increment:PAXG-USD` had been written 24 hours before the upgrade by a build that did not know to ask for the price tick, and the 7-day TTL counted it FRESH -- so `_price_increment_for` read the absent key as "unknown", prices went out unrounded, and the venue rejected the bracket exactly as it had before. The position stayed unprotected, and would have for up to a week. The first cut's own docstring called that behaviour correct. It is correct only in the narrow sense that nothing knew the tick; operationally it waits out a TTL on data already known to be incomplete. The obvious fix -- refetch whenever the key is missing -- has a trap that is worse than the bug: a product whose venue genuinely reports no tick would refetch on EVERY cycle, putting a venue round-trip back into the order-placement path this cache exists to keep out of it. So the writer now records `quote_increment` ALWAYS, explicitly null when the venue reports none, and absence means one thing only: written before anyone asked. `_base_increment_for` grew `force_refresh` rather than having the caller delete the row first. Deleting is the shorter way to force a miss and the wrong one -- a venue call that then failed would have discarded a perfectly good `base_increment` and put SELL sizes back on the wire unquantized, which is #513. One test from #804 asserted the old behaviour and has been replaced rather than deleted: it now pins the narrower claim that survives -- a value present and unparseable is unknown, and is not repaired by guessing the common tick. Verified by mutation on both halves independently: reverting the read guard fails two tests, writing the key only when non-null fails the third. Refs #802. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AoGRoExgHVCsWDHT3ej8mD --- keel/execution/executor.py | 47 ++++++++++++---- tests/execution/test_price_precision.py | 71 +++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 13 deletions(-) diff --git a/keel/execution/executor.py b/keel/execution/executor.py index c96bc55..05c5821 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -379,7 +379,7 @@ def _withdrawals_enabled(repo: Any, now_ts: int) -> bool | None: def _base_increment_for( - broker: Any, repo: Repository, product_id: str, now_ts: int + broker: Any, repo: Repository, product_id: str, now_ts: int, *, force_refresh: bool = False ) -> Decimal | None: """The venue's finest acceptable `base_size` for `product_id`, cached, or `None` if unknown. @@ -397,12 +397,18 @@ def _base_increment_for( """ key = f"{BASE_INCREMENT_PREFIX}{product_id}" cached = repo.get_state(key) - if isinstance(cached, dict): + if not force_refresh and isinstance(cached, dict): fetched_at = cached.get("fetched_at") raw = cached.get("increment") if isinstance(fetched_at, int) and now_ts - fetched_at < BASE_INCREMENT_TTL_SEC: return _coerce_increment(raw) + # `force_refresh` skips the freshness check, never the record: a caller uses it when the + # record is fresh but INCOMPLETE (`_price_increment_for`, for a record predating + # `quote_increment`). Deleting the row first would be the shorter way to force a miss and the + # wrong one -- a venue call that then fails would have thrown away a perfectly good + # `base_increment` and put SELL sizes back on the wire unquantized, which is #513. + if broker is None: # Paper mode passes no broker; expected, not an error (same reasoning as # `_fetch_available_quote`). @@ -422,9 +428,18 @@ def _base_increment_for( # Both increments ride ONE fetch and ONE cached record. Splitting them would double the # venue round-trips inside the order-placement path -- the latency the per-product read in # `get_instrument` exists to avoid -- for two fields of the same response (#802). - record: dict[str, object] = {"increment": str(increment), "fetched_at": now_ts} - if instrument.quote_increment is not None: - record["quote_increment"] = str(instrument.quote_increment) + # `quote_increment` is written ALWAYS, explicitly `None` when the venue reports none. The + # key's PRESENCE is what tells `_price_increment_for` this record was written by a build that + # knew to ask; writing it only when there is a value would make "venue has no tick" + # indistinguishable from "record predates the field", and the two need opposite handling -- + # honour the first, refetch the second. + record: dict[str, object] = { + "increment": str(increment), + "fetched_at": now_ts, + "quote_increment": ( + None if instrument.quote_increment is None else str(instrument.quote_increment) + ), + } repo.set_state(key, record) return increment @@ -439,17 +454,31 @@ def _price_increment_for( unrounded, exactly as they did before #802. That is deliberately NOT a refusal -- refusing here would leave a filled position with no stop at all, which is the outcome #799 documents. - A record written before #802 carries no `quote_increment` key and reads as unknown until it - expires, which is correct: nothing knew the tick when it was written. + **A record that predates the field is a MISS, not an "unknown"** -- and the first cut of this + function got that wrong. It treated the absent key as unknown and let the 7-day TTL run, so a + deployment upgraded mid-TTL kept sending unrounded prices for up to a week, with the position + the fix was cut for sitting unprotected the whole time (live, 2026-09-17: a record written 24 + hours earlier by the previous build counted as fresh, and the venue rejected the bracket + exactly as it had before the upgrade). + + The two cases are distinguishable because the writer above always records the key, explicitly + `None` when the venue reports no tick. So `quote_increment` **absent** means "written before + anyone asked" -> refetch once; **present and null** means "the venue was asked and has none" + -> honour it, and do NOT re-ask on every cycle, which would put a venue round-trip back into + the order path this cache exists to keep out of it. + + A returned `None` still means UNKNOWN: prices go on the wire unrounded, exactly as they did + before #802. That is deliberately NOT a refusal -- refusing here would leave a filled position + with no stop at all, which is the outcome #799 documents. """ key = f"{BASE_INCREMENT_PREFIX}{product_id}" cached = repo.get_state(key) - if isinstance(cached, dict): + if isinstance(cached, dict) and "quote_increment" in cached: fetched_at = cached.get("fetched_at") if isinstance(fetched_at, int) and now_ts - fetched_at < BASE_INCREMENT_TTL_SEC: return _coerce_increment(cached.get("quote_increment")) - _base_increment_for(broker, repo, product_id, now_ts) + _base_increment_for(broker, repo, product_id, now_ts, force_refresh=True) refreshed = repo.get_state(key) if isinstance(refreshed, dict): return _coerce_increment(refreshed.get("quote_increment")) diff --git a/tests/execution/test_price_precision.py b/tests/execution/test_price_precision.py index 683cb0d..7ef8b2d 100644 --- a/tests/execution/test_price_precision.py +++ b/tests/execution/test_price_precision.py @@ -268,12 +268,17 @@ def test_both_increments_share_one_fetch(repo): # noqa: F811 assert broker.calls == 1 -def test_a_record_without_a_quote_increment_reads_unknown(repo): # noqa: F811 - """A cache record written before #802 carries no `quote_increment` key. Unknown is correct -- - nothing knew the tick when it was written -- and unknown must not become a guessed 0.01.""" +def test_an_unparseable_quote_increment_reads_unknown(repo): # noqa: F811 + """Unknown must never become a guessed `0.01`. + + This test used to assert that a record with the key ABSENT also read unknown. That was the + defect, not the contract -- see `test_a_record_written_before_the_field_existed_is_refetched` + below, and the live failure it names. What remains true is the narrower claim: a value that + is present and unusable is unknown, and is not repaired by guessing the common tick. + """ repo.set_state( f"{executor.BASE_INCREMENT_PREFIX}PAXG-USD", - {"increment": "0.00000001", "fetched_at": NOW_TS}, + {"increment": "0.00000001", "quote_increment": "not-a-number", "fetched_at": NOW_TS}, ) assert executor._price_increment_for(_InstrumentBroker(), repo, "PAXG-USD", NOW_TS) is None @@ -292,3 +297,61 @@ def test_no_broker_reads_unknown_rather_than_raising(repo): # noqa: F811 """Paper mode passes no broker. `_base_increment_for` is documented as never raising, and this must not be the function that reintroduces one into the order path.""" assert executor._price_increment_for(None, repo, "PAXG-USD", NOW_TS) is None + + +# -- a cache record that predates the field must not wait out its TTL -------------------------- + + +def test_a_record_written_before_the_field_existed_is_refetched(repo): # noqa: F811 + """The live failure this covers (2026-09-17, keel-live.db). + + `base_increment:PAXG-USD` was written by a pre-#802 build 24 hours before the cycle, so the + 7-day TTL counted it FRESH and `_price_increment_for` returned the absent key as "unknown". + Prices went out unrounded and the venue rejected them -- for up to a week, on exactly the + deployment the fix was cut for, while the position sat unprotected. + + "Unknown" is right when nothing knows the tick. It is wrong when the record simply predates + the question, and the two are distinguishable: a record that was WRITTEN with knowledge of + the field always carries the key, explicitly null when the venue reports none. + """ + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}PAXG-USD", + {"increment": "0.00001", "fetched_at": NOW_TS}, # no `quote_increment` key at all + ) + broker = _InstrumentBroker() + + assert executor._price_increment_for(broker, repo, "PAXG-USD", NOW_TS) == CENT + assert broker.calls == 1, "an incomplete record must be refetched, not waited out" + + +def test_the_refetch_happens_once_and_then_the_record_is_complete(repo): # noqa: F811 + """Self-healing, not a fetch on every cycle.""" + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}PAXG-USD", + {"increment": "0.00001", "fetched_at": NOW_TS}, + ) + broker = _InstrumentBroker() + + executor._price_increment_for(broker, repo, "PAXG-USD", NOW_TS) + executor._price_increment_for(broker, repo, "PAXG-USD", NOW_TS) + + assert broker.calls == 1 + + +def test_a_venue_that_reports_no_tick_is_not_refetched_every_cycle(repo): # noqa: F811 + """The trap in the obvious fix. + + "Refetch when the key is missing" would refetch FOREVER for a product whose venue genuinely + reports no tick, adding a venue round-trip to every order -- the latency the per-product read + exists to avoid. The key is therefore always written, explicitly null, so "absent" means + "written before the field" and nothing else. + """ + broker = _InstrumentBroker(quote_increment=None) + + assert executor._price_increment_for(broker, repo, "X-USD", NOW_TS) is None + assert executor._price_increment_for(broker, repo, "X-USD", NOW_TS) is None + + assert broker.calls == 1, "a venue's honest 'no tick' must be cached, not re-asked" + record = repo.get_state(f"{executor.BASE_INCREMENT_PREFIX}X-USD") + assert "quote_increment" in record, "the key must be written even when the value is unknown" + assert record["quote_increment"] is None