Skip to content

feat(proto): add pagination to ListApplications - #1151

Draft
hanchchch wants to merge 1 commit into
mainfrom
hanch/vul-1330-proto-pagination
Draft

feat(proto): add pagination to ListApplications#1151
hanchchch wants to merge 1 commit into
mainfrom
hanch/vul-1330-proto-pagination

Conversation

@hanchchch

@hanchchch hanchchch commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Why

ListApplications returns one entry per application revision. Accounts that deploy on every pod rollout accumulate hundreds of thousands of them — one production account currently has 547k rows in registered_applications. The RPC has no way for a client to ask for fewer, and the controller sets grpc.max_send_message_length to -1, so a single call both hydrates and serialises an unbounded result set.

That is what OOM-killed the isolate-cloud grpc pods on 2026-08-21 (7 of 10 pods, 5Gi limit) — see VUL-1330 / INFRA-4247.

The column-loading half of the fix is already in flight in isolate-cloud (fal-ai/isolate-cloud#9409, deferring source_code). This PR adds the wire contract needed for the row-count half.

What

message field
ListApplicationsRequest optional int32 page_size = 3
ListApplicationsRequest optional string page_token = 4
ListApplicationsResult optional string next_page_token = 2

Design notes:

  • The token is opaque on purpose. The server will use a keyset cursor over (created_at, application_id); offset paging would skip or repeat rows on a table that is written to constantly. Keeping the encoding server-side means it can change without touching this contract.
  • All three fields are optional so the server can tell "client did not ask" from "client asked for zero" and apply its own default. Clients that predate this change keep working and receive the server's default page.
  • next_page_token absent means last page. Callers must not stop on a short page — the server may return fewer rows than page_size and still have more.

Follow-ups (separate PRs, in order)

  1. This PR → merge + release isolate_proto_v0.34.3
  2. fal SDK: auto-paginate in FalServerlessConnection.list_applications so existing callers keep their current semantics
  3. isolate-cloud: honour page_size/page_token, with a server-side default and ceiling that also bounds old clients

Testing

  • pytest projects/isolate_proto/tests — 11 passed, including two new field-presence tests
  • pre-commit run --files <changed> — passed (ruff/mypy correctly skip isolate_proto)
  • Bindings regenerated via tools/regen_grpc.py --isolate-path <local isolate>; generated files were not hand-edited

🤖 Generated with Claude Code


Note

Cursor Bugbot is generating a summary for commit 4566b65. Configure here.

ListApplications returns one entry per application *revision*, and accounts
that deploy on every pod rollout accumulate hundreds of thousands of them.
There is no way for a client to ask for less, and the controller sets
grpc.max_send_message_length to -1, so a single call can hydrate and
serialise an unbounded result set. That OOM-killed the isolate-cloud grpc
pods on 2026-08-21 (VUL-1330).

Adds page_size/page_token to the request and next_page_token to the result.
The token is deliberately opaque so the server can use a keyset cursor --
offset paging would skip or repeat rows on a table that is written to
constantly -- and change the encoding later without touching the contract.

All three fields are `optional` so the server can distinguish "client did
not ask" from "client asked for zero" and apply its own default; clients
that predate this change keep working and get the server's default page.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 21, 2026

Copy link
Copy Markdown

VUL-1330

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean, additive proto change.

Extended reasoning...

Overview

This PR adds pagination support to the ListApplications RPC in controller.proto: two new optional fields on ListApplicationsRequest (page_size, page_token) and one new optional field on ListApplicationsResult (next_page_token). The generated controller_pb2.py/.pyi bindings are regenerated to match, and two new field-presence tests are added to test_proto.py. No server-side (isolate-cloud) logic is included in this PR — it is purely the wire contract, with the actual pagination behavior deferred to a follow-up PR per the description.

Security risks

None identified. This is a protobuf schema addition with no auth, crypto, or data-handling logic. All new fields are optional, which preserves backward compatibility with existing clients/servers (proto3 field addition is safe and non-breaking).

Level of scrutiny

Low. This is a mechanical, additive proto change: new optional fields only, no field renumbering or removal, no changes to existing message semantics. The generated _pb2.py/_pb2.pyi diffs are consistent with what protoc/regen tooling would produce for this exact schema change (field numbers 3/4 on the request, 2 on the result, correctly wired into the serialized descriptor and offset tables). I verified the .proto source and generated files are in sync by inspecting the new field definitions and their presence-tracking (optional fields get a synthetic oneof, reflected correctly in both files).

Other factors

Test coverage is good for what this PR does — it adds explicit presence tests confirming HasField distinguishes unset from zero/empty, which is exactly the design property the PR calls out as important (server needs to distinguish "client didn't ask" from "client asked for zero"). No outstanding review comments in the timeline. The actual pagination enforcement (server clamping, cursor logic) is explicitly out of scope and deferred to isolate-cloud, so there's no behavioral risk introduced here beyond the wire format itself.

@hanchchch
hanchchch marked this pull request as draft August 24, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant