fix: keep idle traffic from collapsing adaptive pacing - #20
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the datagram deadline checks and clarify the cumulative-idle architecture wording.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR updates native QUIC adaptive pacing to account for connection-wide application-send idle time, preventing ACK/control traffic from appearing as low outbound capacity.
Changes:
- Tracks stream and datagram write activity across pacing and transport waits.
- Filters predominantly idle adaptive samples while preserving active behavior.
- Adds regression coverage and documentation.
| File | Summary | Final review note |
|---|---|---|
internal/tunnel/quic.go |
Tracks stream write activity. | — |
internal/tunnel/pacing.go |
Tracks activity and observes idle time. | — |
internal/tunnel/pacing_write_activity_test.go |
Tests stream activity lifecycle. | — |
internal/tunnel/pacing_idle_integration_test.go |
Adds loopback QUIC regression coverage. | — |
internal/tunnel/pacing_datagram_activity_test.go |
Tests datagram activity and cancellation. | — |
internal/tunnel/pacing_activity_test.go |
Tests shared idle accounting. | — |
internal/tunnel/datagram.go |
Tracks datagram batch activity. | Moderate: use contextError for both deadline guards. |
internal/accel/pacing_idle_test.go |
Tests idle filtering and recovery. | — |
internal/accel/pacer.go |
Adds cumulative idle-aware filtering. | — |
docs/ARCHITECTURE.md |
Documents native pacing behavior. | Nit: clarify that idle time is cumulative across a stable window. |
docs/ACCELERATION.md |
Documents idle-filter semantics. | — |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+998
to
+1000
| if err := context.Cause(queued.ctx); err != nil { | ||
| return err | ||
| } |
Comment on lines
+209
to
+210
| Intervals that are at least half idle, with a stable sampling window of no | ||
| pending sends, are rebaselined instead of turning ACK-only traffic into a low |
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
Fix native QUIC adaptive pacing treating ACK/control traffic during application-send silence as low outbound capacity. This was reproducible when receiving an upload and then sending a download on the same connection.
Verification
make checkandmake race: passed on the final source.make stealth-tools-check: passed; no capture campaign was run.Limits
The last item is one local diagnostic, not a throughput benchmark. Download still slowed toward the end; healthy overall adaptive throughput is not established. The focused integration fixture exercises the real QUIC adapter/controller, not authentication/serverCore. No passive-traffic, browser-equivalence, remote-host, or comparative-performance conclusion is claimed. Raw diagnostics remain local and are not included in this PR.
Exact-head CI
Head
0d7637ab6035c9489e18e9961fef40a4b02c7ab7:Independent code, scope and secret reviews found no remaining blocker.
Post-merge verification
Merged as
57e431f495444147b7e6d048ad3c6bdb586e0217. The merge tree exactly matches the verified PR head (9652abd909ba7e99076897814eb321666f57b920).The local checkout is synchronized with main. No tag or release was created.