chore: Release polish — code quality, bug fixes, and dependency cleanup#136
Open
mcurrier2 wants to merge 1 commit into
Open
chore: Release polish — code quality, bug fixes, and dependency cleanup#136mcurrier2 wants to merge 1 commit into
mcurrier2 wants to merge 1 commit into
Conversation
Bug fixes: - Fix frame corruption in blocking UDS writev partial-write handling - Fix parse_duration_micros truncating fractional values (e.g. "1.5ms") - Add payload_len bounds check in SHM-direct to prevent OOB reads - Fix SHM-direct receive_blocking_timed using wrong timestamp - Add zero-length frame rejection in shared_memory_blocking - Replace panic with graceful error handling in TCP multi-server accept - Fix hardcoded memory_gb (16.0) with actual /proc/meminfo read - Fix hardcoded rust_version with env!(CARGO_PKG_RUST_VERSION) Performance: - Use .with_context(|| format!(...)) for lazy allocation in main.rs Dependency cleanup: - Remove dead dependencies: statistics, crossbeam, rand - Remove unused nix "time" feature (code uses libc::clock_gettime directly) - Fix placeholder repository URL to actual redhat-performance/rusty-comms - Remove dead docs.rs documentation URL (crate not published) Code cleanup: - Remove dead current_timestamp_ns() function from utils.rs - Consolidate identical Message::new/new_for_blocking into inline alias - Remove dead `if true` scaffolding in benchmark_blocking round-trip loop - Move misplaced test into mod tests block in ipc/mod.rs - Replace println! with tracing::trace! in all backpressure tests Documentation fixes: - Fix incorrect clock source, readiness signal, and CLI flag in README - Fix stale "no streaming support" comments (streaming is implemented) - Fix "zero-copy serialization" and "async I/O throughout" claims in lib.rs - Fix "JIT compilation" warmup comment (Rust is AOT compiled) - Rewrite logging.rs module docs to match actual exports - Fix SHM-direct docs (repr, max payload size) - Mark archived blocking plan as HISTORICAL/COMPLETE - Fix dashboard README example commands to use correct CLI flags All 476 tests passing. Zero clippy warnings. AI-assisted-by: Claude Opus 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis release-polish PR fixes eight correctness bugs (UDS write corruption, duration truncation, SHM-direct OOB read, SHM timestamp accuracy, SHM-blocking zero-length frames, TCP server panics, and hardcoded system metadata), removes dead dependencies/code, replaces println! with tracing in tests, and corrects documentation across README, lib.rs, logging.rs, and archived/dashboard docs. ChangesCorrectness Fixes Across Transports and System Info
Dependency and Code Cleanup
Documentation Accuracy
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SharedMemoryDirect as receive_blocking
participant SharedMemory as SHM buffer
Client->>SharedMemoryDirect: request message
SharedMemoryDirect->>SharedMemory: read payload_len
alt payload_len > MAX_PAYLOAD_SIZE
SharedMemoryDirect->>SharedMemory: clear ready flag, signal condvar
SharedMemoryDirect-->>Client: return error
else valid length
SharedMemoryDirect->>SharedMemory: copy_nonoverlapping payload
SharedMemoryDirect-->>Client: return Message with receive_time_ns
end
Related Issues: Suggested labels: bug, dependencies, documentation, cleanup Suggested reviewers: redhat-performance/rusty-comms maintainers familiar with IPC transports and results reporting 🐰 hops through the code, sweeping bugs from every burrow, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.rs (1)
838-857: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-finite input causes panic in
Duration::from_secs_f64.
num_str.parse::<f64>()accepts"inf","infinity", and"nan"(case-insensitive). These pass thenum < 0.0check (both comparisons arefalse), butDuration::from_secs_f64panics on non-finite values. A CLI argument like"infs"or"nans"will crash the process.🐛 Proposed fix
// Validate that the number is non-negative if num < 0.0 { return Err("Duration cannot be negative".to_string()); } + + if !num.is_finite() { + return Err("Duration must be a finite number".to_string()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.rs` around lines 838 - 857, The duration parsing logic in the `src/cli.rs` duration parser can panic because `num_str.parse::<f64>()` allows non-finite values that reach `Duration::from_secs_f64`. Update the parsing path to explicitly reject non-finite numbers right after parsing `num` (before the unit match), and return a normal error for `"inf"`, `"infinity"`, and `"nan"` inputs so the `Duration::from_secs_f64` calls remain safe.
🧹 Nitpick comments (3)
src/ipc/mod.rs (1)
351-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc on
Message::newaboutnew_for_blockingtiming.Lines 351-355 still claim
new_for_blocking()"captures the timestamp using a monotonic clock right before serialization," but the new implementation (Lines 366-374) is a pure alias toMessage::new, so the timestamp is captured at construction time exactly likenew(). The "right before serialization" precision actually comes fromset_timestamp_now()called separately in blocking send paths (seeunix_domain_socket_blocking.rs). This doc note is now misleading.📝 Proposed fix
/// ## Note for Blocking Mode /// - /// For accurate IPC latency measurement in blocking mode, use - /// `new_for_blocking()` instead which captures the timestamp using - /// a monotonic clock right before serialization. + /// `new_for_blocking()` is a semantic alias for this constructor used + /// in blocking transport code. For the most accurate IPC latency + /// measurement, call `set_timestamp_now()` right before serialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ipc/mod.rs` around lines 351 - 375, Update the stale documentation on Message::new and Message::new_for_blocking so it no longer says the blocking constructor captures time “right before serialization.” The current new_for_blocking method is only a semantic alias to Message::new and captures the timestamp at construction time, so revise the comments on both constructors to reflect that behavior. If you want to mention serialization timing, point readers to the blocking send path that calls set_timestamp_now() instead of implying new_for_blocking does it itself.Cargo.toml (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepository URL fixed here, but downstream references still stale.
The Containerfile (per provided context) still embeds
https://github.com/your-org/ipc-benchmarkin OCI image labels. Worth a follow-up to keep metadata consistent across the repo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 10, The repository URL has been updated in Cargo.toml, but the Containerfile still contains the old OCI metadata reference. Update the image labels in the Containerfile to use the same repository URL as the package metadata so the repo-wide references stay consistent. Use the Containerfile label definitions and the existing repository setting in Cargo.toml as the source of truth when making the change.src/results_blocking.rs (1)
1119-1147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the system metadata helpers.
The memory parser and MSRV helper duplicate
ResultsManager’s logic insrc/results.rs. A shared helper keeps async/blocking result metadata from drifting again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/results_blocking.rs` around lines 1119 - 1147, The system metadata helpers here duplicate the logic already used by ResultsManager in src/results.rs, so they should be centralized to avoid drift. Move or reuse the shared memory and MSRV helper logic from ResultsManager so both the blocking and async paths call the same implementation, keeping get_memory_gb and get_rust_version aligned with the existing metadata source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ipc/shared_memory_blocking.rs`:
- Around line 426-430: The read path in read_data_blocking() only checks
data_len against capacity, but it must also verify the full frame is currently
published before copying. Add the same incomplete-frame guard used in
read_data() in shared_memory_blocking.rs, right before the memcpy/copy loop, so
data_len + header size does not exceed the available published bytes and
read_pos is not advanced past write_pos on corrupted frames.
In `@src/results.rs`:
- Around line 1262-1265: The default system-info fallback in the non-Linux
branch now returns 0.0, but test_system_info_default still assumes memory_gb is
always positive. Update the test in test_system_info_default to match the new
contract from the system info logic in results::system_info (or its helper), so
it accepts 0.0 on non-Linux targets while preserving the existing assertion for
Linux.
---
Outside diff comments:
In `@src/cli.rs`:
- Around line 838-857: The duration parsing logic in the `src/cli.rs` duration
parser can panic because `num_str.parse::<f64>()` allows non-finite values that
reach `Duration::from_secs_f64`. Update the parsing path to explicitly reject
non-finite numbers right after parsing `num` (before the unit match), and return
a normal error for `"inf"`, `"infinity"`, and `"nan"` inputs so the
`Duration::from_secs_f64` calls remain safe.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 10: The repository URL has been updated in Cargo.toml, but the
Containerfile still contains the old OCI metadata reference. Update the image
labels in the Containerfile to use the same repository URL as the package
metadata so the repo-wide references stay consistent. Use the Containerfile
label definitions and the existing repository setting in Cargo.toml as the
source of truth when making the change.
In `@src/ipc/mod.rs`:
- Around line 351-375: Update the stale documentation on Message::new and
Message::new_for_blocking so it no longer says the blocking constructor captures
time “right before serialization.” The current new_for_blocking method is only a
semantic alias to Message::new and captures the timestamp at construction time,
so revise the comments on both constructors to reflect that behavior. If you
want to mention serialization timing, point readers to the blocking send path
that calls set_timestamp_now() instead of implying new_for_blocking does it
itself.
In `@src/results_blocking.rs`:
- Around line 1119-1147: The system metadata helpers here duplicate the logic
already used by ResultsManager in src/results.rs, so they should be centralized
to avoid drift. Move or reuse the shared memory and MSRV helper logic from
ResultsManager so both the blocking and async paths call the same
implementation, keeping get_memory_gb and get_rust_version aligned with the
existing metadata source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8d7885ae-cbae-46a7-be7a-186e5388cdc8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlREADME.mddocs/archive/blocking-mode-implementation-plan.mdsrc/benchmark_blocking.rssrc/cli.rssrc/ipc/mod.rssrc/ipc/posix_message_queue.rssrc/ipc/shared_memory.rssrc/ipc/shared_memory_blocking.rssrc/ipc/shared_memory_direct.rssrc/ipc/tcp_socket.rssrc/ipc/unix_domain_socket.rssrc/ipc/unix_domain_socket_blocking.rssrc/lib.rssrc/logging.rssrc/main.rssrc/results.rssrc/results_blocking.rssrc/utils.rsutils/dashboard/README.md
💤 Files with no reviewable changes (1)
- src/utils.rs
Comment on lines
+426
to
+430
| // Validate data length (reject zero or oversized frames) | ||
| if data_len == 0 || data_len > capacity { | ||
| libc::pthread_mutex_unlock(&self.mutex as *const _ as *mut _); | ||
| return Err(anyhow!( | ||
| "Invalid data length: {} exceeds capacity {}", | ||
| "Invalid data length: {} (capacity: {}, min: 1)", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate against published bytes, not just capacity.
This still trusts any corrupted data_len that is <= capacity. If data_len + 4 exceeds the bytes currently published in the ring, the copy at Lines 448-461 will read stale data and then advance read_pos past write_pos, permanently desynchronizing the buffer. read_data() already has the missing incomplete-frame check; read_data_blocking() needs the same guard before copying.
Patch
if data_len == 0 || data_len > capacity {
libc::pthread_mutex_unlock(&self.mutex as *const _ as *mut _);
return Err(anyhow!(
"Invalid data length: {} (capacity: {}, min: 1)",
data_len,
capacity
));
}
+ if self.available_read_data() < data_len + 4 {
+ libc::pthread_mutex_unlock(&self.mutex as *const _ as *mut _);
+ return Err(anyhow!(
+ "Incomplete message: length {} exceeds published bytes",
+ data_len
+ ));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Validate data length (reject zero or oversized frames) | |
| if data_len == 0 || data_len > capacity { | |
| libc::pthread_mutex_unlock(&self.mutex as *const _ as *mut _); | |
| return Err(anyhow!( | |
| "Invalid data length: {} exceeds capacity {}", | |
| "Invalid data length: {} (capacity: {}, min: 1)", | |
| // Validate data length (reject zero or oversized frames) | |
| if data_len == 0 || data_len > capacity { | |
| libc::pthread_mutex_unlock(&self.mutex as *const _ as *mut _); | |
| return Err(anyhow!( | |
| "Invalid data length: {} (capacity: {}, min: 1)", | |
| data_len, | |
| capacity | |
| )); | |
| } | |
| if self.available_read_data() < data_len + 4 { | |
| libc::pthread_mutex_unlock(&self.mutex as *const _ as *mut _); | |
| return Err(anyhow!( | |
| "Incomplete message: length {} exceeds published bytes", | |
| data_len | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ipc/shared_memory_blocking.rs` around lines 426 - 430, The read path in
read_data_blocking() only checks data_len against capacity, but it must also
verify the full frame is currently published before copying. Add the same
incomplete-frame guard used in read_data() in shared_memory_blocking.rs, right
before the memcpy/copy loop, so data_len + header size does not exceed the
available published bytes and read_pos is not advanced past write_pos on
corrupted frames.
Comment on lines
+1262
to
+1265
| #[cfg(not(target_os = "linux"))] | ||
| { | ||
| 0.0 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the default-system-info test with the new fallback.
Line 1262 returns 0.0 on non-Linux targets, but test_system_info_default still asserts memory_gb > 0.0 at Line 1949. That will fail outside Linux. If the fallback is intended, relax the assertion to match the documented contract.
Proposed test fix
- assert!(info.memory_gb > 0.0);
+ assert!(info.memory_gb.is_finite());
+ assert!(info.memory_gb >= 0.0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/results.rs` around lines 1262 - 1265, The default system-info fallback in
the non-Linux branch now returns 0.0, but test_system_info_default still assumes
memory_gb is always positive. Update the test in test_system_info_default to
match the new contract from the system info logic in results::system_info (or
its helper), so it accepts 0.0 on non-Linux targets while preserving the
existing assertion for Linux.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive code quality review and polish pass across the entire codebase in preparation for initial release. Addresses accumulated technical debt, documentation drift, dead code, and several latent bugs discovered during systematic file-by-file review.
Closes #135
Changes
Bug Fixes (8)
writevpartial-write fallback used incorrect offset calculation (written.saturating_sub(4)) that could corrupt message framing when fewer than 4 bytes were writtenparse_duration_microssilently truncated fractional values (e.g. "1.5ms" became "1ms") due toas u64castpayload_lenfrom shared memory beforecopy_nonoverlappingreceive_blocking_timedcaptured a new timestamp instead of using already-capturedmsg.receive_time_nsread_data_blockingdid not rejectdata_len == 0panic!in multi-server accept loop whentry_clone()failsget_memory_gb()returned16.0;rust_versionwas hardcodedDependency Cleanup (4)
statistics,crossbeam,randnix"time" featurerepositoryURL → actualredhat-performance/rusty-commsdocs.rsdocumentation URLCode Cleanup (5)
current_timestamp_ns()fromutils.rsMessage::new/new_for_blockinginto inline aliasif truescaffolding in blocking round-trip loopmod testsblockprintln!withtracing::trace!in backpressure testsDocumentation Fixes (8)
logging.rsmodule docs to match actual exportsPerformance (1)
.with_context(|| format!(...))for lazy allocation in main.rsValidation
-D warnings)cargo fmtcleanTest plan
cargo test— all 476 tests passcargo clippy --all-targets -- -D warnings— zero warningscargo fmt -- --check— no formatting changescargo build --release— builds successfullyStats
21 files changed, 241 insertions(+), 291 deletions(-) (net -50 lines)
Made with Cursor