Add X-Accel-Buffering to disable buffers in proxies #5544
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the HTTP response headers in three methods across different files: Changes
Assessment against linked issues
The changes made in this pull request directly address the objectives outlined in the linked issue by adding a header that prevents buffering, which can enhance log visibility through reverse proxies. The inclusion of the Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supervisor/api/host.py (1)
261-261: LGTM! The X-Accel-Buffering header is correctly implemented.The header is added at the right location, before
prepare(request), and follows NGINX documentation for disabling response buffering. This will fix the issue with buffered log streaming through proxies.Consider adding a comment explaining the header's purpose:
+ # Disable proxy buffering to ensure logs are streamed in real-time response.headers["X-Accel-Buffering"] = "no"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/api/host.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests Python 3.12.8
| @@ -258,6 +258,7 @@ async def advanced_logs_handler( | |||
| if not headers_returned: | |||
| if cursor: | |||
| response.headers["X-First-Cursor"] = cursor | |||
| response.headers["X-Accel-Buffering"] = "no" | |||
There was a problem hiding this comment.
💡 Codebase verification
Add X-Accel-Buffering header to other streaming endpoints
The following streaming endpoints should include the same header to prevent buffering issues:
supervisor/api/proxy.py: Home Assistant EventStream endpointsupervisor/api/ingress.py: Ingress streaming response handlers
🔗 Analysis chain
Verify other streaming endpoints for similar buffering issues.
Let's check if there are other streaming endpoints that might benefit from this header.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other streaming endpoints that might need the X-Accel-Buffering header
# Look for StreamResponse usage which typically indicates streaming endpoints
echo "Searching for other streaming endpoints..."
rg -l "StreamResponse" | grep -v "host.py"
echo "Checking if any of these already use X-Accel-Buffering..."
rg -l "X-Accel-Buffering"
Length of output: 383
Script:
#!/bin/bash
# Check the implementation details of streaming endpoints
echo "Checking streaming implementation in proxy.py..."
rg -A 5 -B 5 "StreamResponse" supervisor/api/proxy.py
echo -e "\nChecking streaming implementation in ingress.py..."
rg -A 5 -B 5 "StreamResponse" supervisor/api/ingress.py
echo -e "\nChecking streaming implementation in utils.py..."
rg -A 5 -B 5 "StreamResponse" supervisor/api/utils.py
Length of output: 3001
Some proxies, notable NGINX, seem to buffer responses by default. For the long running log requests this is not helpful: The user won't see logs since the proxy is buffering the response. It seems that using http2 or explicit `proxy_buffering: no`` in the proxy configuration disables the buffering. However, better yet is to explicitly add the `X-Accel-Buffering: no`` header to the log responses.
Don't buffer ingress and API proxy responses as well.
agners
force-pushed
the
disable-buffering-for-advanced-logs
branch
from
0149118 to
5b840c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/api/ingress.py (1)
280-280: LGTM! The header addition looks good.The
X-Accel-Buffering: noheader is correctly placed before preparing the streaming response. The function already has proper logic to determine when streaming is needed based on content length and response type.Consider moving the header addition to the simple response case as well, as it might benefit from disabled buffering in some scenarios. Here's how:
if ( must_be_empty_body(request.method, result.status) or hdrs.CONTENT_LENGTH in result.headers and int(result.headers.get(hdrs.CONTENT_LENGTH, 0)) < 4_194_000 ): + headers["X-Accel-Buffering"] = "no" # Return Response body = await result.read() return web.Response( headers=headers, status=result.status, content_type=content_type, body=body, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supervisor/api/host.py(1 hunks)supervisor/api/ingress.py(1 hunks)supervisor/api/proxy.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supervisor/api/host.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.12.8
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (1)
supervisor/api/proxy.py (1)
98-98: LGTM! The header addition looks good.The
X-Accel-Buffering: noheader is correctly placed before preparing the response, which will effectively disable buffering for the EventStream responses.Let's verify that this is the only streaming response handler in the file that needs this header:
✅ Verification successful
Verification successful - buffering configuration is correctly implemented
The
X-Accel-Bufferingheader is present in the only HTTP streaming response handler. The other response handler uses WebSocket protocol which doesn't require this header.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other streaming response handlers rg -B 2 -A 2 "StreamResponse|prepare\(request\)" supervisor/api/proxy.pyLength of output: 691
Proposed change
Some proxies, notable NGINX, seem to buffer responses by default. For the long running log requests this is not helpful: The user won't see logs since the proxy is buffering the response.
It seems that using http2 or explicit
proxy_buffering: no`` in the proxy configuration disables the buffering. However, better yet is to explicitly add theX-Accel-Buffering: no`` header to the log responses.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit