Fix spurious NOTFOUND from ContinuePendingRead after a successful disk read

[hexonal]

Addresses part of #1950 (see that issue for the full reproduction and analysis — this PR fixes one of at least two distinct causes of the flakiness described there).

Root cause

In ContinuePendingRead (libs/storage/Tsavorite/cs/src/core/Index/Tsavorite/Implementation/ContinuePending.cs), after a pending (disk) read's Reader() call succeeds, status is only explicitly set to SUCCESS inside the two branches gated by operationState.readCopyOptions.CopyTo (MainLog or ReadCache). When CopyFrom != ReadCopyFrom.None but CopyTo == ReadCopyTo.None — a valid configuration reachable via ReadCopyFrom.Inherit resolution — neither branch runs, and status is left holding whatever FindTagAndTryEphemeralSLock's out-parameter set earlier in the call, which is only meaningful on a failed lock/find and effectively behaves like NOTFOUND here. A fully successful disk read, with the correct value already retrieved into output, was therefore reported back to the caller as NOTFOUND, discarding the correct value.

This is reachable whenever a plain read is forced to disk (record evicted below HeadAddress) under that specific CopyFrom/CopyTo combination — uncommon in normal steady-state operation, but reliably triggered right after recovering into a tighter memory budget than the data was originally written under (exactly what RespAdminCommandsTests.SeSaveRecoverMultipleKeysTest("25k","18k") does, which is how this was found).

Fix

One line: explicitly set status = OperationStatus.SUCCESS; immediately after a successful Reader() call, before the CopyTo-gated branching — matching the already-correct pattern in the synchronous read path (InternalRead.cs, where status = OperationStatus.SUCCESS is set unconditionally right after Reader() succeeds, before any CopyFrom-gated logic runs). The async/pending path was simply missing the equivalent line.

Testing

• Reproduced the underlying symptom via SeSaveRecoverMultipleKeysTest("25k","18k"): on unpatched main, 6/20 (~30%) local runs failed. With this fix, 6/150 (~4%) failed — a ~7x reduction, confirming this addresses a real, distinct cause.
• Ran the broader relevant test surface to check for regressions: RespAdminCommandsTests (56 tests), RespConfigTests + AofShardedTxnRecoveryTests (18), Tsavorite.test.recovery (204), CompletePendingTests/LowMemoryTests/StateMachineDriverTests (33), ReadAddressTests/ReadCacheTests (171). All pass, no regressions observed.

This does not fully close #1950

The residual ~4% failure rate is a second, distinct issue — in a captured failure, ContinuePendingRead's request.logicalAddress was 0 instead of the correct address at completion time. I was not able to pin that down with confidence within a reasonable investigation budget and did not want to guess at a fix for it in this sensitive async-completion path. Full details and a working diagnostic-instrumentation approach are in the issue; happy to keep digging into the second cause separately.

[Copilot]

Pull request overview

This PR fixes an incorrect OperationStatus being returned from Tsavorite’s async/pending read completion path (ContinuePendingRead) after a successful Reader() call, preventing a successfully read value from being reported upstream as NOTFOUND under specific ReadCopyOptions configurations.

Changes:

• Set status = OperationStatus.SUCCESS immediately after a successful Reader() call in ContinuePendingRead, before any CopyTo-gated copy logic runs.
• Add an explanatory comment documenting why this is required and the scenario it addresses.

[hexonal]

CI finished: 209/211 checks green. Two cluster jobs failed:

• Garnet Cluster (ubuntu-latest, net10.0, Debug, Garnet.test.cluster.multilog) — ClusterDivergentReplicasMMTest, failing with TaskCanceledException from GarnetServerNode.GossipAsync
• Garnet Cluster (windows-latest, net10.0, Release, Garnet.test.cluster.vectorsets) — ClusterVectorSetTests.VectorSetMigrateManyBySlot, also preceded by a GossipAsync TaskCanceledException, then an IndexOutOfRangeException in the test's own array indexing

I looked into whether these could be caused by this PR's change before assuming otherwise, since it touches core read-path status handling. My assessment, with the caveats noted:

• Both failures share the same underlying trigger (a gossip round-trip timing out), not a data-correctness symptom — this PR only changes when ContinuePendingRead reports SUCCESS vs a stale status after an already-successful disk read; it doesn't touch cluster gossip, AOF streaming, or migration slot-assignment logic.
• I don't see ReadCopyOptions/ContinuePendingRead referenced anywhere in libs/cluster/Server/Migration or libs/cluster/Server/Replication.
• I don't have permission to re-run just the failed jobs on this commit to check for pure flakiness (not a repo admin), and my local attempt to reproduce these specific cluster tests hit an unrelated environment limitation (fails at cluster bootstrap itself, a different symptom than the CI failure) — so I can't claim a clean before/after confirmation, just the reasoning above.

Flagging this transparently rather than asserting certainty, given this PR touches core Tsavorite read logic. Let me know if you'd like me to dig further, or if this pattern (gossip-timeout flakiness in cluster CI jobs) is already familiar.

[hexonal]

Follow-up on the two cluster failures above — dug further rather than leaving it at "probably unrelated":

ClusterDivergentReplicasMMTest: pulled the raw job log directly. The actual failure chain is AddDelSlotsRange error ERR Slot 0 is already busy → a later IncrBy fails because slot ownership ends up inconsistent → Assert.Fail(). This is a cluster slot-assignment/gossip-convergence race in ClusterManager — no code path through ContinuePendingRead. The GossipAsync TaskCanceledException lines are teardown-time noise; the same pattern shows up throughout both passing and failing tests across the whole job log. Also ran ClusterDivergentReplicasMMTest 160 times (20 iterations × 8 parametrized cases) locally on this PR's branch and, as a control, the same 160 runs on the exact upstream/main base commit: 0/160 failures on both — no measurable difference.

VectorSetMigrateManyBySlot: couldn't get a clean local A/B for this one (blocked by an unrelated local/macOS-arm64 issue — the native diskann_garnet.dylib isn't available in this environment, so every VADD fails locally regardless of branch). Traced the code instead: vector-set migration's WriteOrSendRecordAsync does call into VectorBasicContext.Read()/CompletePending, which can reach ContinuePendingRead if a migrated element was evicted to disk — so there is a real, if narrow, path from this PR's change to this test. But the test only adds 4 tiny elements per primary with no low-memory configuration, making disk eviction implausible here. The failure itself (IndexOutOfRangeException inside the test body) is preceded by the same benign gossip-teardown noise as the other failure.

Net: high confidence the first failure is unrelated (direct root-cause trace + 320-run local parity), moderate-high confidence the second is too (same noise signature, and the one theoretically-reachable path is impractical given the test's tiny data size — the local block on confirming that statistically is an unrelated native-library gap, not something this PR affects). Standing by the PR as-is.

[TedHartMS]

Thanks for the report and investigation. #1957 supersedes this; the change here is not needed, as comments in that PR at this point indicate:

// status is already OperationStatus.SUCCESS here: FindTagAndTryEphemeralSLock (at the top of the loop) returns true
// only after TryEphemeralSLock has set status to SUCCESS, and nothing below changes it before this point.

[hexonal]

Thanks for the correction, and confirming here rather than letting it stand unverified: you're right, and I traced through why.

FindTagAndTryEphemeralSLock (Helpers.cs) only returns true via the path where TryEphemeralSLock (TransientLocking.cs) already set status = OperationStatus.SUCCESS; the failure path returns false before that point, and nothing between there and where I added the line reassigns status. So the line I added really is a structural no-op at that spot, not a hunch that happened to be wrong — the original root-cause claim in this PR (that status was left at some stale non-SUCCESS value) doesn't hold up.

I also went back and re-tested the "30% → 4%" improvement I cited as evidence, since a no-op code change obviously can't have caused a real effect, and the numbers deserved a harder look rather than being left dangling. Built main, this PR's branch, and #1957's branch from fresh clones and ran the same repro loop against all three, sequentially and uncontended this time (the original two sessions weren't run under comparable conditions - system load looks like the actual confound, unrelated tests started flaking too under contention, including a multi-minute hang on main that's probably the same thing as the "45-min CI hang" from the original report). Clean re-run, 30 iterations each on ("25k","18k"): main 1/30, this PR 1/30, #1957 2/30 - statistically indistinguishable. The original "before/after" numbers were almost certainly a session-to-session environment artifact, not a real signal. Closing this out.

One thing worth flagging while I have the harness set up: SeSaveRecoverMultipleKeysTest("25k","18k") still failed occasionally against your #1957 branch in that same clean re-test (2/30, and 4/30 under load) - same signature both times (Key SeSaveRecoverTestKey0693, expected 22-char string, got empty). Small sample, so not conclusive, but it's the same key every time, which matches the pattern from before. Possibly relevant: the residual logicalAddress == 0 case I mentioned in this PR's original description might be the same root cause your fix already addresses, just not fully - your own new SpanByteRecoverySnapshotEvictionTests (direct Tsavorite-level) passed every time for me, so if there's a gap it's plausibly specific to the RESP+AOF-layered recovery path that test doesn't exercise. Flagging in case it's useful before merge; happy to share the raw logs if wanted.

[TedHartMS]

Yes--turns out there was another issue. Try the current 1957 (it's draft now as I'm investigating something related to the earlier fix). Thanks!