Repository navigation
fix(storage): file compare-and-swap no longer deadlocks recover_wal; StorageCas closures are now 'static - #2003
Conversation
zonotope
added
bug
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
This is a clean & clever fix @zonotope. No real headline notes from me other than that I still noticed some of the surprisingly slow wall-clock time conditions described in #2001 even from HEAD of this PR branch. More details in the review below. I don't think that's a hold on merging this, but if you're also able to reproduce what I describe in the following review, it might be cause to either open a narrower follow-up issue or to reclarify this as not entirely fixing/closing #2001. I'll leave it up to you.
Running the read, the closure and the write in one blocking task removes the guard hand-off outright instead of making recover_wal cleverer, and moving the comments onto the code's own names (root gate, key stripe, key lock) made the locking much easier to follow. I reproduced the mechanism: with compare_and_swap put back into the two-hop shape (read in one spawn_blocking, guard handed back to the async task, closure, write in a second spawn_blocking), recover_wal_on_a_runtime_thread_does_not_deadlock_an_in_flight_cas fails at its 10 s timeout, and at the head it passes. I also checked the "no async task holds the root gate" claim against every begin_operation caller — delete_key_blocking (called only inside spawn_blocking at file.rs:1584 and :1602), the write path at :1718, blocking_insert at :1789, and locked_read_blocking at :1842 — and all of them now run on blocking threads. The other StorageCas impls don't need the same treatment: MemoryStorage does read, closure and write under one sync lock with no await, S3Storage has no root gate (conditional PUT plus retry), and EncryptedStorage only delegates. That's also why fluree-db-crypto is in the diff, since a 'static closure can't borrow self; I ran a quick encrypted compare-and-swap round trip over MemoryStorage (the closure sees plaintext, storage holds ciphertext) and it passes at the head.
A few small notes inline, all optional. Two more that have no line to sit on:
- This is more of a question than a suggestion. The PR title feeds the release notes, and with the
breaking-changelabel "Fix CAS Deadlock" will be the entry under Breaking Changes. Something that names the API change would help whoever upgrades, e.g. "fix(storage): file compare-and-swap no longer deadlocks recover_wal; StorageCas closures are now 'static". - One thing I noticed while reproducing: the 28–37 s passing runs in #2001 may be a different cost. With the two-hop shape restored,
file_based_indexing_then_new_connection_loads_and_queriesstill passes alone in 0.23–0.30 s (5 of 5) with a freshTMPDIR. On my machine at this head it once took 188 s, andsampleput all of that inDiskArtifactCache::ensure_capacity→scan_cache_entries, walking a$TMPDIR/fluree_binary_cachethat every test binary shares (about 400k entries and 4.1 GB here), not inrecover_wal. I can't tell what your environment looked like, so this is a hunch rather than a finding — but since this PR closes #2001, it may be worth a line on the issue so the slow-run half doesn't lose its tracking.
Adherence to repo commitments:
- Patterns/abstractions: ✔ reuses
wal::key_lock/wal::read_optand the one-blocking-hop idiom every otherbegin_operationsite already uses; no new construct. - Performance (speed first, memory second): ✔ the commit-path compare-and-swap now pays one blocking hop instead of two and clones
FileStorageonce instead of twice; no new lock; the closure's serde work moves off the async worker; andd8521bddfdrops acreate_dir_allfrom every insert. No performance-degradation risk. - Deployment targets: ✔
FileStorageandwalarecfg(all(feature = "native", not(target_arch = "wasm32"))), so wasm32 only sees the trait signature — I ran the CI wasm32 clippy command locally and it's clean, as are CI'swasm32andwasm-smoke. Solo's standalonefluree-aibuildsFlureeBuilder::file, and its integration tests build a writer and a reader on one path in one process, which is exactly this shape, so the fix reaches it at the next pin bump; solo neither calls nor implementsStorageCas, so the breaking change needs no solo code change. The Lambda hosts use S3, which only sees the signature. A blocking task now finishes its compare-and-swap even if the caller is cancelled, which is fine on the long-lived hosts that useFileStorage. - Testing: ✔ the new unit test is wired into
fluree-db-core's lib tests and goes red on the old shape;key_lock_recreates_a_removed_parent_directorygoes red without theNotFoundretry; CI ran all four new or changed tests by name ata3b74997a. - Conventions:
⚠️ the five commits are subject-only, though the PR body carries the full rationale; fmt and clippy are clean;docs/design/storage-traits.mdlags the new bounds (inline).
Verified locally at a3b74997a: fluree-db-core (942), fluree-db-nameservice (169) and fluree-db-crypto --features nameservice (28) lib tests green; grp_transact cow-cancel and grp_index file-indexing tests green; two-hop and no-retry mutations each turned their test red and were restored; wasm32 clippy (-D warnings) clean.
Approving so you can merge when ready, but maybe worth considering the inline notes first — they're minor, but if you agree they're right, I'd rather see them folded in now than lost in the backlog.
| // Yield so the compare-and-swap task starts its blocking task. | ||
| // Then block the runtime thread until the read has finished. | ||
| tokio::task::yield_now().await; | ||
| std::thread::sleep(Duration::from_millis(200)); |
There was a problem hiding this comment.
🟡 Optional — this sleep makes the regression check timing-dependent.
The test relies on the blocking read finishing inside these 200 ms. When it doesn't (a busy runner), recover_wal takes the root gate before the compare-and-swap does, and the old two-hop shape passes too — so the test can go green on exactly the regression it guards against. And when the read does finish in time, the whole compare-and-swap has usually finished as well, so on the fixed path recover_wal rarely ends up waiting on an in-flight CAS at all.
A deterministic variant: signal from inside the closure, where the root gate is held, and block the runtime thread on that signal instead of sleeping. On the fixed shape the closure runs on the blocking thread, so the signal arrives and recover_wal waits on a CAS that really is in flight; on the old shape the closure needs the blocked runtime thread, so the recv_timeout fails every time. I ran this variant next to yours: green 3 of 3 at the head, red on the two-hop shape (the CAS closure needed the blocked runtime thread: Timeout). Illustrative, replacing the tokio::spawn through the sleep:
let (in_f_tx, in_f_rx) = std::sync::mpsc::channel::<()>();
let cas = tokio::spawn({
let storage = storage.clone();
async move {
storage
.compare_and_swap(HEAD, move |_| {
// The root gate is held here.
let _ = in_f_tx.send(());
std::thread::sleep(Duration::from_millis(100));
Ok(CasAction::<()>::Write(b"x".to_vec()))
})
.await
}
});
tokio::task::yield_now().await;
// Block the runtime thread until the closure runs, so `recover_wal` meets a CAS in flight.
in_f_rx
.recv_timeout(Duration::from_secs(5))
.expect("the CAS closure needed the blocked runtime thread");
FileStorage::new(dir.path()).recover_wal().unwrap();
cas.await.unwrap().unwrap();This is minor and non-blocking — but if you agree it's right, I'd rather see it folded in now than lost in the backlog.
| F: Fn(Option<&[u8]>) -> std::result::Result<CasAction<T>, StorageExtError> | ||
| + Send | ||
| + Sync | ||
| + 'static, |
There was a problem hiding this comment.
🟡 Optional — docs/design/storage-traits.md:322-333 still shows the old bounds.
That mdBook page (in docs/SUMMARY.md:113) restates StorageCas with F: Fn(..) + Send + Sync and T: Send. Since this PR carries the breaking-change label and that page is where someone implementing the trait looks first, it seems worth having the snippet carry + 'static along with the one-sentence reason from the doc comment here. It's a two-line change, so I'd rather see it folded in here than tracked separately.
Commenting here because docs/design/storage-traits.md is not in this diff.
| // Lock released when `locked` is dropped (on Abort path, dropped here) | ||
| }) | ||
| .await | ||
| .unwrap_or_else(|e| match e.try_into_panic() { |
There was a problem hiding this comment.
🟡 Optional — the panic-resume arm from d629fe182 has no test.
If this ever regresses into the Err arm, a panic in a nameservice closure comes back as StorageExtError::io("spawn_blocking join: …"), which a caller can't tell apart from an I/O failure. I wrote a quick #[should_panic(expected = "…")] test that panics inside the closure, and it passes at the head; a second one confirmed the key lock is released afterwards (the next compare-and-swap on the same key writes inside 5 s). Either would pin this in a few lines. Minor and non-blocking — but if you agree, I'd rather it land with this PR than end up in the backlog.
zonotope
changed the title
2 participants
Fixes #2001
Summary
This PR fixes two intermittent CI failures in
fluree-db-api.it_indexing_workflow::file_based_indexing_then_new_connection_loads_and_querieshangs until nextest kills it at 360 s. The cause is a deadlock inFileStoragebetweenrecover_waland an in-flight compare-and-swap. The fix is in the storage layer.it_cached_handle_cow_cancel::cancelled_commit_never_exposes_the_empty_cache_slotfails at its 60 s deadline. The cause is a race in the test itself. The fix is in the test.The deadlock
FlureeBuilder::file(..).build()callsFileStorage::recover_wal.recover_waltakes the root gate's write lock withfutures::executor::block_on, which blocks the calling thread. The root gate is onetokio::sync::RwLockper storage root, shared by everyFileStoragehandle in the process.FileStorage::compare_and_swapholds a read guard on the root gate from its read to its write. It ran the read and the write as twospawn_blockingtasks and ran the closure on the async task between them. The read guard therefore passed through the async task, first in the read's finishedJoinHandleoutput and then in the task's own state. Only a runtime thread can move it on from there.If
recover_walblocks the only runtime thread while a compare-and-swap is between its read and its write, neither can make progress. On a current-thread runtime this is permanent. The test hits it because it builds a second instance on a root where two indexers and a GC task are publishing through the nameservice, which uses compare-and-swap.The same deadlock can occur outside tests. Any sync
build()of a file-backed instance on a root that another handle in the process is writing to can hit it. On a multi-thread runtime it needs every worker thread blocked at once, so it is much less likely there.The fix
FileStorage::compare_and_swapnow runs the read, the closure, and the write in onespawn_blockingtask. The read guard is taken and released on that blocking thread. No async task holds the root gate at any point, sorecover_walonly waits for blocking-pool work that can finish on its own. Every otherbegin_operationcall site already ran on a blocking thread.The blocking task enters the caller's tracing span, so the closure's events and the
cas phasesevent stay under the caller's span.locked_read_blockingandlocked_write_blockingreplace the asyncblocking_locked_readandblocking_locked_write. They are synchronous and must be called off the async runtime. The read takes its key lock throughwal::key_lock, whichblocking_insertalready uses. The comments on this path use the code's names for the root gate, key stripe, and key lock, and theOperationHoldandLockedFilefields are renamed to match.Breaking change
StorageCas::compare_and_swapnow requires a'staticclosure and result type:This lets an implementation run the closure off the calling task. Callers whose closures borrow local values must move owned values in instead. In most cases this means adding
moveand cloning any value that is still used after the call. Generic wrappers that forward a closure need the same bounds.EncryptedStoragenow moves anArcof its key provider into its closure.All implementations and call sites in the workspace are updated. Nothing in the crates excluded from the workspace uses
StorageCas.The cow-cancel test race
The test detected the commit window by polling
handle.is_locked()every millisecond. The commit takes the optimistic path, which holds the write lock only for the commit itself. On a busy runner the commit could take and release the lock between two polls. The poll then ran until its 60 s deadline.The test now wraps
MemoryStoragein aCommitGatethat parks the next commit-blob write once armed. The commit blob is written inside the commit window, under the write lock, so a parked write means a commit is in flight. The test assertsis_locked()at that point, cancels the caller, and then releases the write.Testing
recover_wal_on_a_runtime_thread_does_not_deadlock_an_in_flight_cas(new,fluree-db-core) blocks a current-thread runtime inrecover_walwhile a compare-and-swap is between its read and its write. It passes with this change. With the read's guard handed back to the async task, it deadlocks and fails at its 10 s timeout.t = 0assertion when the commit is awaited inline in place of the shielded task.fluree-db-api: 4536 passed on the final commit, excluding the testcontainers binaries.fluree-db-corestorage tests: 133 passed on the final commit.wal::key_lockcleanup:file_based_indexing_then_new_connection_loads_and_queriespassed 150 of 150, and the cow-cancel test passed 200 of 200. Run alone, the indexing test takes about 0.06 s.fluree-db-core,fluree-db-nameservice, andfluree-db-crypto: 1173 passed, before the same cleanup.-D warningson the touched crates, and the wasm32 clippy job, pass.Not run locally:
it_iceberg_direct,it_storage_s3_testcontainers, andit_vended_credentials_testcontainersneed Docker.S3Storageonly changed itscompare_and_swapsignature. CI is the first run of those tests against this change.Related
#1851 reports a hang with the same shape.
merge_time_travel_survives_reload_from_commitsis a current-thread test that rebuilds a file-backed instance on a root the first instance was writing to. No stack was captured for that hang, so this PR leaves the issue open.