fix(backend): replace hardcoded /tmp paths with Python tempfile module#775
fix(backend): replace hardcoded /tmp paths with Python tempfile module#775m4cd4r4 wants to merge 1 commit into
Conversation
spwoodcock
left a comment
There was a problem hiding this comment.
Cool! I've wanted to see this for ages - thank you 😁
Only comment so far, without a good review, is that the drone-flightplan package is actually in this repo too, under src/backend/packages/drone-flightplan, so could be updated too =)
|
This is really useful & appreciated, but I just realised it was made against It's significantly out of sync now - sorry! |
Refs hotosm#597. Every backend write to a hardcoded /tmp/... path is replaced with tempfile-managed temporary files and directories. This makes the code cross-platform (Windows + macOS) and prevents collisions between concurrent requests in k8s. Backend app changes: - GCP upload: NamedTemporaryFile + finally-unlink around the S3 put - Waypoint flightplan generation (terrain-follow + plain): one tempfile.mkdtemp per request, cleaned up synchronously on the non-download path, or via a FileResponse BackgroundTask after the stream completes on the download path - generate-kmz endpoint: same mkdtemp + BackgroundTask pattern - project_logic.process_waypoints_and_waylines + the terrain-follow branch in update_projects_flight_metrics: TemporaryDirectory context managers (cleanup on exceptions, no more manual rmtree) - jaxa/upload_dem worker: tempfile.mkdtemp + finally shutil.rmtree, replacing the hand-rolled per-project /tmp/tif_processing cleanup - reflight download endpoint: tempfile.mkdtemp + BackgroundTask flightplan_output.build_flightplan_download_response gains an optional cleanup BackgroundTask parameter so routes that write into a temp dir can hand the directory lifetime over to Starlette. This keeps download endpoints from having to choose between leaking the file and deleting it before FileResponse can read it. Vendored drone-flightplan package changes (per @spwoodcock review): - create_flightplan.py: wrap the terrain-follow branch in a tempfile.TemporaryDirectory so the elevation geojson is no longer written to /tmp/output_file_with_elevation.geojson (which collided between concurrent calls) - output/dji.py, output/litchi.py, output/mavlink.py, output/qgroundcontrol.py: replace '/tmp/...' parameter defaults with None and resolve to tempfile.gettempdir() at call time. Preserves caller behaviour (path can still be passed explicitly) while making defaults portable and predictable on Windows and macOS test runs. Verification: - python -m py_compile passes on all touched files - python -m ruff check passes with no findings - grep -rn /tmp src/backend/app/ + src/backend/packages/drone-flightplan/ returns only one comment line referencing the system temp root Upstream drone-flightplan package at hotosm/drone-flightplan will get the same treatment in a follow-up PR so the two stay in sync.
2857e59 to
513c59e
Compare
Contributor Signature RequiredThank you for your contribution! Before we can accept your pull request, you need to sign our Contribution Policy. Why do I need to do this?
How to signTo sign the agreement, please comment on this PR with: I have read the CONTRIBUTING.md document and I hereby sign and agree with the guidelines You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Thanks for the nudge on both, @spwoodcock - this is now rebased onto current I'll send a matching PR against hotosm/drone-flightplan so upstream and the vendored copy stay in sync. CI is running now. |
Fixes #597 (backend portion).
Summary
Every backend write to a hardcoded
/tmp/...path is replaced with temporary files and directories managed by thetempfilemodule. This stops the server's disk from filling up over time because temporary files are now cleaned up automatically (either via context managers,finallyblocks, or StarletteBackgroundTasks), and makes the code portable across Linux/macOS/Windows.Rebased onto current
dev(2026-05-12) and extended per @spwoodcock review to cover the vendoreddrone-flightplanpackage in this same PR.Changes - backend app
app/gcp/gcp_routes.py-save_gcp_filenow writes the upload viatempfile.NamedTemporaryFile(delete=False)and unlinks the file in afinallyblock once the S3 put returns.app/waypoints/waypoint_routes.py- bothgenerate_waypointandgenerate_wmpl_kmzroute flightplan generation through onetempfile.mkdtempdirectory per request. For the download path the temp dir's lifetime is handed toFileResponseas aBackgroundTask(shutil.rmtree) so the file can still stream before being removed. For the non-download path the temp dir is removed synchronously. Errors during generation wipe the temp dir immediately. Thegenerate_kmz_with_placemarksendpoint follows the same mkdtemp + BackgroundTask pattern.app/waypoints/flightplan_output.py-build_flightplan_download_responsegets an optionalcleanup: BackgroundTaskparameter that it passes through toFileResponse(background=...). This keeps the download endpoints from having to choose between "leak the file" and "delete it before FileResponse can read it".app/projects/project_logic.py- both the terrain-follow branch inupdate_projects_flight_metricsand theprocess_waypoints_and_waylineshelper now usetempfile.TemporaryDirectorycontext managers. Cleanup happens automatically, including on exceptions, replacing the hand-rolledos.makedirs+try/finally shutil.rmtree. Also removes the un-created/tmp/{uuid}/dem.tifpath thatupdate_projects_flight_metricswas passing toget_file_from_bucket.app/jaxa/upload_dem.py- the DEM download worker replacesPath(f"/tmp/tif_processing/{project_id}")withtempfile.mkdtemp(prefix=f"dtm-dem-{project_id}-")and afinally: shutil.rmtree(...). The previous hand-rolled glob-and-unlink cleanup is gone.app/projects/classification_routes.py- the reflight download endpoint now writes the KMZ inside atempfile.mkdtempdirectory and schedulesshutil.rmtreeas aBackgroundTaskinstead ofos.removeon the single file.Changes - vendored
drone-flightplanpackagePer @spwoodcock comment - the in-repo copy at
src/backend/packages/drone-flightplan/got the same treatment so the package is consistent with the calling code.drone_flightplan/create_flightplan.py- the terrain-follow branch wraps the elevation-geojson write in atempfile.TemporaryDirectory(prefix="dtm-flightplan-"), replacing the hardcoded"/tmp/output_file_with_elevation.geojson"that collided between concurrent calls.drone_flightplan/output/dji.py,drone_flightplan/output/litchi.py,drone_flightplan/output/mavlink.py,drone_flightplan/output/qgroundcontrol.py- each output writer'soutput_file_pathdefault changes from a hardcoded/tmp/...literal toNone, resolved at call time viatempfile.gettempdir(). Caller-supplied paths still work exactly as before.A matching PR against hotosm/drone-flightplan upstream will follow so the two stay in sync.
Verification
python -m py_compilepasses on all 11 touched filespython -m ruff checkpasses with no findings on all 11 touched filesgrep -rn "/tmp/" src/backend/app/ src/backend/packages/drone-flightplan/returns no matches other than a single comment line referencing the system temp root inupload_dem.pyTest plan
POST /waypoint/task/{task_id}/withdownload=true- file should be served and the temp dir should be gone from the server after the stream