feat: repair multiple permissions docs (fixes)

Author: shepilov

model/permission/permissions.go

@@ -7,11 +7,12 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"slices"
 	"strings"
 	"time"
 
 	build "github.com/cozy/cozy-stack/pkg/config"
-	stackconfig "github.com/cozy/cozy-stack/pkg/config/config"
+	"github.com/cozy/cozy-stack/pkg/config/config"
 	"github.com/cozy/cozy-stack/pkg/consts"
 	"github.com/cozy/cozy-stack/pkg/couchdb"
 	"github.com/cozy/cozy-stack/pkg/couchdb/mango"
@@ -250,12 +251,6 @@ func GetForSharePreview(db prefixer.Prefixer, sharingID string) (*Permission, er
 // docs as part of the read by creating/updating the canonical permission doc
 // and deleting duplicate legacy docs.
 func GetForShareInteract(db prefixer.Prefixer, sharingID string) (*Permission, error) {
-	mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID))
-	if err := mu.Lock(); err != nil {
-		return nil, err
-	}
-	defer mu.Unlock()
-
 	return getOrRepairShareInteractPermissions(db, sharingID)
 }
 
@@ -294,30 +289,41 @@ func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Perm
 }
 
 func getOrRepairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, error) {
-	return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
-		perms, err := getShareInteractPermissions(db, sharingID)
-		if err != nil {
-			return nil, err
-		}
-		if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
-			return canonical, err
-		}
+	perms, err := getShareInteractPermissions(db, sharingID)
+	if err != nil {
+		return nil, err
+	}
+	if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
+		return canonical, err
+	}
 
-		perm, retry, err := repairShareInteractPermissions(db, sharingID, perms)
-		if err == nil && !retry {
-			return perm, nil
-		}
-		if retry && err == nil {
-			return nil, &couchdb.Error{
-				StatusCode: http.StatusConflict,
-				Name:       "conflict",
-				Reason:     "could not repair share-interact permissions after retries",
-			}
-		}
+	return repairShareInteractPermissionsWithLock(db, sharingID)
+}
+
+func repairShareInteractPermissionsWithLock(db prefixer.Prefixer, sharingID string) (*Permission, error) {
+	mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID))
+	if err := mu.Lock(); err != nil {
 		return nil, err
+	}
+	defer mu.Unlock()
+
+	return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
+		return getOrRepairShareInteractPermissionsOnce(db, sharingID)
 	})
 }
 
+func getOrRepairShareInteractPermissionsOnce(db prefixer.Prefixer, sharingID string) (*Permission, error) {
+	perms, err := getShareInteractPermissions(db, sharingID)
+	if err != nil {
+		return nil, err
+	}
+	if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
+		return canonical, err
+	}
+
+	return repairShareInteractPermissions(db, sharingID, perms)
+}
+
 func shareInteractRetryOptions() utils.RetryOptions {
 	return utils.RetryOptions{
 		Attempts:     5,
@@ -344,15 +350,17 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission
 	usable := 0
 	hasDuplicate := false
 	for i := range perms {
-		if perms[i].ID() != canonicalID {
+		p := &perms[i]
+		isCanonical := p.ID() == canonicalID
+		if !isCanonical {
 			hasDuplicate = true
 		}
-		if perms[i].Expired() {
+		if p.Expired() {
 			continue
 		}
 		usable++
-		if perms[i].ID() == canonicalID {
-			canonical = &perms[i]
+		if isCanonical {
+			canonical = p
 		}
 	}
 	if usable == 0 {
@@ -364,7 +372,7 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission
 	return nil, true, nil
 }
 
-func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, bool, error) {
+func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, error) {
 	canonicalID := ShareInteractPermissionID(sharingID)
 	merged := &Permission{
 		PID:      canonicalID,
@@ -391,7 +399,7 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm
 		hasUsablePermission = true
 
 		if len(merged.Permissions) == 0 && len(p.Permissions) > 0 {
-			merged.Permissions = p.Clone().(*Permission).Permissions
+			merged.Permissions = slices.Clone(p.Permissions)
 		}
 		if merged.Metadata == nil && p.Metadata != nil {
 			merged.Metadata = p.Metadata.Clone()
@@ -413,36 +421,27 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm
 		}
 	}
 	if !hasUsablePermission {
-		return nil, false, ErrExpiredToken
+		return nil, ErrExpiredToken
 	}
 
 	if !hasCanonical {
 		if err := couchdb.CreateNamedDoc(db, merged); err != nil {
-			if couchdb.IsConflictError(err) || couchdb.IsFileExists(err) {
-				return nil, true, err
-			}
-			return nil, false, err
+			return nil, err
 		}
 	} else if err := couchdb.UpdateDoc(db, merged); err != nil {
-		if couchdb.IsConflictError(err) {
-			return nil, true, err
-		}
-		return nil, false, err
+		return nil, err
 	}
 
 	for _, p := range duplicates {
 		if err := couchdb.DeleteDoc(db, p); err != nil {
 			if couchdb.IsNotFoundError(err) {
 				continue
 			}
-			if couchdb.IsConflictError(err) {
-				return nil, true, err
-			}
-			return nil, false, err
+			return nil, err
 		}
 	}
 
-	return merged, false, nil
+	return merged, nil
 }
 
 func getFromSource(db prefixer.Prefixer, permType, docType, slug string) (*Permission, error) {
@@ -820,14 +819,14 @@ func CreateShareInteractSet(db prefixer.Prefixer, sharingID string, codes map[st
 		doc.Codes = make(map[string]string)
 	}
 
-	mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID))
+	mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID))
 	if err := mu.Lock(); err != nil {
 		return nil, err
 	}
 	defer mu.Unlock()
 
 	return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
-		existing, err := getOrRepairShareInteractPermissions(db, sharingID)
+		existing, err := getOrRepairShareInteractPermissionsOnce(db, sharingID)
 		if err != nil && !couchdb.IsNotFoundError(err) {
 			return nil, err
 		}

model/permission/permissions_test.go

@@ -17,7 +17,10 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var testPrefix = prefixer.NewPrefixer(0, "test", "permission-tests")
+func testPrefix(t *testing.T) prefixer.Prefixer {
+	t.Helper()
+	return prefixer.NewPrefixer(0, "test", t.Name())
+}
 
 func TestCheckDoctypeName(t *testing.T) {
 	assert.NoError(t, CheckDoctypeName("io.cozy.files", false))
@@ -438,7 +441,8 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
 	}
 
 	config.UseTestFile(t)
-	require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions))
+	db := testPrefix(t)
+	require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
 
 	const sharingID = "sharing-duplicate-interact-permissions"
 	perms := Permission{
@@ -450,7 +454,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
 		}},
 	}
 
