Fix: discard notifications for deleted files instead of logging warnings repeatedly - #389
Open
R0Wi wants to merge 3 commits into
Open
Fix: discard notifications for deleted files instead of logging warnings repeatedly#389R0Wi wants to merge 3 commits into
R0Wi wants to merge 3 commits into
Conversation
The notifications app re-renders every stored notification on each poll of /ocs/v2.php/apps/notifications/api/v2/notifications. If the file a workflow_ocr notification belongs to no longer exists for the user, the notifier logged a warning and fell back to a generic subject, leaving the row in oc_notifications. The warning was therefore emitted again on every single poll and never went away by itself. Throw AlreadyProcessedException instead so Nextcloud removes the obsolete notification, and log the missing file at debug level: a user deleting a file (or moving it to the trashbin, which is outside of /<user>/files and hence not resolvable via the user folder either) after OCR has run is a normal situation, not something worth a warning. Unexpected errors while resolving the file still get logged as an error and keep the previous fallback to the generic subject without a file link. Also switches from getById()/array_shift() to getFirstNodeById(). Fixes #382 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3aC9zhcgYNxtr2n4pZUaE
There was a problem hiding this comment.
Pull request overview
This PR addresses recurring log spam caused by stale workflow_ocr notifications that reference files which have since been deleted. By treating missing-file notifications as obsolete and signaling the Notifications app to discard them, it prevents repeated re-render attempts (and repeated warnings) on every notifications poll.
Changes:
- Make
Notifier::tryGetRichParamForFile()throwAlreadyProcessedExceptionwhen the referenced file can’t be found anymore, so the notification row is removed instead of re-rendered indefinitely. - Downgrade the missing-file log from
warningtodebugand include clearer context about discarding the obsolete notification. - Update unit tests to assert the new behavior (exception thrown + debug log, and no notification rendering work performed).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/Notification/Notifier.php | Throw AlreadyProcessedException for missing-file notifications and switch the missing-file log to debug to stop recurring warnings. |
| tests/Unit/Notification/NotifierTest.php | Adjust mocks and expectations to cover the new “discard obsolete notification” behavior and the updated file-lookup API. |
Comment on lines
+163
to
+165
| // Note:: AlreadyProcessedException has to be thrown before any call to $notification->set... | ||
| // otherwise notification won't be removed from the database | ||
| throw new AlreadyProcessedException(); |
Comment on lines
+148
to
+150
| /** @var File|null $file */ | ||
| $file = $userFolder->getFirstNodeById($fileId); | ||
| $relativePath = $file !== null ? $userFolder->getRelativePath($file->getPath()) : null; |
- Fix PHPDoc type for $file in tryGetRichParamForFile(): getFirstNodeById() returns Node|null, not File|null, so annotate and import accordingly. - Clarify the comment about ordering AlreadyProcessedException before any $notification->set... calls: that constraint applies to the caller (prepare()), not to this method where $notification isn't in scope.
Folder::getFirstNodeById() already declares a native ?Node return type, so the /** @var Node|null $file */ annotation added to satisfy the earlier PR review comment is redundant (psalm: UnnecessaryVarAnnotation) and can infer the type on its own. Drop the annotation and the now-unused Node import. Fixes CI failure: https://github.com/R0Wi-DEV/workflow_ocr/actions/runs/31161983739/job/92814185029
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
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
Fixes #382. When a user deletes a file after workflow_ocr has created a success or error notification for it, the notification's row in
oc_notificationspersists. Because the notifications app re-renders every stored notification on each poll of/ocs/v2.php/apps/notifications/api/v2/notifications, the notifier would log a warning every time—multiple times per minute—cluttering the Nextcloud logs indefinitely.Changes
tryGetRichParamForFile(): Now throwsAlreadyProcessedExceptionwhen the file cannot be found for the given user, instead of logging a warning and returning false. This signals Nextcloud to delete the notification row from the database, stopping the recurring logs.warningtodebugfor the missing-file case, since users deleting files (or moving them to trashbin) after OCR completes is a normal situation, not an error worth warning about.getById()+array_shift()withgetFirstNodeById()for cleaner code.testThrowsAlreadyProcessedExceptionIfFileCannotBeFoundAnymore, which verifies the notification is discarded and onlydebugis logged.Unexpected errors while resolving the file still produce an
errorlog and fall back to the generic subject without a file link.Testing
Unit tests pass (9/9 with 70 assertions) against a local harness.
composer cs:checkis clean. The test suite will run in CI.Generated by Claude Code