fix(cl2k): defer artwork uploads like posters, and report deferred failures - #491
Conversation
…ilures Only the poster path deferred its Drive upload; the three asset makers uploaded inline. Align them — but deferral is only safe once failures are reported, because a deferred upload has no response left to fail on. The caller is told "queued", and until now a later failure existed solely as a log line: the success check is `not written and not uploaded_folders`, and `written` is non-empty by definition whenever we defer, so it always returned success. A deferred failure now logs at error level and sends a failure notification through the same NotificationManager other modules use for async outcomes. That report has to sit ABOVE the `if not uploaded_folders: return` early exit — a total failure leaves that list empty, which is exactly the case worth telling the user about. The API messages match the poster's "— uploading to Drive" wording, and the frontend needs no change: all five save paths already share savedToast, which reads upload_pending. Unchanged by design: /retext and the season batch still upload inline. Inline is the safer trade-off (errors return to the caller); deferral exists only because rclone outruns the 30s request timeout.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughInteractive square-art, background-art, and logo-asset endpoints now defer Drive uploads through ChangesArtwork upload deferral
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ArtworkEndpoint
participant ArtworkGenerator
participant BackgroundTasks
participant NotificationManager
Client->>ArtworkEndpoint: request artwork generation
ArtworkEndpoint->>ArtworkGenerator: generate with deferred upload callback
ArtworkGenerator->>BackgroundTasks: schedule Drive upload
ArtworkEndpoint-->>Client: return upload-in-progress response
BackgroundTasks->>ArtworkGenerator: execute upload
ArtworkGenerator->>NotificationManager: report upload failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
|
@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: 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 `@backend/modules/cl2k_maker.py`:
- Around line 1018-1036: Update the deferred upload flow around load_config() so
configuration-load exceptions are captured as upload failures instead of
returning before reporting. Record the failure in upload_errors and route it
through the existing logger and NotificationManager failure path, while
preserving the queued-success response already returned by the API. Add a
regression test that schedules the task, makes load_config() raise, and verifies
error-level logging plus failure notification.
🪄 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: c8626656-5bef-45e6-8d45-9b7652ade424
📒 Files selected for processing (3)
backend/api/cl2k_maker.pybackend/modules/cl2k_maker.pytests/test_cl2k_upload_deferral.py
A deferred upload has no response left to fail on, so any path that returns without uploading reads to the user as success. Three such paths existed; the earlier commit fixed only the one at the bottom. - upload failures (already fixed) - the config reload failing — logged a warning and returned ABOVE the report - routing changing so nothing claims the type — an INFO log, while the response had promised "uploading to Drive" All three now go through one _report_deferred_failure(), which also takes the request-time full_config: when the reload is what failed, the notification cannot depend on that same reload succeeding. The second and third were found by sweeping the function for the shape after the first was reported, rather than waiting for review to surface them one at a time. Each has a regression test, and the config-reload one was confirmed to fail against the old silent return.
|
@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_upload_deferral.py`:
- Around line 178-193: Update
test_losing_the_route_before_the_task_runs_is_reported to assert the captured
errors list contains an entry with "FAILED", matching the error-level logging
check used by test_a_failed_config_reload_is_reported_not_swallowed. Keep the
existing notification assertions unchanged.
🪄 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: b984b76c-e8d6-48d5-93df-52577ef5308c
📒 Files selected for processing (2)
backend/modules/cl2k_maker.pytests/test_cl2k_upload_deferral.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/modules/cl2k_maker.py
The lost-route test captured logger.error but never asserted on it, so that path could regress to info or warning and still pass — it checked half the reporting contract. Sweeping the file for the same shape found a second one: the success test asserted no notification but not the absence of a failure log, so a regression could report a failure through the log alone and go unnoticed. Both directions are now pinned, and downgrading the error log to warning was confirmed to fail the suite.
1 participant
Summary
Only the poster path deferred its Drive upload; the three asset makers uploaded inline. This aligns all four — and fixes the reason alignment wasn't safe on its own: a deferred upload that failed was reported to the user as success.
Related issue
N/A — found while investigating the duplicate-upload report (#490).
Type of change
The silent failure
defer_upload=background_tasks.add_taskwas wired at exactly one call site, so:generate_for_item(poster)generate_square_artgenerate_background_artgenerate_logo_assetA deferred upload has no response left to fail on. The caller is told "queued", and a later failure existed only as a
logger.warning. The success check isif not written and not uploaded_folders, andwrittenis non-empty by definition whenever we defer — so a poster whose Drive upload failed outright still reported success.Aligning the paths without fixing that would have spread the silence to artwork, so both land together.
A deferred failure now logs at error level and sends a failure notification through the same
NotificationManagerother modules use for async outcomes.Placement matters here. The report has to sit above the
if not uploaded_folders: returnearly exit — a total failure leaves that list empty, which is precisely the case worth reporting. My first version sat below it and reported nothing; the test now pins that.What didn't need changing
The frontend. All five save paths already share
savedToast, which readsupload_pendingand shows "— uploading to Drive…". Artwork picks that up for free. The API success messages were aligned to the poster's wording for consistency of the response itself.Deliberately unchanged
/retextand the season batch still upload inline. Inline is the safer trade-off — errors return to the caller — and deferral exists only because rclone outruns the 30s request timeout on multi-folder poster saves. Worth aligning later if timeouts start biting there.Also unchanged: deferral still requires a local copy. A Drive-only save stages a temp file and deletes it in
finally, so a background task would find nothing to upload — those stay inline regardless of path, which the tests document.Testing
background_tasks, every generator acceptsdefer_upload, a deferred failure notifies and logs at error level, and a clean upload stays quiet.BackgroundTasksmakes after the response — and asserts nothing is reported before it runs.ruffclean; 228 cl2k tests pass. Branch isolation stays additions-only.Summary by CodeRabbit
New Features
Bug Fixes
Tests