fix: stop noisy false-error logs during full sync - #56
Merged
Conversation
deleteAssetsNotIn deferred its temp-table DROP until after tx.Commit(), so the drop always ran on an already-closed transaction and logged "[DB] Failed to drop temp table: sql: transaction has already been committed or rolled back" on every successful full sync. Move the DROP inside the transaction, before Commit; error paths still roll back via defer. doUserFullSync logged "Library sync failed during full sync" for the expected 401/403 a non-admin key gets on /api/libraries, duplicating the accurate warning syncLibraries already emits. Suppress that extra line for the expected auth case; genuine (non-HTTP) failures still log.
There was a problem hiding this comment.
Pull request overview
This PR reduces misleading/noisy log output during full syncs by (1) ensuring temp-table cleanup happens while the SQL transaction is still valid, and (2) avoiding a duplicate “library sync failed” log line for expected 401/403 outcomes when using a non-admin Immich API key.
Changes:
- Suppress the full-sync “Library sync failed…” log when
syncLibrariesreturns anImmichHTTPErrorwith 401/403 (sincesyncLibrariesalready logs the expected condition). - Move
DROP TABLE tmpKeepAssetsinto the transaction indeleteAssetsNotInto prevent running it afterCommit()on a closed transaction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/syncService.go | Avoid duplicate failure-sounding logs for expected non-admin library-access responses during full sync. |
| backend/database.go | Execute temp-table drop before committing the transaction to prevent post-commit “tx already committed/rolled back” errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deleteAssetsNotIn returned an error on DROP failure, diverging from deleteAlbumsNotIn/deleteTagsNotIn which drop inline before Commit and ignore the result. Aborting the tx there could turn a successful asset cleanup into a caller error. Match the established best-effort pattern; the drop still runs inside the transaction, so the original post-Commit failure is still fixed.
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.
Why
Full syncs emit two alarming log lines that are actually harmless, which keeps confusing users running a non-admin Immich API key (a supported config). This is the "ambiguous messages" complaint from #45.
Changes
1.
deleteAssetsNotIn— drop the temp table inside the transactionThe
DROP TABLE IF EXISTS tmpKeepAssetswas deferred, so it ran aftertx.Commit()— always against an already-closed transaction. Every successful full sync logged:Moved the
DROPinside the transaction, right beforeCommit. Error paths still unwind everything viadefer tx.Rollback()(theCREATE TEMP TABLEis transactional), so no cleanup leaks.2.
doUserFullSync— don't re-log the expected non-admin 401/403For a non-admin key,
GET /api/librariesis admin-only and returns 403.syncLibrariesalready logs that accurately (Failed to fetch libraries ... (may require admin key)), but the full-sync caller logged a second, failure-sounding line:Now suppressed when the error is an
ImmichHTTPErrorwith status 401/403. Genuine failures (network, 5xx, …) still log.deleteSyncState("libraryIDBackfillDone")behavior is unchanged.Result
For a non-admin key, a full sync no longer prints the temp-table error nor the duplicate "Library sync failed" line — only the accurate informational
may require admin keywarning remains.Testing
go build ./...,go vet ./...,go test ./...— all green.libraryIDBackfillDonestays unset; the temp table via the full-sync tests). No behavior change, so no new tests.Refs #45