Skip to content

Commit 9542977

Browse files
joocerclaude
andcommitted
Read aggregates in one page, and refuse to page them
Paged $apply results from this feed are silently wrong. The row count is right - it matches the equivalent SQL exactly, 86184 for ssh successes and 91453 for the (ip,response_status) grouping - but the rows are not: some are duplicated and others dropped, differently on every run. $top=100000 (1 page) 86184 rows, 86184 distinct ip, 0 duplicated $top=100000 (again) 86184 rows, 86184 distinct ip - identical $top=25000 (4 pages) 86184 rows, 61348 distinct ip, 24836 duplicated $top=25000 (again) 86184 rows, 57882 distinct ip, 28302 duplicated $top=10000 (9 pages) 86184 rows, 58991 distinct ip SQL over the same window agrees with the single-page read, so the engine, filter and aggregate are all correct; each page appears to be a fresh execution of an unordered query, so $skip lands somewhere different every time. This had already reached production and left no trace: the derived lists carried 130408 lines for 80048 real http hosts and 123701 for 90221 https, so refresh was re-grabbing duplicates inside a single run while tens of thousands of known hosts were missing from both the refresh queue and the discovery exclusion. $top now sits at the server's ceiling, where every query this project issues fits in one page, and grouped_max/distinct_values raise rather than follow a nextLink. The failure mode to design against is a plausible answer, not an error - responsive.py keeps the previous list when a read raises and cannot tell a corrupt answer from a good one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 6957146 commit 9542977

2 files changed

Lines changed: 89 additions & 9 deletions

File tree

src/ichnos/odata.py

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,32 @@
4646
DEFAULT_ODATA_PREFIX = "/api/v4"
4747
DEFAULT_TOKEN_URL = "https://authenticate.opteryx.app/token"
4848

49-
MAX_TOP = 25000
50-
"""The service's documented ceiling for a single request's `$top`; a larger value is
51-
rejected with a 400. Set deliberately high rather than left to the default of 100 -
52-
this module's reads are bulk reads, and the default would turn one page into 250."""
49+
MAX_TOP = 100000
50+
"""The service's ceiling for a single request's `$top` - 100001 is rejected with
51+
"$top must be between 0 and 100000". Set to exactly the ceiling, and that is not just
52+
about efficiency: a read that fits in one page is the only read this feed answers
53+
correctly.
54+
55+
Paginated `$apply` results are unstable. The row *count* is right and matches the
56+
equivalent SQL exactly, but the contents are not: rows are duplicated and others
57+
dropped, differently on every run. Measured against ssh observations, one query,
58+
same data, minutes apart:
59+
60+
$top=100000 (1 page) 86184 rows, 86184 distinct ip, 0 duplicated
61+
$top=100000 (again) 86184 rows, 86184 distinct ip, 0 duplicated - identical
62+
$top=25000 (4 pages) 86184 rows, 61348 distinct ip, 24836 duplicated
63+
$top=25000 (again) 86184 rows, 57882 distinct ip, 28302 duplicated
64+
$top=10000 (9 pages) 86184 rows, 58991 distinct ip
65+
66+
SQL over the same window returns 86184 rows and 86184 distinct ip, so the engine, the
67+
filter and the aggregate are all correct - each page appears to be a fresh execution of
68+
an unordered query, so `$skip` lands somewhere different every time.
69+
70+
The damage was silent. Nothing errors, the row count looks right, and the derived
71+
known-responsive lists simply carried duplicates and were missing hosts: 130408 lines
72+
for 80048 real http hosts, 123701 for 90221 https. Hence `_reject_paged_result` below -
73+
at 100000 nothing this project reads pages today, and the day something does, it must
74+
fail loudly rather than quietly produce a plausible wrong answer."""
5375

5476

