Skip to content

Fix find_fragment timestamp predicate and add property-based tests - #77

Merged
the-mikedavis merged 6 commits into
mainfrom
feature/moar-tests
Mar 30, 2026
Merged

Fix find_fragment timestamp predicate and add property-based tests#77
the-mikedavis merged 6 commits into
mainfrom
feature/moar-tests

Conversation

@lukebakken

@lukebakken lukebakken commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a bug in find_fragment that caused timestamp subscriptions to start from the wrong position, fixes several other bugs in the remote tier read path, and adds comprehensive property-based tests.

Bug fix: find_fragment timestamp predicate

find_fragment used Ts >= FTs to select the manifest fragment for a timestamp subscription. When Ts falls between two fragments — after fragment N's LTs but before fragment N+1's FTs — the predicate incorrectly returned fragment N, causing the reader to deliver the wrong message.

The correct predicate is Ts >= LTs: find the first fragment whose last timestamp is at or after Ts. The read_from_remote_tier_by_timestamp integration test is also fixed to capture Timestamp3 before publishing offset 2.

Other fixes

  • Fix range_spec_to_location_number/2: suffix range used addition instead of subtraction; open-ended byte range returned infinity instead of the remaining file size
  • Fix init_offset_reader/2 calling osiris_log directly for the local path; route through init_local_reader/2 instead
  • Preserve ?C_OSIRIS_LOG_FIRST_OFFSET after local segment deletion; only update the shared atomic used for local/remote tier routing

Tests

  • Add integration tests for reading from the remote tier by offset and timestamp
  • Add property-based tests for range_spec_to_location_number/2, find_fragment timestamp behaviour, find_index_position offset and timestamp specs, and rabbitmq_stream_s3_array (partition_point, binary_search_by, rfind, fold)
  • Add diagnostic logging to surface manifest entry timestamps in CI output
  • Upload CT logs as an artifact on test failure

@lukebakken
lukebakken force-pushed the feature/moar-tests branch 2 times, most recently from 151a078 to 19657ba Compare March 18, 2026 13:59
Comment thread src/rabbitmq_stream_s3_server.erl Outdated
@lukebakken
lukebakken marked this pull request as draft March 24, 2026 18:36
@lukebakken lukebakken changed the title Fix remote tier read path bugs and add tests Fix find_fragment timestamp predicate and add property-based tests Mar 25, 2026
@lukebakken
lukebakken marked this pull request as ready for review March 25, 2026 00:00
@lukebakken
lukebakken marked this pull request as draft March 25, 2026 00:04
@lukebakken
lukebakken marked this pull request as ready for review March 25, 2026 00:16
@lukebakken

Copy link
Copy Markdown
Contributor Author

AI note on the find_fragment predicate change for the reviewer's benefit.

The fix changes the timestamp predicate from Ts >= FTs to Ts >= LTs. This only affects behaviour when there is a gap between fragments — i.e. LTs(N) < FTs(N+1) — which happens when there is a period of inactivity between segment rolls.

For contiguous fragments (LTs(N) ≈ FTs(N+1)), both predicates select the same fragment:

  • Ts strictly within fragment N (FTs(N) < Ts < LTs(N)): both predicates return fragment N
  • Ts at the boundary LTs(N) = FTs(N+1): both predicates return fragment N+1

The bug only manifests when Ts falls in a gap — after LTs(N) but before FTs(N+1). The original predicate returns fragment N (wrong); the new predicate returns fragment N+1 (correct).

In a real environment with continuous message flow, fragments are nearly contiguous and the gap is negligible, which explains why this hasn't been observed in practice. The CI failure exposed it because Timestamp3 was captured during a 200ms sleep, landing in a 406ms gap between two fragments.

The fix is believed to be safe: it does not change behaviour for contiguous fragments, and the property-based tests in unit_SUITE verify correctness for both the contiguous and gap cases across 500 randomly generated manifests.

@the-mikedavis

Copy link
Copy Markdown
Member

Ah yep I think the 🧞 is on the right track here. If a timestamp is between two chunks then we should resolve to the later chunk. (Which is different than offsets, we would resolve to the earlier chunk for a requested offset which is between two chunks.) We should do the same for fragment boundaries 👍

@lukebakken

Copy link
Copy Markdown
Contributor Author

Cool I'll keep working on this.

@lukebakken
lukebakken marked this pull request as draft March 25, 2026 14:58
@lukebakken

Copy link
Copy Markdown
Contributor Author

AI follow-up: the current code does implement the "prefer later fragment for timestamps" behaviour that @the-mikedavis describes.

With the new predicate Ts >= LTs, partition_point returns the index of the first fragment where Ts >= LTs is false — i.e. the first fragment whose last timestamp exceeds Ts. That is the fragment we want: the earliest fragment that could contain Ts. When Ts falls in a gap between fragments, this correctly resolves to the later fragment, consistent with find_index_position's timestamp behaviour.

@lukebakken
lukebakken marked this pull request as ready for review March 25, 2026 15:15
@lukebakken

Copy link
Copy Markdown
Contributor Author

AI note: two documentation commits were added on top of the main fix.

The first rewrites README.md with a plain-language description of what the plugin does and how it hooks into osiris (replacing the one-liner), fixes the build instructions (wrong directory name), and adds docs/configuration.md documenting all active configuration keys: enabling the plugin, S3 bucket and region, region endpoint overrides, and credential resolution order.

