Open
Conversation
This is a refactoring that replaces the filesystem-based Toucher/lockfile mechanism with a database-based heartbeat approach for tracking active uploads. This eliminates the NFS-specific hack (os.access() + os.utime()) that was needed to bust NFS attribute caches in the old approach. The new approach is cleaner and more cloud-native.
Replace the old try/except IntegrityError + cleanup loop pattern with an atomic upsert in Upload.create_upload(). Decouple the upload directory name from the DB primary key via transaction_id. Upload directory is created at this stage, no need to take care for it later. With the upsert logic, the ObjectDeletedError scenario which happened because a concurrent request could delete a stale upload row mid-operation is eliminated: - During push_finish, the heartbeat context manager continuously updates last_ping, keeping the upload fresh throughout the operation - A concurrent request can only take over an upload whose last_ping has expired - Since heartbeat prevents expiry, no other request can claim the row while push_finish is running - The upload object therefore stays valid for the full lifetime of the request — ObjectDeletedError becomes impossible
Coverage Report for CI Build 24565333868Coverage decreased (-0.2%) to 93.085%Details
Uncovered Changes
Coverage Regressions7 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces filesystem lockfile-based upload coordination with database-driven liveness tracking (upload.last_ping) and introduces a stable external upload identifier (upload.transaction_id), enabling concurrent upload handling via an atomic upsert/takeover strategy.
Changes:
- Add
last_pingandtransaction_idcolumns toupload(with backfills and constraints). - Rework upload session creation to use an upsert-based “stale takeover” approach and heartbeat updates instead of lockfiles.
- Update API/controllers/schemas/tests to use
transaction_idin routes and responses.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/migrations/community/e3a7f2b1c94d_add_upload_last_ping.py | Adds upload.last_ping and backfills to support DB-based liveness detection. |
| server/migrations/community/f1d9e4a7b823_add_upload_transaction_id.py | Adds upload.transaction_id, backfills from id, enforces NOT NULL + unique index for stable external transaction IDs. |
| server/mergin/sync/models.py | Implements Upload.create_upload() upsert/takeover strategy, upload_dir based on transaction_id, and heartbeat thread updating last_ping. |
| server/mergin/sync/public_api_controller.py | Switches v1 push flow to Upload.create_upload(), replaces lockfile toucher with upload.heartbeat(), and returns/uses transaction_id. |
| server/mergin/sync/public_api_v2_controller.py | Switches v2 create version flow to Upload.create_upload() and uses upload.heartbeat() during heavy work. |
| server/mergin/sync/permissions.py | Replaces get_upload with get_upload_or_fail() lookup by transaction_id. |
| server/mergin/sync/schemas.py | Updates serialized “uploads” lists to expose transaction_id rather than internal id. |
| server/mergin/sync/utils.py | Removes the old lockfile Toucher helper. |
| server/mergin/sync/public_api.yaml | Updates push_finish description to remove lockfile mention. |
| server/mergin/tests/test_project_controller.py | Updates tests to use transaction_id everywhere and adds coverage for stale upload takeover + cleanup. |
| server/mergin/tests/test_public_api_v2.py | Updates concurrency-related test setup to use Upload.create_upload(). |
| server/mergin/tests/test_db_hooks.py | Updates upload creation in tests to use Upload.create_upload(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MarcelGeo
requested changes
Apr 16, 2026
Collaborator
MarcelGeo
left a comment
MarcelGeo
left a comment
There was a problem hiding this comment.
We could try to run some concurrent push test If this will really help and do not move race conditions to different segments.
- guard against transaction folder errors - make last_ping tz naive - set transaction id to uuid type - remove redundant is_active method
… versions in DB If os.rename failed for moving uploaded data we would end up in broken project with project version in DB but no actual data.
MarcelGeo
approved these changes
Apr 24, 2026
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
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.
This PR contains two fixes / improvements