storage: fix integer overflow in max_timestamp

[jcsp]

Cover letter

Batches with timestamps behind the base_timestamp of the segment could have negative relative timestamp, which overflows when cast to uint32_t to insert to index_state::relative_time_index.

index_state::relative_time_index is later used to update max_timestamp when truncating the log. Hence max_timestamp shooting about 6 weeks into the future.

Fixes #3924

Release notes

• none

[dotnwat]

nice catch. there is something here that needs fixed. two things I'm thinking about, but not sure of:

1. iirc the only query we do on time is related to finding a lower bound. so if a timestamp was artificially further ahead in the future, the affect should merely be to return more data than necessary but that seems like a compliant behavior. I'm less certain about the interaction with the max_timestamp calculation. what is the bug that results from Hence max_timestamp shooting about 6 weeks into the future.?
2. iirc kafka also permits clients to send in arbitrary timestamps, so presumably kafka has to tackle a similar scenario. i wonder if we should poke around and see what upstream behavior is in this case?

[emaxerrno]

subtle.

I was thinking base_timestamp() as the bottom time, but you are right, it has to be 0.

[jcsp]

what is the bug that results from Hence max_timestamp shooting about 6 weeks into the future.?

That bogus timestamp is used for retention.ms enforcement, so we end up failing to delete segments during gc.

I went with a minimal fix here because this is a relatively frequent CI failure that I'd like to get fixed quickly. The general case where clients can send arbitrary timestamps (they can go backward, they can differ by more than 2**32) is incompatible with the way we store relative timestamp in a uint32_t.

In terms of why we're ending up with timestamps <= base_timestamp in our tests, I'm not quite sure, but I don't think we really prevent it: the timestamps are generated in produce.cc before any ordering rules are in effect, so it's entirely possible for incoming batches to be stamped in a different order than they land in the segment. Maybe we should be applying timestamps later, once the order of storage is established.

[jcsp]

I checked how we were getting base_timestamps in the future wrt subsequent messages -- it's raft configuration messages, which can easily be timestamped after actual kafka batches, where the kafka batches are held up waiting for the consensus group to be ready (after the configuration lands).

afaict we don't implement the timestamp->offsets Kafka feature (segment_index::find_nearest(model::timestamp t) is unused). As/when we do implement that:

• it might be necessary to extend the index to track a minimum timestamp per segment, which would be permitted to be lower than the base, and would enable a searcher to correctly find the segment that contains the earliest message with a given timestamp.
• or, maybe we can exclude raft configuration messages from the index so that at least the base+max timestamps reflect the kafka timestamps (including client timestamps if that's what the used).