[Tsavorite] Fix silent data loss when the hash index checkpoint exceeds the OS single-request limit - #2159

Root Cause

SAVE on a server configured with a 2 GiB hash index (--index 2g --index-max-size 2g) reported success while the last 4096 bytes of the persisted hash table were never written. Two independent defects combined:

1. The index was issued as a single oversized request. IndexCheckpointTsavoriteKV.BeginMainIndexCheckpoint and IndexRecoveryTsavoriteKV.BeginMainIndexRecovery only split the table into chunks when totalSize > uint.MaxValue:

int numChunks = 1;
if (totalSize > uint.MaxValue)
{
    numChunks = (int)Math.Ceiling((double)totalSize / (long)uint.MaxValue);
    numChunks = (int)Math.Pow(2, Math.Ceiling(Math.Log(numChunks, 2)));
}

A 2 GiB table is one 2 GiB request; an 8 GiB table is four 2 GiB requests. Linux caps any single read/write/io_submit transfer at MAX_RW_COUNT — INT_MAX rounded down to a page boundary, 0x7ffff000 (2,147,479,552) with 4 KiB pages — and reports the truncated count as a successful completion. Every chunk at or above 2 GiB therefore silently dropped its last 4096 bytes.

2. No completion callback checked how many bytes actually moved. All four index I/O callbacks (main-index flush/read, overflow-bucket flush/read) ignored the numBytes parameter entirely, so the short transfer was indistinguishable from a full one. Main-index recovery was worse still — it only logged hard I/O errors and then continued, bringing up a hash table with unread buckets.

The hybrid log is not affected: it is bounded by LogSettings.kMaxPageSizeBits (128 MB). The index checkpoint is the only in-tree path that can issue a request above 2 GiB.

Description of Change

Two layers: prevent the oversized request, and fail loudly if a short transfer ever happens anyway.

Prevention — cap a single index request at 1 GiB

• Constants.kMaxIoBytesPerRequest (1L << 30). Chosen to sit below every MAX_RW_COUNT variant (0x7ffff000 with 4 KiB pages, 0x7fff0000 with 64 KiB pages) and below int.MaxValue — ManagedLocalStorageDevice casts the length to int for Memory<byte>, so a 2 GiB request there would overflow negative.
• Utility.GetNumIoChunks(totalBytes, maxBytesPerChunk) replaces the Math.Pow/Math.Log arithmetic with an integer loop that doubles the chunk count until each chunk fits. It still returns a power of two, which matters: state[version].size is always a power of two and sizeof(HashBucket) is 64, so the table divides evenly into chunks that preserve the sector alignment devices require.
• Both BeginMainIndexCheckpoint and BeginMainIndexRecovery use it. The read-cache path keeps its existing 32 MiB staging chunk (Math.Min of the two).

Detection — a short transfer now fails the operation

• The four async result structs carry the requested byte count (numBytesToWrite / numBytesToRead) and the chunk or level index.
• All four callbacks compare a nonzero numBytes against the requested length and record a failure. DeviceIOCompletionCallback documents that an implementation may complete successfully without reporting a count, reporting 0 instead, so 0 is treated as "not reported" rather than as a truncation. The MAX_RW_COUNT case this fixes is still caught, because Linux reports the truncated count. The callback's documentation now states the convention these checks rely on.
• Existing int errorCode failure fields became first-failure-wins string fields (Interlocked.CompareExchange(ref field, detail, null)) so the message can name the chunk or level and the byte counts.
• BeginMainIndexRecovery's callback now records hard I/O errors instead of only logging them, and RecoverFuzzyIndexAsync throws TsavoriteException rather than returning a partially populated table.

Lifecycle — a failed recovery closes the device safely

RecoverFuzzyIndexAsync must close the index checkpoint device on failure, because FinalizeMainIndexRecovery (which closes it on the success path) is skipped. Closing it naively is unsafe:

• CountdownWrapper.WaitAsync observed cancellation by completing its own TaskCompletionSource. A cancelled wait therefore returned while reads were still in flight, and stranded the counts those reads would later post — making the real completion permanently unobservable. It now abandons only the wait (Task.WaitAsync), leaving the countdown usable, and exposes DrainAsync for failure paths that must wait for outstanding I/O without observing cancellation.
• Recovery drains both the main-index and the overflow-bucket reads before disposing the device, and the cleanup scope now covers InitializeMainIndexRecovery, which is what opens the device and issues the reads.
• Both issue loops retire the requests they could not submit, so a drain can never wait forever for I/O that was never started.

Prerequisite fix — overflow-bucket recovery no longer reads past EOF

MallocFixedPageSize.BeginRecovery always requested a full 4 MiB page per level, including the short final level at the end of the checkpoint region, so every recovery read past the end of the file. That is benign when the result is ignored but makes a strict length check unusable (Windows returns ERROR_HANDLE_EOF). It now reads exactly what BeginCheckpoint wrote for that level.

