[I] Topic message_expiry near u64::MAX wraps in segment expiry check and deletes every sealed segment [iggy]

["CloneOfAlex (via GitHub)" <gi...@apache.org>]

CloneOfAlex opened a new issue, #4601:
URL: https://github.com/apache/iggy/issues/4601

   ### Bug description
   
   ### What happens
   
   `Segment::is_expired` in `core/partitions/src/segment.rs` computes
   
   ```rust
   self.max_timestamp + duration.as_micros() <= now.as_micros()
   ```
   
   with an unchecked `u64` add. `message_expiry` reaches it from the client unbounded: `parse_u64_or` in `core/common/src/types/options/mod.rs` takes any raw `Uint64` option value, and HTTP create/update topic sends the JSON `u64` the same way. Only `u64::MAX` is the `NeverExpire` sentinel, so `u64::MAX - 1` is a valid `ExpireDuration`.
   
   In a release build the sum wraps to a value just below `max_timestamp`, the comparison is true, and every sealed segment of the topic counts as expired on the next segment cleaner pass. A topic configured for an effectively unlimited retention loses its sealed data instead. In a debug build the same line panics with `attempt to add with overflow`.
   
   The value is easy to reach by accident from the Rust SDK: `IggyDuration::as_micros` casts the `u128` from `Duration::as_micros` with `as u64`, so `IggyExpiry::ExpireDuration(Duration::from_secs(u64::MAX).into())` becomes `18446744073708551616` microseconds, inside the wrapping range.
   
   ### Reproduction
   
   The same expression with release semantics, for a segment sealed at `now`:
   
   ```
   u64::MAX - 1 (raw Uint64 option): expiry=18446744073709551614 -> is_expired=true
   Duration::from_secs(u64::MAX) via IggyDuration::as_micros: expiry=18446744073708551616 -> is_expired=true
   100 years: expiry=3153600000000000 -> is_expired=false
   ```
   
   Failing test (a PR with it marked `#[ignore]` will follow):
   
   ```rust
   #[test]
   fn very_long_expiry_never_expires_a_fresh_segment() {
       let now = IggyTimestamp::now();
       let mut segment = Segment::new(0, IggyByteSize::from(1024u64));
       segment.sealed = true;
       segment.max_timestamp = now.as_micros();
       let expiry = IggyExpiry::ExpireDuration(IggyDuration::from(u64::MAX - 1));
       assert!(!segment.is_expired(now, expiry));
   }
   ```
   
   ### Expected
   
   A very long expiry never expires a segment. Possible fixes, to agree on here first: a `checked_add`/`saturating_add` in `is_expired` (an overflow means "not expired"), and/or rejecting expiry values at admission that cannot be added to a timestamp. `IggyDuration::as_micros` could saturate instead of truncating.
   
   ### Notes
   
   Found during a code review of master `e7bd30d`;
   
   ### Affected area / component
   
   Iggy server
   
   ### Deployment
   
   None
   
   ### Versions
   
   server master e7bd30d (0.11.1-edge.1), Rust SDK master
   
   ### Hardware / environment
   
   _No response_
   
   ### Sample code
   
   _No response_
   
   ### Logs
   
   _No response_
   
   ### Iggy server config
   
   _No response_
   
   ### Reproduction
   
   1. Start a release build of the server.
   2. Create a topic with `message_expiry` = 18446744073709551614 (raw Uint64 topic option, or HTTP create-topic JSON), or from the Rust SDK with `IggyExpiry::ExpireDuration(Duration::from_secs(u64::MAX).into())`.
   3. Send enough messages to seal at least one segment.
   4. Wait for the segment cleaner interval: per the code path the sealed segments are treated as expired and deleted, although the retention is ~584,000 years. (Not yet run end to end against a server; verified at the expression level, see the description.)
   
   Unit-level: the failing test in the description.
   
   ### Contribution
   
   - [ ] I'm willing to submit a pull request to fix this bug
   
   ### Good first issue
   
   - [ ] I think this could be a good first issue for a new contributor


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Topic message_expiry near u64::MAX wraps in segment expiry check and deletes every sealed segment [iggy]

["Gabrieljr42 (via GitHub)" <gi...@apache.org>]

Gabrieljr42 commented on issue #4601:
URL: https://github.com/apache/iggy/issues/4601#issuecomment-6304199015

   Please assign this issue to me.
   
   I would treat an expiry instant that overflows as not expired, using checked_add in Segment::is_expired, and keep the existing inclusive boundary when the sum fits. That is the "overflow means not expired" option in the report. I will wait for the assignment and for agreement on that approach before opening a pull request.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org