Add safety check to avoid breaking everything if the db query fails. #3939
Conversation
…That one is on us.
📝 WalkthroughWalkthroughMultiple job classes and a controller were updated to add or route logs to a new 'jobs' logging channel; a controller's batch update was wrapped in a DB transaction; logging configuration and AppServiceProvider SQL logging types/levels were adjusted. No public method signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Jobs/RecomputeAlbumSizeJob.php (1)
105-105: Migrate line 105 to use Log::channel('jobs') for consistency with all other logs in this job.All seven log statements in RecomputeAlbumSizeJob.php use Log::channel('jobs'), except line 105. The album-not-found warning should be routed to the jobs channel along with the other job-related logs in this class.
🧹 Nitpick comments (2)
app/Jobs/ImportImageJob.php (1)
24-24: Minor inconsistency in logging order compared to ProcessImageJob.The logging is functional, but there's a slight inconsistency: this job logs before updating the status to STARTED (line 71 before line 72), while ProcessImageJob updates status first then logs (lines 104-106). For consistency across job classes, consider logging after the status update.
🔎 Proposed reordering for consistency
- Log::channel('jobs')->info($this->history->job); $this->history->status = JobStatus::STARTED; $this->history->save(); + Log::channel('jobs')->info($this->history->job);Also applies to: 71-73
app/Jobs/RecomputeAlbumStatsJob.php (1)
142-142: LGTM! Consider structured logging for better observability.The logging change is correct. As an optional enhancement, consider using structured logging context instead of string concatenation:
🔎 Optional: Use structured logging context
-Log::channel('jobs')->debug("Computed covers for album {$album->id}: max_privilege=" . ($album->auto_cover_id_max_privilege ?? 'null') . ', least_privilege=' . ($album->auto_cover_id_least_privilege ?? 'null')); +Log::channel('jobs')->debug("Computed covers for album {$album->id}", [ + 'album_id' => $album->id, + 'max_privilege_cover' => $album->auto_cover_id_max_privilege, + 'least_privilege_cover' => $album->auto_cover_id_least_privilege, +]);This makes the log data more easily queryable in log aggregation systems.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/Jobs/ExtractColoursJob.phpapp/Jobs/ExtractZip.phpapp/Jobs/HasFailedTrait.phpapp/Jobs/ImportImageJob.phpapp/Jobs/ProcessImageJob.phpapp/Jobs/RecomputeAlbumSizeJob.phpapp/Jobs/RecomputeAlbumStatsJob.phpapp/Jobs/UploadSizeVariantToS3Job.phpapp/Jobs/WatermarkerJob.phpapp/Providers/AppServiceProvider.phpconfig/logging.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Jobs/ExtractZip.phpapp/Jobs/ProcessImageJob.phpapp/Jobs/ImportImageJob.phpapp/Jobs/ExtractColoursJob.phpapp/Providers/AppServiceProvider.phpapp/Jobs/RecomputeAlbumSizeJob.phpapp/Jobs/RecomputeAlbumStatsJob.phpapp/Jobs/HasFailedTrait.phpapp/Jobs/UploadSizeVariantToS3Job.phpconfig/logging.phpapp/Jobs/WatermarkerJob.php
🧠 Learnings (7)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3644
File: database/migrations/2025_08_24_122020_fix_timeline_config.php:25-29
Timestamp: 2025-08-24T14:33:37.747Z
Learning: In the Lychee codebase, ildyria prefers not to add null checks in migrations when they can guarantee that database keys exist, as indicated by their "Key exists, don't worry about it" response to null-safety suggestions in database/migrations/2025_08_24_122020_fix_timeline_config.php.
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Jobs/ExtractZip.phpapp/Jobs/ProcessImageJob.phpapp/Jobs/ImportImageJob.phpapp/Jobs/ExtractColoursJob.phpapp/Providers/AppServiceProvider.phpapp/Jobs/RecomputeAlbumSizeJob.phpapp/Jobs/RecomputeAlbumStatsJob.phpapp/Jobs/HasFailedTrait.phpapp/Jobs/UploadSizeVariantToS3Job.phpconfig/logging.phpapp/Jobs/WatermarkerJob.php
📚 Learning: 2025-08-21T09:08:59.512Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: app/Console/Commands/Sync.php:212-218
Timestamp: 2025-08-21T09:08:59.512Z
Learning: ProcessImageJob is used by the Web API for single photo uploads and only needs photo title renaming (shall_rename_photo_title). ImportImageJob is used by the CLI for bulk imports and receives the full ImportMode with both photo and album renaming flags from the sync command.
Applied to files:
app/Jobs/ImportImageJob.php
📚 Learning: 2026-01-03T12:36:02.514Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3922
File: app/Listeners/RecomputeAlbumSizeOnPhotoMutation.php:63-69
Timestamp: 2026-01-03T12:36:02.514Z
Learning: In Lychee, "deleting a photo from an album" (removing the photo-album association) is distinct from "deleting a photo entirely". When PhotoDeleted event is dispatched with a specific album_id, it indicates the photo was removed from that particular album only. The photo may still exist in other albums due to the many-to-many relationship, so only the specified album's size statistics need recomputation.
Applied to files:
app/Jobs/RecomputeAlbumSizeJob.php
📚 Learning: 2025-08-23T07:58:55.186Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3642
File: app/Jobs/WatermarkerJob.php:56-65
Timestamp: 2025-08-23T07:58:55.186Z
Learning: In Laravel job classes, exception handling should be done in the failed() method rather than catching exceptions in the handle() method. Laravel automatically calls failed() when handle() throws an exception, following the framework's standard job failure handling pattern.
Applied to files:
app/Jobs/RecomputeAlbumSizeJob.phpapp/Jobs/RecomputeAlbumStatsJob.phpapp/Jobs/HasFailedTrait.phpapp/Jobs/UploadSizeVariantToS3Job.phpapp/Jobs/WatermarkerJob.php
📚 Learning: 2025-08-23T07:59:25.643Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3642
File: app/Jobs/WatermarkerJob.php:67-77
Timestamp: 2025-08-23T07:59:25.643Z
Learning: The WatermarkerJob should not include any retry logic - failed jobs should not call $this->release() and should only log failures and update the JobHistory status to FAILURE.
Applied to files:
app/Jobs/UploadSizeVariantToS3Job.phpapp/Jobs/WatermarkerJob.php
📚 Learning: 2025-08-22T18:56:08.725Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3642
File: resources/js/components/gallery/albumModule/AlbumHero.vue:173-183
Timestamp: 2025-08-22T18:56:08.725Z
Learning: In the needSizeVariantsWatermark function in resources/js/components/gallery/albumModule/AlbumHero.vue, the original variant is intentionally excluded from the watermark check. This is a deliberate design decision to not consider original files when determining if the watermark button should be shown.
Applied to files:
app/Jobs/WatermarkerJob.php
🧬 Code graph analysis (1)
app/Jobs/HasFailedTrait.php (1)
app/DTO/DiagnosticData.php (1)
error(40-43)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
🔇 Additional comments (17)
app/Jobs/UploadSizeVariantToS3Job.php (1)
75-76: LGTM! Job error logging correctly routed to dedicated channel.The migration to
Log::channel('jobs')appropriately segregates job-related errors from general application logs, improving observability and making it easier to debug queue issues.app/Jobs/HasFailedTrait.php (1)
31-31: LGTM! Trait correctly routes job failures to dedicated channel.Since HasFailedTrait is shared across multiple job classes, this change consistently applies the 'jobs' logging channel to all jobs using this trait.
config/logging.php (1)
136-151: LGTM! Job logging channels properly configured.The new logging infrastructure correctly establishes:
- A dedicated
jobs-dailychannel for detailed job tracing at debug level- A
jobsstack channel that combines job logs with error/warning channels for comprehensive monitoringThis aligns with the PR objective to segregate job logs from general application logs.
app/Providers/AppServiceProvider.php (2)
35-35: LGTM! Type safety and context improvements for SQL logging.The addition of explicit
Connectiontyping and usinggetDriverName()for context improves type safety and provides more meaningful logging information.Also applies to: 261-261, 293-293
330-330: Verify the log level change from debug to warning is intentional.The SQL query logging level was changed from
debugtowarningfor slow queries. This behavioral change seems unrelated to the PR's stated objective of adding job logging channels and could impact monitoring/alerting if warning-level logs trigger notifications.Please confirm this change is intentional and aligned with your logging strategy, especially since:
- Slow queries (>100ms by default) will now generate warning-level logs instead of debug-level
- This could increase the volume of warning logs significantly
- The PR description focuses on job logging, not SQL logging level adjustments
Also applies to: 354-354
app/Jobs/WatermarkerJob.php (1)
104-104: LGTM! Watermarker job failures correctly routed to dedicated channel.The migration to
Log::channel('jobs')is consistent with the broader job logging standardization, while preserving all existing context information (variant_id, path, code, exception).app/Jobs/ProcessImageJob.php (1)
31-31: LGTM! Logging enhancement improves observability.The Log facade import and start-of-job logging to the dedicated 'jobs' channel are well-placed and consistent with the PR-wide pattern of centralizing job logs.
Also applies to: 106-106
app/Jobs/ExtractZip.php (1)
80-80: LGTM! Job logging successfully migrated to dedicated channel.The addition of a start-of-job log and migration of error/critical logs to the 'jobs' channel improves observability without altering error handling behavior. The log levels remain appropriate for their respective scenarios.
Also applies to: 134-134, 196-196
app/Jobs/ExtractColoursJob.php (1)
26-26: LGTM! Logging enhancement is functional.The Log facade import and start-of-job logging are correctly implemented. Note: This follows the same before-status-update pattern as ImportImageJob and ExtractZip, which differs from ProcessImageJob's after-status-update approach. Consider standardizing the order across all job classes for consistency.
Also applies to: 66-68
app/Jobs/RecomputeAlbumSizeJob.php (1)
85-85: Job logging successfully migrated to dedicated channel.All job-related logs now route through the 'jobs' channel with appropriate levels. The level adjustment at line 85 (info → debug) reduces noise for skipped jobs, which is a good observability improvement.
Also applies to: 98-98, 120-120, 124-124, 128-128, 191-191
app/Jobs/RecomputeAlbumStatsJob.php (7)
90-90: LGTM! Logging now routes to dedicated jobs channel.The change to use
channel('jobs')->debugis appropriate for job-specific internal coordination messages and aligns with the PR's goal to separate job logs from normal application logs.
103-103: LGTM! Job start logging correctly routed.
118-118: LGTM! Album not found logging correctly routed.
147-147: LGTM! Propagation logging correctly routed.
150-154: LGTM! Exception handling follows Laravel job pattern.The catch-log-rethrow pattern correctly adds context while still allowing Laravel's job failure mechanism to invoke the
failed()method (line 360). This aligns with the framework's standard job failure handling.Based on learnings, Laravel job exception handling should primarily occur in the
failed()method, but this catch-log-rethrow approach appropriately adds contextual logging before propagation stops.
360-364: LGTM! Job failure logging correctly implemented.The
failed()method correctly handles permanent job failures after all retries are exhausted, which aligns with Laravel's job failure handling pattern. The comment appropriately documents that propagation stops on failure.Based on learnings, this is the correct location for job failure handling in Laravel.
106-112: This safety check is semantically justified, but clarify its purpose in the comment.The check prevents computing the "max privilege cover" (what an admin can see) when no admin user exists. While
getPhotoIdForUserdoes handle null users safely via the nullable?Userparameter andapplySearchabilityFilteruses safe navigation ($user?->may_administrate), computing a "max privilege" cover when no privileged user exists produces a meaningless value that duplicates the least-privilege result. The critical log level indicates this is an abnormal system state (admin user should always exist post-installation) requiring investigation, not a code error. The early return avoids unnecessary work during system misconfiguration.Consider updating the comment to clarify:
// Safety check: computing "max privilege" cover is meaningless without an admin user. This indicates system misconfiguration.
That one is on us.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.