feat: migrate engine protocol to ConnectRPC - #4987
Open
abelanger5 wants to merge 4 commits into
Open
abelanger5 wants to merge 4 commits into
abelanger5 wants to merge 4 commits into
Conversation
Replace the google.golang.org/grpc server with connect handlers on net/http. Existing SDKs keep speaking gRPC over HTTP/2 unchanged; the same handlers now also answer gRPC-Web and the Connect protocol. Server (internal/services/grpc): - h2 over TLS, h2c when insecure, HTTP/1.1 for Connect and gRPC-Web; tls and mtls strategies use the same tls.Config as before - HTTP/2 settings carry over the grpc-go behaviour: unlimited concurrent streams, receive windows from grpcStaticStreamWindowSize, server pings every 30s in place of keepalive, no enforcement policy so the 10s client keepalive every SDK uses is tolerated - grpcMaxMsgSize applies to reads and sends - responses are compressed only when the request was, as with grpc-go - the OTLP TraceService Export method is mounted by hand - graceful shutdown drains, then closes after the shutdown timeout Middleware, same order as before: otel (otelconnect, trusting the remote parent as otelgrpc did), logging with the same messages, fields and levels, bearer auth, per-token rate limits, error normalisation, panic recovery, then the unary extension interceptors from ServerConfig.GRPCInterceptors. AddGRPCUnaryInterceptor and pkg/grpc/middleware.CallbackInterceptor keep their names and now carry connect.UnaryInterceptorFunc, so callers compile unchanged. grpc status errors returned by extensions keep their code and message. Services (dispatcher, admin, admin/v1, ingestor, otelcol) implement the generated connect handler interfaces (simple mode, so unary signatures are unchanged) and return connect errors with the same codes, messages and details. net/http panics on a stream write after its handler has returned, where grpc-go returned an error, and the engine sends on streams from other goroutines. Every streaming handler now sends through rpcstream.Sender, which rejects sends after the handler is done. grpc.PreparedMsg has no connect equivalent, so assigned actions are encoded inside Send. pkg/client is not migrated here and doubles as the compatibility probe: server_compat_test.go drives the server with grpc-go over h2c, TLS and mTLS. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Transport:
- a handler can always return: rpcstream.Sender interrupts a send that
is still blocked on a peer which stopped reading, one second after
Close, through the response write deadline
- a call's own timeout (grpc-timeout, Connect-Timeout-Ms) now bounds its
request body reads and response writes, as it did with grpc-go
- unknown services and methods answer gRPC callers with a trailers-only
Unimplemented status and grpc-go's messages instead of an HTTP 404
- the header list limit is back to grpc-go's 16 MiB
- the listener is HTTP/2 only again (h2, or unencrypted HTTP/2 when
insecure); Connect and gRPC-Web are served over HTTP/2
- gzip decompression stops at the message size limit plus one byte
- a nonpositive grpcMaxMsgSize means the 4 MiB default, not no limit
Middleware:
- authentication and rate limits run in a connect request gate, on the
request headers, before any body is read; rejected calls are still
logged with the usual fields
- rate limit rejections carry the historical go-grpc-middleware text
- grpc status errors from extensions keep their details, and a wrapped
coded error keeps the message grpc-go sent for it
- telemetry emits the span and metric names, attributes and status rule
of the otelgrpc stats handler it replaces; otelconnect is dropped
Regression tests are ported from the reviewers' probes. Reports:
docs/reviews/connectrpc-engine-{review,security}.md in the workstation.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+25
to
+27
| // The engine's gRPC server was instrumented by otelgrpc's stats handler. Spans and metrics keep | ||
| // its instrumentation scope, names, attributes and units so that existing dashboards, alerts | ||
| // and trace queries keep matching. |
There was a problem hiding this comment.
These comments describe the previous implementation using phrases such as “was instrumented” and “has always used.” The repository requires comments to explain the current invariant without referring to old behavior. Rewrite them in terms of the compatibility contract that the current code maintains. The same issue appears in internal/services/grpc/middleware/logging.go:17-18 and internal/services/grpc/server.go:340-343. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…he Go floor The point of the connect server is to let callers without gRPC reach the engine: fetch in serverless runtimes speaks HTTP/1.1 by default and cannot negotiate HTTP/2 over plaintext, and load balancers often speak HTTP/1.1 to the backend. Turn HTTP/1.1 back on for both listener modes and close the gap that made us turn it off: - an HTTP/1.1 request body has a read deadline (30s, or the call's own timeout if sooner), so a partial body no longer holds a connection; calls without a body get none, since net/http is already watching the connection and an expiring deadline would cancel them - an HTTP/1.1 unary response has a write deadline from its first byte; server streams stay unbounded - one middleware owns every transport deadline, including the ones derived from grpc-timeout and Connect-Timeout-Ms Unary and server-streaming procedures work over HTTP/1.1; bidirectional streams need HTTP/2 and are refused with 505. go 1.26.6 is the first 1.26 release with the fix for GO-2026-6089, the missing timeout while reading an unencrypted HTTP/2 preface. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
abelanger5
commented
Sep 21, 2026
Comment on lines
-125
to
-135
| _, encodeSpan := telemetry.NewSpan(ctx, "encode-action") | ||
|
|
||
| msg := &grpc.PreparedMsg{} | ||
| err := msg.Encode(worker.stream, action) | ||
| if err != nil { | ||
| encodeSpan.RecordError(err) | ||
| encodeSpan.End() | ||
| return fmt.Errorf("could not encode action: %w", err) | ||
| } | ||
|
|
||
| encodeSpan.End() |
Contributor
Author
There was a problem hiding this comment.
this was done to reduce the chance of flow control for large messages, but ended up not being the root cause of the issue -- the encoding time is very small relative to the network send time, which was what caused flow control
Comment on lines
-32
to
-33
| forbidden := status.Errorf(codes.Unauthenticated, "invalid auth token") | ||
| token, err := auth.AuthFromMD(ctx, "bearer") |
Contributor
Author
There was a problem hiding this comment.
existing clients are compatible, each SDK constructs authorization: bearer <token> and we only accept the first matched header
…ments as current rules - NewServer rejects a missing ingestor, dispatcher, v1 dispatcher, admin or v1 admin service up front instead of mounting whatever was passed; only the OTel collector stays optional - comments in the server and its middleware state the contract the code holds (log fields, telemetry names, compression rule, idle connections) rather than describing the previous implementation - the wire test for idempotency collisions uses the real IdempotencyCollisionError and BulkTriggerIdempotencyCollisionError details, decoded by a grpc-go client the way the SDKs decode them Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
abelanger5
changed the title
mrkaye97
approved these changes
Sep 21, 2026
mrkaye97
left a comment
mrkaye97
left a comment
Contributor
There was a problem hiding this comment.
I feel pretty good about this, should be easy enough to test on staging. there are no changes if we try to regenerate the SDKs right?
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.
Description
Migrates the existing gRPC server to use ConnectRPC instead, which has full compatibility with the existing gRPC setup. This is needed for broader API compatibility with serverless environments, and even frontends (with compatibility with
grpc-web) as we're working on more streaming features. This is a pre-requisite for the serverless-operator work to be completed.The migration is pretty straightforward here because ConnectRPC is generated from protos as well, and the handler signatures for unary RPCs are the same. The primary difference is the streaming RPCs and the middleware, which has been extensively tested.
Some explicit risks are:
http/2for keepalive functionality the migration to the http2 server should preserve the existing behavior.Type of change
Checklist
Changes have been:
🤖 AI Disclosure