Skip to content

fix(multiapp): serve ReadAt from a detached handle on cache eviction race - #2104

Merged
vchaindz merged 1 commit into
masterfrom
fix/multiapp-readat-keynotfound-race
Jul 30, 2026
Merged

vchaindz merged 1 commit into
masterfrom
fix/multiapp-readat-keynotfound-race

Conversation

@vchaindz

Copy link
Copy Markdown
Contributor

Problem

The tag CI for v1.11.1 failed on a flaky unit test:

--- FAIL: TestWritePastFirstChunk
    remote_app_test.go:240: Received unexpected error: key not found

This is unrelated to the v1.11.1 SQL-panic fix (#2100) — that PR only touched embedded/sql/* and pkg/database/*. The failure lives in the remote/multi appendable read path introduced by the recent parallel-prefetch / range-fetch ReadAt work, and reproduces ~3/8 full-package runs of embedded/appendable/remoteapp.

Root cause

In multiapp.appendableFor(), a foreground ReadAt cache-miss:

  1. opens the chunk and Puts it into the fixed-capacity LRU cache via singleflight, then
  2. re-acquires it from the cache (Get) to take its refcount.

A concurrent insert — another foreground miss or a background prefetch goroutine — can evict or Replace that just-inserted entry between steps 1 and 2. The re-acquire then returns cache.ErrKeyNotFound, and ReadAt fails with a spurious key not found for a perfectly valid, readable offset.

Fix

When the post-open re-acquire misses, the data is still readable (locally or remotely), so re-open a detached, self-closing handle (refs=1, evicted) for that single read instead of erroring. ReadAt already Release()s the handle exactly once, which performs the underlying Close — no leak, no cache entry to govern its lifetime.

Test

Adds TestMultiAppConcurrentReadEvictionRace (small cache + prefetch + concurrent sequential readers), which deterministically reproduces the race:

  • Before: 10/10 runs fail with key not found
  • After: 0/20 runs fail; clean under -race

Verification:

  • embedded/appendable/multiapp — 0/10 full-package failures, -race clean
  • embedded/appendable/remoteapp — was ~3/8, now 0/15, -race clean
  • embedded/store green; go build ./... and go vet clean

…race

A foreground ReadAt cache-miss opens and caches a chunk via singleflight,
then re-acquires it from the cache to take its ref. A concurrent insert
(another foreground miss or a background prefetch) could evict or replace
that just-inserted entry in between, so the re-acquire returned
cache.ErrKeyNotFound and ReadAt failed with a spurious "key not found"
for a perfectly valid, readable offset.

This surfaced as flaky failures in
embedded/appendable/remoteapp.TestWritePastFirstChunk (~3/8 full-package
runs), introduced by the parallel-prefetch / range-fetch read path.

Fix: when the post-open re-acquire misses, the data is still readable, so
re-open a detached, self-closing handle (refs=1, evicted) for that single
read instead of erroring. The detached handle is released exactly once by
ReadAt, which performs the underlying Close, so there is no leak.

Adds TestMultiAppConcurrentReadEvictionRace, which deterministically
reproduces the race (small cache + prefetch + concurrent sequential
readers) — 10/10 "key not found" before the fix, 0/20 after, clean under
-race.
@coveralls

coveralls commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Coverage Status

coverage: 84.983% (+0.002%) from 84.981% — fix/multiapp-readat-keynotfound-race into master

@vchaindz
vchaindz merged commit f88bfb9 into master Jul 30, 2026
18 of 19 checks passed
@vchaindz
vchaindz deleted the fix/multiapp-readat-keynotfound-race branch July 30, 2026 07:43
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.

3 participants