fix(cl2k): stop concurrent Drive uploads creating duplicate files - #490
Merged
Conversation
Saving the same artwork twice in quick succession left two files with identical names in the Drive folder instead of one. Google Drive permits duplicate names, and rclone decides create-vs-replace by listing the destination first — so two uploads overlapping in the same folder both see "absent" and both create. The upload is deferred past the HTTP response (rclone outruns the UI timeout), so two quick saves overlap easily. Sequential uploads were always fine, which is why it only happened sometimes. Serialise writes per Drive folder — both writers, since the poster healer renames into the same namespace the maker uploads into. After each upload, check whether the name just written now exists more than once and reap with `rclone dedupe --dedupe-mode newest` when it does, so the fresh copy survives. That also repairs duplicates the lock cannot prevent: ones already in the folder, or written by another instance. The check costs one listing and only runs a destructive command when a real duplicate exists. Verified against the real Drive: the same concurrent probe that produced two copies now produces one. The tests carry a guard that fails if the fake ever stops reproducing the bug, so the fix cannot be silently neutered.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds per-folder locking for Drive uploads and renames, validates rclone-bound inputs, and performs best-effort duplicate cleanup after uploads. New tests cover race prevention, folder parallelism, cleanup behavior, rename coordination, and validation. ChangesDrive upload consistency
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flags this call site as an uncontrolled command line (py/command-line- injection). subprocess is list-form, so there is no shell and no shell injection — the residual risk is argument injection: rclone reads a value starting with "-" as an option rather than data. folder_id and the service-account path already went through validators; local_path and the three OAuth values did not. Validate them with the module's existing _reject_unsafe, so the false-positive claim rests on the inputs actually being checked rather than on the call being list-form alone. Real Google credentials are unaffected — ids are numeric-led, secrets are GOCSPX-…, the token is JSON — and that is asserted by a test alongside the rejection cases. Verified against the live config and a real upload.
5 tasks
Owner
Author
|
@coderabbitai review |
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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/util/cl2k/gdrive_upload.py`:
- Around line 23-28: Shorten the comments in backend/util/cl2k/gdrive_upload.py
at lines 23-28, 109-112, 204-211, and 284-286 to 1–2 line navigational or
instructional notes: retain only the per-folder locking purpose and
multi-process limitation at 23-28, the option-shaped argument constraint at
109-112, the best-effort cleanup contract at 204-211, and that rename uses the
upload lock at 284-286; remove explanatory history and rationale while
preserving existing comment density.
- Around line 222-233: Update the rclone invocation in the upload flow around
subprocess.run to restrict deduplication to the target filename by adding the
appropriate name filter, or alternatively delete only that filename’s duplicate
object IDs. Add a fixture proving duplicates for a different filename remain
untouched, and require rclone version 1.61 or newer for this behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f80f25b-acbb-4ed9-a0c3-f5d35fd5443c
📒 Files selected for processing (2)
backend/util/cl2k/gdrive_upload.pytests/test_cl2k_gdrive_upload_race.py
The reap ran `rclone dedupe` across the whole folder, so uploading one file
could delete another file's duplicates — a destructive side effect outside the
operation's scope, triggered by an unrelated name. Filter it to the uploaded
filename (dedupe honours filters from rclone 1.61; the container runs 1.75).
The filter needs escaping: CL2K names carry `{tmdb-…}` and `{}` is alternation
in an rclone pattern, so an unescaped name matches the wrong thing or nothing.
Verified against the real Drive — staged duplicates of two names, reaped one,
and the other stayed at two copies. A unit test pins both the scope and the
escaping. Comment blocks trimmed to the repo's 1-2 line cap.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_cl2k_gdrive_upload_race.py`:
- Around line 226-240: The fake dedupe branch in fake_run must parse and apply
the command’s --include value instead of hard-coding "poster.png". Assert that
the filter exactly targets the uploaded filename, then remove only the matching
drive entry so the test fails for missing, incorrect, or broad dedupe filters
while preserving other.png duplicates.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6088ef56-04e5-4730-a227-0e750771415d
📒 Files selected for processing (2)
backend/util/cl2k/gdrive_upload.pytests/test_cl2k_gdrive_upload_race.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/util/cl2k/gdrive_upload.py
The fake removed a hard-coded name without reading --include, so a wrong or absent filter still left the other file's duplicates intact and the test passed. It proved nothing about the thing it existed to protect. The fake now parses --include, unescapes it, and collapses only what it matches, and the exact filter value is asserted. Both failure modes were confirmed to fail the test: dropping the filter raises on the missing flag, and pointing it at another file fails on '/other.png' == '/poster.png'.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Summary
Saving the same artwork twice in quick succession left two files with identical names in the Drive folder instead of overwriting. Reproduced, fixed, and verified against the real Drive.
Related issue
N/A — reported from a live save (a logo filed twice into the Logos drive).
Type of change
Root cause
Google Drive permits duplicate names in a folder — names are not unique keys. rclone decides create-vs-replace by listing the destination first, so two uploads overlapping in the same folder both see "absent" and both create.
_persist_posterdefers the upload past the HTTP response (rclone outruns the UI timeout), so two quick saves overlap easily. Sequential uploads were always correct, which is exactly why this only happened sometimes.Measured against the real Drive before the fix:
The fix
Serialise writes per Drive folder. Both writers take the lock, not just the one that showed the symptom: the poster healer's
move_filerenames into the same namespace the maker uploads into, and a rename onto a name an upload is creating duplicates just as readily.Reap after upload. Check whether the name just written now exists more than once, and collapse with
rclone dedupe --dedupe-mode newestwhen it does — so the copy just uploaded is the survivor. This repairs duplicates the lock cannot prevent: ones already in the folder, or written by another instance. It costs one listing per upload and only runs a destructive command when a real duplicate exists, so the normal path never deletes anything.A
threading.Lockis sufficient because CHUB runs a single process (docker topshows onemain.py). That assumption is stated at the lock — running multiple uvicorn workers would need a cross-process lock.Testing
upload_fileconcurrently against a fake that models the real failure shape: a listing, a window, then a create-or-replace.ruffclean; 344 tests pass across the cl2k / gdrive / heal scope. Branch isolation stays additions-only.Note on existing duplicates
The one duplicate already in the Logos drive was cleaned separately with
rclone dedupe --dedupe-mode newest, keeping the most recent upload. All four configured Drive targets now report zero duplicated names.Summary by CodeRabbit
Bug Fixes
Tests