Skip to content

fix: don't hold the storage mapping cache guard across the database query - #740

Open
dispather wants to merge 1 commit into
nextcloud:mainfrom
dispather:fix/storage-mapping-guard-across-await
Open

fix: don't hold the storage mapping cache guard across the database query#740
dispather wants to merge 1 commit into
nextcloud:mainfrom
dispather:fix/storage-mapping-guard-across-await

Conversation

@dispather

Copy link
Copy Markdown

What

get_storage_mapping holds a DashMap shard read guard across the .await of the storage-mapping database query. This drops the guard before the query instead.

Fixes #739

Why

dashmap::mapref::one::Ref is a read guard on the map shard. Taking the write lock for insert blocks the calling thread, not just the task — dashmap 6.x builds its RawRwLock on lock_api + parking_lot_core, so a waiting writer parks the OS thread.

So while load_storage_mapping is in flight:

  1. Task A holds the shard read lock for the whole DB round-trip.
  2. Any task reaching self.cache.insert(...) for the same shard parks a Tokio worker thread.
  3. If enough workers park, none of them reaches its park point, so nobody polls the I/O driver.
  4. A's DB response is therefore never delivered, A never completes, and the guard is never released. The state is self-sustaining — only a restart recovers it.

On my instance this reproduces every 15 hours to 10 days: the process stays alive at 0% CPU with flat memory, both the main HTTP port and --metrics-port stop responding at the same instant, and no further log line is written. Across five freeze captures, zero threads were in epoll_wait while every thread sat in futex_wait; a healthy process of the same build has exactly one thread in epoll_wait. Full write-up in the linked issue.

This was introduced in 1.3.3 by the updating flag refactor (avoid too many concurrent db queries when cache becomes invalid). 1.3.2 did not hold the guard across the await — Option::filter consumed and dropped the Ref before the query. 1.3.4, 1.3.5 and current master are unchanged.

How

The stale-refresh branch and the cache-miss branch had identical tails, so they collapse into one path once the guard is released early:

  • take the guard, return it if the entry is still valid, otherwise flag the refresh and drop the guard;
  • run the query with no guard held;
  • on error, re-acquire briefly to clear the updating flag;
  • insert and return.

No behaviour change other than the lock hold time: a valid entry is still returned under the read guard without querying, a stale entry still serves through updating, and a failed query still clears the flag.

Testing

cargo check --all-targets, cargo clippy --all-targets, cargo test --lib (2 passed) and cargo build all pass on rustc 1.94.0.

Since both shapes compile, I checked separately that the change actually shortens the lock hold. Reducing each shape to a DashMap plus a sleep, and timing an insert on the same key issued while the future is suspended:

before   same-shard insert blocked for 301.2ms   (guard held across the await)
after    same-shard insert took       13.6µs     (guard released before the await)

and that the original shape deadlocks rather than just stalling, once the runtime's workers are occupied by same-shard writers — the task that would wake the guard holder never gets scheduled:

before   1 worker  + 4  concurrent same-shard writers -> no progress within 3s
before   2 workers + 8  concurrent same-shard writers -> no progress within 3s
before   4 workers + 16 concurrent same-shard writers -> no progress within 3s
after    4 workers + 16 concurrent same-shard writers -> completes normally

I did not add this as a regression test in the PR: the reduction is faithful to the locking shape but not to get_storage_mapping itself, and testing the real function needs a controlled stall inside fetch_all plus a shard collision, which I couldn't make non-flaky. Happy to add either version if you'd like one.

I have not been able to prove which lock the threads park on: wchan only says "in a futex", and unwinding the release binary from outside its container fails (static-PIE, no frame pointers — eu-stack and gdb --sysroot both stop at the syscall stub). I've instrumented my instance to dump /proc/<pid>/task/*/syscall at the next freeze and cluster the futex addresses; shared addresses would confirm lock contention, all-distinct addresses would mean the cause is elsewhere. I'll report back either way.

…uery

`DashMap::get` hands out a read guard on the map shard, and taking the
write lock for `insert` parks the calling thread rather than just the
task. Holding that guard across the storage mapping query therefore
blocks any worker that inserts into the same shard while the query is in
flight; once that starves the runtime, nothing polls the reactor, the
query never completes and the guard is never released, so the process
stops serving until it is restarted.

The stale-refresh and cache-miss branches had identical tails, so they
collapse into a single path once the guard is dropped before the query.

Signed-off-by: dispather <62810211+dispather@users.noreply.github.com>
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.

Push server becomes permanently unresponsive after hours-to-days: DashMap guard held across .await in get_storage_mapping

1 participant