Store does synchronous filesystem I/O — including a lock poll that can
std::thread::sleep for up to 5 s — directly inside async fns, with no
spawn_blocking. Each such call occupies a tokio worker thread for its whole
duration instead of yielding.
Found by the third review round on the 0.8.13 release (#580). Pre-existing —
it is in main today — but newly exercised by call sites 0.8.13 adds.
Measured scope
Nine call sites, all reached from an async fn:
| Location |
Calls |
crates/doiget-core/src/orchestrator.rs:2389 |
store.write |
crates/doiget-mcp/src/lib.rs — 1297, 1381, 2002, 2074, 2529, 2601, 2672, 2749 |
read / write / write_user_authored |
The CLI has none. Every store call in commands/bib.rs and
commands/csl.rs is in a synchronous fn; the only async-path write reaches
the store through orchestrator.rs:2389.
The blocking window, from crates/doiget-core/src/store/fs_store.rs:
LOCK_TIMEOUT = 5 s, LOCK_POLL_INTERVAL = 50 ms → up to 100 iterations of
std::thread::sleep(50 ms) in acquire_lock while a worker is held
- plus the uncontended cost of
std::fs::read_to_string / atomic_write
(write_all + sync_all + rename)
Why this is not a release blocker
Recorded so the next person does not have to re-derive it:
- It is not a correctness bug. Nothing produces a wrong answer; requests
get slower under contention.
- It cannot break the arXiv pacing obligation. A blocked worker delays the
rate limiter's sleep_until wake-up, so a paced request goes out later
than its earliest-allowed instant, never earlier. The failure is in the safe
direction for docs/LEGAL.md safeguard 8.
- Contention needs two concurrent writes to the same safekey. Distinct DOIs
take distinct lock files, so a normal batch does not contend.
When it does bite
- Duplicate refs in one
doiget batch, or a metadata-only write racing a PDF
write on the same ref.
- A store root on a network or sync-backed filesystem (Dropbox, OneDrive,
SMB). There sync_all costs hundreds of milliseconds without any
contention, so every write blocks a worker on every fetch. This is the case
worth caring about.
- A long-running
doiget serve: workers held by store I/O also delay the
rate limiter's timers and any other in-flight tool call.
Fix
Wrap the nine sites in tokio::task::spawn_blocking. The data is already
movable — Metadata and Safekey both derive Clone — so each site clones
the key, the metadata and the optional PDF path and moves them in; the store
handle needs to be held as an Arc.
What does not work: making only the lock poll async. acquire_lock would
have to become async, which makes Store::write async, which makes the
whole trait async — a full public-API change (Store is listed in
docs/PUBLIC_API.md) — and it still leaves fs::write + sync_all blocking.
Acceptance
- No
Store method is called directly from an async fn; the nine sites go
through spawn_blocking.
- A test that would fail if one regressed — e.g. asserting the tokio runtime
stays responsive while a contended write is in flight.
docs/STORE.md says which calling convention the trait expects, since the
trait itself cannot express it.
Scoped deliberately as its own PR for 0.8.14 rather than folded into the
0.8.13 cut: nine async call sites is a change that wants its own review, not
one shared with a release.
Storedoes synchronous filesystem I/O — including a lock poll that canstd::thread::sleepfor up to 5 s — directly insideasync fns, with nospawn_blocking. Each such call occupies a tokio worker thread for its wholeduration instead of yielding.
Found by the third review round on the 0.8.13 release (#580). Pre-existing —
it is in
maintoday — but newly exercised by call sites 0.8.13 adds.Measured scope
Nine call sites, all reached from an
async fn:crates/doiget-core/src/orchestrator.rs:2389store.writecrates/doiget-mcp/src/lib.rs— 1297, 1381, 2002, 2074, 2529, 2601, 2672, 2749read/write/write_user_authoredThe CLI has none. Every store call in
commands/bib.rsandcommands/csl.rsis in a synchronousfn; the only async-path write reachesthe store through
orchestrator.rs:2389.The blocking window, from
crates/doiget-core/src/store/fs_store.rs:LOCK_TIMEOUT = 5 s,LOCK_POLL_INTERVAL = 50 ms→ up to 100 iterations ofstd::thread::sleep(50 ms)inacquire_lockwhile a worker is heldstd::fs::read_to_string/atomic_write(
write_all+sync_all+rename)Why this is not a release blocker
Recorded so the next person does not have to re-derive it:
get slower under contention.
rate limiter's
sleep_untilwake-up, so a paced request goes out laterthan its earliest-allowed instant, never earlier. The failure is in the safe
direction for
docs/LEGAL.mdsafeguard 8.take distinct lock files, so a normal batch does not contend.
When it does bite
doiget batch, or a metadata-only write racing a PDFwrite on the same ref.
SMB). There
sync_allcosts hundreds of milliseconds without anycontention, so every write blocks a worker on every fetch. This is the case
worth caring about.
doiget serve: workers held by store I/O also delay therate limiter's timers and any other in-flight tool call.
Fix
Wrap the nine sites in
tokio::task::spawn_blocking. The data is alreadymovable —
MetadataandSafekeyboth deriveClone— so each site clonesthe key, the metadata and the optional PDF path and moves them in; the store
handle needs to be held as an
Arc.What does not work: making only the lock poll async.
acquire_lockwouldhave to become
async, which makesStore::writeasync, which makes thewhole trait async — a full public-API change (
Storeis listed indocs/PUBLIC_API.md) — and it still leavesfs::write+sync_allblocking.Acceptance
Storemethod is called directly from anasync fn; the nine sites gothrough
spawn_blocking.stays responsive while a contended write is in flight.
docs/STORE.mdsays which calling convention the trait expects, since thetrait itself cannot express it.
Scoped deliberately as its own PR for 0.8.14 rather than folded into the
0.8.13 cut: nine async call sites is a change that wants its own review, not
one shared with a release.