Skip to content

Fix streaming deadlock with Rack compression middleware#2529

Closed
justin808 wants to merge 3 commits intomasterfrom
jg/2519-fix-rsc-payload-script
Closed

Fix streaming deadlock with Rack compression middleware#2529
justin808 wants to merge 3 commits intomasterfrom
jg/2519-fix-rsc-payload-script

Conversation

@justin808
Copy link
Member

@justin808 justin808 commented Mar 4, 2026

Summary

  • Sets Content-Encoding: identity on all streaming responses in the Stream concern to prevent Rack::Deflater and Rack::Brotli from iterating the Live::Buffer response body, which causes a deadlock with ActionController::Live

Fixes #2519

Test plan

  • Verify existing streaming specs still pass
  • Test with Rack::Deflater in middleware stack — RSC payload endpoint should respond without hanging
  • Test with Rack::Brotli in middleware stack — same expected behavior

🤖 Generated with Claude Code


Note

Cursor Bugbot is generating a summary for commit 3a633ea. Configure here.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed streaming deadlocks with HTTP compression middleware and prevented SSR streaming hangs and silent error absorption; streaming now sets encoding early and handles client disconnects gracefully.
  • Documentation
    • Changelog updated with entries describing the streaming/compression and RSC payload fixes.
  • Tests
    • Added tests covering Accept-Encoding behavior, Content-Encoding responses, and compressed vs. identity streaming.

Set Content-Encoding: identity on streaming responses to prevent
Rack::Deflater and Rack::Brotli from iterating the Live::Buffer
response body, which causes a deadlock with ActionController::Live.

Fixes #2519

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 4, 2026

Walkthrough

Adds explicit streaming compression handling to choose gzip or identity encoding and set Content-Encoding early. Introduces gzip-wrapping of the streaming writer to emit compressed chunks and append a gzip footer on close, avoiding deadlocks with Rack compression middleware.

Changes

Cohort / File(s) Summary
Stream compression logic
react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb
Add require "zlib" and new methods: setup_streaming_compression, client_accepts_gzip?, and wrap_stream_with_gzip_compression. Set Content-Encoding early and optionally wrap the stream writer to emit gzip-compressed chunks and a gzip footer on close.
Tests: streaming compression
react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
Add Accept-Encoding handling to tests; stub request.headers; update test helpers to accept accept_encoding; add tests verifying Content-Encoding is gzip when client accepts gzip and that gzip output decompresses correctly; verify identity when not.
Changelog
CHANGELOG.md
Add Unreleased entries documenting fix for RSC payload streaming deadlock with Rack compression middleware and related streaming/SSR fixes.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant RackMiddleware as Rack::Deflater/Brotli
  participant Controller as RscPayloadController
  participant StreamWriter as Stream Writer / Live::Buffer
  participant GzipWrapper as Gzip compressor

  Client->>Controller: GET /rsc_payload/Component
  Controller->>Controller: setup_streaming_compression (inspect Accept-Encoding)
  alt client accepts gzip
    Controller->>StreamWriter: wrap writer with GzipWrapper
    Controller->>Controller: set response.headers["Content-Encoding"]="gzip"
  else client does not accept gzip
    Controller->>Controller: set response.headers["Content-Encoding"]="identity"
  end
  Controller->>StreamWriter: write chunks (possibly compressed by GzipWrapper)
  StreamWriter-->>RackMiddleware: stream body chunks (middleware sees Content-Encoding and skips)
  RackMiddleware-->>Client: forward streamed (pre-compressed or identity) chunks
  Client->>RackMiddleware: receive streamed response
  Controller->>GzipWrapper: close (Gzip footer appended) when done
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I curl and hum, a ribbon of gzipped song,
I mark the header early so nothing waits too long.
If you like compression, I wrap with a squeeze,
If not, I say "identity" and stream with ease.
Hop, hop — no more deadlocks, the payloads move along.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix streaming deadlock with Rack compression middleware' directly and specifically describes the main change—addressing the deadlock issue with compression middleware when streaming RSC payloads.
Linked Issues check ✅ Passed The PR fully addresses issue #2519 by implementing streaming compression handling to prevent deadlocks: it sets Content-Encoding headers to signal middleware to skip compression, handles gzip compression when accepted by clients, and includes comprehensive tests verifying the fix.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the streaming deadlock issue: the Stream concern adds compression handling methods, CHANGELOG documents the fix, and tests validate the compression behavior—no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jg/2519-fix-rsc-payload-script

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Mar 4, 2026

Review of PR #2529 — Fix streaming deadlock with Rack compression middleware

Summary: This is a targeted, minimal fix for a real deadlock issue. Setting Content-Encoding: identity before streaming is the right approach to signal to Rack::Deflater (and Rack::Brotli) to skip trying to compress the ActionController::Live response body.


Correctness

The fix is correct. Both Rack::Deflater and Rack::Brotli check for a pre-existing Content-Encoding header and skip compression if one is set:

# Rack::Deflater (simplified)
return [status, headers, body] if headers[CONTENT_ENCODING]

Setting it to identity before Sync do (i.e., before any response body is written) ensures headers are not yet committed when the value is applied, which is the correct ordering with ActionController::Live.


Issues

1. No automated test coverage for the new behavior

The existing stream_spec.rb mocks response via instance_double(ActionController::Live::Response) but does not set up or verify the headers hash. A test verifying that Content-Encoding: identity is set before streaming begins should be added. Without it, a future refactor could silently regress the fix.

Suggested approach in setup_stream_test:

mocked_headers = {}
allow(mocked_response).to receive(:headers).and_return(mocked_headers)

Then in a dedicated test:

it 'sets Content-Encoding: identity to prevent compression middleware deadlocks' do
  _queues, controller, _stream = setup_stream_test(component_count: 0)
  run_stream(controller) { sleep 0.1 }
  expect(controller.response.headers['Content-Encoding']).to eq('identity')
end

2. Missing CHANGELOG entry

The [Unreleased] section in CHANGELOG.md has no entry for this fix. Per project conventions, a ##### Fixed bullet under #### Pro is expected.

3. Rack::Brotli compatibility not verified

The comment correctly calls out Rack::Brotli, but Rack::Brotli's behavior around existing Content-Encoding headers is less well-documented than Rack::Deflater's. The test plan item for Brotli is unchecked — please confirm this is validated before merge.


Minor observations

  • The comment at line 38–41 is good context; no changes needed there.
  • Placement before Sync do is correct — headers must be set before the first response.stream.write call commits them.
  • The PR description has all test plan items unchecked, suggesting this may still be in progress.

Verdict

The code change itself is correct and well-scoped. The two blockers before merge are: (1) add a test covering the new header behavior, and (2) add a CHANGELOG entry. Verifying Rack::Brotli behavior is also important given it's explicitly called out in the fix comment.

# the Live::Buffer response body, which causes a deadlock with ActionController::Live.
# Setting Content-Encoding before streaming tells middleware the response is already
# encoded, so they skip compression entirely. See https://github.com/shakacode/react_on_rails/issues/2519
response.headers["Content-Encoding"] = "identity"
Copy link

Choose a reason for hiding this comment

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

The fix is correct — setting any Content-Encoding value before the response body is written causes Rack::Deflater to skip compression. identity is the most semantically appropriate value ("no transformation applied").

One thing worth verifying: Rack::Brotli (a third-party gem) should behave the same way, but its source is less standardized than Rack::Deflater. Please confirm with the unchecked test plan item before merge.

Also, the existing stream_spec.rb mocks response but doesn't stub headers, so this line currently raises a NoMethodError in unit tests unless the mock is updated. Consider adding allow(mocked_response).to receive(:headers).and_return({}) to setup_stream_test and a dedicated test asserting this header is set.

- Add spec verifying Content-Encoding: identity is set on streaming
  responses to prevent compression middleware deadlock
- Add changelog entry for the fix

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR fixes a deadlock caused by Rack::Deflater and Rack::Brotli iterating the ActionController::Live::Buffer response body during streaming by setting Content-Encoding: identity before the streaming Sync block begins, which causes both middlewares to skip compression entirely.

The fix itself is minimal and the approach is correct for the described scenario. However, there are two issues worth addressing before merging:

  1. Test breakage (blocking): The setup_stream_test helper in stream_spec.rb only stubs response.stream on its instance_double(ActionController::Live::Response), not response.headers. The new header assignment at line 42 executes unconditionally before the Sync block, meaning all 7 test cases that call setup_stream_test will raise RSpec::Mocks::MockExpectationError on the unstubbed :headers call.

  2. Misleading comment (non-blocking): The inline comment states the response "is already encoded," but Content-Encoding: identity actually means no encoding has been applied. The middlewares skip because they check for header presence, not the value. The comment should reflect this distinction to avoid confusion for future maintainers.

Confidence Score: 2/5

  • The runtime fix is conceptually sound but introduces a test regression that will cause existing specs to fail on CI.
  • The single-line change is well-targeted and conceptually sound for preventing the Rack::Deflater/Rack::Brotli deadlock. However, the score is low because the stream_spec.rb test helper (setup_stream_test) was not updated to mock response.headers, which will cause 7+ existing tests to fail with RSpec::Mocks::MockExpectationError — a regression introduced by this PR itself. Additionally, the comment explaining the fix is misleading about what identity encoding signifies, which could confuse maintainers.
  • react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb — needs response.headers stub added to setup_stream_test helper

Sequence Diagram

sequenceDiagram
    participant Client
    participant RackBrotli as Rack::Brotli / Rack::Deflater
    participant StreamConcern as Stream Concern
    participant LiveBuffer as ActionController::Live::Buffer

    Client->>RackBrotli: HTTP Request (Accept-Encoding: gzip/br)
    RackBrotli->>StreamConcern: Forward request
    StreamConcern->>StreamConcern: response.headers["Content-Encoding"] = "identity"
    StreamConcern->>LiveBuffer: response.stream.write(template_string)
    StreamConcern->>LiveBuffer: drain_streams_concurrently (chunks)
    LiveBuffer-->>RackBrotli: Streaming response body
    Note over RackBrotli: Detects Content-Encoding already set<br/>→ skips compression, passes body through
    RackBrotli-->>Client: Streamed response (no deadlock)
Loading

Last reviewed commit: 4e77139

# the Live::Buffer response body, which causes a deadlock with ActionController::Live.
# Setting Content-Encoding before streaming tells middleware the response is already
# encoded, so they skip compression entirely. See https://github.com/shakacode/react_on_rails/issues/2519
response.headers["Content-Encoding"] = "identity"
Copy link

Choose a reason for hiding this comment

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

The setup_stream_test helper in the spec file only stubs .stream on the mocked response, not .headers. This will cause all 7 existing tests that use setup_stream_test to fail with RSpec::Mocks::MockExpectationError: received unexpected message :headers when this line executes.

The test helper needs to add a stub for the headers:

Suggested change
response.headers["Content-Encoding"] = "identity"
def setup_stream_test(component_count: 2)
component_queues = Array.new(component_count) { Async::Queue.new }
controller = StreamController.new(component_queues: component_queues)
mocked_response = instance_double(ActionController::Live::Response)
mocked_stream = instance_double(ActionController::Live::Buffer)
allow(mocked_response).to receive(:stream).and_return(mocked_stream)
allow(mocked_response).to receive(:headers).and_return({})
allow(mocked_stream).to receive(:write)
allow(mocked_stream).to receive(:close)
allow(mocked_stream).to receive(:closed?).and_return(false)
allow(controller).to receive(:response).and_return(mocked_response)
[component_queues, controller, mocked_stream]
end

Comment on lines +38 to +42
# Prevent compression middleware (Rack::Deflater, Rack::Brotli) from iterating
# the Live::Buffer response body, which causes a deadlock with ActionController::Live.
# Setting Content-Encoding before streaming tells middleware the response is already
# encoded, so they skip compression entirely. See https://github.com/shakacode/react_on_rails/issues/2519
response.headers["Content-Encoding"] = "identity"
Copy link

Choose a reason for hiding this comment

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

The comment is slightly misleading. Content-Encoding: identity actually means no encoding has been applied (per RFC 7231), not that the response is already encoded. The reason this prevents compression is that Rack::Deflater and Rack::Brotli both skip compression when they detect any existing Content-Encoding header (presence check), regardless of value.

Consider updating the comment to reflect the actual mechanism:

