model: make next_offset saturate rather than overflow

[nvartolomei]

model::offset::max() is often used to indicate "no upper bound" on operations. E.g. for tiered storage uploads¹, for reading from local storage², etc.

We also do often convert from closed to opened offset intervals representations. E.g. committed offset to LSO and the other way around.

When combined, these can result in unexpected behaviors. In particular, if on a read path the max offset is specified as model::offset_max() but at lower level this is converted into an exclusive offset by calling next_offset(model::offset::max()), the result is model::offset::min() aka -2^63.

This is dangerous. Let's instead saturate the offset similar to how we saturate prev_offset.

We also have a few cases where we just do o + model::offset(1). These should be refactored to use next_offset too.

This isn't fixing any existing known bug. Discovered this while trying to rewrite some logic related to tiered storage uploads.

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v24.1.x
• v23.3.x
• v23.2.x

Release Notes

• none

Footnotes

1. https://github.com/redpanda-data/redpanda/blob/79bf7eed6e04da1d0987b5abd719c4b289dde761/src/v/archival/ntp_archiver_service.cc#L1656 ↩

2. https://github.com/redpanda-data/redpanda/blob/79bf7eed6e04da1d0987b5abd719c4b289dde761/src/v/cluster/migrations/tx_manager_migrator.cc#L219 ↩

[nvartolomei]

I believe the model::offset::max() uses are wrong when used to indicate "no upper bound" for an inclusive offset. Instead model::offset{} should be used. But it isn't easy to follow that role/spot these mistakes in code reviews...

That's wrong! next_offset(model::offset{}) == 0 <=> "model::offset{} means empty"

[vbotbuildovich]

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48850#018f5a23-4eb0-4c27-9c1e-177c0ab0931b

[andrwng]

A little bit scary but I think it makes sense. It's also similar to how prev_offset handles anything below 0 as min()

[dotnwat]

A little bit scary but I think it makes sense. It's also similar to how prev_offset handles anything below 0 as min()

@nvartolomei @andrwng should we log an ERROR message for this case so that ducktape will reveal any cases that we might not know about? then later after some time we can remove it (or not).