Skip to content

fix(flow): raise Core gRPC receive limit - #5448

Open
kunzhao-nv wants to merge 1 commit into
NVIDIA:mainfrom
kunzhao-nv:fix/flow-core-grpc-receive-limit
Open

fix(flow): raise Core gRPC receive limit#5448
kunzhao-nv wants to merge 1 commit into
NVIDIA:mainfrom
kunzhao-nv:fix/flow-core-grpc-receive-limit

Conversation

@kunzhao-nv

Copy link
Copy Markdown
Contributor

Flow's Core client currently uses grpc-go's default 4 MiB receive limit. Core's expected machine, rack, switch, and power-shelf RPCs each return a complete collection in one unary response, so a larger site can exceed that limit. The RPC then fails with ResourceExhausted, and Flow cannot refresh that resource type.

This change sets a 32 MiB receive ceiling on Flow's Core client, matching the existing site-workflow setting. The limit applies to every Core response received through this client and allocates memory only as a response is received. A transport-level test proves that the old default rejects an expected-inventory response over 4 MiB while the Flow client accepts the complete response.

Related issues

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

This is a short-term guard rather than the complete resolution of #4355. The issue remains open to define a bounded pagination or streaming contract for expected inventory above 32 MiB. Because the current RPCs are unary, an oversized or otherwise failed response is returned as an error and is not applied as a partial snapshot.

Verification:

  • go test -p 1 ./flow/... -count=1
  • go test -race ./flow/internal/nicoapi -count=1
  • go vet ./flow/internal/nicoapi
  • go tool golangci-lint run --new-from-rev=origin/main ./flow/internal/nicoapi
  • make lint-go
  • python3 scripts/check_source_headers.py

Expected-inventory RPCs return complete snapshots and can exceed grpc-go's 4 MiB default receive limit. Configure Flow's Core client with the same 32 MiB ceiling used by site-workflow.

Cover the transport boundary with a response that the default client rejects but the Flow client accepts.

Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
@kunzhao-nv
kunzhao-nv requested a review from a team as a code owner August 27, 2026 07:23
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d83e3fa6-5187-4c9a-8075-cdb87f31417b

📥 Commits

Reviewing files that changed from the base of the PR and between 863f636 and cd7b0a3.

📒 Files selected for processing (2)
  • rest-api/flow/internal/nicoapi/grpc.go
  • rest-api/flow/internal/nicoapi/grpc_transport_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of large machine inventory responses.
    • Responses up to 32 MiB can now be received successfully, preventing failures caused by the previous lower limit.
  • Tests
    • Added coverage verifying successful retrieval of multi-megabyte inventories and rejection of responses exceeding the default limit.

Walkthrough

The Core gRPC client now accepts responses up to 32 MiB. Shared dial options preserve TLS credentials and the unary interceptor. A transport test validates rejection at the default limit and successful receipt of a large inventory with Flow’s options.

Changes

Core gRPC transport

Layer / File(s) Summary
Shared Core gRPC dial options
rest-api/flow/internal/nicoapi/grpc.go
Adds a 32 MiB receive-message limit and centralizes transport credentials, the limit, and the unary interceptor. NewClient uses the shared options.
Receive-limit transport test
rest-api/flow/internal/nicoapi/grpc_transport_test.go
Serves a large inventory over an in-memory connection. The test verifies default gRPC rejection and successful receipt through Flow’s configured options.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to cd7b0

This PR raises Flow’s Core gRPC receive limit to support larger expected-inventory responses; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: increasing the Core gRPC receive limit for Flow.
Description check ✅ Passed The description directly explains the gRPC receive-limit issue, the 32 MiB fix, the scope, testing, and remaining limitations.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd7b0a3a1e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

return s.response, nil
}

func TestCoreGRPCDialOptionsReceiveExpectedInventory(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Name the test after coreGRPCDialOptions

This test exercises the newly added coreGRPCDialOptions helper, but its top-level name adds a scenario-specific suffix instead of using the required Test<Function> form. Rename it to TestCoreGRPCDialOptions and retain the existing cases as named t.Run subtests so future scenarios for this helper remain grouped under the same function-level test.

AGENTS.md reference: rest-api/AGENTS.md:L193-L197

Useful? React with 👍 / 👎.

@github-actions

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-08-27 07:26:54 UTC | Commit: cd7b0a3

@thossain-nv thossain-nv added the rest-api Add this label when an issue or PR concerns NICo REST API label Aug 27, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rest-api Add this label when an issue or PR concerns NICo REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants