Add pagination of batch requests - #64061
Conversation
… async client it exposed
| async connect(): Promise<void> { | ||
| if (this.connected) return; | ||
| connect(): Promise<void> { | ||
| if (this.connected) return Promise.resolve(); |
There was a problem hiding this comment.
Somehow the new large-file stress test actually triggered the hang (when used in conjunction with the full suite of API tests) that copilot found a few PRs ago so I went ahead and fixed it here.
There was a problem hiding this comment.
Pull request overview
Adds transparent pagination for large batch API responses.
Changes:
- Adds server-side paging and continuation tokens.
- Reassembles pages in synchronous and asynchronous clients.
- Adds protocol types, options, and pagination tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tsc/internal/api/session.go |
Implements pagination and continuation storage. |
tsc/internal/api/session_batch_test.go |
Tests server pagination behavior. |
tsc/internal/api/proto.go |
Extends and custom-encodes the batch protocol. |
packages/typescript/src/api/sync/client.ts |
Reassembles paginated synchronous responses. |
packages/typescript/src/api/async/client.ts |
Reassembles asynchronous responses and deduplicates connection attempts. |
packages/typescript/src/api/proto.ts |
Excludes nested batch requests from request types. |
packages/typescript/src/api/proto.generated.ts |
Adds pagination protocol fields. |
packages/typescript/src/api/options.ts |
Exposes the response-page size option. |
packages/typescript/test/sync/api-generators.test.ts |
Tests synchronous pagination. |
packages/typescript/test/async/api.test.ts |
Tests asynchronous pagination. |
Suppressed comments (1)
packages/typescript/src/api/options.ts:25
- This is not a strict maximum: the server deliberately returns an intact response when one item alone exceeds the limit (see the added oversized-single-response test). Document that exception so callers do not rely on this option to guarantee that every decoded page stays below a hard transport/string-size ceiling.
/** Maximum encoded byte size of each batch response page. Defaults to 300 million bytes. */
maxResponseBytesPerPage?: number;
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| s.batchResponsePages.Store(continuationToken, batchResponsePage{ | ||
| encodedResponses: slices.Clone(page.encodedResponses[pageLength:]), | ||
| }) |
| pageParams.maxResponseBytesPerPage = this.maxResponseBytesPerPage; | ||
| } | ||
| const page = this.apiRequest("batchRequests", pageParams); | ||
| responses = responses.concat(page.responses); |
| pageParams.maxResponseBytesPerPage = this.options.maxResponseBytesPerPage; | ||
| } | ||
| const page = await this.sendRequestWithTiming(requestType, pageParams); | ||
| responses = responses.concat(page.responses); |
| s.batchResponsePages.Store(continuationToken, batchResponsePage{ | ||
| encodedResponses: slices.Clone(page.encodedResponses[pageLength:]), | ||
| }) |
| /** Maximum encoded byte size of each batch response page. Defaults to 300 million bytes. */ | ||
| maxResponseBytesPerPage?: number; |
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
|
Are any of these copilot review comments valid? |
|
Hmmmm, well, the concat thing is basically true, except we're using concat because The "continuation tokens that aren't used have an unbounded lifetime" is true, I just thought it probably wasn't too important, since only a misbehaving/broken API client would ever encounter it as a problem (and I don't mean "bad user API code", I mean, "ours or a 3rd party client implementation fails to read all the responses") - is it worth scoping them to a specific API client to handle that? I dunno. Feels a bit excessive. We could handle it, but do we need to? And does that even "fix" the issue? A misbehaving client can still start a truckload of batch requests and just never read the response pages and DoS us, but is that our fault, really (and that wouldn't even be fixed by client-scoping them)? And the comment clarity... fair enough, I guess? It's pretty clear from use it's not a hard cut. |
Fixes an issue Titian Cernicova-Dragomir (@dragomirtitian) brought up around the JS max string size (!) for large batches of request responses. On the API client side, there's a
maxResponseBytesPerPageoption that's passed thru to the server with batch requests to limit response sizes , but the server has a default value of 300,000,000 bytes (which is a nice round number just under 66% of the v8 default string max size - to account for base64 encoding overhead) if that's not provided. Paged server responses are automatically reassembled into the full response objects back on the JS end, so nobody should really have to think about the protocol-level pagination.This gets a little weird on the server-side to avoid double-encoding the response messages. Specifically, we have to encode the individual messages early so we can get their size and decide where to paginate and then custom-encode the batch response to be able to directly inline the already-encoded nested response objects. All fine enough things to do, I think, just a little weird compared to other API responses.
A small aside, but this PR also forbids nesting
batchRequestprotocol methods withinbatchRequestcalls - the client already flattens incoming generators, and forbidding it makes pagination much simpler, so seems preferable to complicating pagination further.