fix(notion): make the query page bound configurable and honour Retry-After - #6006
Open
uschtwill wants to merge 1 commit into
Open
fix(notion): make the query page bound configurable and honour Retry-After#6006uschtwill wants to merge 1 commit into
uschtwill wants to merge 1 commit into
Conversation
…After QueryDataSource paginates up to maxQueryPages (50) x maxPageSize (100), so a data source larger than 5000 rows cannot be synced at all: the call returns an error rather than partial results, and because every CLI narrowing flag is applied after it returns, no flag works around it. Sync dies in both directions at once. Refs gastownhall#6004. Two changes, and the second is what makes the first safe: - Client.MaxQueryPages / WithMaxQueryPages lets a caller raise the bound for a large data source. Default behaviour is unchanged when unset. The error at the bound now names the ceiling in rows as well as pages, since rows is the number a caller can compare against their own data source. - doRequest now retries 429 and 529 honouring Retry-After, with exponential fallback clamped at 30s. Notion enforces ~3 requests/second per connection, so raising the page bound without this would trade a clear failure for an intermittent one. Other 5xx retry only for GET/DELETE — replaying a POST that may already have been applied server-side is how duplicates get made. The request body is now marshalled once and the reader rebuilt per attempt; a retry cannot reuse a drained body. Tests cover the configured bound, the unchanged default, Retry-After being obeyed exactly, GET retrying on 5xx, and POST not retrying. Verified they bite: forcing maxRequestAttempts to 1 fails exactly the two retry tests, and ignoring the configured bound fails exactly the bound test. make build and make fmt-check pass; internal/notion and internal/tracker pass with -tags gms_pure_go. golangci-lint was not run locally (not installed on this machine); CI will cover it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #6004.
What this does
QueryDataSourcepaginates up tomaxQueryPages(50) ×maxPageSize(100). Past 5000 rows it returns an error rather than partial results, so sync stops working in both directions — and because every CLI narrowing flag (--issues,--state,--dry-run,bd notion push/pull) is applied after that call returns, none of them is a workaround.Two changes, and the second is what makes the first safe to use:
Client.MaxQueryPages/WithMaxQueryPages— lets a caller raise the bound for a large data source. Unset behaviour is unchanged. The error at the bound now names the ceiling in rows as well as pages, because rows is the number a caller can actually compare against their own data source.Retry-After-honouring retries indoRequest— 429 and 529 always; other 5xx only forGET/DELETE, since replaying a POST that may already have been applied server-side is how duplicates get made. Exponential fallback clamped at 30s.(2) is not optional garnish. Notion enforces roughly 3 requests/second per connection, and the client currently has no retry, no backoff and no 429 handling at all — every non-2xx becomes a hard error. Raising the page bound without backoff would trade a clear, reproducible failure for an intermittent one.
The request body is now marshalled once and the reader rebuilt per attempt; a retry cannot reuse a drained body.
What this deliberately does not do
It does not fix the underlying design, where one unfiltered whole-database fetch feeds both the pull and the push's create-vs-update index, so cost scales with total database size on every sync forever.
I had intended to send
last_edited_time on_or_after Sinceinto the query —FetchOptions.Sincealready exists, is populated from<prefix>.last_sync, and is currently applied client-side inmatchesFetchSinceafter everything has been downloaded. On reading it properly that is not a safe drop-in:FetchIssuesalso relies on seeing the full remote set forshouldBackfillNotionIssue, which discovers pages that exist remotely but are missing locally. Filtering the fetch would silently stop that discovery. It needs a deliberate decision about backfill semantics, which felt like the maintainers' call rather than something to smuggle into this PR. I have written that up in #6004.Testing
New tests cover: the configured bound, the unchanged default,
Retry-Afterbeing obeyed exactly,GETretrying on 5xx, andPOSTnot retrying.Verified the tests actually bite rather than merely pass:
maxRequestAttemptsto 1 fails exactly the two retry tests, and nothing else;make buildandmake fmt-checkpass.internal/notionandinternal/trackerpass with-tags gms_pure_go.golangci-lintwas not run locally — it isn't installed on this machine, so CI will be the first gate for it.Context
Our tracker crossed 5000 rows overnight and sync died in both directions for ~14 hours before anyone noticed, because a failing sync looks exactly like a quiet one. Operator-filed issues kept landing in Notion and never reached bd. Recovery meant trashing 1005 old rows by hand through the Notion API.