Fix HTTPX thread-safety issue with thread-local connections for incremental rendering

[AbanoubGhadban]

Problem

The HTTPX stream_bidi plugin has a critical limitation: only the thread that creates the bidirectional streaming connection can write chunks to the request stream. This causes race conditions in multi-threaded production environments.

Race Condition Scenario

1. First request (Thread A):

   • Creates HTTPX connection
   • Stores in @incremental_connection instance variable
   • Works fine ✅

2. Second request (Thread B - different Puma worker thread):

   • Tries to use same @incremental_connection
   • Attempts to write chunks
   • FAILS ❌ - HTTPX raises error because Thread B didn't create the connection

Root Cause

• Puma handles different HTTP requests on different threads
• HTTPX stream_bidi plugin requires thread affinity for writes
• Shared instance variable causes cross-thread access violations

Solution

Implemented thread-local storage for incremental rendering connections:

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= create_incremental_connection
end

How It Works

• Each Puma worker thread gets its own persistent bidirectional streaming connection
• Stored in Thread.current instead of instance variable
• Automatically isolated between threads
• Proper cleanup via reset_thread_local_incremental_connections

Changes

• Modified incremental_connection to use thread-local storage
• Added reset_thread_local_incremental_connections method
• Updated reset_connection to clean up all thread-local connections
• Removed shared @incremental_connection instance variable

Trade-offs

✅ Benefits

• Eliminates race conditions completely
• Simple implementation using Ruby's built-in thread-local storage
• Each thread has isolated, persistent connection
• Works correctly in production with Puma

⚠️ Limitations

• Higher memory usage (one connection per Puma worker thread)
• Not optimal for high-scale production with many threads (e.g., Puma with 5 threads × 5 workers = 25 connections)
• Temporary solution until better alternatives are available

Long-Term Solutions

This is a temporary workaround. Permanent solutions being considered:

1. Fix HTTPX - Contribute to HTTPX to support multi-threaded bidirectional streaming
2. Migrate to async-http - Use async-http gem which is thread-safe by design

See #2115 for detailed discussion.

Testing

• Code passes RuboCop validation
• Test incremental rendering with multiple concurrent requests
• Verify no race conditions in multi-threaded environment
• Measure memory usage with multiple Puma threads

Related Issues

Fixes #2115

---

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch abanoubghadban/pro2115/fix-httpx-thread-safety-with-thread-local-connections

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Code Review: Thread-Safety Fix for Incremental Rendering

I've reviewed the changes in this PR, which addresses a critical thread-safety issue with HTTPX's stream_bidi plugin in multi-threaded Puma environments. Here's my feedback:

---

✅ What Works Well

1. Core Solution is Sound

The switch from instance variables to thread-local storage correctly solves the immediate problem:

• Each Puma thread gets its own bidirectional streaming connection
• Eliminates cross-thread access violations
• Simple implementation using Ruby's built-in Thread.current

2. TypeScript Changes are Correct

• Making add() and onUpdateReceived() async is appropriate for awaiting promises
• The defensive check if (log.level === 'debug' && result) prevents crashes when result is undefined/null

3. Good Documentation

• PR description clearly explains the problem, solution, and trade-offs
• Inline comments document the thread-local approach
• Acknowledges this is a temporary workaround

---

⚠️ Critical Issues

1. Thread.list Race Condition and Memory Leak Risk

The reset_thread_local_incremental_connections implementation has serious issues:

def reset_thread_local_incremental_connections
  Thread.list.each do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Problems:

1. Thread.list only returns alive threads - If a Puma worker thread terminates after handling requests, its thread-local connection won't be in Thread.list and will leak
2. Race condition - Threads can be created/destroyed during iteration
3. Cross-thread variable access - Accessing another thread's thread-local variables is generally unsafe

Recommended fix:

def reset_thread_local_incremental_connections
  # Only close the current thread's connection
  conn = Thread.current[:react_on_rails_incremental_connection]
  conn&.close
  Thread.current[:react_on_rails_incremental_connection] = nil
end

Or, if you need to reset all connections (e.g., during app reload):

# Track connections in a thread-safe data structure
@connections_mutex = Mutex.new
@all_connections = []

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= begin
    conn = create_incremental_connection
    @connections_mutex.synchronize { @all_connections << conn }
    conn
  end
end

def reset_all_incremental_connections
  @connections_mutex.synchronize do
    @all_connections.each { |conn| conn.close rescue nil }
    @all_connections.clear
  end
end

2. Missing Tests for Thread-Safety

The PR description has this unchecked item:

• Test incremental rendering with multiple concurrent requests
• Verify no race conditions in multi-threaded environment

This is critical - the entire point of the PR is fixing thread-safety, but there are no tests verifying it works. The existing tests in request_spec.rb only test single-threaded scenarios.

---

🔍 Minor Issues

3. TypeScript: Unnecessary .catch() After await

In handleIncrementalRenderRequest.ts:100:

await executionContext.runInVM(chunk.updateChunk, bundlePath).catch((err: unknown) => {
  log.error({ msg: 'Error running incremental render chunk', err, chunk });
});

This pattern (await promise.catch()) swallows errors - the await will never throw. Better to handle in the outer try/catch.

---

📋 Testing Recommendations

1. Local testing with Puma threads (MANDATORY before merge):

   • Configure Puma with multiple threads in config/puma.rb
   • Make concurrent requests that trigger incremental rendering
   • Verify logs show different thread IDs successfully rendering

2. Memory leak testing:

   • Start Puma with 5 threads
   • Make 1000+ incremental rendering requests
   • Monitor connection count to ensure it stabilizes

---

🎯 Action Items Before Merge

Must Fix:

1. ❗ Fix reset_thread_local_incremental_connections - Remove Thread.list iteration or implement proper tracking
2. ❗ Add thread-safety tests - Verify concurrent requests work correctly
3. ❗ Test locally with multi-threaded Puma - Verify the fix actually works in production-like environment

Should Fix:

4. Simplify error handling in TypeScript (await + .catch() pattern)

---

Summary

The core approach is correct and will likely fix the immediate issue, but the Thread.list implementation is problematic and must be addressed. The lack of thread-safety tests is also concerning given the nature of the bug.

Please address the critical issues, add proper tests, and verify in a multi-threaded environment before merging.

[claude]

I have reviewed PR #2116. Here is my comprehensive code review feedback.

[claude]

✅ Strengths

1. Problem Identification: The PR correctly identifies a critical thread-safety issue with HTTPX stream_bidi plugin. The root cause analysis is clear and accurate.

2. Solution Approach: Using thread-local storage via Thread.current is a reasonable short-term solution. Each Puma thread gets its own connection, eliminating cross-thread access violations.

3. Code Quality: Clean implementation with good comments, follows Ruby conventions, and includes proper cleanup method.

4. TypeScript Changes: Making add async and awaiting runInVM ensures proper sequencing of incremental updates.

[claude]

⚠️ Critical Issue #1: Missing Thread-Safety Tests (SEVERITY: HIGH)

The PR adds thread-local storage but does not include tests that verify thread-safety. This is concerning because:

• The entire purpose of this PR is fixing race conditions
• Without tests, we cannot verify the fix works or prevent regressions
• The existing tests in request_spec.rb do not exercise multi-threaded scenarios

Recommendation: Add a test that simulates concurrent requests from multiple threads. Example pattern:

describe "thread-safety" do
  it "handles concurrent incremental render requests" do
    threads = 5.times.map do |i|
      Thread.new do
        stream = described_class.render_code_with_incremental_updates(
          "/render-incremental",
          "console.log()",
          async_props_block: proc { |e| e.call("prop", "val") },
          is_rsc_payload: false
        )
        chunks = []
        stream.each_chunk { |chunk| chunks << chunk }
        chunks
      end
    end
    results = threads.map(&:value)
    expect(results.size).to eq(5)
  end
end

[claude]

⚠️ Critical Issue #2: Potential Memory Leak in Cleanup (SEVERITY: MEDIUM)

The reset_thread_local_incremental_connections method has a subtle bug:

def reset_thread_local_incremental_connections
  Thread.list.each do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Problem: Thread.list only returns ALIVE threads. If a Puma thread terminates without cleanup, the connection remains in thread-local storage but becomes inaccessible. Over time with thread churn, this could accumulate unclosed connections.

Recommendation: Consider using ObjectSpace or a thread-finalizer to ensure cleanup when threads exit, or track connections in a concurrent-safe data structure that can be cleaned up independently of thread lifecycle.

[claude]

⚠️ Critical Issue #3: Inconsistent Async Handling (SEVERITY: LOW-MEDIUM)

In worker.ts, the onUpdateReceived callback is now async:

onUpdateReceived: async (obj: unknown) => {
  await incrementalSink.add(obj);
}

Question: What happens if multiple chunks arrive while one is still processing? Is there queueing or could chunks be processed out of order?

Recommendation: Add a comment explaining the concurrency model. Consider if chunks need to be processed serially (use a queue) or can be parallel.

[claude]

⚠️ Critical Issue #4: Missing Documentation on Limitations (SEVERITY: LOW)

The PR description mentions trade-offs, but the code itself lacks inline documentation about:

• Why thread-local storage is needed
• The memory implications
• When this solution should be revisited

Recommendation: Add a module-level comment explaining this is a TEMPORARY workaround with clear TODOs pointing to issue #2115. For example:

# Thread-local connection management for incremental rendering
#
# IMPORTANT: This uses thread-local storage as a TEMPORARY workaround
# for HTTPX stream_bidi thread-affinity limitations.
#
# Trade-offs:
# - Memory: One connection per thread (5 threads × 5 workers = 25 connections)
# - Thread safety: Eliminates race conditions
#
# TODO: Replace with proper solution (see #2115):
# - Fix HTTPX to support multi-threaded bidirectional streaming, OR
# - Migrate to async-http gem (thread-safe by design)

[claude]

🔍 Additional Concerns

5. Missing Defensive Check: The addition of && result in the debug logging is good, but consider if this could mask issues. If result is unexpectedly null/undefined, we should probably log a warning rather than silently skip.

6. No Performance Impact Assessment: The PR mentions memory usage concerns but does not quantify the performance impact. Consider:

• What is the memory footprint of one HTTPX connection?
• In a typical Puma configuration (5 workers × 5 threads), is 25 connections acceptable?
• Should we add metrics/logging to track connection count in production?

[claude]

📋 Checklist Before Merge

• Add multi-threaded integration test
• Fix potential memory leak in thread cleanup
• Document the temporary nature of this solution in code comments
• Consider adding connection pooling metrics/logging
• Verify the async handling order in TypeScript is correct
• Update the testing section in PR description once tests are added

🎯 Summary

This PR correctly identifies and addresses a critical thread-safety bug, but it needs stronger testing before merge. The lack of thread-safety tests is the most significant gap - this fix is worthless if we cannot verify it works and prevent future regressions.

The solution is reasonable as a temporary fix but should be clearly marked as such in the code. Make sure to track the long-term solutions mentioned in #2115.

Recommendation: Request changes to add thread-safety tests before merging.

[claude]

Code Review

Summary

This PR addresses a critical thread-safety issue with HTTPX bidirectional streaming by implementing thread-local storage for incremental rendering connections. Overall, the approach is sound for a temporary workaround.

---

🔴 Critical Issues

1. Resource Leak in reset_thread_local_incremental_connections

Location: react_on_rails_pro/lib/react_on_rails_pro/request.rb:143-148

def reset_thread_local_incremental_connections
  # Close all thread-local incremental connections
  Thread.list.each do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Problem: Thread.list only returns alive threads. Dead threads (which have finished execution) won't be cleaned up, potentially leaking connections and memory.

Fix: Consider using a more robust cleanup strategy:

def reset_thread_local_incremental_connections
  # Close all thread-local incremental connections
  Thread.list.each do |thread|
    next unless thread.thread_variable?(:react_on_rails_incremental_connection)
    
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Better approach: Track connections in a thread-safe data structure:

@incremental_connections_mutex = Mutex.new
@incremental_connections = {}

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= begin
    conn = create_incremental_connection
    @incremental_connections_mutex.synchronize do
      @incremental_connections[Thread.current.object_id] = conn
    end
    
    # Cleanup when thread exits
    Thread.current.thread_variable_set(:cleanup_registered, true)
    at_exit { cleanup_thread_connection(Thread.current.object_id) }
    
    conn
  end
end

def reset_thread_local_incremental_connections
  @incremental_connections_mutex.synchronize do
    @incremental_connections.each_value(&:close)
    @incremental_connections.clear
  end
end

private

def cleanup_thread_connection(thread_id)
  @incremental_connections_mutex.synchronize do
    conn = @incremental_connections.delete(thread_id)
    conn&.close
  end
end

---

2. Missing Error Handling in Async Operations

Location: packages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderRequest.ts:96-103

add: async (chunk: unknown) => {
  try {
    assertIsUpdateChunk(chunk);
    const bundlePath = getRequestBundleFilePath(chunk.bundleTimestamp);
    await executionContext.runInVM(chunk.updateChunk, bundlePath).catch((err: unknown) => {
      log.error({ msg: 'Error running incremental render chunk', err, chunk });
    });
  } catch (err) {
    log.error({ msg: 'Error processing chunk', err });
  }
}

Problem: The .catch() swallows errors from runInVM, but the outer try/catch won't catch them since they're already handled. The error handling is redundant and potentially confusing.

Fix:

add: async (chunk: unknown) => {
  try {
    assertIsUpdateChunk(chunk);
    const bundlePath = getRequestBundleFilePath(chunk.bundleTimestamp);
    await executionContext.runInVM(chunk.updateChunk, bundlePath);
  } catch (err) {
    log.error({ msg: 'Error running incremental render chunk', err, chunk });
    // Consider: should this rethrow? Should rendering continue or abort?
  }
}

---

⚠️ Medium Priority Issues

3. Race Condition in Connection Creation

Location: react_on_rails_pro/lib/react_on_rails_pro/request.rb:139

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= create_incremental_connection
end

Problem: The ||= operator is not thread-safe for the same thread-local variable if multiple operations within the same thread try to initialize it concurrently. While unlikely with thread-local storage, it's worth noting.

Recommendation: This is likely fine for your use case, but document that create_incremental_connection should be idempotent or that concurrent calls from the same thread are not expected.

---

4. Async/Await Consistency

Location: packages/react-on-rails-pro-node-renderer/src/worker.ts:318-326

The change from synchronous to async is good, but ensure all callers handle the Promise properly:

onUpdateReceived: async (obj: unknown) => {
  // ...
  await incrementalSink.add(obj);
}

Question: Is onUpdateReceived called in a context where async/await is properly handled? If it's a callback, ensure the caller awaits or handles the returned Promise.

---

5. Silent Defensive Check

Location: packages/react-on-rails-pro-node-renderer/src/worker/vm.ts:368

