Repository navigation
Have Thruster send the parts of a file too - #362
Open
namespaceMarcello wants to merge 1 commit into
Open
namespaceMarcello wants to merge 1 commit into
namespaceMarcello wants to merge 1 commit into
Conversation
With X-Sendfile on, Thruster sends a file that is asked for whole. A request with a Range header is still answered by the app with that part, and Puma writes it. A browser asks for a video that way and, past what it reads ahead, takes it at the pace it plays, so a long video keeps one of the worker's threads for as long as it plays. Thruster still holds the browser's request, Range included. So when X-Sendfile is on, the app drops the header from a GET for a file on disk and answers with the whole file. Rack::Sendfile hands that to Thruster, and Thruster sends the part. Nothing changes unless config.action_dispatch.x_sendfile_header is set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Correctness depends on the stacked PR and external Thruster behavior while intentionally changing several HTTP range semantics.
0 open findings
What changed in this PR
Routes Active Storage ranged GETs through Thruster when X-Sendfile is enabled, preventing long-lived downloads from occupying Puma threads.
Changes:
- Removes
Rangefrom eligible disk-service GET requests. - Adds coverage for ranged GET and HEAD behavior.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
config/initializers/active_storage.rb |
Delegates ranged file serving to Thruster. |
test/controllers/ranged_downloads_test.rb |
Tests ranged responses with and without X-Sendfile. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This sits on top of #359 and does nothing without it: it only acts when
config.action_dispatch.x_sendfile_headeris set, and onmainit isn't.With #359, Thruster sends a file that is asked for whole. A request with a
Rangeheader is still answered by the app with that part, and Puma writes it from the request's thread. A<video>asks that way, and past what the browser reads ahead it takes the file at the pace it plays. So a long video keeps a Puma thread for as long as it plays, and as many viewers as the worker has threads keep every other request waiting (measured on #361).Thruster already has what it needs to send the part: it holds the browser's request,
Rangeincluded, and it sends a file with Go'shttp.ServeFile, which answers ranges. It only needs the app to answer with the whole file. So when X-Sendfile is on, abefore_actiononActiveStorage::DiskController#showdropsRangefrom aGET.Rack::Filesthen answers with the whole file,Rack::Sendfileturns that into theX-Sendfileheader, and Thruster sends the part.Measurements
From our machine: a laptop with an AMD Ryzen 9 7940HX, Docker. The production image built from this branch plus the one line of #359, against the image of #359 alone; both with the day-long disk URL of #361, so that a video can be watched to its end. Thruster 0.1.23, Puma 8.0.2, Rack 3.2.7, one CPU and so one Puma worker with five threads, storage on a Docker volume (ext4), plain HTTP/1.1. Headless Chrome 155 on other CPUs of the same machine plays the
<video>of a room; meanwhile a room page is asked for every second and given 4 seconds to answer. The video is 10 minutes, 296 MB, H.264 at 4 Mbit/s. One run of each; the runs on #359 are from earlier the same evening.Five browsers play the video to its end:
One browser, this PR:
HTTP_WRITE_TIMEOUT), Chrome asks for the rest at 321 s and Thruster sends it. 0.05 s of a Puma thread in all.seekedafter 25 and 33 ms and is moving again after 819 and 829 ms (on Have Thruster send whole files from disk again #359: 818 and 815 ms).What a
curlgets, for 31 shapes ofRangeon a 256 MB attachment, a PDF, an empty file and anindex.html: 21 answers have the same status and the same bytes as on #359, among them every request for a single range of the video with no other condition (0-,N-,N-M,-N), which is what a<video>sends, and the one with anIf-Rangethat matches the file's date, which Chrome sends after a seek. TheirContent-Range,Content-Length,Content-Type,Content-Disposition,Cache-ControlandLast-Modifiedare the same too, and they gainAccept-Ranges: bytes.What a client sees change
The other 10 answers are Go's now, not Rack's:
video/mp4,Cache-ControlCache-Control; the sameContent-Range: bytes */NContent-Type: video/mp4multipart/byterangeswith its boundaryIf-Rangethat doesn't match (another date, an ETag): 2If-Unmodified-Sincean older date,If-Match: 2Content-LengthRack::Deflater; 256 MB in 6.1 sindex.html./, which answers 404Limits
X_SENDFILE_ENABLED=falseeveryGETwith aRangegets a 200 with no bytes, where on Have Thruster send whole files from disk again #359 it still got its part. That is measured withcurl; a browser there is not, but a video could not start. The same should hold for Puma asked directly, which is read from the code and not run.index.htmlalready can't be downloaded whole with Have Thruster send whole files from disk again #359 (ReverseProxy read errorwhen servingindex.htmlfrom/publicthruster#57, measured in a comment on #359); with this PR a ranged request for it fails the same way. A fix is on Thruster'ssendfile-index-bugbranch, not merged. With Thruster'smainand that branch merged, built and mounted over the gem's binary in this PR's image, the threeindex.htmlrequests answer with the file again (200, 200, and 206 for the part), the two 412s above carryContent-Length: 0, and the other answers stay as they are here.HTTP_WRITE_TIMEOUT(300 s), as it already does for whole files with Have Thruster send whole files from disk again #359. A client that stopped reading for 40 s and then went on got all 268,435,456 bytes; on Have Thruster send whole files from disk again #359 it got 44,724,333 of them.bytes=0-1probe only the answer is, withcurl. No TLS and no HTTP/2. Client and server on one machine. Synthetic constant-bitrate video. One Puma worker, not three. One run of each case.Checks
test/controllers/ranged_downloads_test.rb: a rangedGETfor a disk URL answers 206 with the part when X-Sendfile is off, as today; 200 with the whole file when it is on; and aHEADstill answers 206. The second fails without the change (Expected response to be a <200: ok>, but was a <206: Partial Content>). Without theGETcheck the third fails, and without the check on the setting the first does.Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.