Skip to content

Conversation

@vvagias
Copy link

@vvagias vvagias commented Jan 27, 2026

Summary

  • Replace blocking std::fs with non-blocking tokio::fs in async functions
  • Use filetime::set_file_mtime() instead of opening files to update mtime
  • Optimize lock patterns: collect items then release lock before I/O
  • Remove TOCTOU race conditions

Performance

  • Before: ~4% latency overhead with --store-kv file
  • After: ~0% latency overhead (matches etcd performance)

Test plan

  • Unit tests pass (cargo test -p dynamo-runtime storage::kv)
  • Local latency benchmarks show 0% overhead vs direct vLLM

Summary by CodeRabbit

  • Chores

    • Added new dependency for file timestamp utilities.
  • Bug Fixes

    • Improved file system reliability through enhanced error handling and robust path validation.
  • Refactor

    • Optimized file storage operations with asynchronous processing and improved concurrency handling to reduce lock contention and enhance performance.

✏️ Tip: You can customize this high-level summary in your review settings.

Replace blocking std::fs operations with non-blocking tokio::fs in async
functions to prevent blocking the tokio runtime. This eliminates latency
overhead when using --store-kv file for local development.

Changes:
- Use tokio::fs::{read,write,remove_file,read_dir} instead of std::fs
- Use filetime::set_file_mtime() instead of opening files to update mtime
- Optimize lock patterns: collect items then release lock before I/O
- Remove TOCTOU race: try operation directly, handle NotFound error

Before: ~4% latency overhead with file store
After: ~0% latency overhead (matches etcd performance)

Signed-off-by: Vasilis Vagias <vvagias@nvidia.com>
@vvagias vvagias requested a review from a team as a code owner January 27, 2026 21:06
@copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 27, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions
Copy link

👋 Hi vvagias! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions bot added external-contribution Pull request is from an external contributor perf labels Jan 27, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

Walkthrough

This pull request introduces the filetime dependency and refactors the file storage I/O layer to use asynchronous tokio operations instead of synchronous std::fs calls. The changes include improved lock management to reduce contention, TOCTOU vulnerability mitigations, and updates to file timestamp handling via the filetime crate.

Changes

Cohort / File(s) Summary
Dependency Addition
lib/runtime/Cargo.toml
Adds filetime crate (v0.2) as a new dependency for file modification time operations.
Async I/O Migration & Lock Optimization
lib/runtime/src/storage/kv/file.rs
Converts synchronous std::fs operations to asynchronous tokio::fs throughout read/write/delete flows. Replaces OpenOptions-based file modification with filetime-based mtime setting in keep_alive. Refactors expiry and deletion logic to collect directory entries under lock but perform I/O outside the lock, reducing contention. Updates directory entry handling, metadata retrieval, and watch event processing for async non-blocking operations. Improves path canonicalization, validation, and error handling; reduces TOCTOU vulnerabilities.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 From sync to async, the files now fly,
Lock contention fades as operations pass by,
Filetime marks moments with swifter care,
TOCTOU shadows vanish in the tokio air! 🚀

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description covers key sections but deviates from the template structure with custom formatting instead of following the required Overview, Details, Where to start, and Related Issues sections. Restructure the description to follow the template: add an Overview section, reorganize Details under that heading, specify files for reviewer focus, and include Related Issues with action keywords.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: optimizing the file-based KV store by switching to async I/O operations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor perf size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant