Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #78 by removing unnecessary Clippy suppressions and making timestamp-to-u64 conversions explicitly safe via clamping, improving robustness in the tunnel heartbeat paths.
Changes:
- Removed an unused
#[allow(clippy::cast_possible_truncation)]in the client handshake code path. - Updated client heartbeat timestamp generation to clamp
u128millis intou64::MAXbefore casting. - Updated server heartbeat-ack timestamp generation to clamp
u128millis intou64::MAXbefore casting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ferrotunnel-core/src/tunnel/client.rs | Removes an unused Clippy allow and clamps heartbeat timestamps before casting to u64. |
| ferrotunnel-core/src/tunnel/server.rs | Clamps heartbeat-ack timestamps before casting to u64, removing the local Clippy suppression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let ts = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis() as u64; | ||
| .as_millis() | ||
| .min(u128::from(u64::MAX)) | ||
| as u64; |
There was a problem hiding this comment.
The timestamp clamping logic is duplicated here and in tunnel/server.rs. Consider extracting a small helper (e.g., timestamp_millis_u64() or clamp_u128_to_u64()) so the conversion policy lives in one place and can be unit-tested independently.
| timestamp: std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis() as u64, | ||
| .as_millis() | ||
| .min(u128::from(u64::MAX)) | ||
| as u64, |
There was a problem hiding this comment.
This timestamp clamping logic now exists in both client and server paths. Consider extracting it into a shared helper (in core/common) to avoid divergence and make the conversion policy easy to test/change in one place.
|
@millia385 thanks again :) can you make two minor changes? and its ready to merge |
MitulShah1
left a comment
MitulShah1
left a comment
There was a problem hiding this comment.
needed to fix minor changes
|
Is this okay? @MitulShah1 |
Hey Thanks, looks good now. |
Hello! 🐧
This PR closes #78.
Changes
ferrotunnel-core/src/tunnel/client.rs
ferrotunnel-core/src/tunnel/server.rs
Testing
make check- no issuescargo test- no issuesAll of the binaries built correctly.
There should be no changes to the functionality of the code, besides improved safety.
Please review, and have a nice day ❤️