Skip to content

Implement upload directory creation and lockfile handling #608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
MarcelGeo wants to merge 1 commit into
base: develop
Choose a base branch
from
+15 −5
Draft

Implement upload directory creation and lockfile handling #608

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions server/mergin/sync/public_api_v2_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,14 +295,23 @@ def create_project_version(id):
if request.json.get("check_only", False):
return NoContent, 204

upload_dir = None
try:
# while processing data, block other uploads
upload = Upload(project, version, upload_changes, current_user.id)
db.session.add(upload)
# Save path as local before any potential rollback expires the ORM instance
upload_dir = upload.upload_dir
# Create dir and lockfile BEFORE committing so concurrent is_active() checks
# always see this upload as active once its DB row is visible to other workers.
os.makedirs(upload_dir)
open(upload.lockfile, "w").close()
Comment on lines +303 to +308
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.makedirs(upload_dir) and creating the lockfile can raise OSError/PermissionError/FileExistsError, but this try only handles IntegrityError. If either filesystem call fails, the request will 500 without rolling back the DB session or cleaning up partially created artifacts. Consider catching OSError around the directory/lockfile creation, calling db.session.rollback(), and moving the directory to tmp (or removing it) before returning an UploadError response.

Copilot uses AI. Check for mistakes.
Comment on lines +305 to +308
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description says lockfile creation was moved to Upload instance creation, but the lockfile/directory creation is still implemented in the controller. Either update the PR description or move the responsibility into Upload (or a helper) to keep the change aligned with the stated intent.

Copilot uses AI. Check for mistakes.
# Creating blocking upload can fail, e.g. in case of racing condition
db.session.commit()
except IntegrityError:
db.session.rollback()
if upload_dir:
move_to_tmp(upload_dir)
# check and clean dangling blocking uploads or abort
for current_upload in project.uploads.all():
if current_upload.is_active():
Expand All @@ -321,16 +330,17 @@ def create_project_version(id):
# Try again after cleanup
upload = Upload(project, version, upload_changes, current_user.id)
db.session.add(upload)
upload_dir = upload.upload_dir
os.makedirs(upload_dir)
open(upload.lockfile, "w").close()
db.session.commit()
Comment on lines 332 to 336
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue in the retry path: filesystem creation (os.makedirs / lockfile write) is not protected, but only IntegrityError is caught. A filesystem failure here will also leave the session in a bad state and return a 500. Handle OSError (rollback + cleanup) consistently in this block too.

Copilot uses AI. Check for mistakes.
move_to_tmp(upload.upload_dir)
except IntegrityError as err:
db.session.rollback()
if upload_dir:
move_to_tmp(upload_dir)
logging.error(f"Failed to create upload session: {str(err)}")
return AnotherUploadRunning().response(409)

# Create transaction folder and lockfile
os.makedirs(upload.upload_dir)
open(upload.lockfile, "w").close()

file_changes, errors = upload.process_chunks(use_shared_chunk_dir=True)
# files consistency or geodiff related issues, project push would never succeed, whole upload is aborted
if errors:
Expand Down
Loading