fix: recover from a file_hash race during upload instead of poisoning the session - #445
Conversation
… the session Signed-off-by: Payalrvs0310@gmail.com <Payalrvs0310@gmail.com>
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe upload endpoint now handles duplicate file-hash insert conflicts by rolling back, retrieving the existing media record, and returning a duplicate result. Tests simulate the race and verify that later uploads continue using the same database session. ChangesUpload race recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Context Summary
Suggested issue links
Use |
ApprovabilityVerdict: Needs human review This bug fix modifies error handling behavior in the upload flow and both changed files are owned by Abhash-Chakraborty, not the PR author. The CODEOWNERS designation indicates the owner should review these changes. You can customize Macroscope's approvability policy. Learn more. |
payalrvs3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/find_api/routers/upload.py`:
- Around line 298-307: Move the Media insert, IntegrityError rollback,
existing-by-file_hash lookup, and duplicate response data out of the upload
router’s _ingest_image flow into the media repository/storage layer. Expose a
repository method that returns the created or existing Media record and
preserves duplicate handling, then have _ingest_image coordinate the request
using that method without direct conflict-recovery logic.
- Around line 298-307: The IntegrityError handler around db.commit must
distinguish a Media.file_hash uniqueness conflict from other constraint
violations before returning the duplicate response. After rollback, inspect the
database error/constraint metadata and only query Media and return status
"duplicate" for the file_hash conflict; re-raise every other IntegrityError, and
add a regression test covering a different constraint failure while the hash
lookup is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d111c8a9-a09f-4dc6-9138-19563969eda4
📒 Files selected for processing (2)
backend/src/find_api/routers/upload.pybackend/tests/test_upload.py
…plicate Signed-off-by: Payalrvs0310@gmail.com <Payalrvs0310@gmail.com>
…hash race The IntegrityError recovery handles the reachable trigger, but the underlying fault is broader: both upload endpoints loop over several files on one shared session, and *any* failed commit leaves that session inactive. The next file in the batch then dies with PendingRollbackError regardless of what went wrong -- an unexpected constraint violation, a dropped connection, a full disk. Both generic handlers now roll back before recording the failure, so a bad file costs one result row instead of every row after it. Already-committed files are untouched; rollback only discards work that never landed. `/api/upload` needed this as much as `/api/upload/bulk` -- it also accepts a list of files on one session, which is easy to miss because the name reads singular. The new test drives a non-file_hash commit failure through the bulk endpoint and asserts the *next* file still uploads. Without the rollback it reports `second.png` as failed.
Abhash-Chakraborty
added
gssoc26
|
Addressing the two CodeRabbit findings on this PR. 1. "Classify the violated constraint before returning The handler does exactly what the finding asks: except IntegrityError as exc:
db.rollback()
if "file_hash" not in str(exc.orig):
raise
existing = db.query(Media).filter(Media.file_hash == file_hash).first()
if existing is None:
raiseIt rolls back first, re-raises anything that is not a I checked the string match holds on both dialects rather than assuming: SQLite emits 2. "Move media insert conflict recovery out of AGENTS.md does say to keep routers thin, so the suggestion is not wrong. But Worth its own issue. Recording it rather than silently dropping it. |
Abhash-Chakraborty
left a comment
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved, with one commit pushed.
The core fix is right, and the tests are better than most. Seeding the winning row and forcing a real SQLite unique-constraint violation — rather than mocking the exception — is what makes this convincing, and the second test correctly proves an unrelated IntegrityError is re-raised instead of being laundered into a duplicate.
What I added: the issue's expected behaviour is that every other file in that batch uploads normally, and that only held for the file_hash path. The generic except Exception in both endpoints still had no rollback, so any other commit failure — an unexpected constraint, a dropped connection — still killed the rest of the batch with PendingRollbackError. That was the second half of the recommendation on #435.
Both handlers now roll back before recording the failure. /api/upload needed it as much as /api/upload/bulk: it also takes a list of files on one session, which is easy to miss because the name reads singular.
The new test drives a non-file_hash commit failure through the bulk endpoint and asserts the next file still uploads — it reports second.png as failed without the rollback.
Verified locally: ruff check/format --check clean, full backend suite 791 passed, 7 skipped.
CodeRabbit's two threads are addressed in the comment above — one was already satisfied by your code, the other is a real but out-of-scope refactor.
3ba0f63
into
Abhash-Chakraborty:canary
2 participants
Summary
_ingest_image()in the upload pipeline dedups byfile_hash, which is a unique database column, but the insert wasn't wrapped for that uniqueness constraint. When two requests upload identical file content at nearly the same time, the losing request's commit raises an unhandledIntegrityError. Nothing rolls back the shared per-request session afterward, so every file processed later in that same batch fails too - even ones completely unrelated to the original collision.Fixes #435
Type of change
Release impact
What changed
_ingest_image(backend/src/find_api/routers/upload.py) in a try/except forIntegrityError: on a race, roll back and return the existing row as a"duplicate"result - the same outcome as if the existing-file check had simply run a few milliseconds later.TestUploadRaceinbackend/tests/test_upload.py, which pre-seeds the "winning" row and fakes the existing-file check missing it once, forcing a real SQLite unique-constraint violation (not a mocked exception), then confirms the session still works for the next file in the same batch.Screenshots / recordings (for UI changes)
N/A - backend-only change.
How to test
cd backend uv run pytest tests/test_upload.py -v uv run pytest tests/ -vManual repro: from two overlapping requests, upload a file with byte-identical content in both at roughly the same time as part of a multi-file batch. Before the fix, every file after the collision in that batch fails too; after the fix, only the colliding file reports
"duplicate"and the rest upload normally.Checklist
canaryunless it is the maintainer promotion PRGSSoC'26 checklist
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests