[V1] Backport #1949: Fix NetworkWriter.AsyncFlushPages hanging FlushEvent waiters on send failure

[hexonal]

Backport of #1949 to release/v1, as promised in #1949 (comment). Sorry for the lag, @vazois — #1949 landed on main while I was away from it.

The gap on release/v1

AsyncFlushPages's catch block logs the exception and disposes the network handler, but never invokes AsyncFlushPageCallback for the page whose send failed. Anything blocked on the corresponding FlushEvent is therefore never released — in particular GarnetClient.InternalExecuteNoResponse, which cluster PUBLISH forwarding calls on Garnet's own network-processing thread.

I checked this branch directly rather than assuming the fix was still needed: its catch block is character-for-character what main's was before #1949, so the hang is reachable here too. This is the sibling half of #1929 — the other half was already backported as #1953.

Deviations from #1949

Applied by hand, not cherry-picked, because the test file moved between branches. The exact differences:

• Production change (libs/client/NetworkWriter.cs, libs/client/Garnet.client.csproj) — byte-identical to what landed on main in 2255520.
• Test file path — test/Garnet.test/GarnetClientTests.cs here, test/standalone/Garnet.test/GarnetClientTests.cs on main.
• Fixture base — this branch's GarnetClientTests derives from AllureTestBase and carries [AllureNUnit]; that is pre-existing branch infrastructure, untouched.
• Two doc-comment sentences reworded. main's copy describes "a thread parked in flushEvent.Wait" and refers to "the previous Wait()-on-a-background-thread approach" — both are leftovers from an earlier iteration of that test during Fix NetworkWriter.AsyncFlushPages hanging FlushEvent waiters on send failure #1949's review and describe code that no longer exists. I reworded them to describe what the test actually does. Test logic is byte-identical; this is the only semantic difference in the file.

Before porting the test I diffed every API it touches across the 245 commits separating the branches: INetworkSender is byte-identical (so the ThrowingNetworkSender stub needs no adjustment), CompletionEvent differs only by a comment and an added ToString() with WaitAsync present on both, NetworkWriter's ctor differs only by main's extra PoolOwnerType param (not used by the test), and GarnetClient's ctor differs only by main's extra clientName param (the test uses named arguments).

The narrow AsyncFlushPageCallback double-invocation edge case disclosed in #1949's description applies identically here and is likewise not addressed.

Test-only note

The test reaches NetworkWriter's private networkSender field to inject a sender that throws, so Garnet.client gains an InternalsVisibleTo entry for Garnet.test — same as on main. The key is the one already used by Garnet.host, Tsavorite.core, GarnetServer and GarnetJSON on this branch, so no new signing material enters a shipping v1 package.

Verification

On this branch, net10.0 Debug:

• Without the NetworkWriter.cs change: fails after 3s — the cancellation token fires because the FlushEvent waiter is never released. I reverted just that hunk and re-ran to confirm this, rather than assuming the test would have caught the bug.
• With it: passes in 96ms.
• GarnetClientTests: 20/20. All three *GarnetClientTests fixtures (GarnetClientTests, RespListGarnetClientTests, RespSortedSetGarnetClientTests): 69/69.
• dotnet format --verify-no-changes: clean on both touched projects.

To be upfront about the limits: my environment is macOS/arm64 with only the .NET 10 runtime installed. net8.0 builds cleanly there (0 warnings, with TreatWarningsAsErrors set repo-wide) but I could not run its tests, and I have no Linux or Windows coverage — CI on this PR is the first place those get exercised.

[Copilot]

Pull request overview

Backports the main-branch fix for a hang in NetworkWriter.AsyncFlushPages on send failure into release/v1, and adds a regression test to ensure FlushEvent waiters are released even when SendResponse throws.

Changes:

