fix: use paginated resource listing to avoid gRPC message size limit#341
Merged
mangelajo merged 5 commits intojumpstarter-dev:mainfrom Mar 20, 2026
Merged
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClientConfig listing methods now aggregate paginated server responses for leases and exporters internally; tests added across e2e, CLI, config pagination unit tests, and gRPC options unit tests. Changes
Sequence DiagramsequenceDiagram
participant CLI as Client (CLI)
participant ClientConfig as ClientConfigV1Alpha1
participant Paginator as _collect_all_leases()
participant Service as gRPC Service
CLI->>ClientConfig: list_leases(filter, only_active)
ClientConfig->>Paginator: _collect_all_leases(svc, page_size=100, ...)
loop while next_page_token != ""
Paginator->>Service: ListLeases(page_token, page_size)
Service-->>Paginator: LeaseList {leases, next_page_token}
Paginator->>Paginator: append leases
end
Paginator-->>ClientConfig: LeaseList {all_leases, next_page_token=None}
ClientConfig-->>CLI: LeaseList (aggregated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
raballew
force-pushed
the
020-grpc-message-size-limit
branch
from
1e2cf60 to
ca699bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
python/packages/jumpstarter/jumpstarter/common/grpc_test.py (1)
11-14: Consider also asserting that other defaults remain intact.The test verifies the override works, but doesn't confirm that non-overridden defaults (e.g.,
grpc.lb_policy_name) are still present. Adding one more assertion would strengthen confidence that the merge behavior is correct.💡 Suggested enhancement
def test_user_options_override_defaults(): user_options = {"grpc.keepalive_time_ms": 50000} options = dict(_override_default_grpc_options(user_options)) assert options["grpc.keepalive_time_ms"] == 50000 + # Verify other defaults are still preserved + assert options["grpc.lb_policy_name"] == "round_robin"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/grpc_test.py` around lines 11 - 14, The test only checks that an overridden option is applied; add an assertion that non-overridden defaults remain intact by asserting that "grpc.lb_policy_name" (or another expected default key) is present in options and equals the value returned by the default provider—i.e., call the default generator (e.g., _default_grpc_options() or the same source used by _override_default_grpc_options) and assert options["grpc.lb_policy_name"] == dict(_default_grpc_options())["grpc.lb_policy_name"] inside test_user_options_override_defaults to verify merge behavior.python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
219-220: Avoid hard-coding the current__wrapped__depth.These assertions are pinned to the exact decorator layering of
get_exporters/get_leasesinpython/packages/jumpstarter-cli/jumpstarter_cli/get.py, not the CLI behavior. A harmless wrapper reorder will break the test while the commands still callconfig.list_*correctly.CliRunner().invoke(...)or a small undecorated helper would make this much less brittle.Also applies to: 237-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py` around lines 219 - 220, The test is brittle because it calls get_exporters.callback.__wrapped__.__wrapped__ (and similarly get_leases at lines indicated); replace these direct double-__wrapped__ calls with a stable invocation: either use click.testing.CliRunner().invoke on the click command (e.g., CliRunner().invoke(get_exporters, ["--output", "text"]) or CliRunner().invoke(get_leases, ...)) to exercise the real CLI entrypoint, or factor out an undecorated helper function that directly calls config.list_exporters / config.list_leases and call that helper from the test; update assertions to verify config.list_* calls or command output rather than relying on decorator depth.e2e/tests.bats (1)
346-348: Make these pagination tests self-contained.Both tests assume
test-client-oidcalready exists, is logged in, and that exporters are already running. That keeps them order-dependent and makes isolated reruns flaky. Please provision and tear down those prerequisites inside the test or a dedicated local helper. Based on learnings: E2E tests in bats should be runnable in isolation. Do not rely on prior suite setup or shared state across tests.Also applies to: 366-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests.bats` around lines 346 - 348, The two pagination tests call wait_for_exporter and then switch to an existing client via the command jmp config client use test-client-oidc, making them order-dependent; make each test self-contained by provisioning and tearing down its prerequisites: create a dedicated test client (e.g. use jmp config client create ... --oidc test-client-oidc or equivalent helper), perform an explicit login for that client (e.g. jmp auth login ...), start exporters inside the test (or call a local helper like start_exporter) and wait_for_exporter, run the pagination assertions, then stop exporters (stop_exporter) and delete the test client (or call a teardown helper) in a trap/cleanup block so the test can run in isolation; replace direct uses of wait_for_exporter and jmp config client use test-client-oidc with these setup/teardown steps in both affected tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests.bats`:
- Around line 370-386: The current test launches 101 background "jmp admin
create exporter -n ... pagination-exp-${i}" jobs and then counts all YAML
"name:" entries from "jmp get exporters -o yaml", which can false-pass; instead
capture and wait on each background create PID (store PIDs when launching and
wait for them) and scope the get query to only the created exporters (e.g.,
filter by namespace and name pattern or label like "pagination-exp-") and assert
the exact count equals 101 (not >=), and when deleting do the same PID tracking:
record delete PIDs for "jmp admin delete exporter --namespace ...
pagination-exp-${i}" and wait on those PIDs to ensure cleanup completed before
finishing the test.
In `@python/packages/jumpstarter/jumpstarter/config/client.py`:
- Around line 202-226: The list_exporters function currently honors the
page_size for svc.ListExporters but ignores it when calling _collect_all_leases;
change the call leases_response = await self._collect_all_leases(svc) to pass
the page_size parameter (leases_response = await self._collect_all_leases(svc,
page_size=page_size)) so lease enrichment uses the requested page_size, and add
a regression test exercising list_exporters(include_leases=True, page_size=1) to
lock this behavior; ensure any _collect_all_leases signature already accepts
page_size or adjust it accordingly.
---
Nitpick comments:
In `@e2e/tests.bats`:
- Around line 346-348: The two pagination tests call wait_for_exporter and then
switch to an existing client via the command jmp config client use
test-client-oidc, making them order-dependent; make each test self-contained by
provisioning and tearing down its prerequisites: create a dedicated test client
(e.g. use jmp config client create ... --oidc test-client-oidc or equivalent
helper), perform an explicit login for that client (e.g. jmp auth login ...),
start exporters inside the test (or call a local helper like start_exporter) and
wait_for_exporter, run the pagination assertions, then stop exporters
(stop_exporter) and delete the test client (or call a teardown helper) in a
trap/cleanup block so the test can run in isolation; replace direct uses of
wait_for_exporter and jmp config client use test-client-oidc with these
setup/teardown steps in both affected tests.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py`:
- Around line 219-220: The test is brittle because it calls
get_exporters.callback.__wrapped__.__wrapped__ (and similarly get_leases at
lines indicated); replace these direct double-__wrapped__ calls with a stable
invocation: either use click.testing.CliRunner().invoke on the click command
(e.g., CliRunner().invoke(get_exporters, ["--output", "text"]) or
CliRunner().invoke(get_leases, ...)) to exercise the real CLI entrypoint, or
factor out an undecorated helper function that directly calls
config.list_exporters / config.list_leases and call that helper from the test;
update assertions to verify config.list_* calls or command output rather than
relying on decorator depth.
In `@python/packages/jumpstarter/jumpstarter/common/grpc_test.py`:
- Around line 11-14: The test only checks that an overridden option is applied;
add an assertion that non-overridden defaults remain intact by asserting that
"grpc.lb_policy_name" (or another expected default key) is present in options
and equals the value returned by the default provider—i.e., call the default
generator (e.g., _default_grpc_options() or the same source used by
_override_default_grpc_options) and assert options["grpc.lb_policy_name"] ==
dict(_default_grpc_options())["grpc.lb_policy_name"] inside
test_user_options_override_defaults to verify merge behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 537cbd63-648a-4af8-8f80-32db6f01480b
📒 Files selected for processing (5)
e2e/tests.batspython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter/jumpstarter/common/grpc_test.pypython/packages/jumpstarter/jumpstarter/config/client.pypython/packages/jumpstarter/jumpstarter/config/client_config_test.py
…umpstarter-dev#337) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
020-grpc-message-size-limit
branch
from
ca699bd to
52ff59b
Compare
The paginated exporter listing test was missing the required -l/--label option when creating exporters, causing all 101 creations to silently fail in the background. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When list_exporters is called with include_leases=True, the page_size parameter was not forwarded to _collect_all_leases, always using the default of 100 regardless of the caller's intent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use --selector to scope the exporter query to only the test-created exporters and assert exact count (101) instead of >= 101. Track PIDs from background create commands to ensure all complete before querying. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/config/client.py (1)
181-191: Add a repeated-token guard to prevent unbounded pagination loops.If the server ever returns the same non-empty
next_page_tokenrepeatedly, both loops can run forever. A small guard makes this path safer.Proposed resiliency patch
async def _collect_all_leases(self, svc, page_size=100, only_active=True, filter=None): from jumpstarter.client.grpc import LeaseList all_leases = [] page_token = None + seen_tokens = set() while True: page = await svc.ListLeases( page_size=page_size, page_token=page_token, filter=filter, only_active=only_active, ) all_leases.extend(page.leases) - if not page.next_page_token: + next_token = page.next_page_token + if not next_token: break - page_token = page.next_page_token + if next_token in seen_tokens: + raise ConnectionError("ListLeases returned a repeated next_page_token") + seen_tokens.add(next_token) + page_token = next_token return LeaseList(leases=all_leases, next_page_token=None)svc = ClientService(channel=await self.channel(), namespace=self.metadata.namespace) all_exporters = [] page_token = None + seen_tokens = set() while True: page = await svc.ListExporters( page_size=page_size, page_token=page_token, filter=filter, ) all_exporters.extend(page.exporters) - if not page.next_page_token: + next_token = page.next_page_token + if not next_token: break - page_token = page.next_page_token + if next_token in seen_tokens: + raise ConnectionError("ListExporters returned a repeated next_page_token") + seen_tokens.add(next_token) + page_token = next_tokenAlso applies to: 209-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/config/client.py` around lines 181 - 191, The pagination loop using svc.ListLeases (the block that appends to all_leases and updates page_token) needs a repeated-token guard: track seen non-empty page tokens (e.g., a set) and if page.next_page_token is non-empty and already seen, stop and surface an error (or raise/return) to avoid an infinite loop; apply the same fix to the other identical loop around lines 209-218. Specifically, update the loop that references page_token, page.next_page_token, svc.ListLeases, and all_leases to insert the seen-token check before assigning page_token and before continuing the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/config/client.py`:
- Around line 181-191: The pagination loop using svc.ListLeases (the block that
appends to all_leases and updates page_token) needs a repeated-token guard:
track seen non-empty page tokens (e.g., a set) and if page.next_page_token is
non-empty and already seen, stop and surface an error (or raise/return) to avoid
an infinite loop; apply the same fix to the other identical loop around lines
209-218. Specifically, update the loop that references page_token,
page.next_page_token, svc.ListLeases, and all_leases to insert the seen-token
check before assigning page_token and before continuing the loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fa93231-79e3-4279-ba2d-11b6a28c35c0
📒 Files selected for processing (5)
e2e/tests.batspython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter/jumpstarter/common/grpc_test.pypython/packages/jumpstarter/jumpstarter/config/client.pypython/packages/jumpstarter/jumpstarter/config/client_config_test.py
✅ Files skipped from review due to trivial changes (3)
- python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py
- python/packages/jumpstarter/jumpstarter/common/grpc_test.py
- python/packages/jumpstarter/jumpstarter/config/client_config_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests.bats
The background process batching caused `wait "$pid"` to fail with status 127 because intermediate bare `wait` calls already reaped all child processes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mangelajo
approved these changes
Mar 20, 2026
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
2 participants
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.
Summary
list_leasesandlist_exportersonClientConfigV1Alpha1, iterating through all pages (page_size=100) instead of returning a single pagelist_leases/list_exportersmethods, keeping only the paginated versions_collect_all_leasesshared helper to avoid duplicating the pagination loop (also used bylist_exporterswhen enriching exporters with lease data)Fixes #337
Test plan
test_list_leases_paginates,test_list_exporters_paginates,test_list_leases_single_page)test_get_exporters_calls_list_exporters,test_get_leases_calls_list_leases)test_default_options_preserve_existing_defaults,test_user_options_override_defaults)paginated lease listing returns all leases)paginated exporter listing returns all exporters)🤖 Generated with Claude Code