Skip to content

Fix HTTP client, binary protocol, and graceful shutdown issues - #89

Merged
fujiwara merged 6 commits into
v2from
fix-review-findings
Jun 11, 2026
Merged

Fix HTTP client, binary protocol, and graceful shutdown issues#89
fujiwara merged 6 commits into
v2from
fix-review-findings

Conversation

@fujiwara

@fujiwara fujiwara commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

What

Fixes for issues found in a code review:

  1. HTTPClient ignored pathPrefix: NewHTTPClient did not assign the parameter, so clients could not reach servers configured with HTTPPathPrefix.
  2. HTTPClient mutated shared URLs: Fetch/FetchMulti rewrote the shared *url.URL in place, a data race under concurrent calls, and the n= query set by FetchMulti leaked into subsequent Fetch requests. Now operates on a copy.
  3. GET /ids accepted invalid n: a negative n caused a panic by make() with a negative capacity, and n=0 returned an empty response. n out of 1-1000 is now rejected with 400.
  4. Binary protocol error responses: a failed binary GET responded with the text protocol ERROR line, which binary clients cannot parse. Error responses are now proper binary headers and echo the requested opcode/opaque. Also fixed a meaningless short write check.
  5. Binary request body size limit and slicing overflow: a malformed header with bodyLen=0xFFFFFFFF could trigger a 4GiB allocation per request, and keyLen+extraLen overflowing uint16 caused a panic that crashed the process. Bodies are now limited to 64KB and the index math is done in int.
  6. Graceful shutdown: the HTTP server passed the already-canceled context to Shutdown (returning immediately without draining), and the gRPC server used Stop which kills in-flight RPCs. Both now drain in-flight requests with a 10s timeout (ShutdownTimeout), and RunHTTPServer/RunGRPCServer wait until the shutdown completes.
  7. Invalid environment variable values: values like KATSUBUSHI_PORT=abc were silently ignored. The process now exits with an error, consistent with how flag.Parse handles invalid commandline values.

Behavior changes

  • GET /ids?n=0 returned 200 with an empty body, now returns 400.
  • Binary protocol error responses now echo the requested opcode instead of a fixed GET opcode.
  • Startup fails on invalid environment variable values instead of silently using the default.

🤖 Generated with Claude Code

fujiwara and others added 5 commits June 11, 2026 22:38
NewHTTPClient did not assign the pathPrefix parameter, so clients
could not reach servers configured with HTTPPathPrefix.

Fetch and FetchMulti mutated the shared *url.URL in place, which
was a data race under concurrent calls and leaked the query
parameter set by FetchMulti into subsequent Fetch requests.
Operate on a copy of the URL instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
A negative n caused a panic by make() with a negative capacity,
and n=0 returned an empty response. Reject n out of the range
1-1000 with 400 Bad Request.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
- Respond to a failed binary GET with a binary error response
  instead of the text protocol "ERROR" line
- Echo the requested opcode and opaque in error responses
- Fix the short write check in writeBinaryError which compared
  against the length of the text error response
- Reject binary requests with a body larger than 64KB to prevent
  a huge allocation by a malformed header

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
The HTTP server passed the already-canceled context to Shutdown,
so it returned immediately without draining in-flight requests.
The gRPC server used Stop which kills in-flight RPCs. Use a new
context with ShutdownTimeout for HTTP and GracefulStop with a
timeout fallback for gRPC, and make RunHTTPServer/RunGRPCServer
wait until the shutdown completes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
Invalid values such as KATSUBUSHI_PORT=abc were silently ignored
and the process started with the default value. Exit with an
error instead, consistent with how flag.Parse handles invalid
commandline values.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>

Copilot AI 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.

Pull request overview

This PR addresses several correctness and operational issues across the HTTP client/server, binary (memcached) protocol handling, graceful shutdown behavior, and configuration via environment variables.

Changes:

  • Fix HTTP client path prefix handling and eliminate shared url.URL mutation in Fetch/FetchMulti; add related tests and validate GET /ids?n= range (1–1000).
  • Improve graceful shutdown behavior for HTTP and gRPC servers with a bounded drain timeout and completion waiting.
  • Harden binary protocol error responses and add a request body size limit; add tests for error responses and oversized bodies; make invalid env var values fail fast.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
README.md Documents the n parameter constraints for GET /ids.
http.go Adds graceful HTTP shutdown waiting, validates n, fixes HTTP client pathPrefix usage and URL mutation.
http_test.go Adds tests for invalid n, pathPrefix handling, and URL isolation/concurrency behavior.
grpc.go Adds graceful gRPC shutdown with timeout and waits for shutdown completion.
cmd/katsubushi/main.go Makes invalid env var values for flags fail fast (exit code 2) via applyEnvToFlag.
cmd/katsubushi/main_test.go Adds test coverage for invalid env var flag values.
binary_protocol.go Adds binary request body size limit and makes binary error responses echo opcode/opaque.
binary_protocol_test.go Adds tests for too-large binary request bodies and binary error response shape.
app.go Introduces ShutdownTimeout constant used by HTTP/gRPC graceful shutdown paths.
app_test.go Updates binary error response expectations to echo requested opcode.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread binary_protocol.go
Comment on lines 83 to +87
if bodyLen < uint32(keyLen)+uint32(extraLen) {
return nil, fmt.Errorf("total body %d is too small. key length: %d, extra length %d", bodyLen, keyLen, extraLen)
}
if bodyLen > maxBinaryBodyLen {
return nil, fmt.Errorf("total body %d is too large. limit: %d", bodyLen, maxBinaryBodyLen)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the overflow was reachable (keyLen=65535, extraLen=1 passes both length checks and panics on slicing, crashing the process since there is no recover in the connection goroutine). Fixed in 0d53ff5 by calculating the indexes in int, with a regression test that reproduces the panic on the old code. Kept keys over 250 bytes accepted since katsubushi ignores key contents.

keyLen + extraLen was calculated in uint16, so crafted headers
with keyLen+extraLen >= 65536 wrapped around and caused a panic
by slicing with out-of-range bounds, crashing the process.
Calculate the indexes in int.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
@fujiwara
fujiwara merged commit a526b23 into v2 Jun 11, 2026
4 checks passed
@fujiwara
fujiwara deleted the fix-review-findings branch June 11, 2026 14:02
@github-actions github-actions Bot mentioned this pull request Jun 11, 2026
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.

2 participants