fix(refresh): keep per-refresh state per refresh, not on singleton services - #248
Open
jordanfelle wants to merge 2 commits into
Open
jordanfelle wants to merge 2 commits into
jordanfelle wants to merge 2 commits into
Conversation
RefreshAuthor commands for different authors run concurrently (RefreshAuthorCommand neither requires disk access nor is type exclusive, and there are several command threads), but the singleton RefreshBookService and RefreshAuthorService kept per-refresh state in plain instance fields: - RefreshBookService._bookMetadataCache (a Dictionary cleared at the start of every RefreshBookInfo while other refreshes read and write it) is now held per async flow. - RefreshAuthorService._authorRefreshRehomeBlueprint (the remote snapshot used to re-home editions, set in GetRemoteChildren and nulled in a finally) is now held per async flow, so one refresh can no longer re-home editions using another author's snapshot or null it under a refresh still using it. - The one-entry memos _editionRefreshMatchingIndex and _bookRefreshMatchingIndex checked the shared field and then read it again for the return value, so a concurrent refresh could swap it in between and the caller got another list's index. The field is now read once. Tests fail on the previous code (two-flow isolation for the cache and the blueprint, and a contention stress test for the memos).
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
The single-book refresh paths (RefreshBookInfo(Book) and the overload with a remote author) reached GetSkyhookData and its cache without ever opening or closing a cache; only the list overload reset it. With the cache now held per async flow, an executor thread kept the last cache between commands, so single-book refreshes could be served stale author metadata and the cache grew without bound. Open a scope (fresh cache, previous one restored on dispose, also on exceptions) at all three entry points.
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #246.
Problem
RefreshAuthorCommandneither requires disk access nor is type exclusive, so per-author refreshes run concurrently (3 command threads by default, more with #222), but singleton services kept per-refresh state in plain fields:RefreshBookService._bookMetadataCache: a plainDictionarycleared at the start of every refresh while other refreshes read and write it (corruption, exceptions, wrong metadata).RefreshAuthorService._authorRefreshRehomeBlueprint: one refresh could re-home editions from another author's snapshot, or null it under a refresh still using it._editionRefreshMatchingIndex/_bookRefreshMatchingIndex: one-entry memos that read the shared field twice, so a concurrent refresh could swap it between the reads and the caller got another list's index.Fix
The first two are held in an
AsyncLocal(one per refresh; the same approach as #218 and #181); the memos read the field once into a local. Single-threaded behaviour is unchanged.AuthorStatisticsService._deferredFileAuthorIdsis a lock-protected shared debounce and is intentionally left alone.Verification
Three tests that fail on the original code: two deterministic two-flow tests (metadata cache, rehome blueprint) and a contention test for the matching indexes (failed within 22 ms on the old code) (3,023 core tests pass). Note: touches lines near #218/#181 in
RefreshBookService, so merging with those needs a trivial rebase.Review update: the metadata cache is also reached from the single-book
RefreshBookInfo(Book)path and the overload that takes a remote author, which never reset it. With a per-flow cache an executor thread that mostly runs single-book refreshes would have kept the previous cache between commands (stale author metadata, unbounded growth), so a scope helper now opens a fresh cache and restores the previous one on dispose (also on exceptions) at all three entry points. Test covers the scope semantics (3,024 core tests pass).