fix(upload): don't take a group sibling or a replaced server asset for a server copy of a photo - #1423
Open
gthb wants to merge 13 commits into
Open
fix(upload): don't take a group sibling or a replaced server asset for a server copy of a photo#1423gthb wants to merge 13 commits into
gthb wants to merge 13 commits into
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.
This was referenced Aug 18, 2026
…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.
This was referenced Aug 19, 2026
… flag replacement() looped while sa.Trashed, reading a field that replaceAsset writes under the index lock from other groups' goroutines, without the lock. An entry in replacedBy exists exactly for the assets replaceAsset marked trashed, so the guard added nothing: loop on the replacedBy lookup alone, as liveID does.
… assets handleGroup wrote the resolved ID back into each asset of the group. An asset that was uploaded and then replaced by a later sibling is in the index, read by other groups' goroutines, so that write was unsynchronized and left the index's entry with an ID other than its key. Resolve inside stackIDs instead, through a liveID function, and leave the assets alone. Also resolve replacements before the "already processed" check in ShouldUpload's checksum path: a file identical to an asset uploaded and then replaced in this run is now advised for the replacement, as any other duplicate of a replaced asset, instead of keeping the stale ID. The replacedBy comment no longer says "server asset": the replaced asset may be one uploaded earlier in the run.
gthb
changed the title
…lacement ShouldUpload resolves a checksum match to its replacement before the "already processed" check, so that a file identical to an asset uploaded earlier in the run, and since replaced by a bigger same-named copy, is advised for the replacement rather than given the deleted upload's ID. The existing test only covered a replaced server asset, which never reaches that check; this one covers the replaced same-run upload.
handleAsset passes the whole group, so la is among the siblings, as the "sa == la" check in the name lookup expects; the comment said "the other assets".
"The asset that stands for the asset with the given ID" described the functions through a metaphor; say directly that a replaced, deleted asset's ID resolves to its replacement's.
liveID and replacement were parallel loops over the same replacedBy map, one keyed on IDs and one on assets, and both liveID call sites in run.go had the asset in hand. Drop liveID and pass the index to stackIDs, which resolves each asset through replacement; one resolution function serves both the advice lookups and the stack assembly.
This branch has not been deployed
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
upload from-google-photoslosing one version of a Google-edited pair (X.jpg+X-edited.jpg, one sidecar). When the edited file goes first (random per run), the original is taken for a smaller/bigger copy of it: skipped, or uploaded as a replacement that deletes the edited version. Stack and tag requests then use the deleted ID and fail.Also fix the takeout's
X(1).jpgduplicate matching a server copy that the run just replaced with a biggerX.jpg: its stack request then fails too.Fixes #1285, fixes #877, fixes #1426. Refs #749, #884, #1384. Writeups and repros: #1422, #1426.
Cause
The adapter names
X-edited.jpgafter its sidecar's titleX.jpg, with the sidecar's date, so once uploaded and indexed,ShouldUploadtakes it for a server-side variant ofX.jpgand applies the heaviest-wins rule: sound for a stray copy, not for a group sibling, a different image by construction. And a replaced asset stays indexed under its checksum and name, so a later duplicate matches a deleted asset.Fix
ShouldUploadskips the asset's own group siblings in the name+date lookup.replaced ID → replacement; matches resolve to the replacement.handleGroupbuilds the stack from distinct, non-empty, replacement-resolved IDs (so ERR Can't create stack error=createStack, POST, http://localhost:2283/api/stacks, 400 Bad Request #749's""cover no longer counts toward the two-ID minimum), and records "stacked" only for assets in the stack request.AlreadyProcessedsets the asset's ID from the already-uploaded asset; it was left empty, so albums and stacks got"".Two e2e tests fail on the unfixed code whichever of a pair goes first;
Test_FromGooglePhotos_EditedPairhas eight pairs, making a lucky pass 1 in 256.Not covered
X.jpgand wins again. Skipping server assets matching a sibling's checksum would repair it but block from-google-photos: a takeout duplicate of a server copy that the run has just replaced gets the deleted asset's ID, and the group's stack fails #1426's legitimate replacement.Notes
Based on
main: the e2e tests need Immich v3 support, whichmainalone has. The diff applies todevelopwithout conflicts.