fix: add test-only LockFile::read_pid for cross-platform testing
#316
Conversation
Fixes an issue encountered in the CI overhaul PR #253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
📝 WalkthroughWalkthroughFile handling in the lock file module was refactored to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dash-spv/src/storage/lockfile.rs (1)
60-71: Consider adding documentation about the Windows-specific testing rationale.The implementation is correct and properly scoped within the test module. However, adding a doc comment explaining that this method exists to avoid Windows exclusive-lock conflicts would improve maintainability.
📝 Suggested documentation improvement
impl LockFile { - /// Reads the PID from the lock file. + /// Reads the PID from the lock file using the same file handle. + /// + /// This method is test-only and reads from the same file handle to avoid + /// Windows-specific issues where opening a second file handle fails due to + /// the exclusive file lock. fn read_pid(&mut self) -> std::io::Result<u32> {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/storage/lockfile.rs
🧰 Additional context used
📓 Path-based instructions (5)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/storage/lockfile.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/lockfile.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Never hardcode network parameters, addresses, or keys
Files:
dash-spv/src/storage/lockfile.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types (thiserror) and propagate errors appropriately
Use tokio runtime for async operations
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality
Use secure random number generation for keys
Never log or expose private keys
Code must target Rust 1.89 minimum supported version (MSRV)
Format code using cargo fmt
Pass clippy linting without warnings
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
Files:
dash-spv/src/storage/lockfile.rs
**/{dash-network,dash-spv,key-wallet}/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use async/await for async operations in network and wallet modules
Files:
dash-spv/src/storage/lockfile.rs
🧠 Learnings (2)
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/storage/lockfile.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Unit tests should live alongside code with `#[cfg(test)]` annotation; integration tests use the `tests/` directory
Applied to files:
dash-spv/src/storage/lockfile.rs
🧬 Code graph analysis (1)
dash-spv/src/storage/lockfile.rs (1)
dash-spv/src/storage/manager.rs (1)
new(63-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: SPV Components Tests
- GitHub Check: Core Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (3)
dash-spv/src/storage/lockfile.rs (3)
3-3: LGTM: Import addition is appropriate.The
OpenOptionsimport is necessary for the new file opening approach and is correctly imported fromstd::fs.
17-22: LGTM: File opening logic correctly adds read capability.The change from
File::createtoOpenOptionswith explicit flags is correct. The flagsread(true).write(true).create(true).truncate(true)maintain the original behavior while adding read capability, which enables reading the PID from the same file handle and resolves the Windows exclusive-lock conflict mentioned in the PR objectives.
89-90: LGTM: Test correctly uses the new read_pid method.The test now reads the PID from the same file handle using
read_pid(), which resolves the Windows-specific test failure. The addition ofmutto the binding is necessary sinceread_pid()takes&mut self.
Open lock file with read+write access and add test-only
read_pidfunction that reads from the same file handle:Fixes an issue in Windows tests encountered in the CI overhaul PR #253 where Windows prevents reading from the lockfile via a second file handle due to exclusive file lock:
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.