fix: bound stalled TCP destination writes across relay transports - #19
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is consistently wired across transports, matches the documented semantics, and is backed by focused unit and integration tests for the key stall-reclamation and H3 edge cases.
Review effort: Lite
Findings: None
What changed in this PR
This PR introduces a server-side per-destination TCP write completion timeout that applies consistently across native TLS/QUIC and web H2/H3 relay transports, preventing stalled destination writes from indefinitely holding relay resources (including the HTTP/3 post-response-FIN reset edge case).
Changes:
- Add
DestinationWriteTimeoutplumbing from CLI/JSON → server configs → sharedserverCore, with0selecting a5mdefault and negative values rejected in both startup and--check. - Wrap newly dialed TCP destinations with a
net.Connimplementation that chunks writes (≤32 KiB), arms/clears per-write deadlines, and preventsio.Copyfast paths from bypassing the bound. - Add unit + integration tests and update documentation/examples to describe the new semantics and operational behavior.
| File | Description |
|---|---|
| internal/tunnel/web_server.go | Wires destination write timeout into combined web server core and H2/H3 configs. |
| internal/tunnel/web_handler.go | Wraps dialed web CONNECT TCP destinations to enforce bounded writes. |
| internal/tunnel/web_h3.go | Adds destination write timeout to H3 server config and core construction. |
| internal/tunnel/web_h2.go | Adds destination write timeout to H2 server config and core construction. |
| internal/tunnel/tls.go | Adds destination write timeout to TLS server config and core construction. |
| internal/tunnel/quic.go | Adds destination write timeout to QUIC server config and core construction. |
| internal/tunnel/common.go | Adds core default/validation for destination write timeout and wraps native destination conns. |
| internal/tunnel/destination_write.go | Implements the write-deadline-owning destination net.Conn wrapper with chunking. |
| internal/tunnel/destination_write_test.go | Unit tests for chunking, deadline behavior, partial errors, and close concurrency. |
| internal/tunnel/destination_write_integration_test.go | End-to-end tests ensuring stalled writes are reclaimed across transports and H3 edge case. |
| internal/tunnel/server_destination_timeout_test.go | Verifies configuration wiring and shared-core behavior across server constructors. |
| internal/tunnel/stream_admission_test.go | Updates constructor calls for the new core parameter. |
| cmd/autocar/server.go | Adds --destination-write-timeout flag and passes it into server constructors. |
| cmd/autocar/preflight.go | Extends local-limit validation to reject negative destination write timeout. |
| cmd/autocar/preflight_test.go | Adds preflight coverage for rejecting negative destination write timeout. |
| cmd/autocar/server_destination_timeout_test.go | Tests CLI/JSON typing, precedence, --check, and “server-only” behavior. |
| docs/WEB_COVER.md | Updates web-cover docs to describe bounded cleanup behavior and semantics. |
| docs/DEPLOYMENT.md | Documents configuration, semantics, and operational guidance for the new timeout. |
| examples/server.json | Adds "destination-write-timeout": "5m" to the example server config. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
--destination-write-timeout/ JSON"destination-write-timeout"defaults to5m; zero selects the same default and negative durations are rejected during startup and offline--check.io.Copyfast paths from bypassing the bound.Semantics
This is a per-write completion deadline, not a kernel-level no-progress timer or total connection/upload timeout. A partial write that returns an error remains an error. The current H3 API still does not expose immediate receive-side cancellation after response FIN; this change guarantees bounded stalled-write cleanup, not immediate reset detection. The wrapper owns the newly dialed destination's write-deadline policy.
Verification
Head:
2d7e066780ade4de7160905431a069ff085932f5.make check,make race,make stealth-tools-check, andgit diff --checkpassed.Exact-head GitHub CI, CodeQL, and Linux netem all passed, including Go 1.25.13/1.27.x tests, race detection, macOS tests, vulnerability scanning, four platform builds, container integration, and OCI image-index build. No unresolved review threads or requested changes at the pre-merge check. No tag or release is created.
Additional diagnostic boundary
A bounded optional native-QUIC throughput sanity is inconclusive: a 4 MiB upload completed in 2.54 s with the wrapper and 2.47 s with it disabled, but the subsequent download exhausted the same 12-second caller budget in both cases. This single-pair diagnostic neither proves throughput equivalence nor attributes the download timeout to this change; retain it for separate diagnosis. No other transport performance results are claimed.
Merged-main verification
Merged as
5fc741bed2c8483d67643fae8408a4dedef2dbf3; its tree exactly matches the tested head (fe7a6a02ca4146be393de4a0542cf3d563f83fe4). The exact merged main passed CI, CodeQL, and Linux netem, including the container integration step. No unresolved review threads at the final check. Local main is fast-forwarded and clean; no tag or release was created.