Implement upload directory creation and lockfile handling#608
Implement upload directory creation and lockfile handling#608
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the v2 project push flow to create the upload transaction directory and lockfile earlier (before committing the Upload row) to reduce race conditions where concurrent workers may mis-detect an upload as inactive.
Changes:
- Create
upload_dirandlockfilebeforedb.session.commit()when starting an upload transaction. - Ensure filesystem artifacts are moved to tmp when an
IntegrityErroroccurs (including the retry path). - Add a local
upload_dirvariable to support cleanup after rollback/instance expiration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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() |
There was a problem hiding this comment.
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.
| db.session.add(upload) | ||
| upload_dir = upload.upload_dir | ||
| os.makedirs(upload_dir) | ||
| open(upload.lockfile, "w").close() | ||
| db.session.commit() |
There was a problem hiding this comment.
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.
| # 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() |
There was a problem hiding this comment.
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.
Move lockfile creation to Upload instance creation to prevent
is_activecollisions within multiple uploads.This could be replaced by upcoming race conditions and lockfile removing.