Repository navigation
fix(dataplane): bound development listener resources - #8
Merged
Merged
Conversation
Restrict the development listener to loopback and bound request handling, body-read waits, and upstream response headers while preserving streaming. Document the defaults and review evidence.
Reviewer's GuideThe PR hardens the development dataplane by enforcing loopback-only listeners, bounding concurrency, request-body idle reads, upstream response-header waits, and header sizes, while deliberately preserving streaming, graceful shutdown, and HTTP upgrade/hijack behavior; documentation and extensive regression/evidence tests define the defaults and scope. Sequence diagram for bounded request handling and streamingsequenceDiagram
participant Client
participant Server
participant Limits as ResourceLimitHandler
participant Proxy
participant Upstream
Client->>Server: HTTP request
Server->>Limits: ServeHTTP
alt concurrency limit reached
Limits-->>Client: HTTP 503 server busy
Limits-->>Client: Connection close
else handler admitted
Limits->>Limits: Install 30s request-body read idle timeout
Limits->>Proxy: ServeHTTP
Proxy->>Upstream: Forward streamed request
Upstream-->>Proxy: Response headers within 30s
Proxy-->>Client: Stream response headers and body
Limits->>Limits: Release handler slot
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/dataplane/server.go" line_range="251-252" />
<code_context>
+ return
+ }
+ body.finished = true
+ _ = body.controller.SetReadDeadline(time.Now())
+ body.mu.Unlock()
+}
+
</code_context>
<issue_to_address>
**Full-duplex uploads stop early**
When a handler enables full duplex and writes or flushes before consuming the request body, `requestBodyDeadlineWriter.WriteHeader`, `Write` and `Flush` call `finishHandler`, which marks the body finished and sets its read deadline to now. `idleTimedRequestBody.Read` then returns `http.ErrBodyReadAfterClose` or is interrupted, so the handler cannot finish receiving the upload.
Keep request-body reads available after a full-duplex handler writes or flushes a response.
Also at `internal/dataplane/server.go:268-279`.
</issue_to_address>
### Comment 2
<location path="internal/dataplane/server.go" line_range="302-314" />
<code_context>
+func (body *idleTimedRequestBody) Close() error {
+ body.mu.Lock()
+ var deadlineErr error
+ if body.hijacking || body.hijacked {
+ body.mu.Unlock()
+ return nil
+ }
+ if !body.finished {
+ deadlineErr = body.controller.SetReadDeadline(time.Now().Add(body.timeout))
+ }
+ body.mu.Unlock()
+ closeErr := body.ReadCloser.Close()
+ body.mu.Lock()
+ if !body.finished && !body.hijacking && !body.hijacked {
+ body.clearDeadline()
+ }
+ body.mu.Unlock()
</code_context>
<issue_to_address>
**Hijack disrupts active body reads**
When a handler hijacks while another goroutine is reading the request body, `finishHijack` marks the body hijacked and clears the read deadline while `activeReads` is nonzero, leaving the read unbounded or letting it consume upgraded-protocol bytes and disrupt connection ownership.
Coordinate hijacking with active body reads so no read remains active when the connection is transferred and its deadline is cleared.
</issue_to_address>
### Comment 3
<location path="internal/dataplane/server.go" line_range="152-155" />
<code_context>
+ h.handler.ServeHTTP(w, r)
+ return
+ }
+ wrappedWriter := &requestBodyDeadlineWriter{ResponseWriter: w, body: body}
+ if _, ok := w.(http.Hijacker); ok {
+ h.handler.ServeHTTP(&hijackableRequestBodyDeadlineWriter{requestBodyDeadlineWriter: wrappedWriter}, r)
+ return
+ }
+ h.handler.ServeHTTP(wrappedWriter, r)
</code_context>
<issue_to_address>
**Disconnect notifications disappear**
When a body-bearing HTTP/1 request reaches a handler that relies on `http.CloseNotifier`, `requestBodyDeadlineWriter` does not forward `http.CloseNotifier`, so the handler's assertion fails or it receives no disconnect notification. The handler can keep work running after the client disconnects.
Preserve `http.CloseNotifier` for body-bearing requests when the underlying writer supports it.
</issue_to_address>
### Comment 4
<location path="internal/dataplane/server.go" line_range="145" />
<code_context>
+ } else if r.ProtoMajor == 1 {
+ w.Header().Set("Connection", "close")
+ }
+ http.Error(w, "server busy", http.StatusServiceUnavailable)
+ return
+ }
</code_context>
<issue_to_address>
**Overload rejections are invisible**
When all request-handler slots are occupied, the full-slot branch returns HTTP 503 without logging or recording the rejection, and the request never reaches the proxy’s completion logging, so operators have no built-in signal when load shedding occurs.
Record overload rejections through a logger or metric before returning 503.
</issue_to_address>
### Comment 5
<location path="internal/dataplane/resource_limits_test.go" line_range="131" />
<code_context>
+ result <- err
+ }()
+ for _, part := range []string{"pro", "gre", "ssi", "ve"} {
+ time.Sleep(70 * time.Millisecond)
+ if _, err := io.WriteString(pipeWriter, part); err != nil {
+ t.Fatal(err)
</code_context>
<issue_to_address>
**Progressive upload test flakes**
When the test process or server goroutine is descheduled for more than the remaining 50 ms between chunks, the test sleeps 70 ms between writes against a 120 ms idle timeout, leaving only 50 ms of scheduling margin. The timeout fires on a valid upload, so the test fails intermittently even when the implementation is correct.
Avoid relying on a 50 ms scheduling margin between writes, for example by reducing the delay or increasing the test timeout.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and this changes an unauthenticated listener trust boundary and adds concurrency, body-read, and upstream-header thresholds; if those decisions or implementations are wrong, requests could be exposed, rejected, or timed out before a revert. Reverting restores the previous behavior, but any exposure or outage that already occurred is not undone.
Blocking findings: internal/dataplane/server.go:252, internal/dataplane/server.go:314, internal/dataplane/server.go:155, internal/dataplane/server.go:145, internal/dataplane/resource_limits_test.go:131
Comment on lines
+251
to
+252
| _ = body.controller.SetReadDeadline(time.Now()) | ||
| body.mu.Unlock() |
There was a problem hiding this comment.
🟡 Medium · Full-duplex uploads stop early
When a handler enables full duplex and writes or flushes before consuming the request body, requestBodyDeadlineWriter.WriteHeader, Write and Flush call finishHandler, which marks the body finished and sets its read deadline to now. idleTimedRequestBody.Read then returns http.ErrBodyReadAfterClose or is interrupted, so the handler cannot finish receiving the upload.
Keep request-body reads available after a full-duplex handler writes or flushes a response.
Also at internal/dataplane/server.go:268-279.
Prompt for AI agents
In `internal/dataplane/server.go` at lines 251-252:
**Full-duplex uploads stop early**
When a handler enables full duplex and writes or flushes before consuming the request body, `requestBodyDeadlineWriter.WriteHeader`, `Write` and `Flush` call `finishHandler`, which marks the body finished and sets its read deadline to now. `idleTimedRequestBody.Read` then returns `http.ErrBodyReadAfterClose` or is interrupted, so the handler cannot finish receiving the upload.
Keep request-body reads available after a full-duplex handler writes or flushes a response.
Also at `internal/dataplane/server.go:268-279`.
Comment on lines
+302
to
+314
| if body.hijacking || body.hijacked { | ||
| return false | ||
| } | ||
| body.hijacking = true | ||
| return true | ||
| } | ||
|
|
||
| func (body *idleTimedRequestBody) finishHijack(succeeded bool) { | ||
| body.mu.Lock() | ||
| body.hijacking = false | ||
| if succeeded { | ||
| body.hijacked = true | ||
| body.clearDeadline() |
There was a problem hiding this comment.
🟡 Medium · Hijack disrupts active body reads
When a handler hijacks while another goroutine is reading the request body, finishHijack marks the body hijacked and clears the read deadline while activeReads is nonzero, leaving the read unbounded or letting it consume upgraded-protocol bytes and disrupt connection ownership.
Coordinate hijacking with active body reads so no read remains active when the connection is transferred and its deadline is cleared.
Prompt for AI agents
In `internal/dataplane/server.go` at lines 302-314:
**Hijack disrupts active body reads**
When a handler hijacks while another goroutine is reading the request body, `finishHijack` marks the body hijacked and clears the read deadline while `activeReads` is nonzero, leaving the read unbounded or letting it consume upgraded-protocol bytes and disrupt connection ownership.
Coordinate hijacking with active body reads so no read remains active when the connection is transferred and its deadline is cleared.
Comment on lines
+152
to
+155
| wrappedWriter := &requestBodyDeadlineWriter{ResponseWriter: w, body: body} | ||
| if _, ok := w.(http.Hijacker); ok { | ||
| h.handler.ServeHTTP(&hijackableRequestBodyDeadlineWriter{requestBodyDeadlineWriter: wrappedWriter}, r) | ||
| return |
There was a problem hiding this comment.
🟡 Medium · Disconnect notifications disappear
When a body-bearing HTTP/1 request reaches a handler that relies on http.CloseNotifier, requestBodyDeadlineWriter does not forward http.CloseNotifier, so the handler's assertion fails or it receives no disconnect notification. The handler can keep work running after the client disconnects.
Preserve http.CloseNotifier for body-bearing requests when the underlying writer supports it.
Prompt for AI agents
In `internal/dataplane/server.go` at lines 152-155:
**Disconnect notifications disappear**
When a body-bearing HTTP/1 request reaches a handler that relies on `http.CloseNotifier`, `requestBodyDeadlineWriter` does not forward `http.CloseNotifier`, so the handler's assertion fails or it receives no disconnect notification. The handler can keep work running after the client disconnects.
Preserve `http.CloseNotifier` for body-bearing requests when the underlying writer supports it.| } else if r.ProtoMajor == 1 { | ||
| w.Header().Set("Connection", "close") | ||
| } | ||
| http.Error(w, "server busy", http.StatusServiceUnavailable) |
There was a problem hiding this comment.
🟡 Medium · Overload rejections are invisible
When all request-handler slots are occupied, the full-slot branch returns HTTP 503 without logging or recording the rejection, and the request never reaches the proxy’s completion logging, so operators have no built-in signal when load shedding occurs.
Record overload rejections through a logger or metric before returning 503.
Prompt for AI agents
In `internal/dataplane/server.go` at line 145:
**Overload rejections are invisible**
When all request-handler slots are occupied, the full-slot branch returns HTTP 503 without logging or recording the rejection, and the request never reaches the proxy’s completion logging, so operators have no built-in signal when load shedding occurs.
Record overload rejections through a logger or metric before returning 503.| result <- err | ||
| }() | ||
| for _, part := range []string{"pro", "gre", "ssi", "ve"} { | ||
| time.Sleep(70 * time.Millisecond) |
There was a problem hiding this comment.
🟡 Medium · Progressive upload test flakes
When the test process or server goroutine is descheduled for more than the remaining 50 ms between chunks, the test sleeps 70 ms between writes against a 120 ms idle timeout, leaving only 50 ms of scheduling margin. The timeout fires on a valid upload, so the test fails intermittently even when the implementation is correct.
Avoid relying on a 50 ms scheduling margin between writes, for example by reducing the delay or increasing the test timeout.
Prompt for AI agents
In `internal/dataplane/resource_limits_test.go` at line 131:
**Progressive upload test flakes**
When the test process or server goroutine is descheduled for more than the remaining 50 ms between chunks, the test sleeps 70 ms between writes against a 120 ms idle timeout, leaving only 50 ms of scheduling margin. The timeout fires on a valid upload, so the test fails intermittently even when the implementation is correct.
Avoid relying on a 50 ms scheduling margin between writes, for example by reducing the delay or increasing the test timeout.
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.
Restrict the development listener to loopback and bound request handling, body-read waits, and upstream response headers while preserving streaming. Document the defaults and review evidence.
Summary by Sourcery
Bound development listener resources and document the resulting defaults and verification coverage.
Bug Fixes:
Enhancements: