Handle RecordInvalid in concurrent asset upload scenario - #1023
Conversation
This arises when the first asset is committed before validation on the second takes place.
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes address duplicate upload races with appropriate test coverage.
Pull request overview
Handles concurrent duplicate Scratch asset uploads by rescuing filename uniqueness validation failures.
Changes:
- Reuses existing assets after duplicate upload races.
- Preserves unrelated validation errors.
- Adds coverage for concurrent uploads and error propagation.
File summaries
| File | Summary |
|---|---|
spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb |
Adds concurrency and validation error coverage. |
app/controllers/api/scratch/assets_controller.rb |
Handles duplicate filename RecordInvalid errors. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test coverage93.58% line coverage reported by SimpleCov. |
mwtrew
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Attachment creation remains vulnerable to a race after the duplicate record is recovered.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
This is done with a row-level lock on the asset row around the S3 upload.
|
Do you have any thoughts as to why this is happening - for example, is this caused by a project having the same asset twice? Or the same user saving a project multiple times in quick succession? Or something else? Just trying to understand in case this points to odd behaviour in the editor uploading the same asset multiple times. |
|
@zetter-rpf I didn't look into why that might be happening from the client side - thought it was worth fixing API-side regardless, but might be worth looking into. |
This arose when the first asset row is committed before validation on the second takes place.
Status
What's changed?