• Ensure AsyncFlushPages signals page completion (via AsyncFlushPageCallback) even when networkSender.SendResponse(...) throws.
• Add a regression test that simulates a send failure and verifies FlushEvent.WaitAsync(...) is released.
• Grant Garnet.test access to Garnet.client internals via InternalsVisibleTo to enable the test’s injection.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
test/Garnet.test/GarnetClientTests.cs	Adds regression coverage validating FlushEvent is signaled on send failure by injecting a throwing network sender.
libs/client/NetworkWriter.cs	Calls AsyncFlushPageCallback in the send-failure catch path to prevent FlushEvent waiters from hanging.
libs/client/Garnet.client.csproj	Adds InternalsVisibleTo for Garnet.test to allow the new test to access NetworkWriter internals.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[hexonal]

The 4 red Garnet.test.cluster legs are pre-existing release/v1 cluster flakiness, not this change. The cleanest evidence is that the changed code never ran:

The catch block this PR touches logs "Exception calling networkSender.SendResponse in AsyncFlushPages" unconditionally, on the line immediately before the added AsyncFlushPageCallback call. That string appears 0 times in all four complete job logs (--log, not just --log-failed), as do NetworkWriter, AsyncFlushPage and FlushEvent. Since the added line is reachable only from inside that catch, it didn't execute.

That zero is meaningful rather than a logging-plumbing artifact: the same string does appear on #1998 (job 90737676753), with the stack running through Garnet.client.NetworkWriter.AsyncFlushPages at NetworkWriter.cs:330. So the logger reaches CI output from the cluster path — #1998 actually hits the very throw this PR is about, on code that doesn't have the fix.

The same failures without this change:

• [release/v1] Fix integer underflow in BITCOUNT ... BIT byte-boundary mask (backport #1994) #1998 job 90737676753 — the same three PrimaryUnavailableRecoveryAsync cases with the same CLUSTERDOWN Hash slot not served at ClusterManagementTests.cs:1472, and the same hashslot 12141 LPUSH/MOVED message.
• [release/v1] Fix integer underflow in BITCOUNT ... BIT byte-boundary mask (backport #1994) #1998 job 90737676792 — ClusterReplicationTLS.ClusterReplicationMultiRestartRecover, same windows/net8.0/Release leg, identical offsets 1,2080064 != 1,0.
• release/v1 push run 29857275929 (commit f8fc4357, this branch's parent) — already failed two cluster legs on its own, including MultipleReplicasWithVectorSetsAndDeletesAsync.

Also worth noting the failure sets are disjoint across the four legs — no test fails in more than one — while the other four cluster legs pass on the same commit. And ClusterPubSubForwardTests.ClusterPublishSurvivesPeerNodeShutdown, the one test that actually exercises cluster PUBLISH forwarding across a peer shutdown, passed in all four failing jobs.

A re-run of the 4 failed jobs whenever convenient would be appreciated.

---

Separately, a correction I owe you, @vazois. On #1949 you asked whether the fix could double-invoke the flush callback and recreate the original bug. I answered that every exception path in ClientTcpNetworkSender.SendResponse is mutually exclusive with a successful completion firing the callback. That was wrong, and I'd rather flag it than leave it standing now that the same code is up for review here.

socket.SendAsync returning false means synchronous completion, so SeaaBuffer_Completed(null, s) runs inline and calls callback(...) — the flush callback has already fired. If anything after that point throws (reusableSaea.Return, throttle.Release, or the callback itself), ClientTcpNetworkSender.cs's catch { ...; throw; } rethrows, and AsyncFlushPages' catch then invokes AsyncFlushPageCallback a second time for the same page. So the two are not mutually exclusive.

It does matter which page: count is one CountWrapper per AsyncFlushPages call, shared by every PageAsyncFlushResult in the batch, so a second decrement reaches zero one page early — FlushEvent.Set() and FlushedUntilAddress would advance with a page still in flight.

What actually keeps this benign is narrower than what I claimed: the throw-after-callback window is effectively confined to teardown, and Dispose() sets disposed = true before disposing FlushEvent, so AsyncFlushPageCallback's catch when (disposed) swallows it — and in that state an early Set() is exactly the release we want. Outside teardown I couldn't construct a throw on that path.

The robust version would be making page completion idempotent (an interlocked flag on PageAsyncFlushResult), but that belongs on main first rather than in a backport. Happy to open it there if you think it's worth doing.