Skip to content

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

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

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)

…gRPC channel

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@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.

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