Suggested change
# Prevent compression middleware (Rack::Deflater, Rack::Brotli) from iterating
# the Live::Buffer response body, which causes a deadlock with ActionController::Live.
# Setting Content-Encoding before streaming tells middleware the response is already
# encoded, so they skip compression entirely. See https://github.com/shakacode/react_on_rails/issues/2519
response.headers["Content-Encoding"] = "identity"
# Prevent compression middleware (Rack::Deflater, Rack::Brotli) from iterating
# the Live::Buffer response body, which causes a deadlock with ActionController::Live.
# Both middlewares skip compression when Content-Encoding is already set (presence check).
# "identity" signals no encoding has been applied, satisfying that check without implying
# the body is pre-compressed. See https://github.com/shakacode/react_on_rails/issues/2519
response.headers["Content-Encoding"] = "identity"

@claude
Copy link

claude bot commented Mar 4, 2026

Review: Fix streaming deadlock with Rack compression middleware

This is a clean, minimal, and correct fix for the deadlock described in #2519. Here's my assessment:

Correctness ✅

Setting Content-Encoding: identity before writing any data is the standard solution for preventing Rack::Deflater and Rack::Brotli from trying to compress ActionController::Live responses. The root cause is that these middlewares call each on the response body to inspect/compress it — which blocks indefinitely on a Live::Buffer. Setting this header early (before Sync do) ensures it goes out in the HTTP response headers before any body data is written.

The RSC payload endpoint (/rsc_payload/:component_name) routes through RscPayloadRenderer#rsc_payload, which calls stream_view_containing_react_components, so the fix is correctly scoped.

Minor Observations

1. Implicit override of any pre-existing Content-Encoding header

response.headers["Content-Encoding"] = "identity"

If a controller or upstream middleware has already set a Content-Encoding header for some reason, this silently replaces it. This is almost certainly the right behavior (streaming and compression are fundamentally incompatible), but a brief comment noting that this is intentional would future-proof it:

# Overwrite any existing Content-Encoding — compression is incompatible with streaming.
response.headers["Content-Encoding"] = "identity"

2. Test uses sleep 0.1 as a synchronization mechanism

The new test follows the existing pattern in the file, so this is consistent. However, since the test only needs to verify a header set synchronously at the very start of stream_view_containing_react_components (before the first Sync do yield), the sleep is technically unnecessary — the header is set before any async work begins. This is harmless but slightly misleading about when the header is set.

3. Changelog placement

The new entry is placed correctly in the "Fixed" section under the in-progress release.

Summary

The fix is well-reasoned and properly targeted. The test coverage is appropriate, the comment in stream.rb clearly explains the why, and the changelog entry is thorough. The approach matches the established Rails community pattern for preventing this specific Rack middleware deadlock. Approving with no blocking concerns.


run_stream(controller) do |_parent|
sleep 0.1
end
Copy link

Choose a reason for hiding this comment

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

The sleep 0.1 is not needed here — Content-Encoding: identity is set synchronously at the very start of stream_view_containing_react_components, before the Sync do block executes any async work. The test would pass with sleep 0 or even without any sleep. Not a bug, just a minor clarity point since the sleep implies the header might be set asynchronously.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 34: The changelog attribution on the RSC payload streaming fix entry has
a username/URL mismatch: the link URL points to "justin808" while the visible
label is "justin"; update the label to match the URL (use "justin808") so the
entry follows the required format "[PR
2529](https://github.com/shakacode/react_on_rails/pull/2529) by
[justin808](https://github.com/justin808)". Locate the changelog entry
containing "Fix RSC payload streaming deadlock with Rack compression middleware"
and change the displayed username to "justin808" to match the link.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d01f4a79-561c-4fae-8bd1-deeff3062640

📥 Commits

Reviewing files that changed from the base of the PR and between 3a633ea and 4e77139.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb

CHANGELOG.md Outdated

##### Fixed

- **Fix RSC payload streaming deadlock with Rack compression middleware**: The RSC payload endpoint (`/rsc_payload/:component_name`) would hang indefinitely when `Rack::Deflater` or `Rack::Brotli` compression middleware was present. Compression middleware attempted to iterate the `ActionController::Live` response body to check size, creating a deadlock. Now sets `Content-Encoding: identity` on all streaming responses to signal middleware to skip compression. [PR 2529](https://github.com/shakacode/react_on_rails/pull/2529) by [justin](https://github.com/justin808).
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix username/link mismatch in changelog attribution.

On Line 34, the link points to justin808 but the label is justin. Use matching username text and URL.

Suggested fix
- ... [PR 2529](https://github.com/shakacode/react_on_rails/pull/2529) by [justin](https://github.com/justin808).
+ ... [PR 2529](https://github.com/shakacode/react_on_rails/pull/2529) by [justin808](https://github.com/justin808).

As per coding guidelines: Format changelog entries as [PR <number>](https://github.com/shakacode/react_on_rails/pull/<number>) by [username](https://github.com/username) without a hash before the PR number.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- **Fix RSC payload streaming deadlock with Rack compression middleware**: The RSC payload endpoint (`/rsc_payload/:component_name`) would hang indefinitely when `Rack::Deflater` or `Rack::Brotli` compression middleware was present. Compression middleware attempted to iterate the `ActionController::Live` response body to check size, creating a deadlock. Now sets `Content-Encoding: identity` on all streaming responses to signal middleware to skip compression. [PR 2529](https://github.com/shakacode/react_on_rails/pull/2529) by [justin](https://github.com/justin808).
- **Fix RSC payload streaming deadlock with Rack compression middleware**: The RSC payload endpoint (`/rsc_payload/:component_name`) would hang indefinitely when `Rack::Deflater` or `Rack::Brotli` compression middleware was present. Compression middleware attempted to iterate the `ActionController::Live` response body to check size, creating a deadlock. Now sets `Content-Encoding: identity` on all streaming responses to signal middleware to skip compression. [PR 2529](https://github.com/shakacode/react_on_rails/pull/2529) by [justin808](https://github.com/justin808).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 34, The changelog attribution on the RSC payload
streaming fix entry has a username/URL mismatch: the link URL points to
"justin808" while the visible label is "justin"; update the label to match the
URL (use "justin808") so the entry follows the required format "[PR
2529](https://github.com/shakacode/react_on_rails/pull/2529) by
[justin808](https://github.com/justin808)". Locate the changelog entry
containing "Fix RSC payload streaming deadlock with Rack compression middleware"
and change the displayed username to "justin808" to match the link.

Instead of disabling compression entirely with Content-Encoding:
identity, apply gzip compression ourselves when the client accepts
it. This prevents the Rack middleware deadlock while preserving
compressed responses.

- When Accept-Encoding includes gzip: compress stream writes with
  Zlib::Deflate and set Content-Encoding: gzip
- When gzip not accepted: set Content-Encoding: identity
- Either way, Rack::Deflater/Brotli see Content-Encoding is set
  and skip, avoiding the Live::Buffer iteration deadlock
- Add tests for gzip compression, identity fallback, and missing
  Accept-Encoding header

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Mar 4, 2026

Review: PR #2529 (final commit — inline gzip compression)

This review covers the final state of the PR after commit 02eae34, which shifted from simply setting Content-Encoding: identity to also doing inline gzip compression when the client accepts it. The root cause analysis and overall approach are sound.


Correctness

The fix is correct: by setting Content-Encoding before writing any body, Rack::Deflater and Rack::Brotli both short-circuit and skip compression, eliminating the Live::Buffer deadlock. Placing setup_streaming_compression before the Sync do block ensures headers aren't committed yet.

The inline gzip path (Zlib::MAX_WBITS + 16 = gzip format, SYNC_FLUSH for progressive delivery) is technically correct — each write call produces a self-contained gzip block the client can progressively decompress.


Issues

1. Bug: gzip;q=0 incorrectly treated as "accepts gzip"

The regex /\bgzip\b/ matches the string "gzip" regardless of the quality value. Per RFC 7231, Accept-Encoding: gzip;q=0 means the client explicitly refuses gzip. Sending a gzip-encoded body to such a client will cause decompression failures.

# Problematic — matches "gzip;q=0"
request.headers["Accept-Encoding"]&.match?(/\bgzip\b/)

Suggested fix — reject q=0 explicitly:

def client_accepts_gzip?
  return false unless respond_to?(:request)
  header = request.headers["Accept-Encoding"]
  return false if header.nil?
  # Match gzip token, but only if quality value is non-zero (or absent)
  header.split(",").any? do |part|
    token, *params = part.split(";").map(&:strip)
    token.casecmp?("gzip") && \!params.any? { |p| p.match?(/\Aq=0(\.0*)?\z/i) }
  end
end

2. Resource leak when close_stream_at_end: false

When a caller passes close_stream_at_end: false, the redefined stream.close is never called, so deflater.finish never runs and deflater.close is never called. The gzip footer is never written (leaving the stream with a truncated gzip body), and the native Zlib resource leaks until GC collects it.

The existing stream_view_containing_react_components docs show this is a supported code path (for error recovery in rescue_from handlers). A guard is needed:

def setup_streaming_compression
  if client_accepts_gzip?
    response.headers["Content-Encoding"] = "gzip"
    wrap_stream_with_gzip_compression
  else
    response.headers["Content-Encoding"] = "identity"
  end
end

At minimum, add a note in the wrap_stream_with_gzip_compression docstring that the caller is responsible for always eventually calling stream.close. Alternatively, guard the gzip path with close_stream_at_end (pass it through or check at the call site).

3. define_singleton_method approach is fragile

Monkey-patching the stream singleton is non-idiomatic and could break if:

  • ActionController::Live::Buffer changes its write/close interface
  • Another concern also wraps the same stream
  • The stream is reused (unlikely for Live, but worth documenting)

A wrapper object would be more robust:

class GzipStreamWrapper
  def initialize(stream, deflater)
    @stream = stream
    @deflater = deflater
  end

  def write(data)
    @stream.write(@deflater.deflate(data.to_s, Zlib::SYNC_FLUSH))
  end

  def close
    final = @deflater.finish
    @stream.write(final) unless final.empty?
  rescue IOError, Errno::EPIPE
    # ignore — client disconnected
  ensure
    @deflater.close
    @stream.close
  end

  def method_missing(name, *args, &block)
    @stream.public_send(name, *args, &block)
  end

  def respond_to_missing?(name, include_private = false)
    @stream.respond_to?(name, include_private) || super
  end
end

However, replacing response.stream entirely may be difficult with the Live internals. If keeping define_singleton_method, at least add a comment explaining that writes must be serialized (they are, via @main_output_queue) for the deflater to produce correct output.


Minor observations

  • Test coverage is good — the three new compression tests cover gzip, identity fallback, and missing header. The gzip test verifying full decompression of the combined output is the right level of integration.
  • respond_to?(:request) guard — since Stream is an ActiveSupport::Concern that includes ActionController::Live, request is always present in a controller context. The guard is overly defensive but harmless.
  • Changelog attribution: The entry shows [justin] but links to justin808 — minor but inconsistent with other entries in the file.

Summary

The fix correctly resolves the deadlock. The q=0 edge case is a real bug worth fixing before merge. The resource leak with close_stream_at_end: false needs at minimum documentation, and ideally a code-level guard.

def client_accepts_gzip?
return false unless respond_to?(:request)

request.headers["Accept-Encoding"]&.match?(/\bgzip\b/)
Copy link

Choose a reason for hiding this comment

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

The regex /\bgzip\b/ matches gzip;q=0, which per RFC 7231 means the client explicitly refuses gzip encoding. Sending a gzip body to such a client will cause decompression failures.

Consider parsing the quality value:

def client_accepts_gzip?
  return false unless respond_to?(:request)

  header = request.headers["Accept-Encoding"]
  return false if header.nil?

  header.split(",").any? do |part|
    token, *params = part.split(";").map(&:strip)
    token.casecmp?("gzip") && !params.any? { |p| p.match?(/\Aq=0(\.0*)?\z/i) }
  end
end

request.headers["Accept-Encoding"]&.match?(/\bgzip\b/)
end

def wrap_stream_with_gzip_compression
Copy link

Choose a reason for hiding this comment

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

Resource leak when close_stream_at_end: false: the redefined close is the only way the deflater gets finalized (deflater.finish + deflater.close). If a caller passes close_stream_at_end: false (a documented code path for error-recovery via rescue_from), the native Zlib object leaks and the gzip footer is never written, producing a truncated/corrupt gzip body.

At minimum, add a note that stream.close must eventually be called when compression is active. Ideally, gate the gzip path on close_stream_at_end or add an ensure in stream_view_containing_react_components that always closes the stream (or finalizes the deflater) when gzip is in use.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02eae340cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

def client_accepts_gzip?
return false unless respond_to?(:request)

request.headers["Accept-Encoding"]&.match?(/\bgzip\b/)

Choose a reason for hiding this comment

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

P2 Badge Honor gzip q=0 when negotiating compression

client_accepts_gzip? currently checks Accept-Encoding with a simple \bgzip\b regex, so headers like gzip;q=0, br are treated as gzip-capable even though q=0 explicitly forbids gzip. In that case this change will force Content-Encoding: gzip, and clients/proxies that rely on quality values can receive an encoding they said they cannot accept. Please parse the quality values (or use Rack’s accept-encoding parsing) before deciding to gzip.

Useful? React with 👍 / 👎.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (1)

400-402: Make the “missing Accept-Encoding” case truly header-absent in test setup.

The helper always creates the key with a nil value, so the “missing header” scenario is not fully exercised.

Suggested fix
-      mocked_request_headers = { "Accept-Encoding" => accept_encoding }
+      mocked_request_headers = {}
+      mocked_request_headers["Accept-Encoding"] = accept_encoding unless accept_encoding.nil?
       mocked_request = double("request", headers: mocked_request_headers) # rubocop:disable RSpec/VerifiedDoubles
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb` around lines 400 -
402, The test currently always sets "Accept-Encoding" in mocked_request_headers
even when accept_encoding is nil, so the missing-header case isn't exercised;
update the setup in the example using mocked_request_headers and mocked_request
so that when accept_encoding is nil you create an empty hash (e.g.
mocked_request_headers = accept_encoding.nil? ? {} : { "Accept-Encoding" =>
accept_encoding }) and then build the mocked_request double and
allow(controller).to receive_messages(response: mocked_response, request:
mocked_request) as before so the controller truly sees no Accept-Encoding
header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb`:
- Around line 136-143: In setup_streaming_compression, add the Vary header for
Accept-Encoding whenever you choose gzip vs identity: when setting
response.headers["Content-Encoding"] (inside the client_accepts_gzip? branch and
the else branch) also ensure response.headers["Vary"] includes "Accept-Encoding"
(append if a Vary header already exists) so caches know the response varies by
Accept-Encoding; update the code around setup_streaming_compression /
client_accepts_gzip? / wrap_stream_with_gzip_compression to set or merge the
Vary header accordingly.
- Around line 145-149: The method client_accepts_gzip? currently returns true if
"gzip" appears anywhere in Accept-Encoding even when q=0; update
client_accepts_gzip? to properly parse the request.headers["Accept-Encoding"]
string (in the ReactOnRailsPro::Concerns::Stream concern) and only return true
when an entry for "gzip" exists with an explicit weight > 0 (i.e., parse
comma-separated tokens, handle optional "q=" parameters, treat missing q as 1.0,
and ignore gzip if q=0). Ensure it still safely handles nil headers and uses
respond_to?(:request) as before.

---

Nitpick comments:
In `@react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb`:
- Around line 400-402: The test currently always sets "Accept-Encoding" in
mocked_request_headers even when accept_encoding is nil, so the missing-header
case isn't exercised; update the setup in the example using
mocked_request_headers and mocked_request so that when accept_encoding is nil
you create an empty hash (e.g. mocked_request_headers = accept_encoding.nil? ?
{} : { "Accept-Encoding" => accept_encoding }) and then build the mocked_request
double and allow(controller).to receive_messages(response: mocked_response,
request: mocked_request) as before so the controller truly sees no
Accept-Encoding header.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fa96c13a-30b6-46fb-89b6-b39560e0c53c

📥 Commits

Reviewing files that changed from the base of the PR and between 4e77139 and 02eae34.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb
  • react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

Comment on lines +136 to +143
def setup_streaming_compression
if client_accepts_gzip?
response.headers["Content-Encoding"] = "gzip"
wrap_stream_with_gzip_compression
else
response.headers["Content-Encoding"] = "identity"
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Set Vary: Accept-Encoding when selecting gzip vs identity.

Encoding now depends on request headers; without Vary, shared caches can serve the wrong variant to other clients.

Suggested fix
 def setup_streaming_compression
+  vary_values = response.headers["Vary"].to_s.split(",").map(&:strip)
+  response.headers["Vary"] = (vary_values | ["Accept-Encoding"]).join(", ")
+
   if client_accepts_gzip?
     response.headers["Content-Encoding"] = "gzip"
     wrap_stream_with_gzip_compression
   else
     response.headers["Content-Encoding"] = "identity"
   end
 end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def setup_streaming_compression
if client_accepts_gzip?
response.headers["Content-Encoding"] = "gzip"
wrap_stream_with_gzip_compression
else
response.headers["Content-Encoding"] = "identity"
end
end
def setup_streaming_compression
vary_values = response.headers["Vary"].to_s.split(",").map(&:strip)
response.headers["Vary"] = (vary_values | ["Accept-Encoding"]).join(", ")
if client_accepts_gzip?
response.headers["Content-Encoding"] = "gzip"
wrap_stream_with_gzip_compression
else
response.headers["Content-Encoding"] = "identity"
end
end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb` around lines
136 - 143, In setup_streaming_compression, add the Vary header for
Accept-Encoding whenever you choose gzip vs identity: when setting
response.headers["Content-Encoding"] (inside the client_accepts_gzip? branch and
the else branch) also ensure response.headers["Vary"] includes "Accept-Encoding"
(append if a Vary header already exists) so caches know the response varies by
Accept-Encoding; update the code around setup_streaming_compression /
client_accepts_gzip? / wrap_stream_with_gzip_compression to set or merge the
Vary header accordingly.

Comment on lines +145 to +149
def client_accepts_gzip?
return false unless respond_to?(:request)

request.headers["Accept-Encoding"]&.match?(/\bgzip\b/)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Honor Accept-Encoding quality values before enabling gzip.

client_accepts_gzip? currently enables gzip whenever "gzip" appears, including gzip;q=0, which explicitly means “do not send gzip”.

Suggested fix
 def client_accepts_gzip?
-  return false unless respond_to?(:request)
-
-  request.headers["Accept-Encoding"]&.match?(/\bgzip\b/)
+  return false unless respond_to?(:request) && request
+
+  encodings = request.headers["Accept-Encoding"].to_s.split(",").map do |entry|
+    coding, *params = entry.strip.downcase.split(";")
+    q_param = params.find { |param| param.strip.start_with?("q=") }
+    q_value = q_param&.split("=", 2)&.last&.to_f
+    [coding, q_value.nil? ? 1.0 : q_value]
+  end.to_h
+
+  (encodings["gzip"] || encodings["*"] || 0.0).positive?
 end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb` around lines
145 - 149, The method client_accepts_gzip? currently returns true if "gzip"
appears anywhere in Accept-Encoding even when q=0; update client_accepts_gzip?
to properly parse the request.headers["Accept-Encoding"] string (in the
ReactOnRailsPro::Concerns::Stream concern) and only return true when an entry
for "gzip" exists with an explicit weight > 0 (i.e., parse comma-separated
tokens, handle optional "q=" parameters, treat missing q as 1.0, and ignore gzip
if q=0). Ensure it still safely handles nil headers and uses
respond_to?(:request) as before.

@justin808
Copy link
Member Author

Closing in favor of #2530, which takes a better approach:

  • Opt-in compress: parameter instead of always-on — only RSC payload opts in, doesn't affect other streaming endpoints
  • Clean GzipOutputStream class instead of define_singleton_method monkey-patching
  • Proper Accept-Encoding parsing via Rack::Utils.q_values instead of simple regex
  • Sets Vary: Accept-Encoding for correct HTTP caching behavior
  • No Content-Encoding: identity fallback — when compression isn't used, leaves headers untouched so Rack middleware can still compress non-streaming responses
  • Explicit output_stream plumbing through drain_streams_concurrently instead of patching response.stream in place

@justin808 justin808 closed this Mar 4, 2026
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.

RSC payload streaming deadlocks with Rack::Deflater and Rack::Brotli compression middleware

1 participant