From fa25c3fe522f36673b840851f39c61f80ffbd2c8 Mon Sep 17 00:00:00 2001 From: Major Date: Mon, 7 Sep 2026 12:09:59 +0200 Subject: [PATCH 1/2] fix(stacker): avoid duplicate stack deletions --- cmd/common.go | 17 +++ cmd/common_test.go | 19 +++ cmd/duplicates.go | 4 +- cmd/fixtrash.go | 4 +- cmd/stacker.go | 29 +++-- cmd/stacker_test.go | 224 +++++++++++++++++++++++++++++++++ pkg/immich/client.go | 79 +++++++++++- pkg/immich/client_test.go | 258 +++++++++++++++++++++++++++++++++++--- 8 files changed, 597 insertions(+), 37 deletions(-) diff --git a/cmd/common.go b/cmd/common.go index cb1256f..26d4dc7 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -26,6 +26,23 @@ func splitCommaList(raw string) []string { return utils.RemoveEmptyStrings(values) } +/************************************************************************************************** +** maskAPIKey renders an API key safe to log. Only the last four characters are kept, which is +** enough to tell several configured keys apart while keeping the secret out of the log file +** that LOG_FILE writes to disk. +** +** @param key - Raw API key +** @return string - Redacted form, e.g. "****cdef" +**************************************************************************************************/ +func maskAPIKey(key string) string { + const visible = 4 + runes := []rune(key) + if len(runes) <= visible { + return "****" + } + return "****" + string(runes[len(runes)-visible:]) +} + /************************************************************************************************** ** filterOutPartnerAssets removes assets not owned by the current user from a fetched list ** and logs how many were dropped. Partner-shared assets surfaced by /search/metadata cannot diff --git a/cmd/common_test.go b/cmd/common_test.go index 9e3109a..940bb83 100644 --- a/cmd/common_test.go +++ b/cmd/common_test.go @@ -106,3 +106,22 @@ func TestSplitCommaList(t *testing.T) { }) } } + +/************************************************************************************************** +** maskAPIKey keeps secrets out of the log file written by LOG_FILE. +**************************************************************************************************/ +func TestMaskAPIKey(t *testing.T) { + cases := map[string]string{ + "": "****", + "abc": "****", + "abcd": "****", + "abcde": "****bcde", + "super-secret-key": "****-key", + "clé-très-secrète": "****rète", + } + for input, expected := range cases { + if got := maskAPIKey(input); got != expected { + t.Errorf("maskAPIKey(%q) = %q, want %q", input, got, expected) + } + } +} diff --git a/cmd/duplicates.go b/cmd/duplicates.go index 7fade30..2bc4e56 100644 --- a/cmd/duplicates.go +++ b/cmd/duplicates.go @@ -41,12 +41,12 @@ func runDuplicates(cmd *cobra.Command, args []string) { } client := immich.NewClient(apiURL, key, false, false, true, withArchived, withDeleted, false, includeVideos, stackConcurrency, nil, "", "", logger) if client == nil { - logger.Errorf("Invalid client for API key: %s", key) + logger.Errorf("Invalid client for API key: %s", maskAPIKey(key)) continue } user, err := client.GetCurrentUser() if err != nil { - logger.Errorf("Failed to fetch user for API key: %s: %v", key, err) + logger.Errorf("Failed to fetch user for API key: %s: %v", maskAPIKey(key), err) continue } logger.Infof("=====================================================================================") diff --git a/cmd/fixtrash.go b/cmd/fixtrash.go index 75be4ef..8f27241 100644 --- a/cmd/fixtrash.go +++ b/cmd/fixtrash.go @@ -66,12 +66,12 @@ func runFixTrash(cmd *cobra.Command, args []string) { func fixTrashForAPIKey(key string, logger *logrus.Logger) { client := immich.NewClient(apiURL, key, false, false, dryRun, true, withDeleted, false, includeVideos, stackConcurrency, nil, "", "", logger) if client == nil { - logger.Errorf("Invalid client for API key: %s", key) + logger.Errorf("Invalid client for API key: %s", maskAPIKey(key)) return } user, err := client.GetCurrentUser() if err != nil { - logger.Errorf("Failed to fetch user for API key: %s: %v", key, err) + logger.Errorf("Failed to fetch user for API key: %s: %v", maskAPIKey(key), err) return } logger.Infof("=====================================================================================") diff --git a/cmd/stacker.go b/cmd/stacker.go index 10c944a..ca7fc73 100644 --- a/cmd/stacker.go +++ b/cmd/stacker.go @@ -128,16 +128,23 @@ func needsStackUpdate(originalStack, expectedStack []string) bool { ** Identifies any child assets that are already part of existing stacks. This is used to ** prevent conflicts when creating new stacks and to handle stack replacement scenarios. ** +** Immich indexes every member asset of a stack to that same stack, so several children can carry +** the same stack ID. The IDs are deduplicated here: one DELETE per distinct stack, since every +** repeat would be answered with 400 "Not found or no stack.delete access" (issue #80). +** ** @param stack - Array of assets to check -** @return []string - Array of stack IDs where conflicts were found +** @return []string - Array of distinct stack IDs where conflicts were found ** @return bool - True if any conflicts were found **************************************************************************************************/ func getChildrenWithStack(stack []utils.TAsset) ([]string, bool) { childrenWithStack := make([]string, 0) + seenStackIDs := make(map[string]bool) for _, asset := range stack[1:] { - if asset.Stack != nil { - childrenWithStack = append(childrenWithStack, asset.Stack.ID) + if asset.Stack == nil || seenStackIDs[asset.Stack.ID] { + continue } + seenStackIDs[asset.Stack.ID] = true + childrenWithStack = append(childrenWithStack, asset.Stack.ID) } return childrenWithStack, len(childrenWithStack) > 0 } @@ -182,12 +189,12 @@ func runStacker(cmd *cobra.Command, args []string) { } client := immich.NewClient(apiURL, key, resetStacks, replaceStacks, dryRun, withArchived, withDeleted, removeSingleAssetStacks, includeVideos, stackConcurrency, filterAlbumIDs, filterTakenAfter, filterTakenBefore, logger) if client == nil { - logger.Errorf("Invalid client for API key: %s", key) + logger.Errorf("Invalid client for API key: %s", maskAPIKey(key)) continue } user, err := client.GetCurrentUser() if err != nil { - logger.Errorf("Failed to fetch user for API key: %s: %v", key, err) + logger.Errorf("Failed to fetch user for API key: %s: %v", maskAPIKey(key), err) continue } logger.Infof("=====================================================================================") @@ -293,7 +300,7 @@ func processStack(client *immich.Client, logger *logrus.Logger, i int, total int if replaceStacks { for _, childID := range childrenWithStack { msg, err := client.DeleteStackCollect(childID, utils.REASON_REPLACE_CHILD_STACK_WITH_NEW_ONE) - if err != nil { + if err != nil || msg == "" { continue } deleteMsgs = append(deleteMsgs, msg) @@ -301,12 +308,14 @@ func processStack(client *immich.Client, logger *logrus.Logger, i int, total int } /********************************************************************************************** - ** Determine action type for logging. + ** Determine action type for logging. The replace wording is driven by deleteMsgs, not by + ** childrenWithStack: a child stack that was already gone, or whose delete failed, produces no + ** message, and the report must not claim a deletion that never happened. **********************************************************************************************/ var actionMsg string if len(originalStackIDs) == 0 { actionMsg = "\t🆕 Creating new stack" - } else if replaceStacks && len(childrenWithStack) > 0 { + } else if len(deleteMsgs) > 0 { actionMsg = "\t🔄 Replacing existing stack (deleted child stacks)" } else { actionMsg = "\t✏️ Updating stack configuration" @@ -384,12 +393,12 @@ func runCronLoopForAllUsers(apiKeys []string, apiURL string, logger *logrus.Logg } client := immich.NewClient(apiURL, key, resetStacks, replaceStacks, dryRun, withArchived, withDeleted, removeSingleAssetStacks, includeVideos, stackConcurrency, filterAlbumIDs, filterTakenAfter, filterTakenBefore, logger) if client == nil { - logger.Errorf("Invalid client for API key: %s", key) + logger.Errorf("Invalid client for API key: %s", maskAPIKey(key)) continue } user, err := client.GetCurrentUser() if err != nil { - logger.Errorf("Failed to fetch user for API key: %s: %v", key, err) + logger.Errorf("Failed to fetch user for API key: %s: %v", maskAPIKey(key), err) continue } logger.Infof("=====================================================================================") diff --git a/cmd/stacker_test.go b/cmd/stacker_test.go index 4334110..77fb840 100644 --- a/cmd/stacker_test.go +++ b/cmd/stacker_test.go @@ -7,14 +7,19 @@ package main import ( "bytes" + "net/http" + "net/http/httptest" "os" "strings" + "sync" "testing" + "github.com/majorfi/immich-stack/pkg/immich" "github.com/majorfi/immich-stack/pkg/stacker" "github.com/majorfi/immich-stack/pkg/utils" "github.com/sirupsen/logrus" "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" ) /************************************************************************************************** @@ -898,3 +903,222 @@ func TestBooleanEnvironmentOverrides(t *testing.T) { }) } } + +/************************************************************************************************** +** Test getChildrenWithStack deduplication. Immich indexes every member asset of a stack to that +** same stack, so several children of one candidate group can carry the same stack ID. Issuing one +** DELETE per child made every repeat fail with 400 "Not found or no stack.delete access". See +** issue #80. +**************************************************************************************************/ +func TestGetChildrenWithStack(t *testing.T) { + stackA := &utils.TStack{ID: "stack-a", PrimaryAssetID: "asset1"} + stackB := &utils.TStack{ID: "stack-b", PrimaryAssetID: "asset4"} + + tests := []struct { + name string + stack []utils.TAsset + expectedIDs []string + expectedHas bool + }{ + { + name: "Children sharing one stack yield a single ID", + stack: []utils.TAsset{ + {ID: "parent", Stack: nil}, + {ID: "child1", Stack: stackA}, + {ID: "child2", Stack: stackA}, + {ID: "child3", Stack: stackA}, + }, + expectedIDs: []string{"stack-a"}, + expectedHas: true, + }, + { + name: "Distinct stacks are all kept, in order", + stack: []utils.TAsset{ + {ID: "parent", Stack: nil}, + {ID: "child1", Stack: stackB}, + {ID: "child2", Stack: stackA}, + {ID: "child3", Stack: stackB}, + }, + expectedIDs: []string{"stack-b", "stack-a"}, + expectedHas: true, + }, + { + name: "Children without a stack are ignored", + stack: []utils.TAsset{ + {ID: "parent", Stack: stackA}, + {ID: "child1", Stack: nil}, + }, + expectedIDs: []string{}, + expectedHas: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + childrenWithStack, hasChildrenWithStack := getChildrenWithStack(tt.stack) + + if hasChildrenWithStack != tt.expectedHas { + t.Errorf("Expected hasChildrenWithStack %v, got %v", tt.expectedHas, hasChildrenWithStack) + } + if len(childrenWithStack) != len(tt.expectedIDs) { + t.Fatalf("Expected %d stack IDs %v, got %d %v", len(tt.expectedIDs), tt.expectedIDs, len(childrenWithStack), childrenWithStack) + } + for i, expected := range tt.expectedIDs { + if childrenWithStack[i] != expected { + t.Errorf("Expected childrenWithStack[%d] to be '%s', got '%s'", i, expected, childrenWithStack[i]) + } + } + }) + } +} + +/************************************************************************************************** +** immichStub records the write calls processStack makes and answers DELETE /stacks/{id} with a +** configurable status, so a test can drive the real client through a real HTTP round trip. +**************************************************************************************************/ +type immichStub struct { + deleteStatus int + mu sync.Mutex + calls []string +} + +func (s *immichStub) record(call string) { + s.mu.Lock() + s.calls = append(s.calls, call) + s.mu.Unlock() +} + +func (s *immichStub) callsSnapshot() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.calls...) +} + +func (s *immichStub) server() *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "/stacks/") { + s.record("DELETE " + r.URL.Path) + w.WriteHeader(s.deleteStatus) + w.Write([]byte(`{"message":"Not found or no stack.delete access"}`)) + return + } + if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/stacks") { + s.record("POST /stacks") + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{"id":"new-stack"}`)) + return + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{}`)) + })) +} + +/************************************************************************************************** +** newTestClient builds a client aimed at the stub. The replaceStacks argument is deliberately +** false: processStack reads the package-level flag, and the client never reads its own copy. +**************************************************************************************************/ +func newTestClient(t *testing.T, url string, logger *logrus.Logger) *immich.Client { + t.Helper() + client := immich.NewClient(url, "test-key", false, false, false, false, false, false, false, 1, nil, "", "", logger) + if client == nil { + t.Fatal("NewClient returned nil") + } + return client +} + +/************************************************************************************************** +** With REPLACE_STACKS=false, a group whose assets all carry a nil Stack is stacked: the early +** return only exists to protect assets that really are still in a stack. +**************************************************************************************************/ +func TestProcessStackStacksGroupWithNoStack(t *testing.T) { + defer teardownTest() + setupTest() + + logger := logrus.New() + logger.SetOutput(&bytes.Buffer{}) + + stub := &immichStub{} + server := stub.server() + defer server.Close() + + replaceStacks = false + client := newTestClient(t, server.URL, logger) + + processStack(client, logger, 0, 1, []utils.TAsset{ + {ID: "asset-b", OriginalFileName: "IMG_1.JPG", Stack: nil}, + {ID: "asset-a", OriginalFileName: "IMG_1.DNG", Stack: nil}, + }) + + assert.Equal(t, []string{"POST /stacks"}, stub.callsSnapshot()) +} + +/************************************************************************************************** +** The mirror case: with REPLACE_STACKS=false, a group whose child still carries a stack is left +** alone. +**************************************************************************************************/ +func TestProcessStackSkipsGroupWhoseChildIsStacked(t *testing.T) { + defer teardownTest() + setupTest() + + logger := logrus.New() + logger.SetOutput(&bytes.Buffer{}) + + stub := &immichStub{} + server := stub.server() + defer server.Close() + + replaceStacks = false + client := newTestClient(t, server.URL, logger) + + processStack(client, logger, 0, 1, []utils.TAsset{ + {ID: "asset-b", OriginalFileName: "IMG_1.JPG"}, + {ID: "asset-a", OriginalFileName: "IMG_1.DNG", Stack: &utils.TStack{ + ID: "stack-1", + PrimaryAssetID: "asset-a", + Assets: []utils.TAsset{{ID: "asset-a"}}, + }}, + }) + + assert.Empty(t, stub.callsSnapshot(), "an asset still in a stack must not be touched") +} + +/************************************************************************************************** +** When every child stack is already gone, no deletion happened, so the report must not claim +** "deleted child stacks". +**************************************************************************************************/ +func TestProcessStackReportDoesNotClaimUnperformedDeletes(t *testing.T) { + defer teardownTest() + setupTest() + + var logs bytes.Buffer + logger := logrus.New() + logger.SetOutput(&logs) + logger.SetLevel(logrus.InfoLevel) + + stub := &immichStub{deleteStatus: http.StatusBadRequest} + server := stub.server() + defer server.Close() + + replaceStacks = true + client := newTestClient(t, server.URL, logger) + + processStack(client, logger, 0, 1, []utils.TAsset{ + {ID: "asset-b", OriginalFileName: "IMG_1.JPG"}, + {ID: "asset-a", OriginalFileName: "IMG_1.DNG", Stack: &utils.TStack{ + ID: "gone-stack", + PrimaryAssetID: "asset-a", + Assets: []utils.TAsset{{ID: "asset-a"}, {ID: "asset-x"}}, + }}, + }) + + output := logs.String() + if strings.Contains(output, "Replacing existing stack") { + t.Errorf("report claims deleted child stacks when none were deleted:\n%s", output) + } + if !strings.Contains(output, "Updating stack configuration") { + t.Errorf("expected the update wording, got:\n%s", output) + } + if strings.Contains(output, "Deleted Stack") { + t.Errorf("report claims a deletion that never happened:\n%s", output) + } +} diff --git a/pkg/immich/client.go b/pkg/immich/client.go index 0fd54b7..d19a89f 100644 --- a/pkg/immich/client.go +++ b/pkg/immich/client.go @@ -9,6 +9,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "sync" "time" @@ -266,7 +267,9 @@ func (c *Client) FetchAllStacks() (map[string]utils.TStack, error) { // refactor moves the reset earlier. Capturing is defensive and makes intent explicit. shouldReset := c.resetStacks shouldRemoveSingle := c.removeSingleAssetStacks + deletedStackIDs := make(map[string]bool) { + var deletedMu sync.Mutex concurrency := max(c.stackConcurrency, 1) sem := make(chan struct{}, concurrency) var wg sync.WaitGroup @@ -288,9 +291,20 @@ func (c *Client) FetchAllStacks() (map[string]utils.TStack, error) { if reason == utils.REASON_RESET_STACK { c.logger.Debugf("🔄 Resetting stack %s", stack.PrimaryAssetID) } - if err := c.DeleteStack(stack.ID, reason); err != nil { - c.logger.Errorf("Error deleting stack: %v", err) + /********************************************************************************** + ** Only a delete this call actually performed may be recorded. A stack that + ** errored, or that the server refused with the ambiguous "not found or no + ** access" 400, may still be on the server; dropping it from stacksMap below + ** would make its assets look unstacked and stack them on a false premise. + ** DeleteStack already logged whatever happened. + **********************************************************************************/ + deleted, _ := c.DeleteStack(stack.ID, reason) + if !deleted { + return } + deletedMu.Lock() + deletedStackIDs[stack.ID] = true + deletedMu.Unlock() }(stack, reason) } wg.Wait() @@ -317,8 +331,17 @@ func (c *Client) FetchAllStacks() (map[string]utils.TStack, error) { } } + /********************************************************************************************** + ** Stacks deleted just above must not stay indexed: their assets would carry a pointer to a + ** stack that no longer exists and the replace path would try to delete it a second time + ** (issue #80). Deleted stacks are skipped in dry run too, so the run reports what a real + ** run would produce. + **********************************************************************************************/ stacksMap := make(map[string]utils.TStack) for _, stack := range stacks { + if deletedStackIDs[stack.ID] { + continue + } for _, asset := range stack.Assets { stacksMap[asset.ID] = stack } @@ -466,21 +489,30 @@ func (c *Client) FetchAssets(size int, stacksMap map[string]utils.TStack) ([]uti ** DeleteStack removes a stack from Immich. ** In dry run mode, it only logs the action without making changes. ** +** The deleted flag reports whether this call is what removed the stack. A stack the server +** refused to delete — already gone, or not ours — yields (false, nil): harmless, but the caller +** must not treat it as a deletion it performed. Dry run reports true, since it models the run +** that would have happened. +** ** @param stackID - ID of the stack to delete ** @param reason - Reason for deletion (for logging) +** @return bool - True when this call removed the stack ** @return error - Any error that occurred during deletion **************************************************************************************************/ -func (c *Client) DeleteStack(stackID string, reason string) error { +func (c *Client) DeleteStack(stackID string, reason string) (bool, error) { msg, err := c.DeleteStackCollect(stackID, reason) if err != nil { - return err + return false, err + } + if msg == "" { + return false, nil } if c.dryRun { c.logger.Warn(msg) } else { c.logger.Info(msg) } - return nil + return true, nil } /************************************************************************************************** @@ -489,6 +521,9 @@ func (c *Client) DeleteStack(stackID string, reason string) error { ** can be folded into a single contiguous per-stack log block instead of interleaving with the ** output of other in-flight stacks. Errors are still returned (and logged here) since they are ** exceptional and should surface immediately. +** +** An empty message with a nil error means the stack was already gone, so there is nothing to +** report. Callers must skip empty messages instead of logging them. **************************************************************************************************/ func (c *Client) DeleteStackCollect(stackID string, reason string) (string, error) { reasonMsg := "" @@ -501,6 +536,10 @@ func (c *Client) DeleteStackCollect(stackID string, reason string) (string, erro } if err := c.doRequest(http.MethodDelete, fmt.Sprintf("/stacks/%s", stackID), nil, nil); err != nil { + if isStackAlreadyGone(err) { + c.logger.Debugf("Stack %s already gone (or not owned), nothing to delete - %s", stackID, reason) + return "", nil + } c.logger.Errorf("Error deleting stack: %v", err) return "", fmt.Errorf("error deleting stack: %w", err) } @@ -895,6 +934,36 @@ func (c *Client) UpdateAlbum(albumID string, updates map[string]interface{}) err return nil } +/************************************************************************************************** +** isStackAlreadyGone returns true when a failed DELETE /stacks/{id} may be ignored because the +** stack is no longer there. The same stack ID legitimately reaches this call twice: Immich merges +** (and drops) stacks server-side on POST /stacks, and the stack snapshot taken at the start of a +** run is never refreshed. +** +** The 400 branch is deliberately ambiguous. Immich's access guard answers BOTH "this stack does +** not exist" and "this stack is not yours" with 400 "Not found or no stack.delete access", so +** matching that body cannot tell the two apart: an ownership denial is treated as already gone. +** That is acceptable because the stack IDs acted on come from the caller's own GET /stacks, and +** because the server refused the delete either way — callers must therefore treat a swallowed +** error as "nothing happened", never as "deleted". A key that lacks the stack.delete scope is +** NOT affected: Immich rejects it in the auth guard with 403 (ForbiddenException, "Missing +** required permission", server/src/services/auth.service.ts) before the access check that +** produces this 400, and 403 is not matched here. The bundled OpenAPI spec documents only the +** 204, so both status codes come from the Immich server source rather than from the spec. +** +** Any other 400 body still surfaces as an error. 404 is covered for future API versions. +**************************************************************************************************/ +func isStackAlreadyGone(err error) bool { + var apiErr *APIError + if !errors.As(err, &apiErr) { + return false + } + if apiErr.StatusCode == http.StatusNotFound { + return true + } + return apiErr.StatusCode == http.StatusBadRequest && strings.Contains(apiErr.Body, "Not found or no stack.delete access") +} + /************************************************************************************************** ** isLikelyLargeLibrary5xx returns true when an error from /stacks looks like a server-side ** 5xx response. On large libraries Immich's GET /stacks endpoint returns 500 because the diff --git a/pkg/immich/client_test.go b/pkg/immich/client_test.go index 17757ac..261faa5 100644 --- a/pkg/immich/client_test.go +++ b/pkg/immich/client_test.go @@ -1,6 +1,8 @@ package immich import ( + "bytes" + "fmt" "io" "net/http" "strings" @@ -980,17 +982,19 @@ func TestGetCurrentUser(t *testing.T) { } /************************************************************************************************ -** Tests for DeleteStack +** Tests for DeleteStackCollect ************************************************************************************************/ -func TestDeleteStack(t *testing.T) { +func TestDeleteStackCollect(t *testing.T) { tests := []struct { - name string - stackID string - reason string - dryRun bool - statusCode int - wantErr bool + name string + stackID string + reason string + dryRun bool + statusCode int + responseBody string + wantErr bool + wantEmptyMsg bool }{ { name: "successful delete", @@ -1009,12 +1013,32 @@ func TestDeleteStack(t *testing.T) { wantErr: false, }, { - name: "stack not found", - stackID: "nonexistent-stack", - reason: "should fail", - dryRun: false, - statusCode: http.StatusNotFound, - wantErr: true, + name: "stack not found is benign", + stackID: "nonexistent-stack", + reason: "already gone", + dryRun: false, + statusCode: http.StatusNotFound, + wantErr: false, + wantEmptyMsg: true, + }, + { + name: "immich access-guard 400 for a deleted stack is benign", + stackID: "already-deleted-stack", + reason: "already gone", + dryRun: false, + statusCode: http.StatusBadRequest, + responseBody: `{"message":"Not found or no stack.delete access"}`, + wantErr: false, + wantEmptyMsg: true, + }, + { + name: "other 400 still fails", + stackID: "stack-400", + reason: "should fail", + dryRun: false, + statusCode: http.StatusBadRequest, + responseBody: `{"message":"Invalid UUID"}`, + wantErr: true, }, { name: "server error", @@ -1039,6 +1063,10 @@ func TestDeleteStack(t *testing.T) { logger := logrus.New() logger.SetOutput(io.Discard) + responseBody := tt.responseBody + if responseBody == "" { + responseBody = `{}` + } client := &Client{ apiKey: "test", apiURL: "http://test/api", @@ -1048,19 +1076,38 @@ func TestDeleteStack(t *testing.T) { Transport: &mockTransport{ response: &http.Response{ StatusCode: tt.statusCode, - Body: io.NopCloser(strings.NewReader(`{}`)), + Body: io.NopCloser(strings.NewReader(responseBody)), }, }, }, } - err := client.DeleteStack(tt.stackID, tt.reason) + msg, err := client.DeleteStackCollect(tt.stackID, tt.reason) if tt.wantErr { assert.Error(t, err) } else { assert.NoError(t, err) } + if tt.wantEmptyMsg { + assert.Empty(t, msg, "an already-gone stack has nothing to report") + return + } + if tt.wantErr { + return + } + + /************************************************************************************** + ** A delete that happened must report it: the message is what the caller logs, and + ** what tells FetchAllStacks the stack really went away. + **************************************************************************************/ + assert.Contains(t, msg, tt.stackID) + assert.Contains(t, msg, tt.reason) + if tt.dryRun { + assert.Contains(t, msg, "(dry run)") + } else { + assert.NotContains(t, msg, "(dry run)") + } }) } } @@ -1441,7 +1488,7 @@ func TestFetchAllStacksResetStacks(t *testing.T) { expectNilMap: false, }, { - name: "remove single asset stacks - map still includes all fetched stacks", + name: "remove single asset stacks - deleted stacks are dropped from the map", resetStacks: false, removeSingleAssetStacks: true, dryRun: false, @@ -1449,7 +1496,7 @@ func TestFetchAllStacksResetStacks(t *testing.T) { {"id": "stack-single", "primaryAssetId": "asset-1", "assets": [{"id": "asset-1"}]}, {"id": "stack-multi", "primaryAssetId": "asset-2", "assets": [{"id": "asset-2"}, {"id": "asset-3"}]} ]`, - expectedMapSize: 3, + expectedMapSize: 2, expectNilMap: false, }, { @@ -2250,3 +2297,178 @@ func TestUpdateAlbum(t *testing.T) { }) } } + +/************************************************************************************************** +** singleAndPairStacksBody is the GET /stacks payload shared by the deletion tests below: one +** single-asset stack, which REMOVE_SINGLE_ASSET_STACKS deletes, and one two-asset stack, which +** it keeps. +**************************************************************************************************/ +const singleAndPairStacksBody = `[ + {"id":"stack-single","primaryAssetId":"asset-a","assets":[{"id":"asset-a"}]}, + {"id":"stack-pair","primaryAssetId":"asset-b","assets":[{"id":"asset-b"},{"id":"asset-c"}]} +]` + +/************************************************************************************************** +** stackDeleteHandler answers DELETE with deleteStatus and every other request with the stacks +** payload, for use as a pathRouterMockTransport handler. The recorded call list then pins both +** the single GET /stacks and the stack IDs that were deleted. +**************************************************************************************************/ +func stackDeleteHandler(deleteStatus int) func(req *http.Request) (int, string) { + return stackDeleteHandlerWithBody(deleteStatus, "") +} + +func stackDeleteHandlerWithBody(deleteStatus int, deleteBody string) func(req *http.Request) (int, string) { + return func(req *http.Request) (int, string) { + if req.Method == http.MethodDelete { + return deleteStatus, deleteBody + } + return http.StatusOK, singleAndPairStacksBody + } +} + +/************************************************************************************************** +** A stack deleted during FetchAllStacks must not stay in stacksMap: its assets would carry a +** pointer to a stack that no longer exists, and the replace path would then try to delete it +** again and get 400 "Not found or no stack.delete access". See issue #80. +**************************************************************************************************/ +func TestFetchAllStacksExcludesDeletedSingleAssetStacks(t *testing.T) { + transport := &pathRouterMockTransport{handler: stackDeleteHandler(http.StatusNoContent)} + client := &Client{ + apiKey: "test", + apiURL: "http://test/api", + logger: newSilentLogger(), + removeSingleAssetStacks: true, + stackConcurrency: 1, + client: &http.Client{Transport: transport}, + } + + stacksMap, err := client.FetchAllStacks() + require.NoError(t, err) + + assert.Equal(t, []string{"/api/stacks", "/api/stacks/stack-single"}, transport.callsSnapshot()) + assert.NotContains(t, stacksMap, "asset-a", "assets of a deleted stack must not stay indexed") + assert.Contains(t, stacksMap, "asset-b") + assert.Contains(t, stacksMap, "asset-c") +} + +/************************************************************************************************** +** DeleteStack is the logging wrapper around DeleteStackCollect. An already-gone stack yields an +** empty message, which must return nil without logging a blank line — the branch the collect +** tests never reach. +**************************************************************************************************/ +func TestDeleteStackAlreadyGoneLogsNothing(t *testing.T) { + var buf bytes.Buffer + logger := logrus.New() + logger.SetOutput(&buf) + logger.SetLevel(logrus.InfoLevel) + + client := &Client{ + apiKey: "test", + apiURL: "http://test/api", + logger: logger, + client: &http.Client{ + Transport: &mockTransport{ + response: &http.Response{ + StatusCode: http.StatusBadRequest, + Body: io.NopCloser(strings.NewReader(`{"message":"Not found or no stack.delete access"}`)), + }, + }, + }, + } + + deleted, err := client.DeleteStack("gone-stack", "already gone") + assert.NoError(t, err) + assert.False(t, deleted, "a stack the server refused was not deleted by this call") + assert.Empty(t, buf.String(), "an already-gone stack must not log at info level") +} + +/************************************************************************************************** +** isStackAlreadyGone only ever swallows an *APIError. A transport failure carries no status code +** and must keep surfacing as an error. +**************************************************************************************************/ +func TestIsStackAlreadyGoneIgnoresNonAPIErrors(t *testing.T) { + assert.False(t, isStackAlreadyGone(io.ErrUnexpectedEOF)) + assert.False(t, isStackAlreadyGone(fmt.Errorf("wrapped: %w", io.ErrUnexpectedEOF))) + assert.True(t, isStackAlreadyGone(&APIError{StatusCode: http.StatusNotFound})) + assert.True(t, isStackAlreadyGone(fmt.Errorf("wrapped: %w", &APIError{ + StatusCode: http.StatusBadRequest, + Body: `{"message":"Not found or no stack.delete access"}`, + }))) + assert.False(t, isStackAlreadyGone(&APIError{StatusCode: http.StatusForbidden, + Body: `{"message":"Missing required permission: stack.delete"}`})) + assert.False(t, isStackAlreadyGone(&APIError{StatusCode: http.StatusBadRequest, + Body: `{"message":"Invalid UUID"}`})) +} + +/************************************************************************************************** +** A stack whose delete FAILED is still on the server, so it must stay indexed in stacksMap. +** Recording the deletion before the call would drop it and make its assets look unstacked for +** the rest of the run. +**************************************************************************************************/ +func TestFetchAllStacksKeepsStacksWhoseDeleteFailed(t *testing.T) { + transport := &pathRouterMockTransport{handler: stackDeleteHandler(http.StatusInternalServerError)} + client := &Client{ + apiKey: "test", + apiURL: "http://test/api", + logger: newSilentLogger(), + removeSingleAssetStacks: true, + stackConcurrency: 1, + client: &http.Client{Transport: transport}, + } + + stacksMap, err := client.FetchAllStacks() + require.NoError(t, err) + + assert.Equal(t, []string{"/api/stacks", "/api/stacks/stack-single"}, transport.callsSnapshot()) + assert.Contains(t, stacksMap, "asset-a", "a stack that failed to delete still exists server-side") +} + +/************************************************************************************************** +** A stack the server REFUSED to delete must stay indexed. The refusal arrives as the ambiguous +** 400 "Not found or no stack.delete access", which the client treats as benign — but benign is +** not the same as deleted: the stack may still be there, and dropping it would make its assets +** look unstacked and get them stacked on a false premise. +**************************************************************************************************/ +func TestFetchAllStacksKeepsStackRefusedByServer(t *testing.T) { + transport := &pathRouterMockTransport{ + handler: stackDeleteHandlerWithBody(http.StatusBadRequest, `{"message":"Not found or no stack.delete access"}`), + } + client := &Client{ + apiKey: "test", + apiURL: "http://test/api", + logger: newSilentLogger(), + removeSingleAssetStacks: true, + stackConcurrency: 1, + client: &http.Client{Transport: transport}, + } + + stacksMap, err := client.FetchAllStacks() + require.NoError(t, err) + + assert.Equal(t, []string{"/api/stacks", "/api/stacks/stack-single"}, transport.callsSnapshot()) + assert.Contains(t, stacksMap, "asset-a", "a refused delete is not a deletion") +} + +/************************************************************************************************** +** Dry run must model what a real run produces: the single-asset stack is reported as removed and +** leaves stacksMap, while no DELETE is ever issued. +**************************************************************************************************/ +func TestFetchAllStacksDryRunExcludesSingleAssetStacks(t *testing.T) { + transport := &pathRouterMockTransport{handler: stackDeleteHandler(http.StatusNoContent)} + client := &Client{ + apiKey: "test", + apiURL: "http://test/api", + logger: newSilentLogger(), + removeSingleAssetStacks: true, + dryRun: true, + stackConcurrency: 1, + client: &http.Client{Transport: transport}, + } + + stacksMap, err := client.FetchAllStacks() + require.NoError(t, err) + + assert.Equal(t, []string{"/api/stacks"}, transport.callsSnapshot(), "dry run must not issue a DELETE") + assert.NotContains(t, stacksMap, "asset-a", "dry run models the map a real run would produce") + assert.Contains(t, stacksMap, "asset-b") +} From 13bf7f3c1bdf1c697091f49e08422fbaa0282049 Mon Sep 17 00:00:00 2001 From: Major Date: Mon, 7 Sep 2026 12:20:29 +0200 Subject: [PATCH 2/2] fix(stacker): preserve stack delete access errors --- pkg/immich/client.go | 68 +++++++++++++++++++++++++-------------- pkg/immich/client_test.go | 36 +++++++++++++++++---- 2 files changed, 73 insertions(+), 31 deletions(-) diff --git a/pkg/immich/client.go b/pkg/immich/client.go index d19a89f..2b92ac7 100644 --- a/pkg/immich/client.go +++ b/pkg/immich/client.go @@ -9,7 +9,6 @@ import ( "net/http" "net/url" "strconv" - "strings" "sync" "time" @@ -44,6 +43,11 @@ type PartialResultError struct { Phase2Failed int } +type stackDeleteCall struct { + done chan struct{} + err error +} + func (e *PartialResultError) Error() string { return fmt.Sprintf("partial result: %d phase-1 failures, %d phase-2 failures", e.Phase1Failed, e.Phase2Failed) } @@ -79,6 +83,8 @@ type Client struct { filterTakenAfter string filterTakenBefore string logger *logrus.Logger + stackDeleteMu sync.Mutex + stackDeleteCalls map[string]*stackDeleteCall } /************************************************************************************************** @@ -148,6 +154,7 @@ func NewClient(apiURL, apiKey string, resetStacks bool, replaceStacks bool, dryR filterTakenAfter: filterTakenAfter, filterTakenBefore: filterTakenBefore, logger: logger, + stackDeleteCalls: make(map[string]*stackDeleteCall), } } @@ -535,18 +542,49 @@ func (c *Client) DeleteStackCollect(stackID string, reason string) (string, erro return fmt.Sprintf("%sDeleted Stack %s (dry run) - %s", reasonMsg, stackID, reason), nil } - if err := c.doRequest(http.MethodDelete, fmt.Sprintf("/stacks/%s", stackID), nil, nil); err != nil { + deleted, err := c.deleteStackOnce(stackID) + if err != nil { if isStackAlreadyGone(err) { - c.logger.Debugf("Stack %s already gone (or not owned), nothing to delete - %s", stackID, reason) + c.logger.Debugf("Stack %s already gone, nothing to delete - %s", stackID, reason) return "", nil } c.logger.Errorf("Error deleting stack: %v", err) return "", fmt.Errorf("error deleting stack: %w", err) } + if !deleted { + return "", nil + } return fmt.Sprintf("%sDeleted Stack %s - %s", reasonMsg, stackID, reason), nil } +func (c *Client) deleteStackOnce(stackID string) (bool, error) { + c.stackDeleteMu.Lock() + if c.stackDeleteCalls == nil { + c.stackDeleteCalls = make(map[string]*stackDeleteCall) + } + if call, exists := c.stackDeleteCalls[stackID]; exists { + c.stackDeleteMu.Unlock() + <-call.done + return false, call.err + } + + call := &stackDeleteCall{done: make(chan struct{})} + c.stackDeleteCalls[stackID] = call + c.stackDeleteMu.Unlock() + + call.err = c.doRequest(http.MethodDelete, fmt.Sprintf("/stacks/%s", stackID), nil, nil) + close(call.done) + if call.err != nil && !isStackAlreadyGone(call.err) { + c.stackDeleteMu.Lock() + if c.stackDeleteCalls[stackID] == call { + delete(c.stackDeleteCalls, stackID) + } + c.stackDeleteMu.Unlock() + } + return call.err == nil, call.err +} + /************************************************************************************************** ** ModifyStack creates or updates a stack in Immich. ** In dry run mode, it only logs the action without making changes. @@ -935,33 +973,15 @@ func (c *Client) UpdateAlbum(albumID string, updates map[string]interface{}) err } /************************************************************************************************** -** isStackAlreadyGone returns true when a failed DELETE /stacks/{id} may be ignored because the -** stack is no longer there. The same stack ID legitimately reaches this call twice: Immich merges -** (and drops) stacks server-side on POST /stacks, and the stack snapshot taken at the start of a -** run is never refreshed. -** -** The 400 branch is deliberately ambiguous. Immich's access guard answers BOTH "this stack does -** not exist" and "this stack is not yours" with 400 "Not found or no stack.delete access", so -** matching that body cannot tell the two apart: an ownership denial is treated as already gone. -** That is acceptable because the stack IDs acted on come from the caller's own GET /stacks, and -** because the server refused the delete either way — callers must therefore treat a swallowed -** error as "nothing happened", never as "deleted". A key that lacks the stack.delete scope is -** NOT affected: Immich rejects it in the auth guard with 403 (ForbiddenException, "Missing -** required permission", server/src/services/auth.service.ts) before the access check that -** produces this 400, and 403 is not matched here. The bundled OpenAPI spec documents only the -** 204, so both status codes come from the Immich server source rather than from the spec. -** -** Any other 400 body still surfaces as an error. 404 is covered for future API versions. +** isStackAlreadyGone only accepts an unambiguous 404. Immich's 400 response can mean either that +** the stack is absent or that the caller lacks access, so it must remain visible to the user. **************************************************************************************************/ func isStackAlreadyGone(err error) bool { var apiErr *APIError if !errors.As(err, &apiErr) { return false } - if apiErr.StatusCode == http.StatusNotFound { - return true - } - return apiErr.StatusCode == http.StatusBadRequest && strings.Contains(apiErr.Body, "Not found or no stack.delete access") + return apiErr.StatusCode == http.StatusNotFound } /************************************************************************************************** diff --git a/pkg/immich/client_test.go b/pkg/immich/client_test.go index 261faa5..a95fa2b 100644 --- a/pkg/immich/client_test.go +++ b/pkg/immich/client_test.go @@ -1022,14 +1022,13 @@ func TestDeleteStackCollect(t *testing.T) { wantEmptyMsg: true, }, { - name: "immich access-guard 400 for a deleted stack is benign", + name: "immich access-guard 400 remains visible", stackID: "already-deleted-stack", - reason: "already gone", + reason: "access refused", dryRun: false, statusCode: http.StatusBadRequest, responseBody: `{"message":"Not found or no stack.delete access"}`, - wantErr: false, - wantEmptyMsg: true, + wantErr: true, }, { name: "other 400 still fails", @@ -2369,8 +2368,8 @@ func TestDeleteStackAlreadyGoneLogsNothing(t *testing.T) { client: &http.Client{ Transport: &mockTransport{ response: &http.Response{ - StatusCode: http.StatusBadRequest, - Body: io.NopCloser(strings.NewReader(`{"message":"Not found or no stack.delete access"}`)), + StatusCode: http.StatusNotFound, + Body: io.NopCloser(strings.NewReader(`{"message":"Not found"}`)), }, }, }, @@ -2382,6 +2381,29 @@ func TestDeleteStackAlreadyGoneLogsNothing(t *testing.T) { assert.Empty(t, buf.String(), "an already-gone stack must not log at info level") } +func TestDeleteStackCollectDeletesEachStackOnce(t *testing.T) { + transport := &pathRouterMockTransport{ + handler: func(req *http.Request) (int, string) { + return http.StatusNoContent, "" + }, + } + client := &Client{ + apiKey: "test", + apiURL: "http://test/api", + logger: newSilentLogger(), + client: &http.Client{Transport: transport}, + } + + firstMsg, firstErr := client.DeleteStackCollect("stack-a", "first group") + secondMsg, secondErr := client.DeleteStackCollect("stack-a", "second group") + + require.NoError(t, firstErr) + require.NoError(t, secondErr) + assert.NotEmpty(t, firstMsg) + assert.Empty(t, secondMsg) + assert.Equal(t, []string{"/api/stacks/stack-a"}, transport.callsSnapshot()) +} + /************************************************************************************************** ** isStackAlreadyGone only ever swallows an *APIError. A transport failure carries no status code ** and must keep surfacing as an error. @@ -2390,7 +2412,7 @@ func TestIsStackAlreadyGoneIgnoresNonAPIErrors(t *testing.T) { assert.False(t, isStackAlreadyGone(io.ErrUnexpectedEOF)) assert.False(t, isStackAlreadyGone(fmt.Errorf("wrapped: %w", io.ErrUnexpectedEOF))) assert.True(t, isStackAlreadyGone(&APIError{StatusCode: http.StatusNotFound})) - assert.True(t, isStackAlreadyGone(fmt.Errorf("wrapped: %w", &APIError{ + assert.False(t, isStackAlreadyGone(fmt.Errorf("wrapped: %w", &APIError{ StatusCode: http.StatusBadRequest, Body: `{"message":"Not found or no stack.delete access"}`, })))