The second fixes four pre-existing issues in docs/README.md: a typo ("are send" → "are sent"), an incorrect unit (fragment max size is 64 MiB not 64 MB), a grammar error ("there prefixes" → "there are prefixes"), and a broken anchor link (#concurrency-control) pointing to a section that was never written.

@the-mikedavis the-mikedavis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I have two thoughts

Comment thread docs/README.md Outdated
Comment thread docs/configuration.md
Comment thread test/s3_streams_SUITE.erl Outdated
lukebakken and others added 6 commits March 30, 2026 14:27
`find_fragment` used `Ts >= FTs` to select the manifest fragment for a
timestamp subscription. This predicate finds the last fragment whose
first timestamp is at or before `Ts`. When `Ts` falls between two
fragments — after fragment N's `LTs` but before fragment N+1's `FTs` —
the predicate incorrectly returns fragment N instead of N+1, causing the
reader to start from the wrong position.

The correct predicate is `Ts >= LTs`: find the first fragment whose last
timestamp is greater than or equal to `Ts`. This correctly identifies
the fragment whose range contains `Ts`, or the earliest fragment after
`Ts` if `Ts` falls in a gap between fragments.

The index position calculation for the timestamp case uses
`min(Idx0, NumEntries - 1)` rather than `saturating_decr` because the
partition point already identifies the target fragment directly.

The `read_from_remote_tier_by_timestamp` integration test is also fixed:
`Timestamp3` must be captured before publishing offset 2 so that
`find_index_position` returns offset 2 rather than offset 3.

- Fix `init_offset_reader/2` calling `osiris_log` directly for the local
  path; route through `init_local_reader/2` instead
- Fix `range_spec_to_location_number/2`: suffix range used addition
  instead of subtraction; open-ended byte range returned `infinity`
  instead of the remaining file size
- Preserve `?C_OSIRIS_LOG_FIRST_OFFSET` after local segment deletion;
  only update the shared atomic used for local/remote tier routing

- Upload CT logs as an artifact on test failure
- Reduce `?MAX_SEGMENT_SIZE_BYTES` to 1 MiB in test builds to trigger
  segment rolls without large payloads

- Add integration tests for reading stream data from the remote tier by
  offset and timestamp
- Convert `unit_SUITE` to use PropEr; add property-based tests for
  `range_spec_to_location_number/2`
- Add property-based tests for `find_fragment` timestamp behaviour,
  covering timestamps within fragment ranges, in gaps between fragments,
  and after all fragments
- Add property-based tests for `rabbitmq_stream_s3_array`:
  `partition_point`, `binary_search_by`, `rfind`, and `fold`
- Add property-based tests for `find_index_position` offset and
  timestamp specs
- Add diagnostic logging to `read_from_remote_tier_by_timestamp` to
  surface manifest entry timestamps and captured timestamps in CI output
- Add `get_range_by_reference/1` and `get_manifest_by_reference/1` test
  helpers to `rabbitmq_stream_s3_server`
- Support container credentials in the integration test group
Rewrite README.md with a plain-language description of what the plugin
does and how it hooks into osiris, replacing the one-liner summary. Fix
the build instructions (wrong directory name). Replace the sparse
Configure section with a link to the new docs/configuration.md.

Add docs/configuration.md documenting all active configuration keys:
enabling the plugin, S3 bucket and region settings, region endpoint
overrides, and credential resolution order (static config, container
credentials endpoint, EC2 instance metadata).
- Fix typo: "are send" -> "are sent"
- Fix unit: fragment max size is 64 MiB not 64 MB
- Fix grammar: "there prefixes" -> "there are prefixes"
- Remove broken `#concurrency-control` anchor link (section was never written)
Co-authored-by: Michael Davis <mcrdavis@amazon.com>
Replace the test-only `get_range_by_reference/1` and
`get_manifest_by_reference/1` helpers in `rabbitmq_stream_s3_server`
with direct calls to the existing `get_range/1` and `get_manifest/1`
functions using the stream ID obtained from the queue's type state.

This removes two test-only `handle_call` clauses and their exported
wrappers from the server. The stream ID is retrieved via
`amqqueue:get_type_state/1` in a new `get_stream_id/2` test helper.

Also use the stream ID to construct a precise `filelib:wildcard` path
for the segment deletion check, replacing the `*` glob that would match
segments from any stream.
When a stream is created but no segment has been flushed yet, the index
file exists on disk but contains no records after the header. The call
to `rabbitmq_stream_s3_array:at/3` on an empty binary crashes with
`badarg`.

Add a guard clause matching `<<>>` that returns a default empty
fragment, consistent with the `[]` branch in `init_manifest/2`.

@the-mikedavis the-mikedavis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@the-mikedavis
the-mikedavis merged commit 4e537fe into main Mar 30, 2026
5 checks passed
@the-mikedavis
the-mikedavis deleted the feature/moar-tests branch March 30, 2026 16:33
@lukebakken lukebakken added this to the 1.0.0 milestone Apr 3, 2026
@the-mikedavis the-mikedavis added C-bug Category: This is a bug. A-log-reader Area: Impl of `osiris_log_reader`. Local and remote stream reads. labels Apr 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-log-reader Area: Impl of `osiris_log_reader`. Local and remote stream reads. C-bug Category: This is a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants