[vinzee]

Summary

Problem

/api/v2/search pages with LIMIT/OFFSET, so each page costs more than the last and offset stops at 10,000.

Solution

Adds an optional cursor request field and a nextCursor response field. offset callers are unchanged.

Cursor paging uses about 30% less ClickHouse CPU than offset paging on real pipedream-logs queries, with identical results. For queries sorted by timestamp, OFFSET stays small however many pages you fetch, and the 10,000-row cap no longer applies.

Paging through one hour of logs, 1,000 rows per page:

// 1. First page: same body as an offset request, plus "cursor": "start"
POST /api/v2/search
{ "sourceId": "65f...", "startTime": "2026-10-25T10:00:00Z", "endTime": "2026-10-25T11:00:00Z",
  "where": "ServiceName:checkout", "maxResults": 1000, "cursor": "start" }
-> { "data": [ /* 1000 rows, newest first */ ], "rows": 1000, "nextCursor": "eyJ2Ijox...Q3a9" }

// 2. Next page: same body, cursor replaced with the returned nextCursor
{ ..., "cursor": "eyJ2Ijox...Q3a9" }
-> { "data": [ /* 212 rows */ ], "rows": 212, "nextCursor": "eyJ2Ijox...b7Kc" }  // short page, not the end

// 3. Keep going until nextCursor is null
{ ..., "cursor": "eyJ2Ijox...b7Kc" }
-> { "data": [ /* 640 rows */ ], "rows": 640, "nextCursor": null }               // done

• Send "start", then pass back each nextCursor until it is null. An empty or short page does not mean the end.
• When cursor is set, offset is ignored. The cursor is opaque.
• Rows sharing a timestamp can be skipped or repeated unless orderBy breaks the tie, as with offset.
• Errors are 400s: a malformed or tampered cursor, or a cursor reused with a different query or range.

How it works

• The range is read in time windows (15m, 6h, 6h, 12h, then 24h). Each page reads one window; boundaries are whole seconds, so second-precision columns like TimestampTime don't lose rows.

• Within a window, each full page shrinks the window to the last row's second (its end for DESC, its start for ASC), so OFFSET only skips that second's rows and page cost stays flat in dense ranges.

• The cursor is base64url(payload).base64url(HMAC), with payload:

  Field	Meaning
  v	Cursor format version
  k	Window number: which time window the next page reads from
  ws, we	Current window's bounds; the window shrinks after each full page
  o	Rows to skip: those already returned from the last row's second
  f	Fingerprint of sourceId, where, whereLanguage, select, orderBy; a mismatch is a 400
  s, e	The query's time range, fixed on the first page so a defaulted endTime doesn't move

• Each page computes the next window from k, ws/we and s/e alone: its near edge is the current window's far edge, and its far edge is the range edge offset by the total length of the first k + 1 windows. Nothing is built up front, so the range has no cap.

• Cursors are HMAC-signed with EXPRESS_SESSION_SECRET, so they can't be edited; replicas need a shared value.

• Queries sorted first by an exact timestamp, or by toStartOfSecond/Minute/FiveMinutes/FifteenMinutes(...), use windows aligned to that key. Others (hour/day keys, non-time keys, DISTINCT selects) read the whole range with plain offsets and keep the 10,000 cap.

Testing

• Unit and integration tests (search.int): every row returned exactly once across window boundaries, dense and burst seconds, compound timestamp expressions, both directions, tampered cursors.
• Against dev ClickHouse, compared with pipedream-logs' offset paging: identical rows (up to 130k), and 29–41% less ClickHouse CPU.

Screenshots or video

n/a

How to test on Vercel preview

n/a

References

• Linear Issue: Partial Results in Paginated Search Invokes Repetitive Search #1403
• Related PRs: fix: v2 search drops rows after a sub-second startTime; make maxResults cap configurable #3381

[changeset-bot]

🦋 Changeset detected

Latest commit: a9e9025

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/api	Minor
@hyperdx/common-utils	Patch
@hyperdx/app	Minor
@hyperdx/otel-collector	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor

1 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor

• 🔵 packages/api/src/routers/external-api/v2/utils/search.ts:1799 — Cursor walks turn source row/time limits into a bare 500 'Query execution failed' → withThrowingOverflow switches read_overflow_mode, timeout_overflow_mode and the other *_overflow_mode settings to throw. On a source that sets max_rows_to_read or max_execution_time with break, a cursor page over a busy window now fails with TOO_MANY_ROWS or TIMEOUT_EXCEEDED. Neither code is in CH_USER_INPUT_ERRORS (v2/search.ts:43), so the client gets a generic 500 and no way to recover. The PR's own int test asserts that 500. Fix: return a 4xx for these codes when the request is a cursor walk, with the same 'narrow the time range or filter' message CURSOR_WINDOW_TOO_DENSE uses, and document the case in the OpenAPI 400 response.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[greptile-apps]

Confidence Score: 4/5

[Medium impact] The change affects paging behavior for callers of external API v2 search.

Fix the unusable final cursor before merging.

Findings

1.  Completed searches end with errors ▶

Summary

Adds signed cursor pagination to /api/v2/search, while preserving offset-only requests.

• External search pages can continue from a signed cursor.
• Cursor searches split timestamp-sorted ranges into smaller windows.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Search request] --> B{Cursor supplied?}
  B -->|No| C[Query full range with offset]
  B -->|Yes| D[Start walk or verify signed cursor]
  D --> E{Timestamp-led search?}
  E -->|Yes| F[Query aligned time window]
  E -->|No| G[Query full range with capped offset]
  F --> H[Return rows and next cursor]
  G --> H
  C --> I[Return rows without next cursor]

Loading

_{Reviews (16) · Last reviewed commit: "feat(api): window cursor walks led by a ..." · Reviewed by Greptile}

[github-actions]

Deep Review

✅ No critical issues found. The windowing algorithm is sound, and the prior review round's P1 concerns (offset missing older rows, defaulted-time cursor invalidation, rounded-timestamp duplicate rows, dense-window rejectable cursors) are genuinely resolved and covered by tests. The exactly-once guarantee holds: compound timestamp expressions carry an exact part whose strict < bound keeps interior windows half-open even when a rounded leading key forces inclusive bounds, and the integration tests assert row identity (not just counts) across boundaries. One performance/reliability item is worth addressing before merge.

🟡 P2 — recommended

• packages/api/src/routers/external-api/v2/utils/search.ts:234 — On the windowed (cut) path the MAX_SINGLE_WINDOW_OFFSET cap is applied only in the !cut branch, so a single time bucket denser than one page makes cutWindowAtBucket return offset + pageLength with no ceiling, and that growing offset is passed straight to the ClickHouse OFFSET, reintroducing the quadratic deep-paging scan cost the feature set out to remove (most easily triggered on rounded 15-minute leading keys where one bucket spans 15 minutes).
  • Fix: Bound the cumulative in-bucket cursor offset on the cut path and return CURSOR_WINDOW_TOO_DENSE when a single bucket cannot be paged within the cap, so page cost stays flat or fails loudly instead of degrading silently.
  • _{security, performance, previous-comments}

🔵 P3 nitpicks (7)

• packages/api/src/routers/external-api/v2/search.ts:364 — The new cursor 400 conditions (invalid/expired cursor, query mismatch, window-too-dense) are not enumerated in the OpenAPI 400 docs, and the documented 200 example omits the now-always-present nextCursor field.
  • Fix: Enumerate the cursor error conditions in the 400 description and add nextCursor to the 200 example in both the JSDoc and openapi.json.
  • _api-contract
• packages/api/src/routers/external-api/__tests__/search.int.test.ts:1121 — The range-change rejection only exercises the changed-startTime branch; the changed-endTime branch and the withThrowingOverflow custom-*_overflow_mode regex branch are untested.
  • Fix: Add a case replaying a cursor with a changed explicit endTime, and one with a source carrying a custom overflow-mode setting.
  • _testing
• packages/api/src/routers/external-api/v2/utils/search.ts:96 — resolveSearchOrderBy derivation branches (orderByExpression, displayedTimestampValueExpression, multi-part tuple) are untested; a wrong derived ORDER BY silently disengages windowing and falls back to offset paging.
  • Fix: Add coverage asserting the derived order leads with the timestamp so windowing engages for sources that omit request-level orderBy.
  • _testing
• packages/api/src/routers/external-api/v2/utils/search.ts:270 — The chart config literal is both annotated and as-cast to ChartConfigWithDateRange, so the cast overrides the assignability check the annotation would perform (repo guide discourages as).
  • Fix: Construct dateRange explicitly so the shape is inferred or satisfies-checked, and drop the as.
  • _{kieran-typescript, project-standards}
• packages/api/src/routers/external-api/v2/utils/searchWindows.ts:158 — nextSearchWindow/cumulativeWindowMs reimplement the progressive-bucketing scheme already in common-utils/core/searchWindows.ts, so API cursor paging and UI offset paging can silently diverge if the bucketing policy changes in only one place.
  • Fix: Add a cross-reference comment or factor the shared window-size math into one module.
  • _{maintainability}
• packages/common-utils/src/core/utils.ts:7 — Foundational core/utils.ts imports stripTypeWrappers from the feature module core/eventDeltas, a layering inversion that becomes a real cycle the moment eventDeltas needs anything from utils.
  • Fix: Move stripTypeWrappers into core/utils.ts and have eventDeltas import it from there.
  • _{maintainability}
• packages/api/src/routers/external-api/v2/utils/search.ts:1 — The file crosses the repo's 300-line guideline (~355 lines) with the new cursor/window state machine densely interleaved in runSearchConfig.
  • Fix: Extract the page-limit and next-cursor computation into a sibling helper module.
  • _{project-standards, maintainability}

---

Reviewers (9): security, performance, api-contract, reliability, testing, maintainability, kieran-typescript, project-standards, previous-comments.

Testing gaps: No end-to-end test asserts bounded query cost (or a loud error) when a single bucket holds far more matching rows than a page — the dense-bucket behavior is only exercised at the pure-function level (searchWindowCut.test.ts:76), where it is asserted to grow unbounded. No test covers rounded 15-minute leading keys under high density, where the P2 offset growth is most impactful.

Coverage note: The correctness and adversarial reviewers did not return before synthesis; their lanes were partially covered by direct verification of the windowing state machine, the half-open/inclusive-bound interaction, and the dense-bucket path against the integration suite.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[vercel]

@vinzee is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

🟠 major — Server issues cursors over its own offset cap, and the next request rejects them as malformed

offset: effectiveOffset + data.length can go past MAX_CURSOR_OFFSET (10,000). Example: a window with more than 12,000 matching rows and maxResults=2000 produces a cursor at offset 12,000, and decodeSearchCursor (utils/searchCursor.ts) rejects it with a 400 'Cursor is malformed'. A 24h window of logs easily holds that many rows, so the walk dies partway. Either split the window into smaller sub-windows when it fills up, or don't cap offsets the server generated itself (e.g. sign the cursor with an HMAC instead of bounding the offset). Add a test that walks past the cap.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in ca22759: MAX_CURSOR_OFFSET is now 1,000,000, and a walk that would exceed it moves to the next window instead of minting a cursor the decoder rejects.

[github-actions]

🔵 minor — The single-window fallback is written out twice

The non-cursor branch here repeats the fallback already inside resolveSearchWindows (line ~70). Add a forceSingleWindow flag to resolveSearchWindows, or pull the fallback into a shared helper, so the two copies can't drift apart.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Fixed in e9a6515: resolveSearchWindows takes { forceSingleWindow }, so the fallback lives in one place.

[github-actions]

🔵 minor — INVALID_CURSOR was added to SearchErrorCode but runSearchConfig never returns it

Remove INVALID_CURSOR from SearchErrorCode and its codeToStatus case. The router answers decode errors itself before calling runSearchConfig, so that case can never run.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Fixed in e9a6515: removed INVALID_CURSOR and its codeToStatus case.

[github-actions]

🔴 critical — The time range saved in the cursor is never used: each page recomputes its windows from that request's start/end times

cursorState.startTime/endTime are only copied into nextCursorState. resolveSearchWindows is still called with the request's startDate/endDate. When endTime is omitted, parseTimeRange (packages/api/src/mcp/tools/query/helpers.ts:257) sets it to a new now() on every call. Window N therefore moves between pages, and rows get skipped or duplicated with no error. The same happens if a client changes startTime/endTime in the middle of a walk, because the fingerprint doesn't include them. Fix: when a cursor is present, build the windows from new Date(cursorState.startTime)/new Date(cursorState.endTime). Also either reject a request whose explicit range differs from the saved one, or add the saved range to the fingerprint check.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 3aa6b9e: a walk computes its windows from the range pinned in the cursor (walkStart/walkEnd from cursorState.startTime/endTime), not the request's. Covered by "pages with a defaulted endTime".

[github-actions]

🟠 major — Single-window cursor walks get past the 10,000 offset cap, up to OFFSET 1,000,000 over a 366-day range

If the order does not lead with the raw timestamp (any custom orderBy, a toStartOf/toDate timestamp, or the tuple default above), resolveSearchWindows returns one window covering the whole pinned range. The cursor offset then grows page by page up to MAX_CURSOR_OFFSET (1M). The fingerprint is an unkeyed hash, so a caller can also forge {o:1000000} and jump straight to that offset. This allows exactly the deep full-range OFFSET scans that the offset field's .max(10_000) prevents. Fix: when windows.length === 1, cap the cursor offset at the same 10,000 limit, or reject cursor for orders that cannot be windowed.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 6550833. A single-window walk now has the same 10k limit as offset; only windowed walks can go up to 1M. A forged cursor past the limit gets a 400. Integration test added.

[pulpdrew]

Looking good, just a few thoughts on the API spec and a small potential bug

[pulpdrew]

I predict a world where we attempt keyset-based pagination, so I think we should be opaque about how the cursor is implemented - the user doesn't need to know about the time windowing, and we shouldn't guarantee it in the API contract.

Suggested change

	* Send the literal string "start" to begin cursor paging, then pass
	* back the nextCursor from each response. When set, offset is
	* ignored; omit cursor entirely to keep the existing offset paging
	* behaviour, which searches the whole requested range.
	*
	* Keep requesting pages until nextCursor is null. A page that
	* returns fewer rows than maxResults does NOT mean the results are
	* exhausted: pages are scoped to a time window, and a window can
	* hold fewer matching rows than the page size. Stopping on a short
	* page silently truncates your results.
	* Send the literal string "start" to begin cursor paging, then pass
	* back the nextCursor from each response. When set, offset is
	* ignored; omit cursor entirely to use offset-based paging.
	*
	* Keep requesting pages until nextCursor is null. A page that is empty or
	* returns fewer rows than maxResults does NOT mean the results are
	* exhausted.

[vinzee]

Applied in f71df79. The cursor description no longer mentions time windows; it just says empty or short pages don't mean the walk is done.

[vinzee]

I actually went a step further and made the implementation close to keyset-based pagination and also HMAC-signed the cursor.

[pulpdrew]

Suggested change

	* Cursor for the next page, or null when the walk is complete or
	* the request was not a cursor walk. Pass it back as cursor. Do
	* not inspect its contents.
	* Cursor for the next page, or null when the walk is complete or
	* the request did not include a cursor. Pass it back as cursor to in the next request to fetch the next page.
	* This is intended as an opaque value, no guarantees are made about its content.

[vinzee]

Applied in f71df79, with the "as cursor to in" typo fixed.

[pulpdrew]

Can we note this limitation for cursor-based pagination in the openapi spec?

[vinzee]

This limitation no longer exists as the next window is calculated from the current one in the cursor.

[pulpdrew]

With a timestamp expression like TimestampTime, Timestamp (or toDateTime(Timestamp), Timestamp), each column gets its own filter against the same window boundary B. windowInclusivity makes windows (start, end], so at each boundary the filters are:

newer: TimestampTime > B AND Timestamp > B
older: TimestampTime <= B AND Timestamp <= B

Take a row with Timestamp = B + 300ms. Its TimestampTime rounds down to B or earlier:

• it fails TimestampTime > B, so the newer window skips it;
• it fails Timestamp <= B, so the older window skips it.

Neither window returns it. Each boundary loses up to a second of rows, whether or not B falls on a whole second.

Fix:

1. Make windows [start, end) in windowInclusivity. The start is always inclusive, and the end is inclusive only on the window holding the caller's endTime (index 0 for DESC, the last index for ASC). When B is on a whole second, floor(t) >= B is true exactly when t >= B, but floor(t) > B misses every row in (B, B+1s). So the strict comparison belongs on the end bound.
2. Snap boundaries between windows down to whole seconds in the API's resolveSearchWindows, after generating the windows. A whole second lines up with every DateTime/DateTime64 precision. Leave the caller's startTime/endTime exact, and drop any window that snapping leaves empty (a sub-second sliver at the range start). Without this step, a walk that leaves endTime unset (so it resolves to now, with milliseconds) has mid-second boundaries and still loses rows.

A similar issue probably impacts the search page, but we can scope the fix to the API / cursor pagination in this PR.

Test: An integration test with a toDateTime(Timestamp), Timestamp source, rows at B−300ms, B and B+300ms, run once with B on a whole second and once mid-second. Each row must come back exactly once. Both cases fail on the current branch (the B+300ms row is dropped).

[vinzee]

Fixed in f71df79, as you suggested:

1. Windows are now [start, end) in both directions. Only the window holding the caller's endTime keeps its end inclusive.
2. resolveSearchWindows rounds the boundaries between windows down to whole seconds. It keeps the caller's startTime/endTime exact, never rounds below startTime, drops windows left empty, and renumbers the rest.

