Stop a pump exit leaving its sibling blocked

[Pixnop]

Closes #89.

The two byte pumps share one cancellation source but nothing used it when a pump ended on its own. A player whose socket died stopped the c to s pump, while s to c stayed blocked reading from a backend with nothing to say, and PumpUntilClosedAsync waits on both. The session therefore outlived the player until the backend noticed independently, which on a quiet connection is its own player timeout. For that whole window nimctl list and the metrics counted someone who had gone, and PlayerDisconnectEvent had not fired, so a plugin doing session accounting saw the same stale picture.

The issue framed this as possibly deliberate, on the theory that the grace window helps a seamless swap survive a client blip. Reading the code, that is not what protects a swap: the swap owns its own cancellation source and installs the next pair only after both old pumps have finished, and it is guarded by swapping throughout. So the window bought nothing.

The fix cancels the shared source when either pump exits outside a swap or an already-running teardown, which is symmetric on purpose: the same gap exists in the other direction, where a backend dropping the connection left the c to s pump blocked on the player instead.

The test drops the player socket on a ready session and asserts the session task finishes, while deliberately leaving the backend open so that nothing but the fix can end it. Against the unfixed code it hangs and fails; with the fix it finishes in about 60ms. Full suite green at 1239.

[Zaldaryon]

Request changes. The new pumpCts?.Cancel() in Nimbus.Proxy/Core/ProxySession.cs at lines 1033-1034 cancels the sibling pump when the client socket exits. The backend sibling then enters the existing !isC2S && !closed && !swapping path at lines 1020-1024, sets kickedByBackend = true, and teardown emits ServerKickedEvent at lines 727-730. I reproduced this on head 4ccd3b96f8f4 by closing the client socket: one ServerKickedEvent was emitted and an Assert.Empty test failed in 162 ms. The new test covers prompt teardown but not event classification. Please record the origin of the first pump exit atomically, avoid inferring a backend kick from a cancellation driven sibling exit, and add regressions for client drop with no event and real backend drop with exactly one event. The CI and Proxy suite are green, but this is a correctness blocker.

[Pixnop]

Fixed, and you were right that the new test only covered teardown timing and said nothing about what the teardown was reported as.

The first pump to exit now claims that spot with a compare-exchange, and both decisions key off it: the kick classification only fires when the s to c pump was the one that ended the session, and the cancellation only fires from the winner, since the loser is already on its way out. StartPumps resets the record, so a swap installing a fresh pair does not inherit the previous pair's winner.

Two regressions added as asked. A client drop asserts no ServerKickedEvent at all, and a real backend hangup asserts exactly one. The second needed RecordingBackend to be able to close its accepted sockets while leaving the listener up, which Dispose does not do: it cancels the read loop and leaves the socket open, so the proxy never sees a hangup.

Checked they bite rather than assuming: with the endedTheSession guard removed from the kick branch and everything else left in place, the client-drop test fails, which is your reproduction.

One thing to know about my numbers rather than yours: a system SDK upgrade landed here mid-review (10.0.110 to 10.0.111) and the private runtime this machine uses for ASP.NET Core has not followed, so ProxyBootTests.ARegistryThatCannotBind_ExitsTwoRatherThanCrashingOnTheWayOut now fails locally by launching the staged binary against a mismatched runtime. It fails the same way on clean indev with none of this branch applied, so it is my environment rather than a regression here, and CI is the reading to trust on that one. Everything else is green: 716 of 717 in the proxy suite, 312, 115 and 97 in the other three.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Zaldaryon]

[P1] The first pump direction is not necessarily the endpoint that failed. PumpAsync catches stream errors from both from.ReadAsync and to.WriteAsync, but this path records only isC2S. A backend reset can therefore make the c->s write fail first and suppress ServerKickedEvent, while a client reset can make the s->c write fail first and emit it. Please record the read or write failure origin and add regressions for both write-failure cases.

[Zaldaryon]

[P1] This reset is not generation-safe. RetireSwapPredecessorAsync proceeds after five seconds even when an old pump is still running, then StartPumps resets this shared record and replaces pumpCts. The old c->s pump can still be waiting in InspectClientChunkAsync on MintReservationAsync, which uses sessionStopToken rather than the predecessor CTS. When it eventually exits, RecordPumpExit can win the new record and cancel the new pump CTS, tearing down the new backend. Please tie exit accounting and cancellation to the pump generation, and add a regression that keeps a predecessor alive past the retirement timeout.

[Zaldaryon]

Request changes.

The previous false kick on client disconnect is fixed, and the requested client-drop and backend-hangup regressions are present. I found two remaining teardown races in the inline comments.

First, RecordPumpExit uses the direction of the first pump to exit as the failure origin. PumpAsync catches failures from both stream reads and writes, so a backend reset during a c->s write can suppress ServerKickedEvent, while a client reset during an s->c write can emit it. The endpoint that failed needs to be recorded directly.

Second, a seamless swap can proceed after its five-second predecessor wait even when an old pump is still inside the reservation call. StartPumps then resets shared exit state and replaces pumpCts; the late old pump can claim the new state and cancel the new pair. Exit accounting needs to be tied to the pump generation.

Verification: dotnet test Nimbus.Proxy.Tests/Nimbus.Proxy.Tests.csproj -c Release passed 717 tests on the PR head and 714 on base indev. The full solution build was attempted, but this environment lacks protobuf-net and VintagestoryAPI for the ServerMod projects. GitHub Build, SonarCloud analysis, and SonarCloud Code Analysis checks are successful.