fix(drive): resumable downloads + async folder-zip build - #359
Merged
Conversation
Large downloads were piped through a gunicorn worker, which caused two failures: - Folder/multi-file zips were streamed with the central directory written last. When a transfer outran the worker timeout the connection was cut mid-stream, leaving a truncated archive with no central directory that unzips as corrupt. The cut is time-bound, so different clients stopped at different sizes. - Single large files held a worker for the whole transfer with no Range support, so a dropped connection on a flaky link restarted from zero. Serve bytes from storage instead of the worker, with HTTP Range/resume: - S3: redirect to a short-lived presigned GET URL. - Local disk: nginx X-Accel-Redirect when `drive_xaccel_prefix` is set, otherwise send_file streams off disk with conditional Range support (no more BytesIO(fh.read()) buffering the whole file into memory). Folders now build in a background job to a temp artifact (disk or S3); the client polls a capability-token status and downloads the finished, Content-Length'd archive over the same resumable path. Archives live for an hour and are swept hourly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@greptileai can you review this PR? |
Large downloads were piped through a gunicorn worker, causing two failures: - Folder/multi-file zips streamed with the central directory written last; when a transfer outran the worker timeout the connection was cut mid-stream, leaving a truncated archive that unzips as corrupt. The cut is time-bound, so different clients stopped at different sizes. - Single large files held a worker for the whole transfer with no Range support, so a dropped connection on a flaky link restarted from zero. Serve bytes from storage with HTTP Range/resume instead of the worker: - S3: redirect to a short-lived presigned GET URL. - Disk: nginx X-Accel-Redirect when drive_xaccel_prefix is set, else send_file streams off disk with conditional Range support (Accept-Ranges advertised on the 200 so browsers resume rather than restart). Folders build in a background job to a temp artifact; the client polls a capability-token status and downloads the finished, Content-Length'd archive over the same resumable path. Archives expire in an hour and are swept hourly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d lifecycle - percent-encode the X-Accel-Redirect URI (raw UTF-8/special chars in a header value 500 under WSGI's latin-1 rule and confuse nginx) - replace secure_filename in Content-Disposition (S3 presigned + X-Accel) with RFC 6266/5987 encoding; secure_filename strips non-Latin names to nothing, regressing what send_file handled correctly - rate-limit download_folder and dedupe identical selections onto the in-flight/ready token: it's guest-reachable and each call cost an hour-long queue job plus a multi-GB artifact - give building cache entries a 3h TTL (1h expired mid-queue-wait and 404'd polls on still-running builds) and raise the client poll timeout past the job's 1h timeout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
frappe.enqueue(build_download_archive, ...) pickles the raw function
object into the job's kwargs. Unpickling it re-imports
suite.drive.api.files before frappe.init(site) runs inside execute_job,
so the module-level `frappe.qb.DocType("File")` in drive/utils/__init__
crashes with "object is not bound" — every build failed this way, and
the RQ failure callback hit the same import crash trying to log it, so
the cache entry never left "building" and the client polled forever.
Pass the dotted string instead, matching every other async
frappe.enqueue call in this codebase — get_attr() resolves it after
frappe.init(site) has already run.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Serve download bytes from storage (S3 presigned URL / nginx / disk) with HTTP Range support instead of streaming through a gunicorn worker.
Folders build in a background job to a temp artifact; the client polls a capability token and downloads the finished, Content-Length'd archive. Artifacts expire hourly. Local-disk deploys can opt into nginx
X-Accel-Redirectviadrive_xaccel_prefix; otherwisesend_filestreams off disk with Range support.🤖 Generated with Claude Code