Skip to content

Close a UDP proxy backend that becomes active after eviction - #2019

Closed
devops-thiago wants to merge 1 commit into
apple:mainfrom
devops-thiago:fix-udp-proxy-socket-leak
Closed

devops-thiago wants to merge 1 commit into
apple:mainfrom
devops-thiago:fix-udp-proxy-socket-leak

Conversation

@devops-thiago

Copy link
Copy Markdown

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

Fixes #2015.

UDPProxyFrontend keeps at most 256 backends in an LRU cache and closes whichever one is evicted. UDPProxyBackend.close() returned early when the backend channel had not finished binding:

func close() {
    guard let channel = state.channel else {
        self.log?.warning("backend - close on inactive channel")
        return
    }
    _ = channel.close()
}

state.channel is nil from construction until channelActive runs, which happens after bind completes. A backend evicted inside that window is never closed. channelActive then adopts the channel, and because the ProxyContext has already been dropped from the cache, no reference remains that could close it. The socket stays open for the lifetime of the process.

The existing warning shows the case was anticipated, but the early return leaves the socket open rather than deferring the close.

This change records the close request in the backend state and has channelActive honour it, closing the channel instead of adopting it and discarding any queued payloads. Both paths run on the event loop the backend is bootstrapped on, since DatagramBootstrap(group: context.eventLoop) and NIOLoopBound(proxy, eventLoop: context.eventLoop) bind them to that loop, so the flag needs no further synchronisation.

UDPProxyBackend changes from file-private to internal so the test can drive the ordering directly. Nothing outside the module references it.

Testing

  • Tested locally
  • Added/updated tests
  • Added/updated docs

testBackendClosedBeforeChannelActiveClosesTheChannel calls close() before the channel becomes active, then activates it through an EmbeddedChannel and asserts the channel is no longer active. The test fails against the current code and passes with this change. testUDPForwarder and testLRUCache continue to pass.

@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from 0467f24 to 510e87b Compare August 7, 2026 19:16
@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from 510e87b to 99a61ac Compare September 2, 2026 00:10
@devops-thiago

Copy link
Copy Markdown
Author

@katiewasnothere I noticed you labeled #2015 stability — thanks for looking. This PR fixes it: a backend evicted from the LRU while its channel is still binding never gets closed, so the socket leaks once channelActive fires. The fix records the close request and honours it in channelActive; both run on the same event loop, so no extra synchronisation is needed. Regression test included.

I have three other small parser fixes open with the same setup — #2016, #2017, #2018 — each one line plus a test, each with a linked issue. All four are rebased onto current main, signed, and pass the unit suite. Since I don't have a merged PR yet, the workflows are waiting on approval to run. If you're able to approve them, that would unblock CI on the set. Happy to adjust anything.

@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from 99a61ac to 861350b Compare September 6, 2026 15:40
@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from 861350b to d34e41d Compare September 14, 2026 10:46
@devops-thiago

Copy link
Copy Markdown
Author

@egernst Pinging you since you fixed a closely related bug in #2260 (ConnectHandler.swift closing the wrong channel reference). This PR fixes the same class of issue in the UDP path: a backend evicted from the LRU while its channel is still binding never gets closed, so the socket leaks once channelActive fires later. The fix records the close request and honours it in channelActive, on the same event loop so no extra synchronisation is needed. Regression test included.

Three other small parser fixes are open with the same setup — #2016, #2017, #2018 — each one line plus a test, each with a linked issue. All four (plus four unrelated fixes: #2090, #2091, #2092, #2093) are rebased onto current main, signed, and passing the unit suite. I don't have a merged PR yet, so the workflows are stuck waiting on approval to run. An approve on any of these would unblock CI for the rest.

@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from d34e41d to 133f4aa Compare September 21, 2026 15:19
@devops-thiago

Copy link
Copy Markdown
Author

@jglogan I pinged @katiewasnothere on Sep 2 and @egernst on Sep 14 about this same batch with no reply, so per the contributing guide I'm trying someone else.

Eight small PRs are open, all rebased onto current main, signed, and passing the unit suite: #2016, #2017, #2018, #2019 (parser/proxy fixes, each one line plus a test) and #2090, #2091, #2092, #2093 (exit code persistence, XPC peer validation, image list filtering, build context error reporting). None have a review yet, so CI is stuck waiting on approval to run. An approve on any one would unblock the workflows for the rest.

Happy to split these up, add more tests, or adjust anything that doesn't fit.

@egernst
egernst self-requested a review September 21, 2026 15:59
@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from 133f4aa to b883996 Compare September 29, 2026 13:41
UDPProxyBackend.close() returned early when the backend channel had not
finished binding, leaving the socket open. The frontend calls close() on
whichever backend the LRU cache evicts, so a backend evicted inside the
bind window was never closed: channelActive then adopted the channel, and
because the ProxyContext had already been dropped from the cache, no
reference remained that could close it. The socket stayed open for the
lifetime of the process.

Record the close request in the backend state and have channelActive
honour it, closing the channel instead of adopting it. Both paths run on
the event loop the backend is bootstrapped on, so the flag needs no
additional synchronisation.

UDPProxyBackend becomes internal so the regression test can drive the
eviction ordering directly.

Fixes apple#2015
@devops-thiago
devops-thiago force-pushed the fix-udp-proxy-socket-leak branch from b883996 to fc39bc9 Compare October 1, 2026 16:26
@devops-thiago

Copy link
Copy Markdown
Author

Closing this together with #2015, which I could not reproduce on a real daemon (details there). The close() before channelActive path is real, but the eviction that would reach it does not happen in practice.

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.

[Bug]: UDP proxy socket leaks when a backend is evicted before its channel becomes active

1 participant