Skip to content

fix(file_store): don't overwrite entries appended via another handle - #2332

Open
Brijesh-Thakkar wants to merge 2 commits into
bitcoindevkit:masterfrom
Brijesh-Thakkar:fix/file-store-stale-handle-append
Open

Brijesh-Thakkar wants to merge 2 commits into
bitcoindevkit:masterfrom
Brijesh-Thakkar:fix/file-store-stale-handle-append

Conversation

@Brijesh-Thakkar

Copy link
Copy Markdown

Description

Fixes #2308.

Store::append writes at the file handle's own cursor, and the file is not opened in append mode. That cursor is only synced on load, dump and the handle's own appends. If another handle to the same file appends in the meantime, an append through the stale handle overwrites that entry. When the entries are the same size the file stays decodable, so the earlier changeset is lost silently.

Before writing, append now scans forward from the handle's cursor using the existing EntryIter, skipping valid entries that other handles added. The cursor ends up right after the last valid entry, and the new changeset is written there.

Notes to the reviewers

  • Replace bincode by postcard #2258 addresses the same issue by seeking to the end of the file before each write. As raised in review there, that makes a corrupted tail unrecoverable, because later entries land behind undecodable data. This PR keeps the current "append only after valid data" behavior: it reuses EntryIter and writes where valid data ends. A test covers appending after a failed dump and after a partial trailing entry.
  • Errors during the scan are handled as follows:
    • Clean EOF ends the scan and the write happens at the cursor.
    • Garbage or a partial entry (UnexpectedEof) ends the scan and the write starts there, as on master.
    • Real I/O errors are returned instead of ignored.
  • Single-handle behavior is unchanged. The cursor is already at EOF, so the scan costs one extra read and the write lands where it always did.
  • No new dependencies, fields or public API changes. Length framing and the set_len rollback from Replace bincode by postcard #2258 are intentionally out of scope.
  • Limitations:
    • Concurrent writers still race. Two handles writing at the same moment can interleave or clobber each other. This change only fixes appends made after another handle has finished writing.
    • A lockfile or a one-handle-per-file design remains the real long-term answer.
    • EntryIter::drop ignores a failed cursor-sync seek, so that one seek failure can go unnoticed.

Changelog notice

Fixed

  • bdk_file_store: Store::append no longer overwrites changesets that were appended through another handle to the same file.

Checklists

All Submissions:

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Copilot AI balanced review requested due to automatic review settings September 30, 2026 05:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@nymius nymius 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.

NACK 7b6f7f8

bincode has been deprecated. Any solution for the file store issues should also carry the migration from bincode to postcard.

Brijesh-Thakkar and others added 2 commits September 30, 2026 23:01
Signed-off-by: Brijesh-Thakkar <brijeshthakkariitian5126@gmail.com>
`bincode` is deprecated. Migrate the file store's on-disk encoding to
`postcard`, as part of the fix for bitcoindevkit#2308 (see also bitcoindevkit#2258).

Each entry is now stored as a `postcard` `u64` varint length prefix
followed by the `postcard`-encoded changeset. The length prefix frames
entries, so a torn or corrupt entry can be told apart from a clean end
of file, and a corrupt, huge length cannot trigger a huge allocation.
Bytes left in a frame after decoding are rejected. Real I/O errors are
reported as `StoreError::Io`, which `Store::append` now returns when
scanning past entries written by another handle.

BREAKING CHANGE:
- The on-disk format changed. Files written by earlier versions cannot
  be read; use new magic bytes so old files fail with
  `StoreError::InvalidMagicBytes`.
- `StoreError::Bincode(bincode::ErrorKind)` is replaced by
  `StoreError::Decode(postcard::Error)`.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@Brijesh-Thakkar
Brijesh-Thakkar force-pushed the fix/file-store-stale-handle-append branch from 7b6f7f8 to 956d277 Compare September 30, 2026 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

file_store: Store::append through a second handle overwrites changesets appended via another handle

3 participants