From 921d294ee4d47c786499f7a0be6253fa2e5a1686 Mon Sep 17 00:00:00 2001
From: Dominik Schulz <dominik.schulz@gauner.org>
Date: Fri, 23 Dec 2022 22:26:20 +0100
Subject: [PATCH] Disable possible git concurrency during fsck (#2486)

By using the default queue some git operations might still have been
running when gopass did attempt to update exported keys. Disabling
the queue in the inner loop makes more sense since it's more predictable
and less brittle and using the queue there wouldn't help much anyway.

Fixes #2459

RELEASE_NOTES=[BUGFIX] Fix possible concurrency issues in fsck.

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
---
 internal/queue/background.go |  5 +++--
 internal/store/leaf/fsck.go  | 36 +++++++++++++++++++++++++++---------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/internal/queue/background.go b/internal/queue/background.go
index 01b6fefd92..5bdca2f783 100644
--- a/internal/queue/background.go
+++ b/internal/queue/background.go
@@ -29,7 +29,8 @@ type Queuer interface {
 	Idle(time.Duration) error
 }
 
-// WithQueue adds the given queue to the context.
+// WithQueue adds the given queue to the context. Add a nil
+// queue to disable queuing in this context.
 func WithQueue(ctx context.Context, q *Queue) context.Context {
 	return context.WithValue(ctx, ctxKeyQueue, q)
 }
@@ -37,7 +38,7 @@ func WithQueue(ctx context.Context, q *Queue) context.Context {
 // GetQueue returns an existing queue from the context or
 // returns a noop one.
 func GetQueue(ctx context.Context) Queuer {
-	if q, ok := ctx.Value(ctxKeyQueue).(*Queue); ok {
+	if q, ok := ctx.Value(ctxKeyQueue).(*Queue); ok && q != nil {
 		return q
 	}
 
diff --git a/internal/store/leaf/fsck.go b/internal/store/leaf/fsck.go
index 5f9eb11cba..93f232ca3a 100644
--- a/internal/store/leaf/fsck.go
+++ b/internal/store/leaf/fsck.go
@@ -10,6 +10,7 @@ import (
 	"github.com/gopasspw/gopass/internal/backend"
 	"github.com/gopasspw/gopass/internal/diff"
 	"github.com/gopasspw/gopass/internal/out"
+	"github.com/gopasspw/gopass/internal/queue"
 	"github.com/gopasspw/gopass/internal/store"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
 	"github.com/gopasspw/gopass/pkg/debug"
@@ -33,26 +34,49 @@ func (s *Store) Fsck(ctx context.Context, path string) error {
 		return fmt.Errorf("storage backend compaction failed: %w", err)
 	}
 
-	pcb := ctxutil.GetProgressCallback(ctx)
-
 	// then we'll make sure all the secrets are readable by us and every
 	// valid recipient
 	if path != "" {
 		out.Printf(ctx, "Checking all secrets matching %s", path)
 	}
 
+	if err := s.fsckLoop(ctx, path); err != nil {
+		return err
+	}
+
+	if err := s.storage.Push(ctx, "", ""); err != nil {
+		if !errors.Is(err, store.ErrGitNoRemote) {
+			out.Printf(ctx, "RCS Push failed: %s", err)
+		}
+	}
+
+	return nil
+}
+
+func (s *Store) fsckLoop(ctx context.Context, path string) error {
+	pcb := ctxutil.GetProgressCallback(ctx)
+
+	// disable network ops, we will push at the end. pushing on possibly
+	// every single secret could be terribly slow.
+	ctx = ctxutil.WithNoNetwork(ctx, true)
+
+	// disable the queue, for batch operations this is not necessary / wanted
+	// since different git processes might step onto each others toes.
+	ctx = queue.WithQueue(ctx, nil)
+
 	names, err := s.List(ctx, path)
 	if err != nil {
 		return fmt.Errorf("failed to list entries for %s: %w", path, err)
 	}
 	debug.Log("names (%d): %q", len(names), names)
 	sort.Strings(names)
+
 	for _, name := range names {
 		pcb()
 		if strings.HasPrefix(name, s.alias+"/") {
 			name = strings.TrimPrefix(name, s.alias+"/")
 		}
-		ctx := ctxutil.WithNoNetwork(ctx, true)
+
 		debug.Log("[%s] Checking %s", path, name)
 
 		if err := s.fsckCheckEntry(ctx, name); err != nil {
@@ -64,12 +88,6 @@ func (s *Store) Fsck(ctx context.Context, path string) error {
 		out.Errorf(ctx, "Failed to update public keys: %s", err)
 	}
 
-	if err := s.storage.Push(ctx, "", ""); err != nil {
-		if !errors.Is(err, store.ErrGitNoRemote) {
-			out.Printf(ctx, "RCS Push failed: %s", err)
-		}
-	}
-
 	return nil
 }