Conversation
Add an e2e test uploading a takeout with eight photos and their Google-edited versions (one sidecar per pair, as Google exports them): all sixteen must be uploaded and each pair stacked. It fails on the current code: the edited version is indexed under the sidecar's title, i.e. the original's name, with the original's capture date. When it happens to be processed first, the original is taken for a smaller or bigger server copy of it, and is either not uploaded at all or uploaded as a replacement that force-deletes the edited version. The order within a pair is random (map iteration in the adapter), hence eight pairs: all of them coming out right by chance is 1/256. The e2e utils gain GetAllAssetList (GetAllAssets keys by file name, and both assets of a pair are named alike) and GetAllStacks (v3's search/metadata does not report stack membership). Refs simulot#1285, simulot#877
ShouldUpload looks for a server asset with the same name and capture date as the local one, and advises to skip the upload (server has a bigger copy) or to replace the server asset (server has a smaller copy). Assets uploaded earlier in the run are part of that lookup. For a Google Photos edited pair, X.jpg and X-edited.jpg, the adapter gives both the sidecar's title X.jpg as OriginalFileName and the same capture date, and the two are grouped to be stacked. When X-edited.jpg is processed first, X.jpg then finds it as "an asset with the same name and date": if X.jpg is the smaller file it is not uploaded, if it is the bigger one it replaces X-edited.jpg, and replaceAsset force-deletes the edited version that was uploaded a moment ago. In the latter case the stack is then created with the deleted ID (400 or 500 from the server, depending on how far the deletion got) and the next tag batch that contains it fails as a whole. Pass the group to handleAsset and have ShouldUpload skip the asset's own siblings; they are about to be stacked together, so by construction they are not the same photo at another size. Same-name matches against pre-existing server assets, or against same-run uploads from other directories, are unchanged. Fixes simulot#1285, simulot#877
handleGroup put the cover's ID first unconditionally, even when empty, and never checked for duplicates. An empty ID happens when the cover was discarded or failed to upload; the same ID twice happens when two assets of the group are backed by the same server asset. Both end in a rejected POST /api/stacks (issue simulot#749 shows the "" case). Collect the IDs through stackIDs, which drops empty ones and repeats, and record the "stacked" event only when a stack is actually created, so the final report counts what happened. The AlreadyProcessed case (same checksum seen earlier in the run) now sets the asset's ID from the already-uploaded asset, as the other "already on the server" cases do; it was left empty, so albums and stacks were requested with "".
A blank line separated the comment from the func declaration, so it was not a doc comment; the siblings parameter added to it is only visible in godoc once the two are joined.
An asset of the group without a server ID (discarded, or failed to upload) is not part of the stack request; don't record a "stacked" event for it.
…tack Add an e2e test in two runs. The first uploads a small photo.jpg. The second uploads a takeout with a bigger photo.jpg of the same date, its edited version, and photo(1).jpg, Google's duplicate, byte-identical to the small copy on the server. It fails on the current code: the bigger photo.jpg replaces the server's copy, which is force-deleted, but photo(1).jpg is matched to that copy by checksum, either before the replacement (then its ID goes stale) or after (the index still lists the deleted asset under its checksum), and the group's stack is requested with the deleted ID: 400 from the server, no stack.
…ement replaceAsset marks the replaced server asset as trashed but leaves it in the index, under its checksum and its name. A later local file with the same content (Google's X(1).jpg duplicate of a photo whose bigger X.jpg just replaced the server's copy) or the same name and date was matched to it and got the ID of an asset that no longer exists, and the group's stack was requested with that ID and rejected. A local file matched to the server asset before the replacement kept the stale ID as well. Record the replacement in the index. ShouldUpload resolves a matched asset to its replacement before comparing, so a duplicate of the deleted copy is advised as "bigger copy on the server" for the replacement, and handleGroup resolves the group's IDs the same way before stacking.
Author
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
1 participant
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.
What
Fix
from-google-photoslosing the stack of a group that contains a takeout duplicate (X(1).jpg) of a server copy that the same run replaced with a biggerX.jpg. The duplicate was matched to the replaced, force-deleted asset and got its ID; the group'sPOST /api/stacksthen failed with400 Not found or no asset.update access(or left a stack whose primary asset is deleted, when the server had not finished deleting it yet). 17 such groups in a 26k-asset import, after #1423.Fixes #1426. Refs #1285.
Cause
replaceAssetmarks the replaced server asset as trashed but leaves it in the index under its checksum and its name. A later local file with the same content, or the same name and date, is matched to it and gets the ID of an asset that no longer exists. A local file matched to the server asset before the replacement keeps the stale ID as well, since nothing revisits it.Fix
immichIndex.replaceAssetrecordsreplaced ID → replacement.ShouldUploadresolves a matched asset to its replacement before comparing: a duplicate of the deleted copy is advised as "bigger copy on the server" for the replacement (so it is not uploaded, and takes the replacement's ID); a same-named, same-dated file is compared with the replacement.handleGroupresolves the group's IDs the same way before stacking, for files matched before the replacement happened; withstackIDs(fix(upload): don't take a group sibling or a replaced server asset for a server copy of a photo #1423) the duplicate then collapses onto the replacement in the stack request.An e2e test (
Test_FromGooglePhotos_ReplacedCopy, two runs: the small copy, then the takeout with the bigger copy, its edited version and the duplicate) fails before the fix and passes after; a unit test covers both lookup paths inShouldUpload.Not covered
An album addition queued for the duplicate before the replacement is flushed later with the stale ID, so that addition presumably fails for that asset (not observed; the replacement is added to the same albums by its own processing, so the album is not left empty).
Notes
Stacked on #1423 (branch
fix/edited-sibling-not-server-duplicate): it relies onstackIDsdeduplicating the group's IDs, and changes the same lines ofShouldUpload. Merge #1423 first; this PR then shows only its own two commits.