5577
class ODataError(Exception):
@@ -130,10 +152,15 @@ def iter_rows(
130152
base_url: str = DEFAULT_ODATA_BASE,
131153
prefix: str = DEFAULT_ODATA_PREFIX,
132154
get: Optional[Callable[..., Any]] = None,
155+
single_page: bool = False,
133156
) -> Iterator[Dict[str, Any]]:
134157
"""Yield every row of a query, following `@odata.nextLink` until the feed stops
135158
offering one.
136159
160+
`single_page=True` refuses to follow the link at all and raises instead, for callers
161+
whose answer would be silently wrong if it did - see MAX_TOP for the measurements.
162+
Paging this feed returns the right number of rows with the wrong rows in them.
163+
137164
`path` is the three-part `{workspace}/{collection}/{dataset}` address - the same
138165
triple the Upload API's `Target` uses. `query` is a pre-encoded query string; it is
139166
not built from a dict on purpose, see the module docstring.
@@ -157,6 +184,13 @@ def iter_rows(
157184
next_link = payload.get("@odata.nextLink")
158185
if not next_link:
159186
break
187+
if single_page:
188+
raise ODataError(
189+
f"{path}: result needs more than one page at $top={MAX_TOP}, and paged "
190+
"reads from this feed are not trustworthy - the row count is right but "
191+
"rows are duplicated and dropped non-deterministically (see MAX_TOP). "
192+
"Narrow the query or fix the feed; do not page it."
193+
)
160194
# Relative, and already percent-encoded - concatenate, never re-encode.
161195
url = next_link if next_link.startswith("http") else f"{base_url}{next_link}"
162196

@@ -198,7 +232,8 @@ def grouped_max(
198232
query = f"$apply={quote(apply_expr, safe='()/,')}&$top={top}"
199233
return [
200234
row
201-
for row in iter_rows(path, query, token=token, base_url=base_url, prefix=prefix, get=get)
235+
for row in iter_rows(path, query, token=token, base_url=base_url, prefix=prefix,
236+
get=get, single_page=True)
202237
if all(row.get(c) is not None for c in columns)
203238
]
204239

@@ -230,7 +265,8 @@ def distinct_values(
230265

231266
values = []
232267
for row in iter_rows(
233-
path, query, token=token, base_url=base_url, prefix=prefix, get=get
268+
path, query, token=token, base_url=base_url, prefix=prefix, get=get,
269+
single_page=True,
234270
):
235271
value = row.get(column)
236272
if value is not None:

tests/test_responsive.py

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
from ichnos.odata import ODataError
77
from ichnos.odata import distinct_values
88
from ichnos.odata import fetch_access_token
9+
from ichnos.odata import grouped_max
10+
from ichnos.odata import iter_rows
911
from ichnos.responsive import fetch_responsive_hosts
1012
from ichnos.responsive import read_responsive_file
1113
from ichnos.responsive import read_responsive_hosts
@@ -49,7 +51,7 @@ def test_distinct_values_composes_the_filter_inside_apply():
4951
)
5052
assert "%24apply" not in calls[0] # we build it, we don't re-encode it
5153
assert "$apply=filter(protocol%20eq%20%27http%27)/groupby((ip))" in calls[0]
52-
assert "$top=25000" in calls[0]
54+
assert "$top=100000" in calls[0]
5355

5456

5557
def test_iter_rows_follows_nextlink_without_re_encoding_it():
@@ -60,7 +62,8 @@ def test_iter_rows_follows_nextlink_without_re_encoding_it():
6062
{"value": [{"ip": "1.1.1.1"}], "@odata.nextLink": "/api/v4/ws/coll/observations?%24skip=1"},
6163
{"value": [{"ip": "2.2.2.2"}]},
6264
]
63-
ips = distinct_values("ws/coll/observations", "ip", get=_fake_get(pages, calls))
65+
ips = [r["ip"] for r in iter_rows("ws/coll/observations", "$top=1",
66+
get=_fake_get(pages, calls))]
6467
assert ips == ["1.1.1.1", "2.2.2.2"]
6568
assert calls[1] == "https://odata.opteryx.app/api/v4/ws/coll/observations?%24skip=1"
6669

@@ -78,7 +81,7 @@ def get(url, headers=None, timeout=None):
7881
return FakeResponse(pages[0])
7982

8083
with pytest.raises(ODataError) as exc:
81-
distinct_values("ws/coll/observations", "ip", get=get)
84+
list(iter_rows("ws/coll/observations", "$top=1", get=get))
8285
assert "500" in str(exc.value)
8386

8487

@@ -254,3 +257,44 @@ def test_a_host_that_only_ever_failed_is_not_a_refresh_target():
254257
)
255258

256259
assert [ip for ip, _ in hosts] == ["real"]
260+
261+
262+
def test_an_aggregate_read_refuses_to_paginate():
263+
"""Paged `$apply` results from this feed are silently wrong. The row count is right
264+
and matches the equivalent SQL exactly, but the contents are not - rows are
265+
duplicated and others dropped, differently on every run. Measured against ssh
266+
observations, same query, same data, minutes apart:
267+
268+
$top=100000 (1 page) 86184 rows, 86184 distinct ip, 0 duplicated
269+
$top=100000 (again) 86184 rows, 86184 distinct ip - byte-identical
270+
$top=25000 (4 pages) 86184 rows, 61348 distinct ip, 24836 duplicated
271+
$top=25000 (again) 86184 rows, 57882 distinct ip, 28302 duplicated
272+
273+
So the failure mode to design against is not an error, it is a plausible answer -
274+
and it had already reached production, where the derived lists carried 130408 lines
275+
for 80048 real http hosts. Nothing this project reads needs a second page at
276+
$top=100000, and the day something does it must stop: responsive.py keeps the
277+
previous list when a read raises, and has no way to tell a corrupt answer from a
278+
good one."""
279+
paged = [
280+
{"value": [{"ip": "203.0.113.1", "last_at": "2026-08-01T00:00:00Z"}],
281+
"@odata.nextLink": "/api/v4/ws/coll/observations?%24skip=1"},
282+
{"value": [{"ip": "203.0.113.2", "last_at": "2026-08-02T00:00:00Z"}]},
283+
]
284+
285+
with pytest.raises(ODataError, match="more than one page"):
286+
grouped_max("ws/coll/observations", "ip", "observed_at", "last_at",
287+
get=_fake_get(paged))
288+
289+
290+
def test_a_single_page_aggregate_read_is_returned_normally():
291+
"""The guard must not fire on the normal case - every real query fits today."""
292+
rows = grouped_max(
293+
"ws/coll/observations", ("ip", "response_status"), "observed_at", "last_at",
294+
get=_fake_get([{"value": [
295+
{"ip": "203.0.113.1", "response_status": "success",
296+
"last_at": "2026-08-01T00:00:00Z"},
297+
]}]),
298+
)
299+
assert rows == [{"ip": "203.0.113.1", "response_status": "success",
300+
"last_at": "2026-08-01T00:00:00Z"}]

0 commit comments

Comments
 (0)