Skip to content

Initial version for zip download rework#426

Merged
MarcelGeo merged 23 commits intodevelopfrom
rewrite_zip_download
Apr 28, 2025
Merged

Initial version for zip download rework#426
MarcelGeo merged 23 commits intodevelopfrom
rewrite_zip_download

Conversation

@varmar05
Copy link
Collaborator

@varmar05 varmar05 commented Apr 11, 2025

Rewrite download zip functionality so it creates a background task for zip construction first and only serves content when ready.

So far this is only prototype with some basic workflow

merginmaps-server   | [ACCESS] 172.18.0.1 GET /v1/project/download/mergin/test format=zip HTTP/1.1 202 50 http://localhost:8080/projects/mergin/test/tree "Mozilla/5.0 (X11; Linux x86_64; rv:137.0) Gecko/20100101 Firefox/137.0" 104532 <8>
celery-worker       | [2025-04-11 11:55:51,044: INFO/ForkPoolWorker-1] Task mergin.sync.tasks.create_project_version_zip[d9d90279-ae16-4afd-bd10-2d79cc719df6] succeeded in 0.13913617400248768s: None

TODO:

  • rename endpoint (make it private)
  • fix tests
  • remove unused download functions and libraries, clean up
  • frontend
  • optionally move locking from FS to redis [NOT PLANNED]
  • update nginx config, make sure buffering settings are correct [:question: ]
  • make sure, that cleanup of partial is ok - add to controller safe check if .partial is not update more than 30minutes.

Dashboard

  • introducing polling for download zip file in following way:

  • sending HEAD request to the server until 200 is not returned, if 200 is returned -> download file

  • added loading indicator and disable dowloading
    image

  • added maximum of 100 retries for download

🔨

  • do not download files to js blob, use traditional anchor click (sending path to filesaver.saveFile() method)

@coveralls
Copy link

coveralls commented Apr 17, 2025

Pull Request Test Coverage Report for Build 14704633098

Details

  • 119 of 131 (90.84%) changed or added relevant lines in 10 files are covered.
  • 40 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.6%) to 91.705%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server/mergin/sync/public_api_controller.py 1 2 50.0%
server/mergin/sync/tasks.py 24 29 82.76%
server/mergin/sync/private_api_controller.py 23 29 79.31%
Files with Coverage Reduction New Missed Lines %
server/mergin/sync/storages/disk.py 15 81.01%
server/mergin/sync/storages/storage.py 25 44.44%
Totals Coverage Status
Change from base Build 14619150676: -0.6%
Covered Lines: 6910
Relevant Lines: 7535

💛 - Coveralls

harminius and others added 2 commits April 25, 2025 11:22
- add test for 200
- increase max download size limit to 20GB
- functions description
- partial archive expiration in seconds
@varmar05
Copy link
Collaborator Author

Could you guys also double check we have disabled response buffers on proxy for this endpoint?

- update endpoint for  project download to new path
- there was trailing slash, so it was ignored before :(
@MarcelGeo
Copy link
Collaborator

MarcelGeo commented Apr 25, 2025

Could you guys also double check we have disabled response buffers on proxy for this endpoint?

@varmar05

I updated no buffer endpoint in nginx.conf. See 3f31b3f. There are 🏂 questions about that:

  • there was trailing slash on the end of directive, I think It was not working before (could we check our deployments?)
  • there was enabled / trying to enable to X-Accel-Buffering with true. By default it's "yes" - it's not causing bad UX with existing download solution?
  • I disabled buffering, but after testing on staging, we could introduce changes

@MarcelGeo
Copy link
Collaborator

@varmar05 introduced configurable buffering for x accel in download zip.

@MarcelGeo MarcelGeo merged commit ad0014f into develop Apr 28, 2025
4 checks passed
@MarcelGeo MarcelGeo deleted the rewrite_zip_download branch April 28, 2025 09:41
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.

4 participants