fix: bound stalled writes on shared H2 connections - #30
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Shared connection failure and TLS timeout semantics warrant final human review.
Review effort: Lite
Findings: None
What changed in this PR
Adds configurable HTTP/2 physical-write timeouts to bound stalled shared connections and improve cleanup.
Changes:
- Wires timeout configuration through clients, CLI, and JSON.
- Adds lifecycle, TLS, recovery, and healthy-session tests.
- Documents timeout behavior and operational semantics.
| File | Description |
|---|---|
internal/tunnel/web_h2_write_timeout_test.go |
Tests stalled-write cleanup. |
internal/tunnel/web_h2_write_timeout_integration_test.go |
Tests TLS behavior and recovery. |
internal/tunnel/web_h2_client.go |
Configures and validates H2 write timeouts. |
internal/tunnel/web_client.go |
Propagates timeout to H2 fallback. |
docs/WEB_COVER.md |
Documents timeout behavior. |
docs/DEPLOYMENT.md |
Documents operational semantics. |
cmd/autocar/h2_write_timeout_test.go |
Tests CLI, JSON, defaults, and validation. |
cmd/autocar/common.go |
Adds CLI configuration and wiring. |
💡 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
http2.Transport.WriteByteTimeout, preventing an unread response close from waiting indefinitely behind a stalled connection writer.WebH2ClientConfig.WriteByteTimeout/WebClientConfig.H2WriteByteTimeoutand the shared CLI/JSONh2-write-timeoutsetting. Zero uses a 30-second default; negative durations are rejected. Native and H3 transports are unchanged.Regression evidence
net.Pipe.Writedeadline errors; the affected connection fails and a subsequent Dial authenticates a fresh session without manually changing client state.make check, fullmake race, repeated targeted race tests and independent review passed.6e6579ef037022ed7eff37b456eff873501d64ffpassed CI (all 11 jobs), CodeQL, and Linux namespace/netem. Docker integration and OCI export steps ran successfully; their logs and the netem success marker were checked. No open review threads were present at this verification.Semantics and scope
This is shared physical-write protection, not an idle timeout or a stream-local deadline. TLS write timeout can terminate all streams on that connection, including after partial network progress. Normal per-stream cancellation remains isolated. Stream Close can wait for the pending write budget and cleanup; this is bounded recovery, not an immediate-Close guarantee. The 30-second operational default is configurable, not a claim of a universally optimal value.
No dependency, wire authentication, congestion-controller, release or tag change. The controlled byte bridge is a deterministic correctness fixture, not an Internet throughput or browser-similarity experiment.
Merge receipt
Merged as
45bd8e54f38a846ded9144da0004a7acf1e19681. Its tree0e30f4f389f2b6916af0166060f26f236218ba34is identical to the tested PR head. Exact-main push CI (all 11 jobs), CodeQL, and Linux namespace/netem all passed. Main Docker integration, OCI export and netem logs were checked. Local main is synchronized; no release or tag was created.