Skip to content

[Tsavorite] Handle ObjectAllocator device read failures - #2075

Merged
Ted Hart (TedHartMS) merged 4 commits into
mainfrom
tedhar-fix-async-read-page-errors
Aug 19, 2026
Merged

[Tsavorite] Handle ObjectAllocator device read failures#2075
Ted Hart (TedHartMS) merged 4 commits into
mainfrom
tedhar-fix-async-read-page-errors

Conversation

@TedHartMS

@TedHartMS Ted Hart (TedHartMS) commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Root Cause

ObjectAllocatorImpl.AsyncReadPageCallback logged device read failures but signaled completion without preserving the error. Its header-read and partial-sector read-back callers therefore continued after failed reads. Zero-error short reads were also treated as successful completions.

Description of Change

Device read completion now carries the error code, transferred byte count, and exception, and existing-data reads succeed only when the requested length is transferred.

Key changes:

  • Convert zero-error short reads into EndOfStreamException failures
  • Throw before main-log or object-log truncation when the page-header read fails, preventing success-only OnTruncate callbacks and allowing a later retry
  • Route partial-sector flush read failures through the existing flush failure pipeline without submitting a replacement write or advancing FlushedUntilAddress
  • Keep front-partial snapshot writes safe by using the zero-filled pooled buffer for the unwritten prefix of a fresh snapshot device
  • Include the caught exception's full ToString() output in the OnPagesClosedWorker fatal message so CI logs contain its type, message, inner exceptions, and stack trace even when the runtime omits the FailFast exception argument; formatting runs inside try/finally so a broken custom ToString() cannot bypass fail-fast termination
  • Add deterministic numeric-error and short-read tests for flush, truncation, retry, and snapshot behavior

Key Technical Details

Affected types:

  • PageAsyncReadResult<TContext> - carries the full device completion result
  • ObjectAllocatorImpl<TStoreFunctions> - validates exact reads and prevents unsafe follow-on work
  • AllocatorBase<TStoreFunctions, TAllocator> - preserves complete page-close exception diagnostics in fail-fast output
  • ObjectAllocatorDeviceFailureTests - injects device errors and EOF-style short reads

Testing

  • Regression fixture passes on .NET 10 and .NET 8
  • Related object-log deletion, scan, compaction, and snapshot recovery tests pass on .NET 10
  • Tsavorite.core builds successfully for .NET 8 and .NET 10
  • Changed-file formatting verification passes

Issues Fixed

Fixes #2073

Propagate device errors and short reads instead of treating callback completion as success. Abort unsafe flush and truncation work, preserve fresh snapshot behavior, and add deterministic regression coverage.

Fixes #2073

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8ade2dd0-3bdc-4523-9ca9-5b5dcddec9d2

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds reliable ObjectAllocator device-read failure propagation to prevent truncation and flush corruption.

Changes:

  • Propagates device errors, short reads, and exceptions.
  • Stops dependent flush/truncation operations after failed reads.
  • Adds regression tests for failure, retry, and snapshot paths.

Reviewed changes

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

File Description
ObjectAllocatorDeviceFailureTests.cs Adds deterministic device-failure regression tests.
PageAsyncResultTypes.cs Carries complete read completion details.
ObjectAllocatorImpl.cs Validates reads and safely routes failures.

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

Embed the caught exception's full ToString output in the OnPagesClosedWorker FailFast message so CI logs include its type, message, inner exceptions, and stack trace.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8ade2dd0-3bdc-4523-9ca9-5b5dcddec9d2
Format the caught page-close exception inside a try/finally so Environment.FailFast always executes. Fall back to the exception type if a custom Exception.ToString implementation throws.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8ade2dd0-3bdc-4523-9ca9-5b5dcddec9d2
@TedHartMS
Ted Hart (TedHartMS) merged commit da80a42 into main Aug 19, 2026
448 of 449 checks passed
@TedHartMS
Ted Hart (TedHartMS) deleted the tedhar-fix-async-read-page-errors branch August 19, 2026 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AsyncReadPageCallback logs the device error then signals completion as success, and both call sites proceed on the failed read

3 participants