Skip to content

fix: bound stalled TCP destination writes across relay transports - #19

Merged
cppla merged 1 commit into
mainfrom
codex/destination-write-timeout
Sep 22, 2026
Merged

cppla merged 1 commit into
mainfrom
codex/destination-write-timeout

Conversation

@cppla

@cppla cppla commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Add a server-side TCP destination write completion budget across native QUIC/TLS and web H2/H3.
  • --destination-write-timeout / JSON "destination-write-timeout" defaults to 5m; zero selects the same default and negative durations are rejected during startup and offline --check.
  • Split destination writes into at most 32 KiB operations, arm and clear deadlines around each operation, preserve partial-write errors, and prevent io.Copy fast paths from bypassing the bound.
  • Reclaim stalled destination writes and stream slots even after an H3 response FIN followed by a stream reset, without closing usable siblings. Preserve normal half-closes, idle reads, and long uploads that make successful write progress.
  • UDP and public cover handlers are unchanged; no new dependencies.

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.

  • Final local make check, make race, make stealth-tools-check, and git diff --check passed.
  • Real client/server regressions cover QUIC/TLS/H2/H3 stalled writes and H3 post-FIN reset cleanup, typed timeout, admission recovery, and sibling survival. Targeted race runs repeated five times passed.
  • The wrapper-disabled negative control fails all five stalled-write scenarios; it retains the new configuration/testing interface and isolates the new behavior.
  • Normal H3 response FIN followed by idle time and a longer upload succeeds. Unit tests cover fast-path bypass, large and short writes, partial errors, deadline cleanup, concurrent Close, and continued reads/writes after idle.
  • CLI/JSON precedence, strict duration typing, server-only configuration, five constructor entry points, and combined H2/H3 shared configuration verified.
  • Independent implementation/configuration review and scoped secrets/artifacts audit passed.

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.

Copilot AI lite review requested due to automatic review settings September 22, 2026 13:15
@cppla
cppla merged commit 5fc741b into main Sep 22, 2026
14 checks passed

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.

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 DestinationWriteTimeout plumbing from CLI/JSON → server configs → shared serverCore, with 0 selecting a 5m default and negative values rejected in both startup and --check.
  • Wrap newly dialed TCP destinations with a net.Conn implementation that chunks writes (≤32 KiB), arms/clears per-write deadlines, and prevents io.Copy fast 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.

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