Add gRPC keepalive so idle connections survive a NAT/conntrack boundary - #35
Add gRPC keepalive so idle connections survive a NAT/conntrack boundary#35dkulp wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds gRPC keepalive configuration on both the client and server to prevent idle connections from being dropped by NAT/conntrack devices, which previously surfaced later as connection resets and caused fallback to local compilation.
Changes:
- Configure client-side gRPC keepalive (
WithKeepaliveParams) to ping on idle connections, including when there are no active RPCs. - Configure server-side gRPC keepalive parameters plus an enforcement policy that permits the client’s ping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/client/grpc-client.go | Adds gRPC client keepalive parameters (idle pings + PermitWithoutStream). |
| cmd/nocc-server/main.go | Adds gRPC server keepalive params and enforcement policy to accept client pings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| grpc.KeepaliveEnforcementPolicy(keepalive.EnforcementPolicy{ | ||
| MinTime: 10 * time.Second, // tolerate client pings as often as every 10s | ||
| PermitWithoutStream: true, // ...even with no active RPC | ||
| }), |
There was a problem hiding this comment.
This one is deliberate, and I have documented why at the new shared constant rather than leaving it looking arbitrary.
MinTime is the minimum interval the server will tolerate, measured against the interval it actually observes. The client pings on a 20s timer, but scheduling jitter, a loaded single-core client, or network delay can make a ping arrive fractionally early. Setting MinTime equal to the client cadence leaves zero margin and converts ordinary jitter into a too_many_pings GOAWAY, which is the exact failure this PR exists to prevent. Half the client interval is a conservative margin.
On the "protect against misconfigured clients" side: the cost of the slack is that a buggy client could ping twice as often as ours does, which is a couple of packets a minute per connection. That seemed clearly cheaper than risking the failure mode the tighter bound would reintroduce.
| grpc.WithKeepaliveParams(keepalive.ClientParameters{ | ||
| Time: 20 * time.Second, // ping after 20s of inactivity | ||
| Timeout: 10 * time.Second, // wait 10s for the ping ack | ||
| PermitWithoutStream: true, // ping even with no active RPC | ||
| }), |
There was a problem hiding this comment.
Agreed, and done. The values now live in internal/common/keepalive.go as shared constants used by both the client dial options and the server options, so the two cannot drift.
I kept them as constants rather than flags for now, since a mismatch is a correctness issue between the two sides rather than something an operator should be tuning independently. Happy to add flags on top if you would rather they be configurable.
Client and server had no keepalive configured, so an idle gRPC stream crossing a router/NAT/conntrack boundary (e.g. client on one subnet, helper on another) could be silently evicted. The next use surfaced as "connection reset by peer", after which the daemon marks that remote permanently unavailable and drops the rest of the build to slow local compilation -- brutal on single-core clients. Enable keepalive with PermitWithoutStream on both ends: on slow clients the connection sits idle *between* files with no active RPC, which is exactly when it got dropped. The server's EnforcementPolicy is widened to permit the client's ping cadence, otherwise it would answer the pings with a too_many_pings GOAWAY. The two sides have to stay compatible, so the values live in one place (internal/common/keepalive.go) rather than inline at each site. MinTime is deliberately half of the ping interval rather than equal to it: the server measures the interval it observes, so jitter, a loaded client or network delay can make a ping sent on a 20s timer arrive fractionally early, and matching the two exactly would turn ordinary jitter into a GOAWAY. Rollout note: upgrade servers before clients -- a new client pinging an old server (default 5-min MinTime) would trip too_many_pings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ba4a3c4 to
76e8d7a
Compare
The problem
Neither the client nor the server configures gRPC keepalive, so an idle stream crossing a router / NAT / conntrack boundary — client on one subnet, helper on another — can be silently evicted. The next use surfaces as
connection reset by peer, after which the daemon marks that remote permanently unavailable and drops the rest of the build to local compilation.PermitWithoutStreamis the important part: on a slow client the connection sits idle between files, with no active RPC, which is exactly when it gets dropped.The change
internal/client/grpc-client.go):WithKeepaliveParams— ping after 20s of inactivity, 10s ack timeout,PermitWithoutStream: truecmd/nocc-server/main.go): matchingKeepaliveParams, plus aKeepaliveEnforcementPolicywithMinTime: 10sandPermitWithoutStream: trueThe enforcement policy is required: gRPC's server default is
MinTime: 5 * time.MinutewithPermitWithoutStream: false, so without widening it the server answers the client's idle-connection pings with atoo_many_pingsGOAWAY — the exact failure this is meant to fix.Compatibility note, please read
Upgrade servers before clients. A new client pinging an old server (default 5-minute
MinTime) will triptoo_many_pings. Client and server are already expected to be the same build in practice, but the ordering matters during a rollout.I'm happy to make the intervals configurable, or gate them behind a flag, if you'd rather not change the defaults for everyone. The values above are conservative but arbitrary.
Unlike the other three PRs I've opened, this one is a behaviour change rather than a plain bug fix, so it seemed worth keeping separate.
Testing
gofmt/go vetclean; thetests/suite gives the same results as master on my machine. Note that suite is short-lived, so it does not exercise the idle path this targets — the real evidence is operational: this was pulled from a fork where BeagleBone clients on a different subnet from the helper were losing remotes mid-build, and it stopped after this change.