if (log.level === 'debug' && result) {

Question: What does it mean when result is falsy? Is this:

• An empty string (valid result)?
• undefined (error case)?
• null (intentional no-op)?

Recommendation: Add a comment explaining why the result check is needed, or log a warning if it's unexpected:

if (log.level === 'debug') {
  if (result) {
    log.debug('result from JS: ' + smartTrim(result));
    // ... save debug output
  } else {
    log.debug('Empty result from JS execution');
  }
}

---

✅ Strengths

1. Clear problem identification - The PR description excellently documents the race condition
2. Appropriate temporary solution - Thread-local storage is a pragmatic fix
3. Good test coverage expectations - Tests listed in PR description are appropriate
4. Async conversion is correct - Making add() async properly propagates backpressure

---

📋 Testing Recommendations

Since the PR mentions tests are incomplete (unchecked boxes), ensure you cover:

Ruby Side:

1. Multi-threaded stress test:

   threads = 10.times.map do
     Thread.new do
       100.times { ReactOnRailsPro::Request.incremental_connection }
     end
   end
   threads.each(&:join)
   # Verify no exceptions, all threads got connections

2. Connection cleanup test:

   # Create connections in multiple threads
   # Call reset_thread_local_incremental_connections
   # Verify all connections are closed (mock/spy on close method)

3. Thread death scenario:

   # Create connection in a thread
   # Let thread die
   # Verify connection is eventually cleaned up

TypeScript Side:

1. Async error propagation:

   • Test that errors in runInVM are properly logged
   • Verify streaming continues/aborts appropriately on errors

2. Backpressure handling:

   • Test that awaiting add() properly applies backpressure when VM is slow

---

🔧 Performance Considerations

The PR mentions memory usage concerns with one connection per thread. Consider:

1. Connection pooling: Instead of one-per-thread, use a small pool with thread-safe access
2. Lazy connection timeout: Close idle connections after N seconds
3. Monitoring: Add metrics for active connection count

Example with pooling (future improvement):

@connection_pool = ConnectionPool.new(size: 5) { create_incremental_connection }

def incremental_connection
  @connection_pool.with { |conn| yield conn }
end

---

🏁 Summary

Block merge? No - This is an improvement over the current broken state.

Required before merge:

1. ✅ Fix the thread cleanup leak (Issue TODO for first version #1)
2. ✅ Simplify async error handling (Issue Make work with turbolinks and better helper #2)
3. ✅ Complete the test checklist

Nice to have:

• Document the thread-safety guarantees
• Add connection count metrics
• Consider connection pooling for future optimization

---

Great work identifying and fixing this subtle concurrency bug! The thread-local approach is solid for a temporary fix while you explore the long-term solutions (HTTPX fix or async-http migration).

---

🤖 Generated with Claude Code

[claude]

Code Review: Thread-Local HTTPX Connections for Incremental Rendering

Overview

This PR addresses a critical thread-safety issue in HTTPX's stream_bidi plugin by implementing thread-local connections. I've reviewed the changes across both Ruby and TypeScript files.

---

🟢 Strengths

1. Problem Correctly Identified

The root cause analysis is solid - HTTPX's stream_bidi plugin requires thread affinity for writes, which conflicts with Puma's multi-threaded request handling.

2. Clean Ruby Implementation

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= create_incremental_connection
end

• Simple, idiomatic Ruby solution
• Leverages built-in thread-local storage
• Follows Ruby conventions

3. Proper Cleanup

The reset_thread_local_incremental_connections method properly iterates through all threads and closes connections:

Thread.list.each do |thread|
  conn = thread[:react_on_rails_incremental_connection]
  conn&.close
  thread[:react_on_rails_incremental_connection] = nil
end

4. TypeScript Async/Await Consistency

Converting add() to async properly propagates async operations through the call chain.

---

🟡 Concerns & Recommendations

1. Resource Leaks (CRITICAL)

Problem: Thread-local storage persists for the lifetime of the thread. In Puma, worker threads are long-lived. If a thread handles requests for different apps/instances, the connection stored in Thread.current[:react_on_rails_incremental_connection] might become stale or point to the wrong renderer.

Scenarios:

• App reloads during development
• Zero-downtime deployments where worker processes get recycled
• Configuration changes to renderer_url

Current code:

def reset_connection
  @standard_connection&.close
  @standard_connection = nil
  reset_thread_local_incremental_connections  # Only called here\!
end

When is reset_connection called? If it's only called during initialization or manual resets, connections may remain open indefinitely.

Recommendations:

1. Document when reset_connection is called (initialization? config reload? manual only?)
2. Consider adding a TTL or staleness check for thread-local connections
3. Add connection validation before reuse:

   def incremental_connection
     conn = Thread.current[:react_on_rails_incremental_connection]
     return conn if conn && connection_valid?(conn)
     
     Thread.current[:react_on_rails_incremental_connection] = create_incremental_connection
   end

2. Thread.list Safety (MEDIUM)

Problem: Thread.list can include threads from other parts of the application, and accessing thread-local variables of arbitrary threads has edge cases:

• Threads might be terminating while you iterate
• Other libraries might use the same symbol name (unlikely but possible)
• Background job threads, Rails initialization threads, etc.

Current code:

Thread.list.each do |thread|
  conn = thread[:react_on_rails_incremental_connection]
  conn&.close
end

Recommendations:

1. Wrap in exception handling:

   def reset_thread_local_incremental_connections
     Thread.list.each do |thread|
       begin
         conn = thread[:react_on_rails_incremental_connection]
         conn&.close
       rescue => e
         Rails.logger.warn("Failed to close incremental connection: #{e}")
       ensure
         thread[:react_on_rails_incremental_connection] = nil
       end
     end
   end

2. Consider maintaining a registry of threads that have connections:

   @threads_with_connections = Concurrent::Set.new
   
   def incremental_connection
     Thread.current[:react_on_rails_incremental_connection] ||= begin
       @threads_with_connections.add(Thread.current)
       create_incremental_connection
     end
   end

3. Memory Usage Documentation (LOW)

The PR description mentions memory concerns ("5 threads × 5 workers = 25 connections"), but:

• What's the actual memory footprint per connection?
• How does this compare to the connection pool overhead?
• At what scale does this become a problem?

Recommendation: Add a comment in the code:

# Thread-local connection for incremental rendering
# Trade-off: One connection per Puma worker thread (e.g., 5 workers × 5 threads = 25 connections)
# Memory overhead: ~X KB per connection (TODO: measure)
# This is a temporary solution until HTTPX supports multi-threaded bidirectional streaming

4. TypeScript: Error Handling (LOW)

In handleIncrementalRenderRequest.ts:

await executionContext.runInVM(chunk.updateChunk, bundlePath).catch((err: unknown) => {
  log.error({ msg: 'Error running incremental render chunk', err, chunk });
});

The error is logged but not propagated. Is this intentional? Should the sink be notified of failures?

5. TypeScript: Defensive Null Check (GOOD)

The change in vm.ts is excellent:

-      if (log.level === 'debug') {
+      if (log.level === 'debug' && result) {

This prevents potential issues with smartTrim(undefined). Nice catch!

---

🔴 Critical Issues

Missing Test Coverage

The PR checklist shows:

• Code passes RuboCop validation
• Test incremental rendering with multiple concurrent requests ❌
• Verify no race conditions in multi-threaded environment ❌
• Measure memory usage with multiple Puma threads ❌

This is a critical fix for a threading bug. Without tests:

1. We can't verify the race condition is actually fixed
2. Future refactors might reintroduce the bug
3. We don't know the performance/memory impact

Recommendations:

1. Add an RSpec test that simulates concurrent requests:

   it 'handles concurrent incremental render requests' do
     threads = 10.times.map do
       Thread.new { Request.incremental_connection }
     end
     threads.each(&:join)
     
     # Verify each thread got its own connection
     # Verify no exceptions were raised
   end

2. Add a test for reset_thread_local_incremental_connections:

   it 'cleans up thread-local connections on reset' do
     Request.incremental_connection # Create connection
     Request.reset_connection
     
     # Verify connection was closed
     # Verify thread-local variable is nil
   end

3. Add integration test with actual incremental rendering under concurrent load

---

📋 Additional Suggestions

1. Add Observability

Track connection creation for monitoring:

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= begin
    Rails.logger.debug { "[ReactOnRailsPro] Creating thread-local incremental connection for thread #{Thread.current.object_id}" }
    create_incremental_connection
  end
end

2. Consider Connection Pool Limits

With thread-local connections, you bypass HTTPX's connection pool limits. If Puma has 5 threads × 5 workers = 25 threads, you'll have 25 bidirectional connections to the Node renderer. Does the renderer handle this? Is there a max connection limit?

3. Documentation Update

Update the Pro README or configuration docs to mention:

• This is a temporary workaround
• Memory implications for high-thread-count deployments
• Link to issue Investigate fixes for race conditions happens at HTTPX when run asynchronously #2115 for context

---

🎯 Verdict

Overall: Good temporary fix, but needs safety improvements and tests before merging.

Before Merging:

1. ✅ MUST: Add exception handling to reset_thread_local_incremental_connections
2. ✅ MUST: Add basic concurrency tests
3. ✅ SHOULD: Add connection staleness validation
4. ✅ SHOULD: Add observability logging
5. ✅ COULD: Document memory implications in code comments

After Merging:

1. Measure actual memory usage in production
2. Pursue long-term fix (HTTPX patch or migrate to async-http)
3. Monitor for connection leaks

---

📚 References for Long-Term Fix

As mentioned in #2115, consider:

1. async-http gem - Thread-safe by design, works with async gem you already use
2. HTTPX patch - Contribute upstream fix for stream_bidi thread-safety

Great work identifying and addressing this critical race condition! With the suggested improvements, this will be a solid temporary solution while pursuing a permanent fix.

---

Generated with Claude Code - Feel free to push back on any suggestions!

[claude]

Code Review - PR #2116: HTTPX Thread-Safety Fix

Thank you for tackling this critical thread-safety issue! The overall approach is sound, but I've identified several important concerns that need to be addressed before merging.

---

🔴 Critical Issues

1. Memory Leak Risk in reset_thread_local_incremental_connections

Location: react_on_rails_pro/lib/react_on_rails_pro/request.rb:144-149

Problem: This implementation iterates over ALL threads in the Ruby VM, including threads that may not be related to Puma workers (background jobs, other gems' threads, etc.). This is:

• Inefficient: Scales poorly as thread count grows
• Unsafe: Accessing thread-local variables from other threads is generally discouraged
• Incomplete: Dead threads remain in Thread.list until garbage collected, so their connections won't be cleaned up

Recommended Solution: Use a class-level registry to track only threads that actually created connections. This ensures you only touch threads that actually have connections, and avoids accumulating dead thread references.

---

2. Connection Not Closed on Thread Death

Problem: When a Puma worker thread dies (e.g., during graceful shutdown or worker restart), the connection stored in Thread.current is not automatically closed. This can lead to:

• Lingering open connections
• Resource exhaustion on the Node renderer side
• Port exhaustion in high-restart scenarios

Recommended Solution: Implement connection cleanup in Puma's on_worker_shutdown hook or use thread finalizers.

---

3. Missing Thread-Safety in create_incremental_connection

Question: Is create_incremental_connection thread-safe? If multiple requests hit the same Puma thread simultaneously (e.g., during high concurrency), you could have a race condition where two threads try to create a connection at the same time.

Recommended Solution: Add mutex protection using a double-check pattern to avoid unnecessary locking.

---

⚠️ Important Concerns

4. TypeScript: onUpdateReceived Made Async Without Error Handling

Location: packages/react-on-rails-pro-node-renderer/src/worker.ts:318

Problem: Making this function async changes its error handling behavior. If add() returns a rejected promise, the async function wrapper converts it to a rejected promise. Who catches this rejected promise? If the caller doesn't await this function, the error could be silently swallowed or cause unhandled promise rejections.

Questions:

1. Does the caller of onUpdateReceived await the result?
2. Is there error handling at the caller level?
3. Should errors in add() be fatal or should processing continue?

---

5. TypeScript: Removed .catch() in Favor of await

Location: packages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderRequest.ts:100

Analysis: This changes the behavior from fire-and-forget to sequential processing.

Question: Is the performance impact acceptable? If runInVM is slow, this could block chunk processing.

Recommendation: Clarify the intent:

• If sequential processing is required (chunks must be processed in order), this is correct
• If parallel processing is acceptable, consider using Promise.all() or keeping the original fire-and-forget pattern

---

6. Missing Test Coverage for Thread-Safety

Problem: The PR description mentions unchecked items for testing concurrent requests and verifying no race conditions, but I don't see any new tests added. Thread-safety bugs are notoriously difficult to debug in production.

Recommendation: Add integration tests that simulate concurrent requests with multiple threads creating connections, and verify cleanup works correctly.

---

💡 Performance & Design Considerations

7. High Memory Usage with Many Threads

The PR description acknowledges this isn't optimal for high-scale production with many threads (e.g., Puma with 5 threads × 5 workers = 25 connections).

Suggestions:

1. Document the recommended Puma configuration (fewer threads per worker?)
2. Add monitoring/logging for connection count
3. Consider a connection pool with a maximum size to prevent resource exhaustion

---

8. Alternative: Connection Pool per Process, Not Thread

Alternative Approach: Instead of one connection per thread, consider one connection per Puma worker process keyed by Process.pid.

Pros:

• Lower memory usage (one connection per worker process, not per thread)
• Automatically handles worker restarts (new PID = new connection)
• Simpler cleanup (just track PIDs, not threads)

Cons:

• Requires mutex protection for thread-safety within a worker
• Still subject to HTTPX's thread affinity issue if multiple threads try to write

Question: Does HTTPX allow a single bidirectional connection to be shared across threads if writes are mutex-protected?

---

🔍 Minor Issues

9. Defensive Nil Check Seems Unnecessary

Location: packages/react-on-rails-pro-node-renderer/src/worker/vm.ts:368

Adding && result to the debug check suggests there's a case where result could be null/undefined, but the code above ensures result is always a string. Either remove the check or add a comment explaining why it's needed.

---

📋 Action Items

Must Fix Before Merging:

• Fix memory leak in reset_thread_local_incremental_connections (Issue TODO for first version #1)
• Add connection cleanup on thread death (Issue Make work with turbolinks and better helper #2)
• Add thread-safety tests (Issue Detailed Setup and Usage Docs #6)
• Verify onUpdateReceived error handling (Issue Add integration tests #4)

Should Consider:

• Add mutex protection to incremental_connection (Issue Add linting and CI scripts #3)
• Document performance implications of await changes (Issue Add Unit tests #5)
• Evaluate connection-per-process alternative (Issue Port console polyfill from react-rails #8)
• Add monitoring for connection count (Issue Clean up app/startup/clientApp.jsx and app/startup/serverApp.jsx #7)

Nice to Have:

• Clarify nil check reasoning (Issue Support configuration options #9)
• Add configuration docs for recommended Puma setup

---

🎯 Overall Assessment

Strengths:

• Correctly identifies the root cause (HTTPX thread affinity)
• Implements a pragmatic solution with thread-local storage
• Acknowledges limitations and trade-offs transparently

Concerns:

• Critical memory leak risk in cleanup logic
• Missing test coverage for the core thread-safety fix
• Potential unhandled promise rejections in TypeScript changes

Recommendation: Request Changes - The core fix is sound, but the implementation needs refinement to prevent production issues.

---

Great work on identifying and documenting this issue! The fix is heading in the right direction. Let me know if you'd like help implementing any of these suggestions. 🚀

[claude]

Code Review - Thread Safety Fix for HTTPX Incremental Rendering

I've reviewed the changes in this PR that address the critical thread-safety issue with HTTPX bidirectional streaming. Here's my analysis:

---

✅ Strengths

1. Correct Root Cause Analysis: The PR accurately identifies the HTTPX stream_bidi limitation where only the thread that creates a connection can write to it.

2. Appropriate Solution: Thread-local storage is the right approach for this problem. Using Thread.current ensures each Puma worker thread gets its own isolated connection.

3. Proper Cleanup: The reset_thread_local_incremental_connections method correctly iterates through all threads to clean up connections.

4. TypeScript Changes Look Good: Converting onUpdateReceived and sink.add to async/await is the right approach for handling asynchronous chunk processing.

---

⚠️ Potential Issues & Recommendations

1. Critical Bug: Thread Enumeration Race Condition

def reset_thread_local_incremental_connections
  Thread.list.each do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Problem: Thread.list only returns threads that exist at the moment it's called. In a Puma server:

• Threads may be created/destroyed dynamically
• Thread-local variables persist even if you can't enumerate the thread
• You could leak connections if threads exit without cleanup

Recommendation:

# Better approach: Track connections in a thread-safe structure
@incremental_connections_mutex = Mutex.new
@incremental_connections = {}

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= begin
    conn = create_incremental_connection
    @incremental_connections_mutex.synchronize do
      @incremental_connections[Thread.current.object_id] = conn
    end
    conn
  end
end

def reset_thread_local_incremental_connections
  @incremental_connections_mutex.synchronize do
    @incremental_connections.each_value(&:close)
    @incremental_connections.clear
  end
end

2. Resource Leak on Thread Death

When a Puma thread exits (e.g., during graceful shutdown or thread pool resizing), the connection stored in Thread.current may not be cleaned up.

Recommendation: Add thread finalization:

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= begin
    conn = create_incremental_connection
    
    # Clean up when thread exits
    Thread.current.thread_variable_set(:connection_finalizer, 
      Thread.current.add_finalizer { conn.close rescue nil })
    
    conn
  end
end

3. Missing Error Handling in TypeScript

onUpdateReceived: async (obj: unknown) => {
  // ...
  await incrementalSink.add(obj);
}

Issue: The async function doesn't have a top-level try/catch. While there's error handling inside add(), the promise rejection from onUpdateReceived could be unhandled.

Recommendation: Ensure proper error propagation or add top-level error handling.

4. VM Debug Logging Guard Could Hide Errors

-      if (log.level === 'debug') {
+      if (log.level === 'debug' && result) {

Question: What happens when result is null, undefined, 0, false, or empty string? These might be valid results that should be logged in debug mode. Consider:

if (log.level === 'debug' && result \!== undefined) {

5. Testing Concerns

From the PR description:

• Test incremental rendering with multiple concurrent requests
• Verify no race conditions in multi-threaded environment

Critical: These tests are essential and should be completed before merging. The thread-safety fix cannot be validated without them.

Suggested test approach:

RSpec.describe 'Incremental rendering thread safety' do
  it 'handles concurrent requests from different threads' do
    threads = 10.times.map do
      Thread.new do
        # Make incremental render request
        # Verify connection works correctly
      end
    end
    threads.each(&:join)
    
    # Verify all requests succeeded
    # Verify no connection errors
  end
end

---

📊 Performance & Scalability

Memory Impact: As noted, this creates one connection per Puma thread. For puma -t 5:5 -w 5:

• 5 workers × 5 threads = 25 connections
• Each HTTP/2 connection has overhead (TCP, TLS, buffers)

Recommendation: Document the memory impact in the Pro configuration guide and suggest appropriate Puma thread counts for incremental rendering.

---

🔒 Security Considerations

No major security concerns, but:

1. Ensure create_incremental_connection properly validates the renderer URL
2. Consider connection timeout configurations for abandoned threads

---

📝 Documentation Needs

1. Add comment explaining why thread-local storage is needed (link to HTTPX issue/limitation)
2. Document the memory/scaling implications for Pro users
3. Update troubleshooting guide for connection pool sizing

---

🎯 Verdict

Conditional Approval - The approach is sound, but critical issues must be addressed:

Before merging:

1. ✅ Fix the thread enumeration race condition (use tracked connections)
2. ✅ Add thread finalization for cleanup
3. ✅ Complete multi-threaded testing
4. ⚠️ Review VM logging guard condition
5. 📖 Add documentation comments

After merging:

• Monitor production memory usage
• Track connection pool exhaustion
• Plan for long-term solution (HTTPX fix or async-http migration)

---

Great work identifying and addressing this critical thread-safety issue! The core solution is solid, just needs some defensive programming around thread lifecycle management.

🤖 Review generated with Claude Code

[claude]

Code Review

Thank you for addressing this critical thread-safety issue! The PR correctly identifies and fixes the HTTPX bidirectional streaming limitation with multi-threaded environments. Here's my detailed review:

---

✅ Strengths

1. Correct Problem Identification

• The root cause analysis is spot-on: HTTPX's stream_bidi plugin requires thread affinity for write operations
• The race condition scenario is well-documented and clearly explained

2. Appropriate Solution for the Constraint

• Thread-local storage (Thread.current) is the right approach given HTTPX's limitation
• Each Puma worker thread gets isolated connections, eliminating race conditions

3. Good Documentation

• Clear inline comments explaining the trade-offs
• Honest acknowledgment that this is a temporary solution
• Long-term alternatives documented in the PR description

---

⚠️ Critical Issues

1. Resource Leak Risk in reset_thread_local_incremental_connections

Location: react_on_rails_pro/lib/react_on_rails_pro/request.rb:144-148

def reset_thread_local_incremental_connections
  # Close all thread-local incremental connections
  Thread.list.each do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Problem: Thread.list only returns currently alive threads. Dead threads with open connections won't be cleaned up, causing:

• Memory leaks (connection objects remain in memory)
• File descriptor leaks (TCP connections stay open)
• In Puma with thread pooling, threads often die/restart

Recommended Fix:

# Add to reset_connection
def reset_connection
  @standard_connection&.close
  @standard_connection = nil
  reset_thread_local_incremental_connections
end

def reset_thread_local_incremental_connections
  # Close connections from all threads (alive and dead)
  ObjectSpace.each_object(Thread) do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close if conn.respond_to?(:close)
    thread[:react_on_rails_incremental_connection] = nil
  rescue StandardError => e
    # Thread might be dead or inaccessible
    Rails.logger.warn("[ReactOnRailsPro] Failed to close thread-local connection: #{e.message}")
  end
end

Alternative: Use Thread.current.thread_variable_get/set which is more robust than Thread.current[:key].

---

2. Missing Connection Cleanup on Thread Death

Problem: When Puma kills a worker thread, the thread-local connection won't be closed automatically.

Recommendation: Add thread finalizers or use Puma's on_worker_shutdown hook:

# In your Rails initializer or engine configuration
if defined?(Puma)
  Puma::Server.current&.add_shutdown_hook do
    ReactOnRailsPro::Request.reset_connection
  end
end

# Or use thread-local finalizers
def incremental_connection
  conn = Thread.current[:react_on_rails_incremental_connection]
  return conn if conn

  conn = create_incremental_connection
  Thread.current[:react_on_rails_incremental_connection] = conn
  
  # Register finalizer to close connection when thread dies
  ObjectSpace.define_finalizer(conn, proc { conn.close rescue nil })
  
  conn
end

---

3. TypeScript Changes: Async Without Await

Location: packages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderRequest.ts:96-100

add: async (chunk: unknown) => {
  try {
    assertIsUpdateChunk(chunk);
    const bundlePath = getRequestBundleFilePath(chunk.bundleTimestamp);
    await executionContext.runInVM(chunk.updateChunk, bundlePath).catch((err: unknown) => {
      log.error({ msg: 'Error running incremental render chunk', err, chunk });
    });

Issues:

1. Swallowing Errors: The .catch() swallows errors without re-throwing. If runInVM fails, the caller won't know.

2. Error Handling Pattern: Mixing async/await with .catch() is inconsistent. Use try/catch instead:

add: async (chunk: unknown) => {
  try {
    assertIsUpdateChunk(chunk);
    const bundlePath = getRequestBundleFilePath(chunk.bundleTimestamp);
    
    try {
      await executionContext.runInVM(chunk.updateChunk, bundlePath);
    } catch (err) {
      log.error({ msg: 'Error running incremental render chunk', err, chunk });
      // Consider: should we re-throw here or continue processing?
      // Currently silently continues which might be intentional
    }
  } catch (err) {
    log.error({ msg: 'Invalid update chunk format', err, chunk });
    throw; // Re-throw assertion errors
  }
}

---

4. Unrelated Change in vm.ts

Location: packages/react-on-rails-pro-node-renderer/src/worker/vm.ts:368

-      if (log.level === 'debug') {
+      if (log.level === 'debug' && result) {

Question: What's the relationship between this null-check and the thread-safety fix? This appears to be an unrelated bug fix that should be in a separate commit/PR for clarity.

---

🔍 Testing Concerns

Missing Test Coverage

The PR description shows:

• Code passes RuboCop validation
• Test incremental rendering with multiple concurrent requests ❌
• Verify no race conditions in multi-threaded environment ❌
• Measure memory usage with multiple Puma threads ❌

Critical: According to CLAUDE.md, you MUST NOT claim a fix without local testing:

NEVER claim a test is "fixed" without running it locally first

Recommendations:

1. Add a multi-threaded integration test:

# spec/react_on_rails_pro/request_spec.rb
describe 'thread-safety' do
  it 'handles concurrent incremental rendering requests' do
    threads = 10.times.map do
      Thread.new do
        ReactOnRailsPro::Request.send(:incremental_connection)
      end
    end
    
    connections = threads.map(&:value)
    
    # Each thread should have its own connection
    expect(connections.uniq.size).to eq(10)
    
    # All connections should be functional
    connections.each { |conn| expect(conn).to be_a(HTTPX::Session) }
  end
end

2. Test connection cleanup:

it 'cleans up thread-local connections on reset' do
  # Create connections in multiple threads
  threads = 5.times.map { Thread.new { ReactOnRailsPro::Request.send(:incremental_connection) } }
  threads.each(&:join)
  
  # Verify connections exist
  expect(Thread.current[:react_on_rails_incremental_connection]).to be_present
  
  # Reset should close all
  ReactOnRailsPro::Request.reset_connection
  
  # Should be nil after reset
  expect(Thread.current[:react_on_rails_incremental_connection]).to be_nil
end

---

📝 Minor Issues

1. RuboCop Compliance

Ensure bundle exec rubocop passes (required by CLAUDE.md).

2. Documentation

Consider adding a warning in the configuration docs about the memory implications of this approach:

# config/initializers/react_on_rails_pro.rb
ReactOnRailsPro.configure do |config|
  # Note: Incremental rendering uses thread-local HTTP connections.
  # With Puma (5 threads × 5 workers = 25 connections), ensure your
  # node renderer can handle the connection pool size.
  config.renderer_http_pool_size = 50 # Should be > (threads × workers)
end

3. CHANGELOG Entry

Update /CHANGELOG_PRO.md (as per CLAUDE.md monorepo guidelines):

#### [Unreleased]
##### Fixed
- [PR 2116](https://github.com/shakacode/react_on_rails/pull/2116) by [AbanoubGhadban](https://github.com/AbanoubGhadban): Fix HTTPX thread-safety issue with thread-local connections for incremental rendering

---

🎯 Summary

Verdict: Approve with required changes

Must Fix Before Merge:

1. ✅ Fix resource leak in reset_thread_local_incremental_connections (use ObjectSpace.each_object(Thread))
2. ✅ Add connection cleanup on thread death (finalizers or Puma hooks)
3. ✅ Add multi-threaded integration tests to prevent regression
4. ✅ Update CHANGELOG_PRO.md

Recommended:

1. Improve error handling in TypeScript add() method
2. Separate unrelated vm.ts change into its own commit
3. Document memory implications in configuration

Excellent work on the analysis and temporary solution! This unblocks production use while you pursue the long-term async-http migration. 🚀

---

cc: @justin808 for awareness of the temporary workaround and memory implications.

[claude]

Code Review

Thank you for addressing the thread-safety issue with HTTPX bidirectional streaming! This is a critical fix for production environments. Here's my detailed review:

---

✅ Strengths

1. Correct Problem Identification

The PR correctly identifies that HTTPX's stream_bidi plugin has thread-affinity requirements, which causes race conditions in multi-threaded environments like Puma.

2. Simple, Effective Solution

Using Thread.current for thread-local storage is the right approach for this problem. Each Puma worker thread gets its own connection, eliminating cross-thread access.

3. Good Documentation

The PR description clearly explains the problem, solution, trade-offs, and long-term alternatives.

4. Async Handling Improvements

The TypeScript changes properly convert onUpdateReceived and related methods to async, ensuring proper error handling in the async flow.

---

🔍 Issues & Concerns

1. Critical: Resource Leak in reset_thread_local_incremental_connections ⚠️

Location: react_on_rails_pro/lib/react_on_rails_pro/request.rb:143-148

def reset_thread_local_incremental_connections
  # Close all thread-local incremental connections
  Thread.list.each do |thread|
    conn = thread[:react_on_rails_incremental_connection]
    conn&.close
    thread[:react_on_rails_incremental_connection] = nil
  end
end

Problems:

1. Thread.list is unreliable: It only returns threads that are currently alive. Dead threads with connections won't be cleaned up, causing connection leaks.

2. Race condition: Between checking Thread.list and accessing thread[:react_on_rails_incremental_connection], the thread could terminate or the value could change.

3. No error handling: If conn.close raises an exception, cleanup stops and remaining connections leak.

Recommended Fix:

def reset_thread_local_incremental_connections
  # Close all thread-local incremental connections
  Thread.list.each do |thread|
    begin
      conn = thread[:react_on_rails_incremental_connection]
      conn&.close
    rescue StandardError => e
      Rails.logger.warn("[ReactOnRailsPro] Error closing incremental connection: #{e.message}")
    ensure
      thread[:react_on_rails_incremental_connection] = nil
    end
  end
end

Better long-term solution: Track connections in a thread-safe registry:

@incremental_connections_mutex = Mutex.new
@incremental_connections = {}

def incremental_connection
  thread_id = Thread.current.object_id
  @incremental_connections_mutex.synchronize do
    @incremental_connections[thread_id] ||= create_incremental_connection
  end
end

def reset_thread_local_incremental_connections
  @incremental_connections_mutex.synchronize do
    @incremental_connections.each_value do |conn|
      conn.close rescue nil
    end
    @incremental_connections.clear
  end
end

---

2. Missing: Connection Cleanup on Thread Exit ⚠️

When a Puma worker thread exits (e.g., during graceful shutdown or thread pool resizing), its connection is never cleaned up because it's stored in Thread.current.

Recommended: Add a finalizer or at_exit hook:

def incremental_connection
  conn = Thread.current[:react_on_rails_incremental_connection]
  return conn if conn

  conn = create_incremental_connection
  Thread.current[:react_on_rails_incremental_connection] = conn
  
  # Ensure cleanup when thread exits
  Thread.current[:react_on_rails_incremental_cleanup] ||= proc do
    Thread.current[:react_on_rails_incremental_connection]&.close
  end
  
  conn
end

---

3. Incomplete Fix: @standard_connection Still Shared 🤔

Location: react_on_rails_pro/lib/react_on_rails_pro/request.rb:10-13

The fix only addresses incremental_connection, but connection (standard connection) is still a shared instance variable (@connection on line 86).

Questions:

• Does the standard connection have similar thread-safety requirements?
• If not, document WHY it's safe to share but incremental connections aren't

Recommendation: Add a comment explaining the difference:

# Standard connection is thread-safe and can be shared across threads
def connection
  @connection ||= create_connection
end

# Incremental connection requires thread-local storage due to HTTPX stream_bidi limitations
def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= create_incremental_connection
end

---

4. TypeScript: Silent Error Handling ⚠️

Location: packages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderRequest.ts:100

await executionContext.runInVM(chunk.updateChunk, bundlePath).catch((err: unknown) => {
  log.error({ msg: 'Error running incremental render chunk', err, chunk });
});

Issue: Errors are logged but swallowed. The caller has no way to know if chunk processing failed.

Question: Should chunk processing failures:

1. Abort the entire incremental render stream?
2. Send an error signal back to the client?
3. Continue processing (current behavior)?

Recommendation: Document the intended error handling strategy and consider propagating critical errors.

---

5. Testing Gap: No Concurrent Request Tests ❌

The PR description mentions:

• Test incremental rendering with multiple concurrent requests
• Verify no race conditions in multi-threaded environment

This is the MOST CRITICAL test for this fix! The entire purpose is to fix race conditions under concurrent load.

Recommendation: Add a test that:

1. Spawns multiple threads
2. Each thread makes incremental rendering requests concurrently
3. Verifies all requests succeed without errors
4. Verifies each thread got its own connection

Example test structure:

it 'handles concurrent incremental rendering requests safely' do
  threads = 10.times.map do
    Thread.new do
      # Make incremental rendering request
      # Verify it succeeds
    end
  end
  
  threads.each(&:join)
  # Verify no errors occurred
end

---

📋 Minor Issues

6. Logging Improvement

Add logging when creating thread-local connections to help debug connection pooling issues:

def incremental_connection
  Thread.current[:react_on_rails_incremental_connection] ||= begin
    Rails.logger.debug { "[ReactOnRailsPro] Creating incremental connection for thread #{Thread.current.object_id}" }
    create_incremental_connection
  end
end

7. TypeScript: Unnecessary Null Check

Location: packages/react-on-rails-pro-node-renderer/src/worker/vm.ts:368

if (log.level === 'debug' && result) {

Question: Can result be null/undefined here? If so, document why. If not, the check is unnecessary.

---

🎯 Recommendations Summary

Must Fix Before Merge:

1. ✅ Add error handling to reset_thread_local_incremental_connections
2. ✅ Add concurrent request tests to verify the fix works
3. ✅ Consider connection cleanup on thread exit

Should Fix:

4. Document why standard connection doesn't need thread-local storage
5. Clarify error handling strategy for chunk processing failures

Nice to Have:

6. Add debug logging for connection creation
7. Consider migrating to a connection registry pattern for better lifecycle management

---

📚 Additional Notes

Memory Impact

With Puma configured as 5 workers × 5 threads = 25 threads, you'll have 25 persistent connections. Monitor memory usage in production.

Alternative Approach

Consider lazy connection creation + aggressive cleanup:

• Create connection on first use per thread
• Close connection after N seconds of inactivity
• This reduces steady-state memory while maintaining thread-safety

Long-term Path

The PR mentions migrating to async-http. I recommend:

1. File an issue with HTTPX about thread-safety
2. Evaluate async-http as a replacement
3. Keep this as a temporary workaround with clear TODO comments

---

Great work identifying and addressing this critical issue! The core solution is sound, but please address the resource leak and testing gaps before merging.

🤖 Review generated with Claude Code