This is safe because the recovery-side lastLevelSize is derived from num_ofb_bytes, which is the sum of the sector-aligned write sizes — so the computed length equals the aligned size actually written and is itself sector-aligned. The out numBytesRead value is unchanged.

Key Technical Details

Affected types:

Behavioral changes:

• A checkpoint or recovery that previously succeeded silently on truncated I/O now throws TsavoriteException. This is the intended fix — a loud failure is strictly better than a corrupt index.
• A cancelled CountdownWrapper.WaitAsync still throws, but no longer poisons the countdown. Nothing depended on the old behavior; it made the countdown unusable afterwards.
• TsavoriteException message text changed ("...failed with error code N" → "...failed: chunk N wrote X of Y bytes"). A repo-wide grep confirmed no test asserts the old strings.
• BeginMainIndexCheckpoint, BeginMainIndexRecovery and the test-only RecoverFuzzyIndexAsync overload gained a trailing optional internal maxIoBytesPerRequest parameter, defaulted to the production value. It exists so the chunking is falsifiable in a test without allocating a multi-gigabyte table.

Performance: none on any hot path. The extra work is one uint comparison per index I/O completion, on a path that runs once per checkpoint.

What NOT to Do (for future agents)

• ❌ Don't enforce "transferred == requested" in the device layer. Short reads are legitimate for the hybrid log — AsyncGetFromDisk retries on them when a record straddles the end of a read. A blanket device-level check breaks the log path.
• ❌ Don't treat numBytes == 0 as a short transfer. DeviceIOCompletionCallback permits a successful completion to omit the count. Only a nonzero count below the requested length proves truncation.
• ❌ Don't dispose a device from a failure path without draining. Cancelling a wait does not cancel the I/O it was waiting for; the callbacks are still going to run against that device.
• ❌ Don't complete a shared TaskCompletionSource to implement cancellation. It strands the counts the outstanding I/O will post and makes the real completion unobservable. Use Task.WaitAsync(token).
• ❌ Don't split requests inside the device. It would need callback fan-in on a hot path, and the IDevice API is uint-bounded anyway. Split at the caller, where the chunk count is already a concept.
• ❌ Don't raise the cap to MAX_RW_COUNT itself. The value must also stay under int.MaxValue for Memory<byte>-based devices, and the constant varies with page size across platforms.
• ❌ Don't make the chunk count non-power-of-two. Chunks must divide the (power-of-two) table evenly and stay sector-aligned.

Edge Cases

Testing

New fixture libs/storage/Tsavorite/cs/test/test.recovery/IndexCheckpointIoTests.cs (17 cases) plus three device doubles in SimulatedFlakyDevice.cs: TruncatingIoDevice (reproduces kernel truncation and records every (offset, length) issued), ZeroCountReportingDevice, and ThrowOnNthReadDevice.

• IndexIoChunksStayWithinOsRequestLimit — 7 cases from 1 MiB to 4 TiB (including 2 GiB, the reported size); asserts every chunk is under MAX_RW_COUNT and int.MaxValue, that chunks tile the table exactly, and that the split is no finer than necessary.
• IndexLargerThanRequestCapRoundTripsWithoutTruncation — end-to-end round trip with the cap lowered to 1 MiB on a 4 MiB table and the device capped at 1 MiB, so an oversized request would lose data. Asserts zero truncations on write and read plus bucket-for-bucket table equality.
• MainIndexCheckpointFailsOnShortWrite, OverflowBucketCheckpointFailsOnShortWrite, MainIndexRecoveryFailsOnShortRead, OverflowBucketRecoveryFailsOnShortRead — each asserts TsavoriteException rather than silent success.
• ZeroReportedByteCountIsNotTreatedAsShortTransfer — a device that reports 0 round-trips a checkpoint and recovery.
• CancellingAWaitLeavesTheCountdownDrainable — a cancelled wait throws, and the drain completes only after the last callback has run.
• RecoveryFailsPromptlyWhenAChunkCannotBeIssued — a device that fails to submit one of four chunk reads fails recovery without leaving the countdown short.
• OverflowBucketRecoveryReadsOnlyWhatWasWritten — compares summed read bytes against written bytes.
• MultiChunkIndexCheckpointRoundTrips — the read-cache path, 64 MiB table → two 32 MiB chunks.

Falsification — every fix was reverted independently to confirm the tests actually catch the bug: reverting the short-I/O detection fails exactly the 5 detection tests; reverting the chunking to the old uint.MaxValue logic fails IndexLargerThanRequestCapRoundTripsWithoutTruncation; reverting the nonzero guard fails ZeroReportedByteCountIsNotTreatedAsShortTransfer; reverting the CountdownWrapper change fails CancellingAWaitLeavesTheCountdownDrainable; reverting request retirement hangs and fails RecoveryFailsPromptlyWhenAChunkCannotBeIssued.

End-to-end, against a live server with --index 2g --index-max-size 2g under strace -e trace=io_submit:

Regression runs: Tsavorite.test.recovery 223 passed / 0 failed / 13 skipped (Azure); Tsavorite.test 290 passed; Tsavorite.test.hlog 580 passed; Garnet.test RespAdminCommandsTests + NativeAllocatorServerTests + checkpoint-recover 66 passed / 2 skipped. dotnet build Garnet.slnx clean on net8.0 and net10.0 with 0 warnings; dotnet format clean on both solutions.

Follow-up: first-principles review

Three independent model reviews of the PR surfaced further problems in the same code paths. Each is fixed with a test that fails when only that fix is reverted.

Correctness

• Recovery could retire a chunk twice. A device may invoke the completion callback synchronously and then throw out of the submit; the compensating loop assumed it had not, so it decremented that chunk a second time. The countdown then reached zero while earlier reads were still writing into the table, and the caller closed the device under them — the exact use-after-free the drain layer exists to prevent. Both read loops now claim a one-shot retirement guard, matching what the write side already did. Reachable in-tree: LocalMemoryDevice propagates callback exceptions.
• ShardedStorageDevice.WriteAsync reported the last shard's transferred count, not the aggregate. For a write spanning shards that is a nonzero value below the requested length, so short-transfer detection would reject a checkpoint that actually succeeded — the only false-positive source in the detection layer. A shard reporting 0 now makes the aggregate unknown rather than short, on both the read and write paths.
• Overflow-bucket recovery derived its level layout from a record count. The checkpoint rounds its final level up to a sector, so for a record size that does not divide the sector the read was both short and unaligned, which the O_DIRECT paths reject outright. The layout now comes from the persisted byte count, which is ground truth.
• MallocFixedPageSize.BeginCheckpoint had no failure path. A submit that threw left the outstanding count above zero, so checkpointTcs never completed and every waiter on the checkpoint task blocked forever — the symmetric half of the hang already fixed on the recovery loops. It now retires the levels it never issued.
• Stale recovery state. BeginRecovery and InitializeMainIndexRecovery now drop the previous recovery's countdown and error before allocating, so a stale error cannot fail a later recovery and a failure before the countdown exists cannot drain against stale state.

Errors

• Error channels now carry the originating exception alongside the detail (new IoFailure, CAS'd as one object so both publish atomically). Failures surface as TsavoriteIOException with the device exception as InnerException rather than as flattened text.

Lifecycle

• Read-cache staging runs under try/finally, so a throw cannot leave a pooled thread epoch-protected and pin the safe-to-reclaim boundary for the life of the process.
• A staging buffer is released exactly once when a submit throws after the callback has already run.

Removals

• Deleted IsIndexFuzzyCheckpointCompleted and IsCheckpointCompleted. Both polled the outstanding-IO count, which the new failure path also drives to zero, so they reported success for a checkpoint that failed — the same silent-success class this PR fixes. Neither had a caller.
• Deleted the error recording inside the issue-loop catch blocks: the raw exception always propagates before anything reads the error field, so those calls were unobservable.

Tests added (all falsified individually)

New device doubles: ThrowOnNthWriteDevice, CallbackThenThrowDevice, CallbackExceptionDevice, DeferredCompletionDevice, and SingleDeviceCheckpointManager.

Regression runs: Tsavorite.test.recovery 230 passed / 0 failed, Tsavorite.test.hlog 580 passed / 0 failed, Tsavorite.test 290 passed / 0 failed. dotnet build Garnet.slnx clean on net8.0 and net10.0; dotnet format clean on both solutions.

Unrelated build fix carried by this PR

Rebasing onto main (5c6e59536, #2151) surfaced a pre-existing build break that blocks this PR's own CI.

#2151 added Garnet.test.TestUtils.EnsurePortSlotResolved() to the [SetUpFixture] in test/standalone/Garnet.test/TestBase.cs. That file is link-compiled into Tsavorite.test.csproj, which does not compile TestUtils.cs, so Tsavorite.slnx fails to build on both TFMs:

TestBase.cs(46,9): error CS0234: The type or namespace name 'TestUtils'
does not exist in the namespace 'Garnet.test'

Verified on a pristine origin/main worktree, so it is not introduced by this branch. CI missed it because the tsavorite path filter in ci.yml watches only libs/storage/Tsavorite/**, and #2151 touched only test/** and .github/** — so every Tsavorite job was skipped. This PR does touch Tsavorite, so those jobs run and would fail.

Commit f399f4b68 gates the call behind a GARNET_TEST_UTILS symbol defined by the two projects that actually compile TestUtils.cs (Garnet.test, Garnet.test.cluster). It is isolated in its own commit and can be reverted or split out independently.

The fail-fast behavior #2151 added is unchanged — verified by running Garnet.test with GARNET_TEST_PORT_SLOT=999999, which still errors in GlobalUnhandledExceptionHandling OneTimeSetUp before any test runs. #2151's own tests still pass: PortSlotTests 18/18, ClusterPortBandTests 4/4.

Issues Fixed

Fixes #2134