diff --git a/.github/workflows/test-quark-delete-reliability.yml b/.github/workflows/test-quark-delete-reliability.yml new file mode 100644 index 00000000000..c1c12e3d233 --- /dev/null +++ b/.github/workflows/test-quark-delete-reliability.yml @@ -0,0 +1,38 @@ +name: test-quark-delete-reliability + +on: + pull_request: + branches: [ 'main' ] + +jobs: + validate: + runs-on: ubuntu-latest + env: + GOPROXY: https://proxy.golang.org,direct + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: '1.22' + + - name: Check formatting + run: | + test -z "$(gofmt -l drivers/quark_uc/driver.go drivers/quark_uc/delete_reliability.go drivers/quark_uc/delete_reliability_test.go)" + + - name: Vet Quark driver + run: go vet ./drivers/quark_uc + + - name: Test Quark driver + run: go test ./drivers/quark_uc + + - name: Race test Quark driver + run: go test -race ./drivers/quark_uc + + - name: Shuffled repeated race test + run: go test -race -shuffle=on -count=2 ./drivers/quark_uc + + - name: Test op and WebDAV + run: go test ./internal/op ./server/webdav diff --git a/drivers/quark_uc/delete_reliability.go b/drivers/quark_uc/delete_reliability.go new file mode 100644 index 00000000000..ea1ce091a59 --- /dev/null +++ b/drivers/quark_uc/delete_reliability.go @@ -0,0 +1,139 @@ +package quark + +import ( + "context" + "fmt" + "net/http" + "strings" + "time" + + "github.com/alist-org/alist/v3/drivers/base" + "github.com/alist-org/alist/v3/internal/model" + "github.com/go-resty/resty/v2" + log "github.com/sirupsen/logrus" +) + +const ( + deleteControlMaxAttempts = 3 + deleteControlInitialBackoff = 250 * time.Millisecond + deleteControlMaxBackoff = 500 * time.Millisecond +) + +type deleteFileInfoResp struct { + Resp + Data struct { + List []File `json:"list"` + } `json:"data"` +} + +func hasQuarkDeleteErrorTokenPrefix(msg, token string) bool { + if msg == token { + return true + } + if !strings.HasPrefix(msg, token) || len(msg) == len(token) { + return false + } + switch msg[len(token)] { + case ' ', ',', ':', '\t': + return true + default: + return false + } +} + +func isRetryableQuarkDeleteError(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(strings.TrimSpace(err.Error())) + return hasQuarkDeleteErrorTokenPrefix(msg, "inner error") && strings.Contains(msg, "requestid") +} + +func waitDeleteRetry(ctx context.Context, delay time.Duration) error { + timer := time.NewTimer(delay) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +func (d *QuarkOrUC) deleteFileExistsByFID(fid string) (bool, error) { + var resp deleteFileInfoResp + _, err := d.request("/file", http.MethodGet, func(req *resty.Request) { + req.SetQueryParam("fids", fid) + }, &resp) + if err != nil { + return false, err + } + for _, file := range resp.Data.List { + if file.Fid == fid { + return true, nil + } + } + return false, nil +} + +func (d *QuarkOrUC) removeReliable(ctx context.Context, obj model.Obj) error { + fid := obj.GetID() + data := base.Json{ + "action_type": 1, + "exclude_fids": []string{}, + "filelist": []string{fid}, + } + + backoff := deleteControlInitialBackoff + hadTransient := false + for attempt := 1; attempt <= deleteControlMaxAttempts; attempt++ { + if err := ctx.Err(); err != nil { + return err + } + + _, err := d.request("/file/delete", http.MethodPost, func(req *resty.Request) { + req.SetBody(data) + }, nil) + if err == nil { + return nil + } + + retryable := isRetryableQuarkDeleteError(err) + if retryable { + hadTransient = true + } + + // A retryable response is ambiguous: Quark may have accepted the delete + // before returning the provider error. Verify the immutable FID directly + // before replaying the destructive request. After an earlier transient, + // also verify a later non-retryable response so an already-completed + // delete is not turned back into a failure. + if retryable || hadTransient { + exists, verifyErr := d.deleteFileExistsByFID(fid) + if verifyErr == nil && !exists { + log.Warnf("quark delete returned an error but fid=%s is absent; treating delete as success: %v", fid, err) + return nil + } + if verifyErr != nil { + log.Warnf("quark delete fid verification failed attempt=%d/%d fid=%s: %v", attempt, deleteControlMaxAttempts, fid, verifyErr) + } + } + + if !retryable { + return err + } + if attempt == deleteControlMaxAttempts { + return fmt.Errorf("quark delete transient provider error after %d attempts: %w", attempt, err) + } + + log.Warnf("quark delete transient provider error attempt=%d/%d fid=%s: %v; retrying", attempt, deleteControlMaxAttempts, fid, err) + if err := waitDeleteRetry(ctx, backoff); err != nil { + return err + } + backoff *= 2 + if backoff > deleteControlMaxBackoff { + backoff = deleteControlMaxBackoff + } + } + return fmt.Errorf("quark delete retry loop exhausted unexpectedly") +} diff --git a/drivers/quark_uc/delete_reliability_test.go b/drivers/quark_uc/delete_reliability_test.go new file mode 100644 index 00000000000..7c26ed66464 --- /dev/null +++ b/drivers/quark_uc/delete_reliability_test.go @@ -0,0 +1,302 @@ +package quark + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/alist-org/alist/v3/internal/model" + "github.com/go-resty/resty/v2" +) + +func newDeleteTestDriver(serverURL string) *QuarkOrUC { + client := resty.New() + client.SetRetryCount(0) + return &QuarkOrUC{ + Addition: Addition{Cookie: "test-cookie"}, + conf: Conf{ + api: serverURL + "/1/clouddrive", + pr: "ucpro", + referer: "https://pan.quark.cn", + }, + client: client, + } +} + +func writeDeleteJSON(w http.ResponseWriter, status int, value any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(value) +} + +func deleteTestObject(fid string) model.Obj { + return &model.Object{ID: fid, Name: "chunk.bucket.4"} +} + +func TestIsRetryableQuarkDeleteError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "observed provider error", err: errors.New("inner error, requestId 95sg27-abc"), want: true}, + {name: "case insensitive", err: errors.New("Inner Error: RequestId abc"), want: true}, + {name: "missing request id", err: errors.New("inner error"), want: false}, + {name: "token extension plural", err: errors.New("inner errors, requestId abc"), want: false}, + {name: "token extension suffix", err: errors.New("inner error_x, requestId abc"), want: false}, + {name: "permission", err: errors.New("permission denied"), want: false}, + {name: "transport timeout", err: errors.New("context deadline exceeded"), want: false}, + {name: "wrapped unrelated", err: errors.New("delete status: 500, inner error, requestId abc"), want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isRetryableQuarkDeleteError(tt.err); got != tt.want { + t.Fatalf("isRetryableQuarkDeleteError(%q) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + +func TestRemoveReliableRetriesTransientWhenFIDStillExists(t *testing.T) { + deleteCalls := 0 + infoCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/1/clouddrive/file/delete": + deleteCalls++ + if deleteCalls == 1 { + writeDeleteJSON(w, http.StatusInternalServerError, Resp{ + Status: 500, Code: 500, Message: "inner error, requestId delete-1", + }) + return + } + writeDeleteJSON(w, http.StatusOK, Resp{Status: 200, Code: 0}) + case r.Method == http.MethodGet && r.URL.Path == "/1/clouddrive/file": + infoCalls++ + if got := r.URL.Query().Get("fids"); got != "fid-1" { + t.Fatalf("fids=%q, want fid-1", got) + } + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, + "code": 0, + "data": map[string]any{"list": []map[string]any{{ + "fid": "fid-1", "file_name": "chunk.bucket.4", "file": true, + }}}, + }) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + if err := d.Remove(context.Background(), deleteTestObject("fid-1")); err != nil { + t.Fatalf("Remove: %v", err) + } + if deleteCalls != 2 || infoCalls != 1 { + t.Fatalf("deleteCalls=%d infoCalls=%d, want 2/1", deleteCalls, infoCalls) + } +} + +func TestRemoveReliableTreatsConfirmedAbsentFIDAsSuccess(t *testing.T) { + deleteCalls := 0 + infoCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/1/clouddrive/file/delete": + deleteCalls++ + writeDeleteJSON(w, http.StatusInternalServerError, Resp{ + Status: 500, Code: 500, Message: "inner error, requestId ambiguous-1", + }) + case r.Method == http.MethodGet && r.URL.Path == "/1/clouddrive/file": + infoCalls++ + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, + "code": 0, + "data": map[string]any{"list": []any{}}, + }) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + if err := d.Remove(context.Background(), deleteTestObject("fid-gone")); err != nil { + t.Fatalf("Remove: %v", err) + } + if deleteCalls != 1 || infoCalls != 1 { + t.Fatalf("deleteCalls=%d infoCalls=%d, want 1/1", deleteCalls, infoCalls) + } +} + +func TestRemoveReliableRecoversIfRetrySeesNonTransientAfterEarlierTransient(t *testing.T) { + deleteCalls := 0 + infoCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/1/clouddrive/file/delete": + deleteCalls++ + if deleteCalls == 1 { + writeDeleteJSON(w, http.StatusInternalServerError, Resp{ + Status: 500, Code: 500, Message: "inner error, requestId ambiguous-2", + }) + return + } + writeDeleteJSON(w, http.StatusBadRequest, Resp{ + Status: 400, Code: 400, Message: "object not found", + }) + case r.Method == http.MethodGet && r.URL.Path == "/1/clouddrive/file": + infoCalls++ + if infoCalls == 1 { + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, + "code": 0, + "data": map[string]any{"list": []map[string]any{{ + "fid": "fid-2", "file_name": "chunk.index.4", "file": true, + }}}, + }) + return + } + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, + "code": 0, + "data": map[string]any{"list": []any{}}, + }) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + if err := d.Remove(context.Background(), deleteTestObject("fid-2")); err != nil { + t.Fatalf("Remove: %v", err) + } + if deleteCalls != 2 || infoCalls != 2 { + t.Fatalf("deleteCalls=%d infoCalls=%d, want 2/2", deleteCalls, infoCalls) + } +} + +func TestRemoveReliablePreservesNonTransientErrorWithoutProbe(t *testing.T) { + deleteCalls := 0 + infoCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/1/clouddrive/file/delete": + deleteCalls++ + writeDeleteJSON(w, http.StatusForbidden, Resp{ + Status: 403, Code: 403, Message: "permission denied", + }) + case "/1/clouddrive/file": + infoCalls++ + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, "code": 0, "data": map[string]any{"list": []any{}}, + }) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + err := d.Remove(context.Background(), deleteTestObject("fid-3")) + if err == nil || !strings.Contains(err.Error(), "permission denied") { + t.Fatalf("err=%v, want permission denied", err) + } + if deleteCalls != 1 || infoCalls != 0 { + t.Fatalf("deleteCalls=%d infoCalls=%d, want 1/0", deleteCalls, infoCalls) + } +} + +func TestRemoveReliableFailsClosedAfterRetryBudget(t *testing.T) { + deleteCalls := 0 + infoCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/1/clouddrive/file/delete": + deleteCalls++ + writeDeleteJSON(w, http.StatusInternalServerError, Resp{ + Status: 500, Code: 500, Message: "inner error, requestId persistent", + }) + case "/1/clouddrive/file": + infoCalls++ + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, + "code": 0, + "data": map[string]any{"list": []map[string]any{{ + "fid": "fid-4", "file_name": "chunk.bucket.4", "file": true, + }}}, + }) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + err := d.Remove(context.Background(), deleteTestObject("fid-4")) + if err == nil { + t.Fatal("want error") + } + if !strings.Contains(err.Error(), "after 3 attempts") { + t.Fatalf("unexpected error: %v", err) + } + if deleteCalls != deleteControlMaxAttempts || infoCalls != deleteControlMaxAttempts { + t.Fatalf("deleteCalls=%d infoCalls=%d, want %d/%d", deleteCalls, infoCalls, + deleteControlMaxAttempts, deleteControlMaxAttempts) + } +} + +func TestRemoveReliableCanceledContextDoesNotSendRequest(t *testing.T) { + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + writeDeleteJSON(w, http.StatusOK, Resp{Status: 200, Code: 0}) + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + err := d.Remove(ctx, deleteTestObject("fid-5")) + if !errors.Is(err, context.Canceled) { + t.Fatalf("err=%v, want context.Canceled", err) + } + if calls != 0 { + t.Fatalf("calls=%d, want 0", calls) + } +} + +func TestDeleteFileExistsByFIDRequiresExactFID(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/1/clouddrive/file" { + http.NotFound(w, r) + return + } + if got := r.URL.Query().Get("fids"); got != "target-fid" { + t.Fatalf("fids=%q, want target-fid", got) + } + writeDeleteJSON(w, http.StatusOK, map[string]any{ + "status": 200, + "code": 0, + "data": map[string]any{"list": []map[string]any{{ + "fid": "different-fid", "file_name": "same-name", "file": true, + }}}, + }) + })) + defer srv.Close() + + d := newDeleteTestDriver(srv.URL) + exists, err := d.deleteFileExistsByFID("target-fid") + if err != nil { + t.Fatalf("deleteFileExistsByFID: %v", err) + } + if exists { + t.Fatal("different FID must not verify target existence") + } +} diff --git a/drivers/quark_uc/driver.go b/drivers/quark_uc/driver.go index 264150dc920..217a6149403 100644 --- a/drivers/quark_uc/driver.go +++ b/drivers/quark_uc/driver.go @@ -145,15 +145,7 @@ func (d *QuarkOrUC) Copy(ctx context.Context, srcObj, dstDir model.Obj) error { } func (d *QuarkOrUC) Remove(ctx context.Context, obj model.Obj) error { - data := base.Json{ - "action_type": 1, - "exclude_fids": []string{}, - "filelist": []string{obj.GetID()}, - } - _, err := d.request("/file/delete", http.MethodPost, func(req *resty.Request) { - req.SetBody(data) - }, nil) - return err + return d.removeReliable(ctx, obj) } func (d *QuarkOrUC) Put(ctx context.Context, dstDir model.Obj, stream model.FileStreamer, up driver.UpdateProgress) error {