-	err := couchdb.CreateDoc(testPrefix, &Permission{
+	err := couchdb.CreateDoc(db, &Permission{
 		Type:        TypeShareInteract,
 		Permissions: perms.Permissions,
 		Codes: map[string]string{
@@ -459,7 +463,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
 		SourceID: consts.Sharings + "/" + sharingID,
 	})
 	require.NoError(t, err)
-	err = couchdb.CreateDoc(testPrefix, &Permission{
+	err = couchdb.CreateDoc(db, &Permission{
 		Type:        TypeShareInteract,
 		Permissions: perms.Permissions,
 		Codes: map[string]string{
@@ -469,27 +473,27 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	repaired, err := GetForShareInteract(testPrefix, sharingID)
+	repaired, err := GetForShareInteract(db, sharingID)
 	require.NoError(t, err)
 	require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID())
 	require.Equal(t, map[string]string{
 		"alice@example.test": "alice-token",
 		"bob@example.test":   "bob-token",
 	}, repaired.Codes)
 
-	all, err := getShareInteractPermissions(testPrefix, sharingID)
+	all, err := getShareInteractPermissions(db, sharingID)
 	require.NoError(t, err)
 	require.Len(t, all, 1)
 	require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())
 
-	repaired, err = GetForShareInteract(testPrefix, sharingID)
+	repaired, err = GetForShareInteract(db, sharingID)
 	require.NoError(t, err)
 	require.Equal(t, map[string]string{
 		"alice@example.test": "alice-token",
 		"bob@example.test":   "bob-token",
 	}, repaired.Codes)
 
-	all, err = getShareInteractPermissions(testPrefix, sharingID)
+	all, err = getShareInteractPermissions(db, sharingID)
 	require.NoError(t, err)
 	require.Len(t, all, 1)
 }
@@ -500,7 +504,8 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
 	}
 
 	config.UseTestFile(t)
-	require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions))
+	db := testPrefix(t)
+	require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
 
 	const sharingID = "sharing-expired-duplicate-interact-permissions"
 	rules := Set{{
@@ -511,7 +516,7 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
 	}}
 	expiredAt := time.Now().Add(-time.Hour).Format(time.RFC3339)
 
-	err := couchdb.CreateNamedDoc(testPrefix, &Permission{
+	err := couchdb.CreateNamedDoc(db, &Permission{
 		PID:         ShareInteractPermissionID(sharingID),
 		Type:        TypeShareInteract,
 		Permissions: rules,
@@ -521,7 +526,7 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
 		SourceID: consts.Sharings + "/" + sharingID,
 	})
 	require.NoError(t, err)
-	err = couchdb.CreateDoc(testPrefix, &Permission{
+	err = couchdb.CreateDoc(db, &Permission{
 		Type:        TypeShareInteract,
 		Permissions: rules,
 		Codes: map[string]string{
@@ -532,14 +537,14 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	repaired, err := GetForShareInteract(testPrefix, sharingID)
+	repaired, err := GetForShareInteract(db, sharingID)
 	require.NoError(t, err)
 	require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID())
 	require.Equal(t, map[string]string{
 		"alice@example.test": "alice-token",
 	}, repaired.Codes)
 
-	all, err := getShareInteractPermissions(testPrefix, sharingID)
+	all, err := getShareInteractPermissions(db, sharingID)
 	require.NoError(t, err)
 	require.Len(t, all, 1)
 	require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())
@@ -551,7 +556,8 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
 	}
 
 	config.UseTestFile(t)
-	require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions))
+	db := testPrefix(t)
+	require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
 
 	const sharingID = "sharing-canonical-interact-permissions"
 	md := metadata.New()
@@ -566,13 +572,13 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
 		Metadata: md,
 	}
 
-	first, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{
+	first, err := CreateShareInteractSet(db, sharingID, map[string]string{
 		"alice@example.test": "alice-token",
 	}, perms)
 	require.NoError(t, err)
 	require.Equal(t, ShareInteractPermissionID(sharingID), first.ID())
 
-	second, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{
+	second, err := CreateShareInteractSet(db, sharingID, map[string]string{
 		"bob@example.test": "bob-token",
 	}, perms)
 	require.NoError(t, err)
@@ -585,7 +591,7 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
 	require.NotNil(t, second.Metadata)
 	require.True(t, second.Metadata.UpdatedAt.After(first.Metadata.UpdatedAt))
 
-	all, err := getShareInteractPermissions(testPrefix, sharingID)
+	all, err := getShareInteractPermissions(db, sharingID)
 	require.NoError(t, err)
 	require.Len(t, all, 1)
 	require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())

model/sharing/sharing.go

@@ -400,7 +400,7 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe
 	if stored, ok := updated.Codes[key]; ok {
 		return stored, nil
 	}
-	return code, nil
+	return "", fmt.Errorf("share-interact code for %s was not stored in sharing %s", key, s.SID)
 }
 
 func (s *Sharing) createInteractPermissions(inst *instance.Instance, m *Member) (string, error) {

pkg/utils/retry.go

@@ -2,7 +2,7 @@ package utils
 
 import (
 	"context"
-	"math/rand"
+	"math/rand/v2"
 	"time"
 )
 
@@ -19,7 +19,8 @@ type RetryOptions struct {
 	// MaxDelay caps the exponential backoff delay before jitter is applied.
 	MaxDelay time.Duration
 	// JitterFactor adds a random delay between 0 and the current backoff delay
-	// multiplied by JitterFactor. For example, 0.25 adds up to 25% jitter.
+	// multiplied by JitterFactor. This jitter is one-sided: it can only extend
+	// the delay, never shorten it. For example, 0.25 adds up to 25% jitter.
 	// Values lower than or equal to 0 disable jitter.
 	JitterFactor float64
 	// ShouldRetry can be used to retry only some errors. When nil, every
@@ -109,14 +110,14 @@ func backoffDelay(initial time.Duration, attempt int, maxDelay time.Duration) ti
 	delay := initial
 	for i := 0; i < attempt; i++ {
 		if delay > maxRetryDelay/2 {
-			delay = maxRetryDelay
-		} else {
-			delay *= 2
-		}
-		if maxDelay > 0 && delay > maxDelay {
-			return maxDelay
+			return cappedDelay(maxRetryDelay, maxDelay)
 		}
+		delay *= 2
 	}
+	return cappedDelay(delay, maxDelay)
+}
+
+func cappedDelay(delay time.Duration, maxDelay time.Duration) time.Duration {
 	if maxDelay > 0 && delay > maxDelay {
 		return maxDelay
 	}
@@ -141,7 +142,7 @@ func addJitter(delay time.Duration, jitterFactor float64) time.Duration {
 	if maxJitter <= 0 {
 		return delay
 	}
-	jitter := time.Duration(rand.Int63n(int64(maxJitter)))
+	jitter := time.Duration(rand.Int64N(int64(maxJitter)))
 	if maxRetryDelay-delay < jitter {
 		return maxRetryDelay
 	}