Skip to content

add x-stream-offset header to StreamReader responses#1774

Merged
kickster97 merged 3 commits intomainfrom
fix-1773
Mar 11, 2026
Merged

add x-stream-offset header to StreamReader responses#1774
kickster97 merged 3 commits intomainfrom
fix-1773

Conversation

@kickster97
Copy link
Member

@kickster97 kickster97 commented Mar 2, 2026

WHAT is this pull request doing?

After stream offsets were removed from disk storage(#1479), the x-stream-offset header is added dynamically at read time. The shift?() path handles this correctly, but the read() path (used by StreamReader for the /api/queues/:vhost/:name/stream endpoint) was missed.

Fixes #1773

This PR adds an offset parameter to StreamMessageStore#read() and updates StreamReader#each to track and pass the offset through, so the header is added consistently across both code paths.

HOW can this pull request be tested?

Run the added apec

@kickster97 kickster97 requested a review from a team as a code owner March 2, 2026 10:23
@claude
Copy link

claude bot commented Mar 2, 2026

No issues found.

The implementation correctly mirrors the existing offset-header pattern used in shift? and shift_requeued. Offset tracking across segment boundaries is handled properly (offset only incremented on successful read). The add_offset_header allocation is consistent with the existing consumer delivery path.

@claude
Copy link

claude bot commented Mar 2, 2026

Code review: No issues found.

The changes are minimal and correct:

  • The offset value from find_offset was already being returned but discarded; now it is properly captured and passed through to read.
  • The add_offset_header helper was already used in the shift? (consumer) path; this PR correctly extends it to the StreamReader path.
  • No other callers of read exist, so the signature change is safe.
  • No allocation concerns — StreamReader#each is not a hot path.

Copy link
Member

@snichme snichme left a comment

Choose a reason for hiding this comment

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

The read method is currently only used by the stream reader, I don't think it's a good design to add offset as an additional parameter to that method, it's somethign that can be handled outside the method.

@kickster97 kickster97 requested a review from snichme March 10, 2026 15:55
@kickster97 kickster97 merged commit 9348a0c into main Mar 11, 2026
17 of 18 checks passed
@kickster97 kickster97 deleted the fix-1773 branch March 11, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP API missing x-stream-offset header

3 participants