chore: Release polish — code quality, bug fixes, and dependency cleanup#136
Open
mcurrier2 wants to merge 2 commits into
Open
chore: Release polish — code quality, bug fixes, and dependency cleanup#136mcurrier2 wants to merge 2 commits into
mcurrier2 wants to merge 2 commits 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 IPC transport, duration parsing, and system metadata issues, removes unused dependencies and code, replaces test ChangesCorrectness fixes across transports and system metadata
Dependency and code cleanup
Documentation accuracy
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Receiver
participant SharedMemoryDirect
participant SharedMemory
Receiver->>SharedMemoryDirect: receive_blocking()
SharedMemoryDirect->>SharedMemory: read payload_len
alt invalid payload length
SharedMemoryDirect->>SharedMemory: clear ready and signal condition variable
SharedMemoryDirect-->>Receiver: return error
else valid payload length
SharedMemoryDirect->>SharedMemory: copy payload
SharedMemoryDirect-->>Receiver: return message timestamp
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
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.
- Linux: reads /proc/meminfo for actual memory, falls back to 16.0 GB - macOS: runs sysctl -n hw.memsize for actual memory, falls back to 16.0 GB - Windows/other: returns 16.0 GB as safe default - Fixes CI test failures (test_system_info_default, test_get_system_info) on macOS and Windows runners where the previous implementation returned 0.0 for non-Linux platforms - Pre-commit hook: all checks pass except cargo audit (known upstream advisories in crossbeam-epoch, bincode, anyhow, rand — not our code) AI-assisted-by: Claude Opus 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/results.rs`:
- Line 1260: Update the fallback documentation preceding the implementations in
src/results.rs at lines 1260-1260 and src/results_blocking.rs at lines 1132-1132
to state that detection failures and unsupported platforms return 16.0 instead
of 0.0; no implementation changes are needed.
🪄 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: 078e5767-05da-43bd-8e05-0c45112ecd88
📒 Files selected for processing (2)
src/results.rssrc/results_blocking.rs
| } | ||
| } | ||
| } | ||
| 16.0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep fallback documentation aligned with the implementation.
Both changed implementations return 16.0 on detection failure or unsupported platforms, while their preceding documentation still promises 0.0.
src/results.rs#L1260-L1260: document the new16.0fallback.src/results_blocking.rs#L1132-L1132: document the new16.0fallback.
📍 Affects 2 files
src/results.rs#L1260-L1260(this comment)src/results_blocking.rs#L1132-L1132
🤖 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` at line 1260, Update the fallback documentation preceding the
implementations in src/results.rs at lines 1260-1260 and src/results_blocking.rs
at lines 1132-1132 to state that detection failures and unsupported platforms
return 16.0 instead of 0.0; no implementation changes are needed.
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