Skip to content

fix(supervisor-middleware): configure HTTP/2 keepalive on middleware gRPC channel - #5

Draft
letv1nnn wants to merge 1 commit into
mainfrom
fix-middleware-grpc-keepalive
Draft

fix(supervisor-middleware): configure HTTP/2 keepalive on middleware gRPC channel#5
letv1nnn wants to merge 1 commit into
mainfrom
fix-middleware-grpc-keepalive

Conversation

@letv1nnn

@letv1nnn letv1nnn commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

The middleware gRPC channel was the only long-lived client in the repo without HTTP/2 keepalive config. Without it, idle connections are silently reaped by intermediaries and dead peers go undetected until the next evaluation attempt.

This is a prerequisite for NVIDIA#2428 (WebSocket middleware), where long-lived bidirectional streams make a stale connection session-fatal rather than a single-retry inconvenience.

Related Issue

NVIDIA#2474

Changes

  • Added http2_keep_alive_interval, keep_alive_while_idle, keep_alive_timeout, and http2_adaptive_window to the Endpoint builder in crates/openshell-supervisor-middleware/src/remote.rs. The configuration is the same as in the crates/openshell-core/src/grpc_client.rs gRPC channel builder.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Summary by CodeRabbit

  • Bug Fixes
    • Improved connection reliability by configuring HTTP/2 keep-alive behavior and adaptive flow control.
    • Added timeout handling for idle connections.

@letv1nnn

Copy link
Copy Markdown
Owner Author

Review: does this solve NVIDIA#2474?

Yes for the core fix. The four keepalive settings requested in the issue's Proposed Fix are applied to the middleware Endpoint in remote.rs, and the values match the repo-standard channel in crates/openshell-core/src/grpc_client.rs:139-145 exactly:

  • http2_keep_alive_interval(10s)
  • keep_alive_while_idle(true)
  • keep_alive_timeout(20s)
  • http2_adaptive_window(true)

Placed on the builder before the TLS branch, so both http:// and https:// paths inherit it. Duration is already imported. This closes the primary gap: idle middleware channels now emit PING frames and detect dead peers proactively instead of failing on the next evaluation.

Gaps vs. the issue

  1. No tests, but the checklist claims otherwise. The issue's Testing section explicitly asks to "assert the constructed endpoint carries the expected keepalive settings," yet the diff adds zero test code. The PR checklist still checks [x] Unit tests added/updated. Either add the endpoint-assertion test (following the pattern used by openshell-core/openshell-sdk) or uncheck the box. Keepalive is hard to assert meaningfully through tonic's opaque Endpoint, so unchecking is a defensible call — but the checkbox should not claim tests that don't exist.

  2. plan.rs left untouched (acceptable). The issue flagged crates/openshell-gateway-interceptors/src/plan.rs connect_endpoint as having the same gap plus a missing connect timeout — still bare on this branch (Endpoint::from_shared(...).connect(), no keepalive, no connect_timeout). The issue said "consider," not "must," and keeping the PR scoped to the middleware channel matches the repo's scope-discipline guidance. Recommend a follow-up issue for plan.rs rather than expanding this PR.

Verdict

Correct, minimal, matches the house pattern. Solves the stated issue. Fix the test-checkbox claim before merge; file a follow-up for the plan.rs gap.

@letv1nnn
letv1nnn marked this pull request as draft August 4, 2026 10:38
@letv1nnn

letv1nnn commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 49 minutes.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1f66e6c0-c5d8-4f51-b74c-10ef40021aeb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Changes

Remote middleware connection

Layer / File(s) Summary
Configure HTTP/2 connection settings
crates/openshell-supervisor-middleware/src/remote.rs
RemoteMiddlewareService::connect configures HTTP/2 keep-alive behavior, keep-alive timeout, and adaptive flow-control windows before TLS and connection setup. Additional blank lines separate connection sections.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: derekwaynecarr, maxamillion, mrunalp

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: configuring HTTP/2 keepalive for the supervisor middleware gRPC channel.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-middleware-grpc-keepalive

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

@letv1nnn

letv1nnn commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
crates/openshell-supervisor-middleware/src/remote.rs (1)

33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a regression test for the endpoint settings.

The builder chain matches crates/openshell-core/src/grpc_client.rs:125-145 and covers both HTTP and HTTPS paths. Add an endpoint-configuration test for the four settings, or uncheck the completed unit-test checklist item.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-supervisor-middleware/src/remote.rs` around lines 33 - 37,
Add a regression test for the endpoint builder configuration in the relevant
remote client setup, covering http2_keep_alive_interval, keep_alive_while_idle,
keep_alive_timeout, and http2_adaptive_window for both HTTP and HTTPS paths,
matching the existing grpc_client endpoint test pattern; otherwise remove the
completed unit-test checklist indication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/openshell-supervisor-middleware/src/remote.rs`:
- Around line 33-37: Add a regression test for the endpoint builder
configuration in the relevant remote client setup, covering
http2_keep_alive_interval, keep_alive_while_idle, keep_alive_timeout, and
http2_adaptive_window for both HTTP and HTTPS paths, matching the existing
grpc_client endpoint test pattern; otherwise remove the completed unit-test
checklist indication.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1360a6e6-a9f2-4ceb-bd3e-b572cb12de57

📥 Commits

Reviewing files that changed from the base of the PR and between df69804 and dc3b339.

📒 Files selected for processing (1)
  • crates/openshell-supervisor-middleware/src/remote.rs

…gRPC channel

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn
letv1nnn force-pushed the fix-middleware-grpc-keepalive branch from dc3b339 to 5c87d01 Compare August 5, 2026 22:59
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