Serialize per-submission ops via a row lock#61
Open
norbusan wants to merge 1 commit into
Open
Conversation
Two browsers on the same submission could race uploads against an in-flight preflight / directives / compile, producing Frankenstein inputs or out-of-order results that clobbered the canonical output. `api.save()` already holds `SELECT ... FOR UPDATE` on the submission row for the duration of `event.execute()`, so events were covered. The gap was direct `file_store` mutations in review.py that bypass save(). This change: - Adds a `SubmissionLocked` domain exception (intentionally not a `SaveError` subclass, so existing `except SaveError` handlers don't swallow it into a 500). - Adds `SubmitApi.lock_submission(submission_id)` to acquire the same row lock from a separate session; implemented in LegacySubmitImplementation, no-op in NullImplementation, delegating in PubsubEventSubmitImplementation. - Translates MySQL `ER_LOCK_WAIT_TIMEOUT` (1205) to `SubmissionLocked` in both `lock_submission` and the existing `_load(..., lock_row=True)` path. - Registers a Flask error handler that returns HTTP 409 Conflict. - Wraps the two review.py mutation sites with `api.lock_submission(...)`; explicitly NOT around the subsequent `start_preflight(...)` (which would deadlock on the same row from a different session via api.save()). - Adds `SubmissionFileStore.rebuild_source_package(submission_id)` and calls it in `start_preflight` so tex2pdf-api receives a tar reflecting the live `src/`, not whatever `store_source_package` last wrote. The rebuild is byte-deterministic (sorted listing, gzip mtime=0, zeroed TarInfo fields). - Adds an AST-based guard test that flags any `store_*`/`delete_*` call on a file_store in `ui/controllers/**.py` not inside a `with api.lock_submission(...)` block. - Adds unit tests for errno-1205 translation, commit/rollback paths, Flask handler -> 409, and tar rebuild determinism.
bdc34
reviewed
May 22, 2026
| file_store.store_user_decisions(submission_id, new_decisions) | ||
| for path in files_to_delete: | ||
| file_store.delete_source_file(submission_id, path) | ||
| api = current_app.api |
Contributor
There was a problem hiding this comment.
It seems like we can make this an EventWithSideEffects and use the patter that already exists in that class.
bdc34
reviewed
May 22, 2026
| # subsequent start_preflight() goes through api.save(), which | ||
| # takes its own row lock from a separate session — re-entering | ||
| # the lock from here would deadlock on the same row. | ||
| with api.lock_submission(submission_id): |
Contributor
There was a problem hiding this comment.
This seems like it could be a EventWithSideEffects too.
bdc34
requested changes
May 22, 2026
bdc34
left a comment
bdc34
left a comment
Contributor
There was a problem hiding this comment.
I want to discuss if the changes to the submission and it's files in review_files should just be done in EventWithSideEffect.
I'd like to avoid adding a new locking paradigm to the app if it can be avoided.
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
2 participants
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.
Two browsers on the same submission could race uploads against an in-flight preflight / directives / compile, producing Frankenstein inputs or out-of-order results that clobbered the canonical output.
api.save()already holdsSELECT ... FOR UPDATEon the submission row for the duration ofevent.execute(), so events were covered. The gap was directfile_storemutations in review.py that bypass save(). This change:SubmissionLockeddomain exception (intentionally not aSaveErrorsubclass, so existingexcept SaveErrorhandlers don't swallow it into a 500).SubmitApi.lock_submission(submission_id)to acquire the same row lock from a separate session; implemented in LegacySubmitImplementation, no-op in NullImplementation, delegating in PubsubEventSubmitImplementation.ER_LOCK_WAIT_TIMEOUT(1205) toSubmissionLockedin bothlock_submissionand the existing_load(..., lock_row=True)path.api.lock_submission(...); explicitly NOT around the subsequentstart_preflight(...)(which would deadlock on the same row from a different session via api.save()).SubmissionFileStore.rebuild_source_package(submission_id)and calls it instart_preflightso tex2pdf-api receives a tar reflecting the livesrc/, not whateverstore_source_packagelast wrote. The rebuild is byte-deterministic (sorted listing, gzip mtime=0, zeroed TarInfo fields).store_*/delete_*call on a file_store inui/controllers/**.pynot inside awith api.lock_submission(...)block.