Skip to content

fix(storage): only remove keeper entry when its own PieceRef drops - #16

Closed
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-storage-only-remove-keeper-entry-when-its-own-3e082f
Closed

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-storage-only-remove-keeper-entry-when-its-own-3e082f

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 11, 2026

Copy link
Copy Markdown

Detail bug report: View on Detail

Bug

The disk-cache Keeper holds in-flight Pieces so Store::load can serve the newest not-yet-flushed value from memory instead of a stale value from disk. PieceRef::drop removed the keeper entry by key only. Since Keeper::insert overwrites an existing same-key entry in place, dropping an older (superseded) PieceRef while a newer one for the same key was still alive deleted the newer entry. Store::load then fell through to disk and returned the already-flushed older value — a stale read that broke read-your-writes for the window between the two flushes. Reachable in production whenever a key is re-enqueued before its prior flush completes (e.g. memory churn / evict_all).

Fix

  • The identity check needs the underlying record pointer, but Piece.record is private to foyer-memory, so Piece::ptr_eq(&self, other: &Self) -> bool was added to foyer-memory (compares the record pointer). Clones of one insert share the pointer; distinct inserts differ.
  • PieceRef::drop now removes the shard entry only when the in-table piece is the same record this PieceRef inserted (self.piece.ptr_eq(o.get())), so a superseded ref's drop is a no-op and the current entry survives.
  • Minimal local fix, relying on the pointer-identity invariant that holds by construction (Piece::clone shares the record pointer; each memory::insert allocates a fresh Arc<Record> — so a superseded insert's pointer never matches the newer in-table piece).

Testing

Versioned unit tests:

  • Piece::ptr_eq contract test (foyer-memory): clones of one insert compare equal; distinct inserts compare unequal.
  • Keeper regression test (foyer-storage): two same-key pieces inserted, then the superseded ref is dropped — the keeper must keep returning the newer value; dropping the current ref then removes the entry. Fails without the fix with "keeper lost entry after dropping superseded piece ref".
  • Keeper drop-order test: dropping the current ref first (entry removed) then the orphaned superseded ref (no-op, no panic) pins the guard in both drop orders.

End-to-end (verified locally, not committed): a Store-level test forced two same-key pieces into separate flush batches — batch 1 (v1) flushes and drops its PieceRef while batch 2 (v2) is held in flight, then Store::load is issued mid-window. With the fix it served the in-flight v2 from the keeper instead of the stale on-disk v1; after releasing the batch the final load returned v2 from disk. Deterministic across 20 runs; reverting only the keeper guard reproduced the stale on-disk read. That test additionally requires a one-line fix to PsyncIoEngineConfig::build, which currently discards the configured write/read IO latency (hardcodes None) — a separate test-infra defect, so neither that plumbing fix nor the store test is included here; the committed keeper unit tests fully cover the invariant.

Routine checks: cargo check (foyer-memory, foyer-storage, foyer), cargo fmt --check, and cargo clippy are clean (only a pre-existing workspace-level unknown-lint notice remains). Full nextest suites pass with strict_assertions — foyer-memory 32/32, foyer-storage 26/26 — the foyer hybrid end-to-end fuzzy test passes, and doctests (including the nightly-featured build) pass. No checks were left unverified.


Automatic Fixes PRs can be configured here.

@detail-app

detail-app Bot commented Oct 12, 2026

Copy link
Copy Markdown
Author

Closing this pull request because it has had no human activity in 30 days (since 2026-09-11) and was opened 30 days ago. Detail closes its pull requests once they are more than 30 days old and have had no human activity for 14 days.

If this change is still useful, reopen the pull request and Detail won't close it again. To keep a Detail pull request open, add the detail-keep-open label to it.

@detail-app detail-app Bot closed this Oct 12, 2026
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.

0 participants