Skip to content

Fix Int32 overflow in SSE retry field - #771

Merged
hahwul merged 1 commit into
masterfrom
fix/sse-retry-overflow
Aug 15, 2026
Merged

Fix Int32 overflow in SSE retry field#771
hahwul merged 1 commit into
masterfrom
fix/sse-retry-overflow

Conversation

@hahwul

@hahwul hahwul commented Aug 15, 2026

Copy link
Copy Markdown
Member

What

EventStream#send rendered the SSE retry field with .to_i (Int32). A retry span longer than ~24.8 days (Int32::MAX milliseconds) overflowed and raised OverflowError inside the streaming handler, breaking the response. Switched to .to_i64.

Bug case (before this PR)

sse "/events" do |stream, _|
  stream.send("update", retry: 60.days)  # 5_184_000_000 ms
  # => raises OverflowError: Arithmetic overflow
end

Values within the normal range (seconds/minutes) are unaffected — the emitted bytes are identical.

Test

Added a regression spec asserting retry: 60.days renders retry: 5184000000.

EventStream#send cast retry.total_milliseconds (Float64) with .to_i (Int32),
so a retry span over ~24.8 days (Int32::MAX ms) raised OverflowError inside
the streaming handler. Use .to_i64.
@hahwul

hahwul commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

It's a simple patch for overflow prevention. and CI passed, so I'll self-merge it!

@hahwul
hahwul merged commit 7d5994f into master Aug 15, 2026
38 checks passed
@hahwul
hahwul deleted the fix/sse-retry-overflow branch August 15, 2026 01:36
@Sija

Sija commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Maybe using .to_u64 would be even better?

@hahwul

hahwul commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Hi @Sija

Thanks for the suggestion! Nothing guards against negative spans here, so .to_u64 would raise on them inside the streaming handler. My reading was that negatives are harmless anyway since clients ignore the field per spec, hence .to_i64.

Happy to add a guard in a follow-up PR that skips the field when it's negative. With that in place .to_u64 works fine too, if you'd rather have the type say it. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants