Repository navigation
Conversation
…ot shipped) Implement `serve` in the Go port on net/http as it is (D28): the four endpoints with the Python service's bodies and error envelopes, a 64-request in-flight limit, the 1 MiB body limit, server timeouts, panic recovery and a 0.5 s drain on SIGTERM/SIGINT. Add structured JSON logs on stderr and a `healthcheck` subcommand that doesn't load tables. Record the net/http transport behavior the port takes in the parity decisions table and document serve, healthcheck and the log format in the router README. The image still runs the Python service. Part of #10261. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Transport and privacy guarantees still need verification with the pinned Go toolchain.
3 open issues
What changed in this PR
Adds an HTTP adapter to the router’s existing Go core while leaving the production image on Python.
Changes:
- Implements HTTP endpoints, request limits, timeouts, startup validation and graceful shutdown.
- Adds structured JSON logging and a standalone healthcheck command.
- Adds HTTP, privacy and lifecycle tests, plus Go-port documentation.
| File | Description |
|---|---|
containers/router/tests/fixtures/parity/README.md |
Records transport differences from Python. |
containers/router/README.md |
Documents Go serving, logging and healthchecks. |
containers/router/go/server.go |
Implements HTTP handling and safeguards. |
containers/router/go/server_test.go |
Tests endpoints, limits, errors and privacy. |
containers/router/go/serve.go |
Implements startup, bind validation and healthchecks. |
containers/router/go/serve_test.go |
Tests configuration, startup and shutdown. |
containers/router/go/logging.go |
Provides JSON logging and sanitization helpers. |
containers/router/go/logging_test.go |
Tests logging format and privacy helpers. |
containers/router/go/cli.go |
Adds serve and healthcheck commands. |
containers/router/go/cli_test.go |
Updates CLI coverage and shutdown-test support. |
Review effort: Max
| envelopes of the Python service: | ||
|
|
||
| - At most 64 requests are in flight, `/healthz` included. Further requests get a bare 503. | ||
| - A declared or streamed body over 1,048,576 bytes gets 413 `payload_too_large`. |
Comment on lines
+825
to
+827
| if strings.Contains(string(r.body), canary) || strings.Contains(strings.Join(slices.Collect(maps.Keys(r.header)), ","), canary) { | ||
| t.Errorf("%s %s echoes the canary: %s", method, target, r.body) | ||
| } |
| | `serve` startup errors | Plain one-line text | JSON log records; other commands keep plain text | | ||
| | Trailing slash | Starlette redirects `/healthz/` to `/healthz` with 307 | 404 `not_found`, like any unknown path | | ||
| | Unsupported transfer coding | Uvicorn answers a bare 400 | Go's HTTP/1.1 server answers a bare 501, as RFC 9112 recommends | | ||
| | Paths that aren't clean, such as `//healthz` and `/a/../healthz` | 404 `not_found` | `ServeMux` redirects to the cleaned path with 307 | |
This branch has not been deployed
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
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.

Closes #10326
Part of #10261 (step 3 of 7).
Summary
This PR adds the Go router's
servecommand: the four HTTP endpoints onnet/http, structured JSON logs on stderr, and ahealthchecksubcommand that the Go image will use instead of Python. It follows D28 in the port plan: usenet/httpas it is, and record where its transport behavior differs from the Python service.Nothing ships yet. The image still runs Python.
Changes
containers/router/go/(about 540 lines, standard library only):server.go: the handler for/healthz,/capabilities,/v1/routeand/v1/classify, with the Python service's success bodies and error envelopes. It also holds the limits: 64 requests in flight (/healthzincluded, then a bare 503), a 1,048,576-byte body limit (413payload_too_large), and the server timeouts. A handler panic becomes a 500internal_error, or an aborted connection if the response has already started.serve.go: startup (load and validate the tables, check the bind address, listen), and shutdown on SIGTERM or SIGINT with a 0.5 s drain before closing the remaining connections.logging.go: one JSON record per line with a fixed field set, and an allowlist so request data never reaches a log record.cli.go: theserveoptions and environment variables shared with the Python CLI, and thehealthcheck [--url URL]command (exit 0 only on a 204, 1 s timeout, no proxy, no redirects).test_http.pyand of theservetests intest_cli.py: the endpoints and envelopes, content-type and method handling, body limit (declared and streamed), the concurrency limit, panics, the bind and log-level validation, startup failures and their exit codes, and shutdown that drains or aborts an in-flight request.serve(limits, timeouts, shutdown, the log events table and what's never logged) andhealthcheck, and says thatdifferential.pydoesn't compare HTTP;CI already runs the Go job (gofmt, vet, tests, with
-raceandgovulncheckon Linux) from #10313, sotest-router.ymldoesn't change.Decisions for maintainers
These rows are added to the decisions table in
tests/fixtures/parity/README.md, and merging this PR approves them. All arenet/httpbehavior that the port keeps rather than reimplements://healthz, get a 307 redirect to the clean path instead of a 404.request_timeout. Server timeouts (10 s for headers, 30 s for the request, 30 s for the response) close the connection instead.openapi.yamldrops the 504 at the cut-over.OPTIONS *gets an empty 200 instead of a 404.GET http://host/healthzis routed by its path instead of getting a 404.Notes for reviewers
requestrecord, because no status was sent. Python logs nothing structured for this either, and the api-proxy logs its own side. A slow handler isn't an abort: if the api-proxy gives up while the router is still computing, the request is still logged. The OpenTelemetry step can surface aborts as error spans.requestrecord. D28 dropped the rate-limited overload log, so a burst of overload produces a burst of log lines.Validation
In
containers/router, with Go 1.27.2 and Python 3.12 with the locked dependencies:gofmt -l,go vetandgo testpass ingo/; statement coverage is 96%. The uncovered lines are mostly unreachable encoding-failure branches and the Linux-only SIGTERM name.gofmt -l,go vet,go testandgo test -racepass, including the real SIGTERM test.pytest: 1,180 passed.ruff check,ruff format --checkandpython tests/parity_corpus.py --checkpass. The Python code is unchanged; these cover the README files that the Python tests read.govulncheckwasn't run locally; CI runs it. The module has no dependencies outside the standard library.