From fba001601058451f531febc1e4817c406dcc19da Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 16 Aug 2026 09:51:39 -0400 Subject: [PATCH 1/2] fix(data): page `market_feed` under the venue's candle-per-request cap `poll_once` asked for every bar between the last stored candle and now in ONE request. Coinbase rejects a span over ~350 intervals with a 400 `INVALID_ARGUMENT: "number of candles requested should be less than 350"`, and because the error propagates out of `poll_once` it does not degrade that one product -- it takes down the whole agent cycle, for every product. `history.py` has paged under this cap since it was written (`MAX_CANDLES_PER_REQUEST = 300`), which is why `keel fetch` never hit it and the agent loop did. Two modules fetch candles; only one knew about the limit. Observed in production. ZEC-USD hourly sat 570 bars stale in `keel.db`, and ZEC is on the paperforward allowlist, so paperforward died on its first poll for two consecutive days -- 10 failures on 2026-08-14, 12 on 2026-08-15. The other 18 allowlisted products were one bar behind and perfectly healthy; one stale series was enough to stop all of them. `backfill` had the same unbounded shape by a different route: `_missing_ranges` groups missing timestamps into CONTIGUOUS ranges, and a contiguous range is itself arbitrarily long. Both call sites now go through `_capped_ranges`. A span that already fits still costs exactly one request, so the ordinary one-bar-behind poll is unchanged -- pinned by `test_poll_once_still_uses_one_call_for_a_small_gap`. The existing fakes could not have caught this: they served any span asked for, so they were strictly more permissive than the venue. The new tests use a `VenueCappedFakeClient` that raises the real 400 above 350 intervals. Verified against the live venue too, replaying the actual 570-bar ZEC gap on a copy of the production DB: 569 candles written across 2 requests of 300 and 269, where `main` issues a single 570 and fails. 2727 -> 2730 tests. Co-authored-by: Claude Opus 5 (1M context) --- keel/data/market_feed.py | 68 +++++++++++++++++++------- tests/data/test_market_feed.py | 88 ++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 17 deletions(-) diff --git a/keel/data/market_feed.py b/keel/data/market_feed.py index c6476083..eabe24bc 100644 --- a/keel/data/market_feed.py +++ b/keel/data/market_feed.py @@ -14,6 +14,7 @@ import time from typing import TYPE_CHECKING +from keel.data.history import MAX_CANDLES_PER_REQUEST from keel.types import Candle, Granularity if TYPE_CHECKING: @@ -50,6 +51,33 @@ def _align_up(ts: int, gran_sec: int) -> int: return ((ts + gran_sec - 1) // gran_sec) * gran_sec +def _capped_ranges(start: int, end: int, gran_sec: int) -> list[tuple[int, int]]: + """Split the inclusive `[start, end]` span into requests of at most the venue's cap. + + Coinbase rejects any candles request spanning more than ~350 intervals with a 400 + `INVALID_ARGUMENT`. `history.py` has always paged under that cap; this module did not, + so a single unbounded request was issued no matter how far behind a series had fallen. + That is fine until it isn't: a product only has to go `MAX_CANDLES_PER_REQUEST` bars + stale for the request to 400, and because the exception propagates out of `poll_once` + it takes the whole agent cycle down with it -- for *every* product, not just the stale + one. Observed in production 2026-08-14/15, when ZEC-USD hourly sat 570 bars stale and + stopped the paperforward agent on two consecutive days. + + Returns one `(start, end)` pair when the span already fits, so the ordinary + one-bar-behind poll still costs exactly one request. + """ + if end < start: + return [] + step = MAX_CANDLES_PER_REQUEST * gran_sec + ranges: list[tuple[int, int]] = [] + window_start = start + while window_start <= end: + window_end = min(end, window_start + step - gran_sec) + ranges.append((window_start, window_end)) + window_start = window_end + gran_sec + return ranges + + def _missing_ranges(expected: list[int], present: set[int], gran_sec: int) -> list[tuple[int, int]]: """Group the `expected` ts values not in `present` into contiguous `(start, end)` ranges.""" missing = [ts for ts in expected if ts not in present] @@ -103,15 +131,17 @@ def backfill( c.ts for c in repo.get_candles(product_id, granularity, window_start, latest_closed) } - for range_start, range_end in _missing_ranges(expected, existing, gran_sec): - fetched = client.get_candles(product_id, granularity, range_start, range_end) - gap_candles = [ - c - for c in fetched - if window_start <= c.ts <= latest_closed and c.ts not in existing - ] - if gap_candles: - total_written += repo.upsert_candles(product_id, granularity, gap_candles) + for gap_start, gap_end in _missing_ranges(expected, existing, gran_sec): + # A contiguous gap is itself unbounded, so page it under the venue's cap. + for range_start, range_end in _capped_ranges(gap_start, gap_end, gran_sec): + fetched = client.get_candles(product_id, granularity, range_start, range_end) + gap_candles = [ + c + for c in fetched + if window_start <= c.ts <= latest_closed and c.ts not in existing + ] + if gap_candles: + total_written += repo.upsert_candles(product_id, granularity, gap_candles) return total_written @@ -145,14 +175,18 @@ def poll_once( continue fetch_start = last_ts + gran_sec if last_ts is not None else latest_closed - fetched = client.get_candles(product_id, granularity, fetch_start, latest_closed) - new_candles: list[Candle] = [ - c - for c in fetched - if c.ts <= latest_closed and (last_ts is None or c.ts > last_ts) - ] - if new_candles: - total_written += repo.upsert_candles(product_id, granularity, new_candles) + # Page under the venue's per-request candle cap. A series that has fallen far + # enough behind would otherwise be one oversized request that 400s and, because + # the error propagates, kills the cycle for every other product too. + for range_start, range_end in _capped_ranges(fetch_start, latest_closed, gran_sec): + fetched = client.get_candles(product_id, granularity, range_start, range_end) + new_candles: list[Candle] = [ + c + for c in fetched + if c.ts <= latest_closed and (last_ts is None or c.ts > last_ts) + ] + if new_candles: + total_written += repo.upsert_candles(product_id, granularity, new_candles) return total_written diff --git a/tests/data/test_market_feed.py b/tests/data/test_market_feed.py index 12f69e56..2a4e9171 100644 --- a/tests/data/test_market_feed.py +++ b/tests/data/test_market_feed.py @@ -203,3 +203,91 @@ def test_is_fresh_false_when_no_candles_stored(repo): assert not is_fresh( repo, "BTC-USD", Granularity.ONE_HOUR, now_ts=NOW, max_age_sec=200 ) + + +# -- venue candle-per-request cap --------------------------------------------- +# +# Coinbase rejects any candles request spanning more than ~350 intervals with a 400 +# `INVALID_ARGUMENT: "number of candles requested should be less than 350"`. `history.py` +# has always paged under that cap; `market_feed` did not, so a product whose series had +# fallen far enough behind made every agent cycle die on the first poll. Observed in +# production 2026-08-14/15: ZEC-USD hourly sat 570 bars stale and took the paperforward +# agent down for two consecutive days. + + +class VenueCappedFakeClient(FakeClient): + """`FakeClient` that fails like the real venue when asked for too many candles. + + A fake that silently serves any span cannot catch this bug -- the old code passed every + existing test precisely because the fakes were more permissive than Coinbase. + """ + + def get_candles( + self, product_id: str, granularity: Granularity, start: int, end: int + ) -> list[Candle]: + span = (end - start) // GRAN_SEC + 1 + if span > 350: + raise RuntimeError( + "400 Client Error: Bad Request " + '{"error":"INVALID_ARGUMENT","error_details":"start and end argument is ' + 'invalid - number of candles requested should be less than 350 "}' + ) + return super().get_candles(product_id, granularity, start, end) + + +def _stale_series(hours_behind: int) -> tuple[dict, int]: + """A full hourly series ending at LATEST_CLOSED, plus the ts of the last *stored* bar.""" + oldest = LATEST_CLOSED - hours_behind * GRAN_SEC + ts_values = list(range(oldest, LATEST_CLOSED + 1, GRAN_SEC)) + series = {("ZEC-USD", Granularity.ONE_HOUR): [_candle(ts) for ts in ts_values]} + return series, oldest + + +def test_poll_once_chunks_requests_under_the_venue_candle_cap(repo): + """The production failure: 570 hourly bars behind is one 570-candle request, and 400s.""" + series, oldest = _stale_series(570) + repo.upsert_candles("ZEC-USD", Granularity.ONE_HOUR, [_candle(oldest)]) + client = VenueCappedFakeClient(series) + + written = poll_once(client, repo, ["ZEC-USD"], [Granularity.ONE_HOUR], now_ts=NOW) + + assert written == 570, "every missing bar should be fetched, across as many calls as it takes" + assert len(client.calls) > 1, "a 570-bar gap cannot be served by a single request" + for _product, _gran, start, end in client.calls: + span = (end - start) // GRAN_SEC + 1 + assert span <= 350, f"requested {span} candles in one call -- the venue rejects >350" + + +def test_poll_once_still_uses_one_call_for_a_small_gap(repo): + """Chunking must not add requests for the ordinary case -- one bar behind is one call.""" + series, oldest = _stale_series(1) + repo.upsert_candles("ZEC-USD", Granularity.ONE_HOUR, [_candle(oldest)]) + client = VenueCappedFakeClient(series) + + written = poll_once(client, repo, ["ZEC-USD"], [Granularity.ONE_HOUR], now_ts=NOW) + + assert written == 1 + assert len(client.calls) == 1 + + +def test_backfill_chunks_requests_under_the_venue_candle_cap(repo): + """`backfill` groups missing bars into contiguous ranges, which are likewise unbounded.""" + hours = 400 + oldest = LATEST_CLOSED - hours * GRAN_SEC + ts_values = list(range(oldest, LATEST_CLOSED + 1, GRAN_SEC)) + series = {("ZEC-USD", Granularity.ONE_HOUR): [_candle(ts) for ts in ts_values]} + client = VenueCappedFakeClient(series) + + backfill( + client, + repo, + ["ZEC-USD"], + [Granularity.ONE_HOUR], + history_days=hours // 24 + 1, + now_ts=NOW, + ) + + assert client.calls, "backfill should have requested something" + for _product, _gran, start, end in client.calls: + span = (end - start) // GRAN_SEC + 1 + assert span <= 350, f"requested {span} candles in one call -- the venue rejects >350" From b2db66740f3b0387515508f5cf10b4be49845108 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 16 Aug 2026 09:51:59 -0400 Subject: [PATCH 2/2] fix(compliance): put a margin between the discovery pre-filter and the gate The pre-filter was pinned NUMERICALLY EQUAL to `ScreenPolicy.min_median_daily_volume`, with the stated intent that equality kept discovery from being stricter than the criterion it screens for. The intent is right. Equality does not achieve it. The two are different statistics: the pre-filter reads a ONE-DAY venue snapshot of 24h quote volume, while the gate medians `volume * close` over YEARS of cached history. Same units, so they look comparable; wildly different distributions, so an equal threshold is crossed constantly in both directions by ordinary day-to-day variation. Roughly half those crossings hide an asset the gate would have admitted, and nothing anywhere reports the exclusion. Measured, not argued. On 2026-08-15 one quiet day put five already-attested assets under the equal floor while their true medians ran 3.08x-6.32x OVER it. That looked like bad luck. On 2026-08-16 a sweep run at a lowered floor surfaced three assets that had never appeared in fifteen prior discovery runs and are admissible on the gate's own statistic: asset 24h snapshot vs floor real gate statistic vs floor FIL 413,040 0.41x 3,483,442 3.48x OP 356,063 0.36x 2,585,626 2.59x JASMY 301,861 0.30x 4,161,283 4.16x Not considered and rejected -- never seen. The filter that exists only to bound the request count was silently deciding the candidate universe. So the invariant becomes a MARGIN rather than an equality, expressed as `DISCOVERY_FLOOR_MARGIN = 4` and derived from the admission floor instead of restated, so the two cannot drift and the relationship is what the tests pin. 4 leaves ~3x headroom below the lowest ratio yet observed on an admissible asset (JASMY's 0.30x). The cost is bounded: on the 2026-08-16 sweep it took the candidate list from 35 to 82 of 920 venue products, still comfortably the "cut ~900 down to a shortlist" job the filter is for. `test_the_discovery_floor_matches_the_admission_liquidity_floor` asserted the equality and so encoded the bug; it is rewritten to assert the margin, and to pin the pre-filter below that observed 0.30x ratio. Also fixes a pin that had already gone stale while looking authoritative: `test_build_discover_report_applies_default_volume_floor_matching_assets_discover` restated the floor as a literal and its docstring still claimed `5000000` long after the default became `1000000`. It now compares against the shared constant. Co-authored-by: Claude Opus 5 (1M context) --- keel/cli.py | 9 ++++--- keel/commands/admission.py | 25 ++++++++++-------- keel/compliance/screen.py | 29 +++++++++++++++++++- tests/commands/test_admission.py | 45 +++++++++++++++++++++++--------- 4 files changed, 80 insertions(+), 28 deletions(-) diff --git a/keel/cli.py b/keel/cli.py index d1575c7d..329dfeb5 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -763,11 +763,14 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N @assets_group.command("discover") @click.option("--quote", default=None, help="Settlement currency (default: config.quote_currency).") @click.option( - "--min-volume-24h", default="1000000", show_default=True, + "--min-volume-24h", + default=str(screen_mod.DiscoveryPolicy().min_quote_24h_volume), + show_default=True, help="Cheap pre-filter on the venue's reported 24h quote volume. Bounds the request count; it " "is NOT a liquidity criterion -- a 24h snapshot is a different statistic from the median the " - "gate applies, so use --probe-liquidity for that. Set equal to the admission floor so " - "discovery cannot be stricter than the criterion it screens for.", + "gate applies, so use --probe-liquidity for that. Deliberately set WELL BELOW the admission " + "floor (1/4 of it), not equal to it: the two measure different things, so an equal threshold " + "still hides assets the gate would admit -- on 2026-08-16 it was hiding three of them.", ) @click.option("--limit", default=25, show_default=True, help="Show at most this many candidates.") @click.option( diff --git a/keel/commands/admission.py b/keel/commands/admission.py index 950a7b76..9dbe0b26 100644 --- a/keel/commands/admission.py +++ b/keel/commands/admission.py @@ -61,23 +61,26 @@ DEFAULT_PROPOSALS_DIR = "~/keel/proposals" #: Mirrors `keel assets discover --min-volume-24h`'s own default (`keel/cli.py::assets_discover`). -#: A literal here, not an import from `keel.cli`, for the same reason `ScreenFn` is injected -#: rather than importing `_screen_product` directly: importing `keel.cli` from this module would -#: create the cycle `cli -> tui -> admission -> cli`. `test_build_discover_report_applies_ -#: default_volume_floor_matching_assets_discover` reads the CLI option's own default and asserts -#: it equals this constant, so the two cannot silently drift apart. -#: Discovery's 24h-volume pre-filter, pinned EQUAL to `ScreenPolicy.min_median_daily_volume` so a -#: sweep can never be stricter than the gate it feeds. It bounds how many products get probed; it -#: is not a liquidity verdict (that is `--probe-liquidity`, which computes the gate's own median). +#: Derived from `DiscoveryPolicy` rather than restated as a literal, so there is one definition of +#: the pre-filter floor and its relationship to the admission floor. Importing `keel.cli` here +#: would create the cycle `cli -> tui -> admission -> cli`; importing `keel.compliance.screen` +#: does not, and this module already depends on it. +#: It bounds how many products get probed; it is NOT a liquidity verdict (that is +#: `--probe-liquidity`, which computes the gate's own median). #: #: Was 5,000,000 until 2026-08-08. At that floor the sweep returned 9 candidates and exactly one #: unsettled survivor; at a lower floor, seven more cleared BOTH mechanical gates -- FET among them #: at $2.94M/24h, i.e. invisible to the sweep while measuring 4.8x the admission floor. The floor, #: not the market, was the binding constraint on the candidate pipeline. #: -#: `tests/commands/test_admission.py` pins this to the CLI option, to `DiscoveryPolicy`'s default -#: and to the admission floor; all four move together or the suite fails. -DEFAULT_MIN_QUOTE_24H_VOLUME = Decimal("1000000") +#: Was then pinned EQUAL to `ScreenPolicy.min_median_daily_volume` until 2026-08-16, on the theory +#: that equality kept the sweep from being stricter than the gate. It did not -- the two are +#: different statistics, and the equal floor was still hiding admissible assets. See +#: `DISCOVERY_FLOOR_MARGIN` for the measurements that changed it. +#: +#: `tests/commands/test_admission.py` pins this to the CLI option and to `DiscoveryPolicy`'s +#: default, and pins the MARGIN against the admission floor; all move together or the suite fails. +DEFAULT_MIN_QUOTE_24H_VOLUME = DiscoveryPolicy().min_quote_24h_volume # -- 2a. shortlist location (offline) ------------------------------------------------------------ diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index b974c70f..52bc3bec 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -462,6 +462,29 @@ class Candidate: quote_24h_volume: Decimal +#: How far BELOW the admission floor the discovery pre-filter sits. +#: +#: This is a RATIO on purpose, and it used to be 1 (the two were pinned numerically equal) with +#: the stated intent that "discovery cannot be stricter than the criterion it screens for". That +#: intent is right; equality does not achieve it. The pre-filter reads a ONE-DAY venue snapshot +#: while the gate medians `volume * close` over YEARS of history -- same units, different +#: statistics -- so an equal threshold is crossed constantly in both directions by ordinary +#: day-to-day variation, and roughly half those crossings hide an asset the gate would admit. +#: +#: Measured, not assumed. On 2026-08-15 a quiet day put five already-attested assets under the +#: equal floor while their true medians ran 3.08x-6.32x OVER it. On 2026-08-16 a sweep at a +#: lowered floor surfaced three admissible assets that had NEVER been seen in fifteen prior +#: discovery runs -- FIL, OP and JASMY, whose 24h snapshots sat at 0.41x, 0.36x and 0.30x the +#: admission floor while their gate statistics measured 3.48x, 2.59x and 4.16x OVER it. +#: +#: 4 gives ~3x headroom below the lowest ratio actually observed on an admissible asset (0.30x). +#: The cost is bounded and small: on the 2026-08-16 sweep it took the candidate list from 35 to +#: 82 out of 920 venue products, which is still the "cut ~900 to a shortlist" job this filter +#: exists to do. Raise the ratio if admissible assets are still being hidden; lower it only with +#: evidence that probe volume has become the binding constraint. +DISCOVERY_FLOOR_MARGIN = 4 + + @dataclass(frozen=True) class DiscoveryPolicy: """The cheap pre-filter, run on venue metadata BEFORE fetching any history. @@ -471,7 +494,11 @@ class DiscoveryPolicy: """ quote_currency: str = "USD" - min_quote_24h_volume: Decimal = Decimal("1000000") + #: Derived from the admission floor rather than restated, so the two cannot drift and the + #: SAFETY MARGIN between them is the invariant -- see `DISCOVERY_FLOOR_MARGIN`. + min_quote_24h_volume: Decimal = ( + ScreenPolicy().min_median_daily_volume / DISCOVERY_FLOOR_MARGIN + ) def median_daily_quote_volume(candles: Sequence[Any]) -> Decimal: diff --git a/tests/commands/test_admission.py b/tests/commands/test_admission.py index 792add02..c078bf33 100644 --- a/tests/commands/test_admission.py +++ b/tests/commands/test_admission.py @@ -646,18 +646,22 @@ def test_build_discover_report_excludes_allowlist_assets(repo: Repository): def test_build_discover_report_applies_default_volume_floor_matching_assets_discover(repo): - """`keel assets discover --min-volume-24h` defaults to `5000000` -- read straight from the - CLI option's own default (by name, not position, so a decorator reorder cannot silently - break this pin) so the two can never drift apart silently.""" + """`build_discover_report` and `keel assets discover --min-volume-24h` share one floor. + + The CLI option's default is read by name (not position, so a decorator reorder cannot + silently break this pin) and compared against the shared constant rather than a restated + literal -- a literal here went stale once already (this docstring claimed `5000000` long + after the default became `1000000`) and pinned nothing while looking like it did. + """ cli_option = next( p for p in cli_module.assets_discover.params if p.name == "min_volume_24h" ) default_floor = Decimal(cli_option.default) - assert default_floor == Decimal("1000000") + assert default_floor == DEFAULT_MIN_QUOTE_24H_VOLUME config = _config(allowlist=[]) - below = _venue_product("DOGE", volume="999999") - above = _venue_product("SHIB", volume="1000001") + below = _venue_product("DOGE", volume=str(default_floor - 1)) + above = _venue_product("SHIB", volume=str(default_floor + 1)) report = build_discover_report([below, above], config) @@ -732,14 +736,29 @@ def test_every_discovery_floor_default_agrees(): assert DiscoveryPolicy().min_quote_24h_volume == DEFAULT_MIN_QUOTE_24H_VOLUME -def test_the_discovery_floor_matches_the_admission_liquidity_floor(): +def test_the_discovery_floor_sits_safely_below_the_admission_liquidity_floor(): """Discovery should not hide assets the gate would admit. - The sweep's floor is a 24h snapshot and the gate's is a median over history -- different - statistics (which is why `--probe-liquidity` exists). Pinning the two NUMBERS equal keeps the - pre-filter from being stricter than the criterion it screens for: at the old 5x-higher floor, - FET sat at $2.94M/24h and never appeared, while measuring 4.8x the admission floor. + This test used to assert the two floors were EQUAL, on the reasoning that equality kept the + pre-filter from being stricter than the criterion it screens for. The goal was right and the + mechanism was wrong: the sweep's floor is a ONE-DAY snapshot and the gate's is a median over + YEARS (which is why `--probe-liquidity` exists), so equal numbers are crossed constantly by + ordinary variation -- and about half those crossings hide an admissible asset. + + It was hiding real ones. On 2026-08-16, FIL / OP / JASMY sat at 0.41x / 0.36x / 0.30x the + admission floor on 24h volume while their actual gate statistics measured 3.48x / 2.59x / + 4.16x OVER it; none had appeared in fifteen prior discovery runs. So the invariant is now a + MARGIN, not an equality. """ - from keel.compliance.screen import ScreenPolicy + from keel.compliance.screen import DISCOVERY_FLOOR_MARGIN, ScreenPolicy + + admission_floor = ScreenPolicy().min_median_daily_volume + + assert DEFAULT_MIN_QUOTE_24H_VOLUME < admission_floor, ( + "an equal-or-higher pre-filter hides assets the gate would admit" + ) + assert DEFAULT_MIN_QUOTE_24H_VOLUME == admission_floor / DISCOVERY_FLOOR_MARGIN - assert DEFAULT_MIN_QUOTE_24H_VOLUME == ScreenPolicy().min_median_daily_volume + # The lowest 24h/floor ratio observed on an asset that actually cleared the gate was JASMY's + # 0.30x. The pre-filter must stay below that, or the same class of asset is hidden again. + assert DEFAULT_MIN_QUOTE_24H_VOLUME < admission_floor * Decimal("0.30")