[zupengwang]

Description

StorageBackend::RemoveFile() currently sleeps for 50 microseconds before acquiring its striped path mutex. StoreObject() already holds that same mutex across file creation, the complete write, and eviction-queue insertion, so acquiring the mutex serializes deletion with those operations. The fixed sleep adds latency without providing an additional ordering guarantee.

This change removes the delay and adds a concurrent store/remove regression test. The test races both operations on the same path 100 times and verifies that the final state is either a complete file or no file, never a partial file.

On a local ext4 Samsung SSD 870, deleting 10,000 one-byte file-per-key objects sequentially improved as follows over seven fresh-file runs (file creation excluded from timing):

Variant	Median	Mean	Min–max
main (a11f54e5)	1,339,407 us	1,348,307 us	1,320,589–1,421,868 us
Patched	116,142 us	118,107 us	113,686–126,515 us

The median bulk-removal time decreased by 91.33% (11.53x). This benchmark covers the single-threaded local POSIX file-per-key path; it does not measure network or GPU transfer performance.

Module

• Transfer Engine (mooncake-transfer-engine)
• Mooncake Store (mooncake-store)
• Reshard (mooncake-reshard)
• Mooncake EP (mooncake-ep)
• Mooncake PG (mooncake-pg)
• Integration (mooncake-integration)
• P2P Store (mooncake-p2p-store)
• Python Wheel (mooncake-wheel)
• Common (mooncake-common)
• Mooncake RL (mooncake-rl)
• CI/CD
• Docs
• Other

Type of Change

• Bug fix
• New feature
• Refactor
• Breaking change
• Documentation update
• Performance improvement
• Other

How Has This Been Tested?

Test commands:

storage_backend_test \
  --gtest_filter=StorageBackendTest.RemoveFileSerializesWithConcurrentStore \
  --gtest_repeat=10 --gtest_break_on_failure

storage_backend_test --gtest_repeat=3 --gtest_break_on_failure
file_storage_test --gtest_repeat=3 --gtest_break_on_failure
offload_on_evict_test --gtest_repeat=3 --gtest_break_on_failure

Test results:

• Unit tests pass
• Integration tests pass (not applicable; no public integration behavior changed)
• Manual benchmark done: seven baseline and seven patched fresh-file runs

The focused test passed 10/10 runs (1,000 total writer/remover races). The full binaries passed all three repetitions: 101/101 storage_backend_test, 18/18 file_storage_test, and 15/15 offload_on_evict_test per repetition.

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh
• I have run pre-commit on the files changed in this PR and all hooks pass
• Documentation is not applicable because no user-facing API or configuration changed
• I have added tests to prove my changes are effective
• The change is below 500 LOC, so an RFC is not required

The changed-line formatting script (LLVM 20), direct codespell, git diff --check, and EOF checks pass. The complete pre-commit wrapper did not finish because its Python 3.14 first-run hook environment setup stalled locally; this box remains intentionally unchecked until it is rerun successfully in a compatible environment.

AI Assistance Disclosure

• No AI tools were used
• AI tools were used (specify below)

OpenAI Codex was used to inspect the synchronization path, screen open issues and PRs for semantic overlap, prepare the focused test and benchmark, and draft this description. The human submitter reviewed every changed line and is responsible for the change.

[Copilot]

Pull request overview

Removes a fixed delay in StorageBackend::RemoveFile() that previously added latency without providing stronger ordering guarantees, relying instead on the existing striped per-path mutex for serialization with StoreObject(). Adds a regression test to validate concurrent store/remove behavior on the same path.

Changes:

• Removed the 50µs sleep in StorageBackend::RemoveFile() and documented the intended serialization behavior via the striped path lock.
• Added a concurrent store/remove regression test that races both operations repeatedly and asserts outcomes are either a fully-written file or no file.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
mooncake-store/src/storage_backend.cpp	Removes the timing-based deletion delay and documents lock-based serialization in RemoveFile().
mooncake-store/tests/storage_backend_test.cpp	Adds a regression test that races StoreObject() and RemoveFile() on the same path to prevent partial-file outcomes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[zupengwang]

Addressed in c6f30d80bd4f8e9615e5789b7e8eedcec72b5a58: the test now calls ASSERT_TRUE(backend.Init(0)) before using the backend. I also replaced the probabilistic post-join assertion with a deterministic FIFO barrier inside the store write critical section. The test verifies that RemoveFile remains blocked until the writer is released.

[fcczzz]

The change looks correct: the 50 µs sleep predates the striped path lock and provides no ordering guarantee, while StoreObject() and RemoveFile() already serialize the complete write/delete transitions under the same lock. I found no blocking issue.

Two non-blocking test suggestions:

1. Please call backend.Init(0) before using the backend, as the public contract requires initialization before other operations.
2. The current post-join “complete file or no file” assertion does not actually prove serialization; POSIX open/unlink behavior can satisfy it even if the path mutex is removed entirely. A deterministic barrier/fault-injection inside the write critical section would make the test verify that RemoveFile() is blocked until the write finishes.

[zupengwang]

@fcczzz Thanks for the review. I addressed both suggestions in c6f30d80bd4f8e9615e5789b7e8eedcec72b5a58:

• Call backend.Init(0) before using the backend.
• Replace the probabilistic race loop with a deterministic FIFO barrier. StoreObject fills the FIFO while holding the per-path write lock; the test confirms RemoveFile is still blocked, then drains the FIFO and verifies both operations complete in order. This keeps the synchronization entirely in the test and avoids adding a production test hook/API.

Validation:

• Focused test: 20/20 passes.
• Counterfactual with the RemoveFile path lock removed: 10/10 failures at the blocking assertion.
• Full storage_backend_test: 101 tests x 3 passes.
• file_storage_test: 18 tests x 3 passes.
• offload_on_evict_test: 15 tests x 3 passes.
• clang-format 20.1.8 --dry-run --Werror, codespell, and git diff --check pass.

[fcczzz]

LGTM