Conversation
When one direction ended outside a swap, the other stayed blocked on a read that only returned once the far end noticed by itself. Until then the session sat in the table with PlayerDisconnectEvent unfired, so list and the metrics counted a player who had already gone. Both pumps share one cancellation source, so whichever exits first now stops the other.
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
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.
Cancelling the sibling made it exit through the same branch a backend hanging up does, so a client dropping its socket raised ServerKickedEvent. The first pump to exit now claims that spot with a compare-exchange, and both the kick classification and the cancellation key off it, so the loser's exit is read as what it is.
|
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. Two regressions added as asked. A client drop asserts no Checked they bite rather than assuming: with the 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 |
| // the cancellation below reached it. Claiming that spot atomically is what keeps a client | ||
| // drop from being read as a backend kick, since the sibling it cancels exits through this | ||
| // same method and would otherwise look exactly like a backend that hung up. | ||
| bool endedTheSession = Interlocked.CompareExchange(ref firstPumpExit, isC2S ? 1 : 2, 0) == 0; |
There was a problem hiding this comment.
[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.
| private void StartPumps() | ||
| { | ||
| // A swap installs a fresh pair, so the previous pair's winner must not be inherited. | ||
| Interlocked.Exchange(ref firstPumpExit, 0); |
There was a problem hiding this comment.
[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
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
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.
|
@Pixnop for you to review the changes requested. |
The first pump to exit was recorded by its direction, but a pump ends on a failed read from one end or a failed write to the other. A backend reset that lands on the c->s write was read as the client leaving and silenced the kick, and a client reset that lands on the s->c write was read as the backend hanging up and raised a false one. The pump now records which endpoint ended it: the read side is the 'from' end, the write side is the 'to' end, a chunk the proxy itself refused is the proxy, and an exit that only happened because the pair's source was cancelled reports nothing at all. Only a real endpoint exit may claim the pair and stop its sibling, and only a backend one may set the kick. The exit record was also shared across pairs and reset by StartPumps. A swap gives the old pumps a bounded wait and then commits anyway, which it has to: an old c->s pump can be parked in the initial reservation mint, and that call waits on the session token rather than on the predecessor source. The straggler then landed in the new pair's record, claimed it, and cancelled the backend the player had just been moved to. Each pair now owns its own source and its own claim, so a straggler can only ever reach the pair it was started with, and the kick is further restricted to the pair currently installed because 'swapping' is already false again by the time it exits. PumpUntilClosedAsync had the same blind spot from the other side and now re-waits when the pair it was waiting on is no longer the installed one. Regressions wedge both pumps inside a write at once, so the reset on one side is only visible to the pump writing towards it and the failing endpoint is the opposite of the noticing direction. Both fail on the first of 25 sessions against the direction-based rule. The predecessor case holds a fake registry mint open across a swap whose retirement wait is turned down, then releases it and asserts the new upstream still carries the player's bytes; it fails against the shared record. The five-second retirement wait is now an internal field so that test does not have to sit through it.
|
Both P1s addressed in 147303f. On the first one, the pump no longer says anything about its own direction. It records which endpoint ended it: a failed or zero read is the On the second one, there is no shared exit record left to reset. One thing your comment did not name and the fix needs. Both write-failure regressions wedge the two pumps inside a write at the same time: the backend stops draining so the c->s pump parks in its write upstream, and the backend keeps sending to a player socket nothing reads for so the s->c pump parks in its write downstream. In that state a reset on one side is only visible to the pump writing towards it and the pump on the other side is blocked against a socket that is still fine, so the endpoint that failed is the opposite of the direction that notices, which is precisely the case the old rule got wrong. Resets are abortive ( Suite counts here: Proxy 721 tests, 720 passing, run three times in a row for the concurrency; Registry.Core 312, Cli 115, ServerMod.Protocol 97, all green. The one failure is |
S2223 on the new field: a settable static property says the same thing to the test without leaving a writable static field on the type.
A static the regression lowers to 250ms lowers it for every session in the process, and the suites run classes in parallel, so any seamless swap happening alongside that test retired its own predecessor on the shortened cap. Restoring it in a finally does not close the window. It is an instance property now, defaulting to five seconds, and the regression sets it on its own session.
|
The retirement cap is a per session property now rather than a static one: the regression lowering it to 250ms was lowering it for every session in the process, and xUnit runs the test classes in parallel, so any seamless swap running alongside it retired its own predecessor on the shortened cap. Restoring the static in a finally does not close that window. Production default is still five seconds and there is no config entry for it, since this is not an operator knob. |
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Please address the pump-pair snapshot race described in the inline comment and update this branch to current indev. The two issues from my 2026-08-31 review are addressed in this head, and the Proxy suite passes locally with 721 tests. Build & ServerMod and both Sonar checks pass. I will review the refreshed head after these updates.
| // A swap installs a fresh pair, and the fresh pair gets a fresh generation: nothing of the | ||
| // previous pair's exit is carried over and nothing of it is left for a straggler to reach. | ||
| var gen = new PumpGeneration(CancellationTokenSource.CreateLinkedTokenSource(sessionStopToken)); | ||
| pumps = gen; |
There was a problem hiding this comment.
[P1] Publish the generation and pump tasks atomically. PumpUntilClosedAsync snapshots pumps and then reads pumpC2S and pumpS2C separately. A race here can pair this new generation with the predecessor tasks. If those tasks complete after swapping becomes false, the loop treats the predecessor as the current pair, returns, and RunAsync tears down the newly installed session. Store the generation and both tasks together, or synchronize the snapshot, and add a deterministic regression for this interleaving.
A session's pump pair is published in two steps: StartPumps sets the `pumps` field to the new generation, and a seamless swap clears `swapping` right after. PumpUntilClosedAsync used to read these apart, in publish order, around the await it uses to wait for a pump pair to end (`pumps` then the two pump task fields before it, `pumps` then `swapping` after it): - Right after StartPumps installs a new generation, the swap that called it has not yet cleared `swapping`. A predecessor pump that outlived its retirement, or the loop's own thread getting preempted, could land the loop's re-check in the gap where a new generation is live but the swap has not finished, and it would tear the session down as though the pumps had failed rather than moved to a new backend. - Inside PumpUntilClosedAsync itself, the loop re-reads `pumps` and then `swapping` as two separate volatile reads. A swap's own two writes (`pumps = gen`, `swapping = false`) can land between them, pairing a stale `pumps` read with a fresh `swapping` read and producing the same wrong teardown. Both are closed by giving the loop a single atomic view and by reading fields in the reverse of the order they are published in: - PumpUntilClosedAsync takes one snapshot of `pumps` per iteration and works only off the object that snapshot returns (its own generation, its own exit signal), rather than a second field a swap could still be assigning. - The loop reads `swapping` before it re-reads `pumps`, not after. A swap that retired a generation always claims `swapping` before it moves on, so a false read of `swapping` can only happen once the new generation is already published; reading them in that order means the `pumps` read that follows is guaranteed to see the new generation whenever `swapping` has already gone false. With the loop waiting on one exit signal per generation, the signal has to fire however a pump ends. It is therefore tied to that pump's own Task completing (a continuation StartPumps attaches to each pump task), not to a call inside PumpAsync's `finally`: a fault before PumpAsync's own `try` (the buffer allocation) or one thrown by RecordPumpExit itself would skip such a call and leave the loop waiting forever. The continuation only runs after the finally, so RecordPumpExit's decision is made first. For the same reason StartPumps fetches both upstream and client streams before publishing the new generation, not after. A Close() racing the previous order could throw ObjectDisposedException out of GetStream() after publication, and a published generation with a pump that never started has no task whose completion could signal its exit. Fetching first means a published generation always starts both pumps. Four regressions cover this: two stragglers racing two swaps so the loop's re-snapshot lands mid-swap, a buffer allocation fault before the pump's try, a Close() racing StartPumps between publication and both pumps actually starting, and a swap's two writes landing between the loop's own two reads. The buffer-fault and Close() tests guard the new signalling rather than reproduce a bug of the published code. The first three use two internal seams (AfterPumpsPublished, AfterLoopSnapshot), the fourth a third (AfterSwapFlagRead), all null in production and invoked only through `?.Invoke()`. Left alone, as before: a new-generation pump exiting while `swapping` is still true can still suppress the kick classification. That predates this change and is a separate follow-up.
|
The pump-pair snapshot race is fixed in c07588b. The pump generation now carries its own cancellation source and both of its exit signals, so the single read of Working through the interleaving turned up three more things, all changed in the same commit. Each pump's exit signal now comes from its own task completing, rather than from a call inside Four regression tests cover this in The branch now includes current indev (#137). The Proxy suite is green locally apart from the one known |
The AfterSwapFlagRead hook now holds the loop between its two reads until the swap has returned, instead of sleeping, so the interleaving is forced rather than likely. The test also asserts that the hook fired and that the gate did not time out, so it cannot pass without the loop being held.
|
The fourth regression now holds the loop between its two reads until the swap has actually returned instead of sleeping 300 ms, so the interleaving is forced every run; it still fails 10/10 with the old read order and passes 20/20 with the fix. |
|
2 participants
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
PumpUntilClosedAsyncwaits 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 windownimctl listand the metrics counted someone who had gone, andPlayerDisconnectEventhad 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
swappingthroughout. 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.