Harden TsavoriteLog commit-failure handling and fix fast-commit recovery poisoning

[Ted Hart (TedHartMS)]

Summary

Fixes a host-crashing CI failure in CommitRecordBoundedGrowthTest (LogFastCommitTests) and the two underlying Tsavorite defects it exposed.

The original crash was a CommitFailureException with device error 0xFFFFFFFF thrown from an unguarded log.Commit() on a background thread, which terminated the whole test host. Investigating that surfaced a separate, production-relevant defect: a CalculateReadOnlyAddress address overflow caused by fast-commit recovery leaving the log poisoned after a swallowed exception.

Changes

1. Test + device-error hardening (commit 94ba1f96)

• Parameterized CommitRecordBoundedGrowthTest with [Values(1, -1)] int numThreads (-1 ⇒ ProcessorCount / 2, else 1).
• Background commit threads now capture the first exception, stop the others, and fail the test assertively instead of throwing unhandled on a raw Thread and killing the test host.
• Plumbed the real device exception through StorageDeviceBase.LastError → CommitInfo.Exception → CommitFailureException.InnerException, so a commit failure surfaces the underlying device error instead of an opaque error code.

2. Fast-commit recovery poisoning + address overflow (commit a4f03ae3)

Root cause. In FastCommitMode, recovery sets HeadAddress / CommittedUntilAddress = long.MaxValue as "shut up safe guards" sentinels before scanning the log tail. If RestoreHybridLogAsync then throws (device/metadata mismatch), the TsavoriteLog sync constructor swallowed it with a bare catch { }, leaving the log poisoned with a sentinel HeadAddress. The first Enqueue routed through CalculateReadOnlyAddress(tail, long.MaxValue), whose page arithmetic overflowed to a negative address:

• Debug: tripped the ReadOnlyAddress ... must not be less than HeadAddress assert.
• Release: silently returned long.MinValue, which the > ReadOnlyAddress guard filtered out, so ReadOnlyAddress never advanced, pages never flushed, and the next page-turning enqueue blocked forever (with unbounded memory growth) — worse than the Debug assert.

Fixes:

• TsavoriteLog: added ResetRecoveryState(), which rolls the allocator back to a clean, empty state (mirroring the existing "unable to recover using any available commit" reset) on the non-tolerated failure path of RestoreLatestAsync / RestoreSpecificCommitAsync before rethrowing. The sync constructor now logs and rethrows recovery failures instead of swallowing them, so a failed recovery fails fast rather than presenting an empty log as if recovery had succeeded (silent data loss). This matches the async CreateAsync path, which already propagates recovery failures.
• AllocatorBase: CalculateReadOnlyAddress now clamps headAddress >= tailAddress to return tailAddress, so a sentinel/out-of-range headAddress can never overflow the page arithmetic regardless of source (defense-in-depth).

Why the fail-fast constructor change is safe for Garnet

• The sync recovery path only runs when TryRecoverLatest = true.
• Garnet AOF sets TryRecoverLatest = false, so the durability path never runs it and is unaffected.
• PubSub uses the default but on fresh/NullDevice logs, so it only fail-fasts on genuine corruption.
• First startup (no commit) hits info.UntilAddress == 0 → resets to empty and never throws.

Testing

• New 48-thread fast-commit stress reproduction failed on iteration 1 before the fix; 400/400 iterations pass after (harness kept out of the committed test).
• Full test.hlog suite: 550 passed, 0 failed (2 Azure cases skipped).
• Tolerate path (FlakyLogTestTolerateFailure) still recovers surviving entries best-effort; non-tolerate paths (FlakyLogTestCleanFailure, FlakyLogTestConcurrentWriteFailure) still surface CommitFailureException.
• Release build clean (net8.0 + net10.0, 0 warnings); dotnet format clean.

[Copilot]

Pull request overview

Hardens TsavoriteLog commit failures and fast-commit recovery state handling.

Changes:

• Captures background commit failures safely in tests.
• Propagates device exceptions through commit failures.
• Resets failed recovery state and guards address calculations.

Reviewed changes

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

Show a summary per file

File	Description
LogFastCommitTests.cs	Adds threaded commit coverage.
TsavoriteLog.cs	Fails fast and resets recovery state.
CommitInfo.cs	Carries device exceptions.
CommitFailureException.cs	Supports inner exceptions.
StorageDeviceBase.cs	Tracks device errors.
RandomAccessLocalStorageDevice.cs	Records write exceptions.
NativeStorageDevice.cs	Records submission exceptions.
ManagedLocalStorageDevice.cs	Records write exceptions.
LocalStorageDevice.cs	Records write exceptions.
AllocatorBase.cs	Clamps addresses and propagates errors.

Suppressed comments (2)

libs/storage/Tsavorite/cs/src/core/TsavoriteLog/TsavoriteLog.cs:267

• Rethrowing here leaves all resources initialized earlier in the constructor undisposed: the owned LightEpoch, allocator, commit queue, and default commit manager cannot be disposed because the object never escapes. Repeated recovery failures can therefore leak epoch slots, memory, and handles. Add partial-construction cleanup before rethrowing (noting that inflightWord is allocated only after this block), or move recovery to a factory that disposes the log on failure.

                    throw;

libs/storage/Tsavorite/cs/src/core/TsavoriteLog/TsavoriteLog.cs:2890

• The root fast-commit recovery failure still has no committed regression test. Add a deterministic device/metadata mismatch test that throws after the sentinel addresses are installed, verifies the recovery exception propagates, and then verifies retry/enqueue cannot observe a poisoned address state. The bounded-growth test does not execute this recovery path.

                        // Recovery failed after the fast-commit scan set the "shut up safe guards" sentinels
                        // (CommittedUntilAddress / HeadAddress = long.MaxValue). Roll the allocator back to a clean, empty
                        // state so a failed recovery never retains a log poisoned with HeadAddress == long.MaxValue, which
                        // overflows CalculateReadOnlyAddress on the next enqueue.
                        ResetRecoveryState();

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.