Added your integration test: a toDateTime(Timestamp), Timestamp source with rows at B-300ms, B and B+300ms, for B on a whole second and mid-second. Both cases fail on the previous commit (one row dropped each) and pass now. Also added unit tests for the boundary rounding and the inclusivity rules. (#3381 has since merged, which covers real coarse/precise column pairs; this change also covers expressions like toDateTime(Timestamp) that have no column type.)

[github-actions]

🟠 major — Windowing stays on for Date-typed timestamp columns, so each window returns the whole day's rows

roundsTimestamp only checks the expression text for toStartOf or toDate(. timeFilterExpr in packages/common-utils/src/core/renderChartConfig.ts:1065-1079 also forces >=/<= on toDate(bound) when the column's type is Date/Date32. For a source whose timestampValueExpression is a bare Date column (e.g. EventDate) ordered by that column, every window that touches a given day returns all of that day's rows, so one walk repeats them across many pages. Fix: check the column type (Metadata.getColumn / classifyTimestampType) before windowing, and fall back to a single window when it's a date.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 278b5a2. Windowing now checks each part of the timestamp expression, including the column type via metadata. A part whose type is Date/Date32 counts as rounded, so a bare EventDate source gets one window. Added tests for Date and Nullable(Date32).

[github-actions]

🔵 minor — Docs suggest cursor paging fixes the tied-timestamp problem, but it still uses OFFSET inside a window

Inside a window, cursor paging still sends LIMIT/OFFSET with the same ORDER BY (utils/search.ts limit block). Rows that share a timestamp can still be skipped or repeated between pages, exactly as with offset. Either add a tiebreaker to the sort or say in the offset/cursor docs (and in openapi.json) that the same caveat applies within a window.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Done in 278b5a2: the cursor description now says that, as with offset, rows sharing a timestamp can be skipped or repeated between pages unless orderBy breaks the tie.

[github-actions]

🔵 minor — windowInclusivity moved to common-utils, but the UI search pager still uses windows inclusive at both ends

packages/app/src/hooks/useOffsetPaginatedQuery.tsx:246-253 queries each window from the same generators with the default both-inclusive range. A row exactly on a shared boundary is still shown on two pages in the UI. Spread windowInclusivity(timeWindow, windows.length) into windowedConfig there too, or keep the helper in the API if only the API should use it.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Same as #3247 (comment): real, but existing UI behaviour. This PR only changes imports in useOffsetPaginatedQuery.tsx, so I'll fix it in a follow-up PR.

[github-actions]

🔵 minor — ordersByTimestamp is a second, stricter version of isTimestampExpressionInFirstOrderBy

isTimestampExpressionInFirstOrderBy (packages/common-utils/src/core/utils.ts:1131) answers the same question with a looser includes match, which the UI uses to decide whether to window. Now the API and UI disagree on queries like toHour(Timestamp) DESC. Either add a strict option to the existing helper or note the twin and the reason for the difference in a doc comment, as the repo's DRY convention requires.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Added a comment in 278b5a2. The API needs the stricter match: windows only preserve the requested order when the first sort key is the timestamp itself. With a derived key like toHour(Timestamp), rows would come back out of order across windows. Changing the shared helper would also change how the UI decides to window, which is outside this PR.

[github-actions]

🟠 major — Cursor walks lose time windows on any source whose timestamp expression contains a toStartOf…, including toStartOfMinute(Timestamp), Timestamp

The check roundsTimestamp matches toStartOf anywhere in timestampValueExpression. That includes the common compound form toStartOfMinute(Timestamp), Timestamp. For a compound expression, timeFilterExpr (packages/common-utils/src/core/renderChartConfig.ts:1068-1081) ANDs the conditions, and the raw column still uses a strict <. So the windows really are half-open: the renderChartConfig test 'compound expression and dateRangeEndInclusive=false (raw col exclusive, toStartOf inclusive)' pins this. Because the check fires anyway, these sources fall back to a single window. The offset cap there is MAX_SINGLE_WINDOW_OFDSET, so every walk fails with CURSOR_WINDOW_TOO_DENSE after 10,000 rows. Fix: disable windowing only when every part from splitAndTrimWithBracket(timestampValueExpression) is rounded. You can test each part with parseToStartOfFunction / toDate( from core/utils.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 278b5a2. Windowing is turned off only when every part is rounded (toStartOf*, toDate, or a Date-typed column). toStartOfMinute(Timestamp), Timestamp keeps its windows because the raw Timestamp part is compared strictly; a lone toStartOfMinute(Timestamp) still gets one window. Tests cover both.

[github-actions]

🟠 major — Offset cap depends on how many windows the range produced, not on whether windowing applies, so the default 15m range hits the 10k cap

offsetCap uses windows.length === 1. A timestamp-ordered range of 15 minutes or less (this includes the default 15m lookback from parseTimeRange in mcp/tools/query/helpers.ts) produces exactly one window, so it gets the 10,000 cap and returns CURSOR_WINDOW_TOO_DENSE. A 16-minute range over the same data gets 1,000,000 in its first window and succeeds. The 10k cap is only meant for the case where windowing is off. Have resolveSearchWindows return whether windowing was engaged (canWindow) and pick the cap from that, then add a test for a ≤15m range with more than 10k rows.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 57d5f56. Windowed walks no longer have an offset cap: each full page cuts the window at the last row's second, so the offset only skips that second's rows. The 10,000 cap now applies only to walks that can't be windowed, whatever the range length.

[github-actions]

🟠 major — An unsigned cursor can request OFFSET up to 1,000,000, 100x the cap on the offset field

Cursors are plain base64 JSON, and the fingerprint is just a hash of fields the client already knows. Any client can mint {w:0,o:999999} and get a ClickHouse OFFSET 999999 over a 15m to 24h window. The offset field caps this at 10,000 to control cost, and the cursor skips that cap. Even an honest walk through a dense window pays a cost that grows with depth, which is what this PR set out to remove. Either sign the cursor with an HMAC so o can only come from the server, or keep MAX_CURSOR_OFFSET at the 10,000 field cap and tell clients to narrow dense windows. Better still, page within a window by (timestamp, tiebreak) keyset instead of OFFSET.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 57d5f56. Cursors are HMAC-signed with EXPRESS_SESSION_SECRET, so a client can't mint or edit one, and MAX_CURSOR_OFFSET is gone: offsets are server-issued and bounded by the rows in one second.

[github-actions]

🔵 minor — keepsExactTime copies the timestamp-part classification in timeFilterExpr, and the two already disagree

timeFilterExpr in common-utils/src/core/renderChartConfig.ts (around lines 962-1076) decides which parts get inclusive bounds using parseToStartOfFunction, a case-sensitive /^toDate\s*\(/, and a Date/Date32 type check. keepsExactTime re-implements this with a case-insensitive /i regex and classifyTimestampType. If the two drift, windows that should be half-open render with inclusive bounds, and rows on boundaries come back twice. Move this into one exported helper in common-utils, e.g. one that says whether a part keeps exact time, and use it in both places.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Fixed in 57d5f56. Both now use isRoundedTimestampPart from common-utils, so timeFilterExpr and the API classify timestamp parts the same way.

[github-actions]

🟠 major — DISTINCT cursor walks are still time-windowed, so values repeat across windows

Only cutColumn is dropped for a DISTINCT select. resolveFirstSearchWindow is still called with forceSingleWindow: !cursorState, so the walk steps through windows (15m, 6h, …) and each window's SELECT DISTINCT is deduplicated separately. With select: 'DISTINCT Body' over a 1h range, a Body present in both the newest 15m window and the next window is returned twice. The PR description says DISTINCT walks page with plain offsets instead. Fix: pass forceSingleWindow: !cursorState || isDistinct (compute the DISTINCT check before resolving the window) so these walks use one window over the whole range and keep the 10,000 offset cap.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in 839d3de: DISTINCT selects now use one window over the whole range (10,000 offset cap), so values aren't repeated across windows.

[github-actions]

🔵 minor — DISTINCT walk test never crosses a window boundary

All 30 rows sit inside the 2 minutes before endTime, which is within the first 15m window, so the test cannot catch duplicates across windows. Add rows with the same Body both in the newest 15m and older than 15m before endTime, then assert each value appears exactly once.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Done in 839d3de: the test now puts the same values in two windows.

[github-actions]

🔵 minor — CURSOR_QUERY_MISMATCH added to SearchErrorCode but never returned by runSearchConfig

runSearchConfig only ever returns CURSOR_WINDOW_TOO_DENSE. The query-mismatch case is produced by decodeSearchCursor (as CursorDecodeError) and answered inline in the router at search.ts:463. Remove CURSOR_QUERY_MISMATCH from SearchErrorCode and from the codeToStatus case in packages/api/src/routers/external-api/v2/search.ts:401.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Removed in 839d3de.

[github-actions]

🔵 minor — Overflow-mode test passes on any failure, including an unrelated crash

expect(res.status).not.toBe(200) passes for a 500 from any cause. Assert the specific status and error message that a ClickHouse result-rows overflow error should produce, so the test proves withThrowingOverflow replaced the source's break setting.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Done in 839d3de: the test now also checks that offset paging on the same source returns 200, so only the cursor override can explain the failure, and asserts the 500.

[github-actions]

🟠 major — Cursor walk breaks a source's max_result_rows=N/break limit when maxResults equals N, and the client gets an opaque 500

A cursor walk always asks for pageLimit + 1 rows (search.ts:249). withThrowingOverflow (line 47) also forces result_overflow_mode=throw. Take a source with max_result_rows=1000, result_overflow_mode=break and a request with maxResults: 1000, cursor: 'start': it asks for 1001 rows, ClickHouse throws TOO_MANY_ROWS_OR_BYTES, and that type is not in CH_USER_INPUT_ERRORS (v2/search.ts:42), so the client gets a generic 500 'Query execution failed' on every page. The same request with offset paging works. Fix: before querying, clamp pageLimit to the source's max_result_rows minus 1. Alternatively, catch the overflow error type and return a 400 that tells the caller to lower maxResults or the source limit.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[vinzee]

Fixed in a476439. A cursor page asks for at most the source's max_result_rows rows. When that leaves no room for the lookahead row, a full page counts as "more in this window". Covered by a walk with max_result_rows=5 and maxResults 5 and 100.

[github-actions]

🔵 minor — Overflow test passes on any server error, not only the overflow it targets

The test only checks res.status === 500, and any failure gives that status: a SQL error from the injected cursor column, a missing __hdx_cursor_ts, or a metadata failure. Assert that the error is the ClickHouse overflow, for example by spying on the logger or searchQueryErrors for chType TOO_MANY_ROWS_OR_BYTES. Also add a case where maxResults equals the source's max_result_rows, to cover the lookahead row.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Fixed in a476439. The test now uses max_rows_to_read=1 with read_overflow_mode=break and asserts the logged ClickHouse error is TOO_MANY_ROWS. The max_result_rows case has its own test (above).

[github-actions]

🔵 minor — Window-size schedule reimplemented next to generateTimeWindows* in common-utils

cumulativeWindowMs and the nextSearchWindow loop reimplement the schedule that generateTimeWindowsDescending/generateTimeWindowsAscending define in common-utils/src/core/searchWindows.ts (window sizes in order, with the largest repeating). referenceWindows in searchWindows.test.ts already rebuilds the same windows from those functions by generating them, flooring interior bounds to the second, and dropping empty windows. Use that approach: build the window list from the shared generator and index into it by ordinal. If you keep the stepping version, add a doc comment that names its twin, as the repo conventions require.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Keeping this. The API steps from one window to the next without generating the whole list, so it needs the schedule as a function of the previous window. The equivalence test in searchWindows.test.ts checks it against generateTimeWindows* for both directions.

[github-actions]

🔵 minor — 10,000 offset cap is defined twice and must be kept in sync by hand

MAX_SINGLE_WINDOW_OFFSET = 10_000 says it 'Matches the offset request field's cap', but that cap is a separate literal at .max(10_000) in v2/search.ts:272. Use the exported constant in the zod schema too, so the two values cannot drift apart.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[vinzee]

Fixed in 61b9dd9: the offset schema uses MAX_SINGLE_WINDOW_OFFSET.

[vinzee]

Replies to the summary findings:

• OVERFLOW_MODE_SETTINGS misses other *_overflow_mode settings: fixed in a476439. Any source setting ending in _overflow_mode (or _overflow_mode_leaf) is forced to throw, on top of the three defaults.
• Dead typeof baseSelect === 'string' guard: removed in a476439.
• Unbounded offset inside one bucket: the offset only skips rows that share the last row's second, so it is bounded by rows per second, not by window depth. Removing it needs a unique row key (for example _block_number/_block_offset), which is a follow-up.
• Nullable(Date) / LowCardinality(Date) now get inclusive bounds: not a change from this PR. timeFilterExpr on main already strips type wrappers before the Date check (from fix: v2 search drops rows after a sub-second startTime; make maxResults cap configurable #3381); isRoundedTimestampPart keeps that behaviour.
• Metadata lookup on every page: real, one getColumn query per windowed page, small next to the page query itself. Carrying the column type in the cursor would grow the cursor and add a stale-schema case, so I'm leaving it as is.

[greptile-apps]

Completed searches end with errors

With orderBy: "Body ASC", exactly 10,000 matches, maxResults: 100, and source max_result_rows: 100, the last full page returns another cursor instead of finishing. This condition assumes more rows exist because the source limit prevents reading an extra row.

That cursor has offset 10,000, so the next request returns 400 before querying, even though nothing remains. Allow an end-of-results check at the cap so the walk can finish with nextCursor: null.

[github-actions]

🔵 minor — windowed return field duplicates cut != null and only tests read it

resolveFirstSearchWindow sets windowed: false exactly when cut == null, and true otherwise. runSearchConfig never reads windowed; only searchWindows.test.ts does. Drop the field and have the tests assert on first.cut, so the same fact isn't kept in two places.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}