[flozano]

Fixes the two issues @drakkan raised in #623, plus a third one I found while reproducing them. All three are in passive_multiplexer.go and only apply with PassiveTransferPortMultiplexing on.

Fixes

A late Close() deleted another session's reservation. release deleted by IP, without checking whose reservation it was. If A's data connection was dispatched, B reserved the same port from the same IP, and then A's transfer ended, A's Close() removed B's reservation. B's connection was dropped and B got a 425 after ConnectionTimeout. release now takes the reservation and only deletes that one.

deliver racing Close() could strand a connection. deliver could pass its closedCh check, lose the CPU, and send to connCh after Close() had drained it, leaving the connection with nobody to read or close it. deliver now decides under stateMu, and Close() sets closing under that lock before draining. The send can't block: connCh has room for the one connection a reservation ever gets.

Concurrent first use of a port gave spurious failures. getOrCreate bound outside the lock, so when sessions from different IPs hit an unused port at once, the loser's bind failed with "address already in use" and that session skipped a port it could have shared. With a single port, that's a 421. The lock is now held across the bind. That serializes binds within a server, but a bind is quick and each port is only bound once.

released was only ever written, so closing replaces it.

Measurements

12 s of PASV + LIST in a loop on loopback, with 5 ms between PASV and the data connection, and ConnectionTimeout set to 2 s. "Lost" means the data connection never paired, "refused" means a 421 on PASV. "Before" was measured on v0.32.6, whose passive_multiplexer.go is identical to main.

Scenario	Multiplexing off	Multiplexing on, before	Multiplexing on, after
Same IP, 8 sessions, 9 ports	16,904 ok	9,923 ok, 23 lost	17,264 ok
Same IP, 4 sessions, 9 ports	8,772 ok	7,190 ok, 4 lost	8,876 ok
8 IPs, 8 sessions, 1 port	2,168 ok, 4,067 refused	16,831 ok, 3 refused	17,234 ok

With 8 sessions from one IP on 2 ports, there are still 421s (3,486 in the run above): that's the expected limit of one pending reservation per IP and port, and those sessions are now refused up front instead of losing transfers.

Testing

• go test -parallel 20 -race ./... passes.
• golangci-lint v2.15.0 reports 0 issues.
• The three new tests each failed 5 out of 5 runs without the fix, and pass with it. TestPassiveReservationDeliverCloseRace is a stress test, so a run on broken code isn't guaranteed to catch the race.

Not addressed

Connections are matched by raw source IP before WrapPassiveListener runs, and this is inherent to multiplexing:

• With IPMatchDisabled, a data connection from an IP other than the control connection's is still closed. Tested.
• With a PROXY protocol listener, a data connection arriving through the proxy has the proxy's IP, so it wouldn't match either. This comes from reading the code, I haven't tested it with a real proxy.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.67%. Comparing base (c508623) to head (9c234ab).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
+ Coverage   91.26%   91.67%   +0.40%     
==========================================
  Files          13       13              
  Lines        2164     2149      -15     
==========================================
- Hits         1975     1970       -5     
+ Misses        176      166      -10     
  Partials       13       13              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[drakkan]

I am not convinced that for _, candidate := range getPassivePortCandidates(portRange) is the right approach for multiplexing. The candidates are returned in a random order; couldn't a reservation lead to the opening of a new listener, even when one already exists and can serve that IP address? I believe the correct approach is to check the existing listeners in sequence and only open a new port if none of them are able to serve the IP address.

I may be wrong here, I just did a quick look at the code

[drakkan]

Thank you. I agree that IPMatchDisabled cannot be supported; that’s fine, it shouldn’t be used at all.

I believe the proxy protocol can be supported, try to use WrapPassiveListener