fix(client): prevent resource exhaustion from long-lived DNS sessions#2724
Open
fix(client): prevent resource exhaustion from long-lived DNS sessions#2724
Conversation
…ssions DNS is one-shot (one query, one response), but each forwarded DNS query was holding a transport session open until the 30-second write-idle timeout. Under sustained DNS load this causes session accumulation and eventual resource exhaustion, breaking VPN connectivity. Two fixes: - forward: close the transport session immediately after delivering the first DNS response, instead of waiting for the idle timeout. - truncate: create the base transport session lazily, only on the first non-DNS packet. DNS-only flows never open a base session at all. Fixes #2679 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fortuna
changed the title
Open
1 task
fortuna
commented
Mar 14, 2026
fortuna
commented
Mar 14, 2026
Includes explanations of the PacketProxy/PacketRequestSender/ PacketResponseReceiver abstractions, forward and truncate modes, dynamic switching, and Mermaid diagrams for each. Also adds field comments to forwardPacketRespReceiver and truncatePacketReqSender per code review feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes transport session buildup caused by DNS-over-UDP interception by ensuring DNS-only traffic does not keep long-lived underlying transport sessions open, preventing resource exhaustion and VPN connectivity failures (see #2679).
Changes:
- Forward mode: close the underlying UDP transport session immediately after the first DNS response is delivered.
- Truncate mode: lazily create the base transport session only when the first non-DNS packet is sent (DNS-only flows never create it).
- Add targeted regression tests and package-level documentation explaining the DNS interception model.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
client/go/outline/dnsintercept/forward.go |
Closes the underlying session after first resolver response (adds sync.Once-guarded close). |
client/go/outline/dnsintercept/forward_test.go |
Adds assertions/tests for early-close behavior and non-DNS response behavior. |
client/go/outline/dnsintercept/truncate.go |
Defers base session creation until first non-DNS packet; adds locking to manage lazy init. |
client/go/outline/dnsintercept/truncate_test.go |
Adds DNS-only regression test ensuring no base session allocation occurs. |
client/go/outline/dnsintercept/README.md |
New documentation describing abstractions, modes, and dynamic switching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Use <br/> for line breaks (not \n) and replace subgraph with a hexagon node to avoid label overlap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- forward: guard sender field with a mutex to satisfy the Go memory model. The reader goroutine started by base.NewSession is launched before sender is assigned, so without synchronization the write is not guaranteed to be visible to WriteFrom. - truncate: use errors.Join to propagate errors from both base.Close and trunc.Close instead of silently dropping the trunc error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If a DNS response arrives before NewSession sets wrapper.sender, once.Do would fire with sender == nil, skip the close, and never run again — leaking the session until the idle timeout. Fix: record pendingClose = true inside once.Do when sender is nil. NewSession checks this flag immediately after setting sender and closes it if needed. The mutex guarantees the two critical sections are ordered: either sender is visible to once.Do, or pendingClose is visible to NewSession. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses transport session accumulation caused by UDP DNS interception by ensuring DNS-related sessions don’t stay open longer than necessary, reducing the risk of resource exhaustion under sustained DNS load in the Outline client.
Changes:
- Forward mode: close the underlying transport session immediately after delivering the first DNS response.
- Truncate mode: lazily create the base transport session only when the first non-DNS packet is observed (DNS-only flows never allocate it).
- Add/extend tests and add a package README documenting the DNS interception architecture and modes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| client/go/outline/dnsintercept/forward.go | Adds early-close logic for forwarded DNS responses using synchronization to avoid multiple close attempts from the resp path. |
| client/go/outline/dnsintercept/forward_test.go | Adds assertions/tests for early-close behavior and ensuring non-DNS responses don’t trigger early close. |
| client/go/outline/dnsintercept/truncate.go | Defers base session creation until first non-DNS packet; DNS-only sessions stay local. |
| client/go/outline/dnsintercept/truncate_test.go | Adds a DNS-only test ensuring no base session is created and Close remains safe. |
| client/go/outline/dnsintercept/README.md | Documents how interception works, including forward/truncate modes and switching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| pending := wrapper.pendingClose | ||
| wrapper.mu.Unlock() | ||
| if pending { | ||
| base.Close() |
| } | ||
| resp.mu.Unlock() | ||
| if s != nil { | ||
| s.Close() |
Comment on lines
+83
to
96
| wrapper := &forwardPacketRespReceiver{PacketResponseReceiver: resp, fpp: fpp} | ||
| base, err := fpp.base.NewSession(wrapper) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| wrapper.mu.Lock() | ||
| wrapper.sender = base | ||
| pending := wrapper.pendingClose | ||
| wrapper.mu.Unlock() | ||
| if pending { | ||
| base.Close() | ||
| } | ||
| return &forwardPacketReqSender{base, fpp}, nil | ||
| } |
WriteFrom can only fire after the caller has sent a packet via WriteTo, which requires having the PacketRequestSender, which is only returned after NewSession sets sender. So sender is always non-nil when WriteFrom runs, and pendingClose is unnecessary. The mutex is still required: without it, the Go memory model does not guarantee the reader goroutine (started inside base.NewSession) sees the write to sender, since the goroutine start happens before the assignment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jyyi1
approved these changes
Mar 20, 2026
Contributor
jyyi1
left a comment
jyyi1
left a comment
There was a problem hiding this comment.
The code LGTM, with a small performance related comment.
| return req.trunc.WriteTo(p, destination) | ||
| } | ||
| return req.PacketRequestSender.WriteTo(p, destination) | ||
| req.mu.Lock() |
Contributor
There was a problem hiding this comment.
Will the lock cause any performance degradation? Probably use a double-lock pattern?
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.
DNS is one-shot (one query, one response), but each forwarded DNS query was holding a transport session open until the 30-second write-idle timeout. Under sustained DNS load this causes session accumulation and eventual resource exhaustion, breaking VPN connectivity.
See dnsintercept/README.md for the context to help review this PR.
Two fixes:
Should fix #2679