fix(gitsync): dashboard edits right after a pull are committed and pushed

internal/configsync/configsync.go

@@ -353,6 +353,17 @@ func (s *Syncer) WriteAppSidecar(slug string) error {
 	return nil
 }
 
+// RenderAppSidecar returns the bytes WriteAppSidecar would write for slug
+// now, without writing. Git sync uses it to recognize its own re-render of a
+// pulled sidecar.
+func (s *Syncer) RenderAppSidecar(slug string) ([]byte, error) {
+	sidecar, err := s.buildAppSidecar(slug)
+	if err != nil {
+		return nil, fmt.Errorf("RenderAppSidecar %s: %w", slug, err)
+	}
+	return encodeYAML(*sidecar)
+}
+
 // buildAppSidecarPair returns both the non-secret AppSidecar and the AppSecrets
 // for the slug. Used by WriteAppSidecar; callers that only need the sidecar
 // (archive tombstones) can use buildAppSidecar.
@@ -1116,16 +1127,25 @@ func atomicWriteYAMLMode(path string, mode os.FileMode, v any) error {
 		return fmt.Errorf("mkdir %s: %w", filepath.Dir(path), err)
 	}
 
+	b, err := encodeYAML(v)
+	if err != nil {
+		return fmt.Errorf("%s: %w", path, err)
+	}
+	return atomicWriteFile(path, b, mode)
+}
+
+// encodeYAML renders v the way every sidecar file is written.
+func encodeYAML(v any) ([]byte, error) {
 	var buf bytes.Buffer
 	enc := yaml.NewEncoder(&buf)
 	enc.SetIndent(2)
 	if err := enc.Encode(v); err != nil {
-		return fmt.Errorf("yaml encode %s: %w", path, err)
+		return nil, fmt.Errorf("yaml encode: %w", err)
 	}
 	if err := enc.Close(); err != nil {
-		return fmt.Errorf("yaml encode close %s: %w", path, err)
+		return nil, fmt.Errorf("yaml encode close: %w", err)
 	}
-	return atomicWriteFile(path, buf.Bytes(), mode)
+	return buf.Bytes(), nil
 }
 
 // staleTmpAge is how old a leftover temp file must be before

internal/configsync/configsync_test.go

@@ -1067,11 +1067,18 @@ func TestDebouncedGlobalWritesBothFiles(t *testing.T) {
 
 	syncer.ScheduleGlobalWrite()
 
-	// Wait for debounce to fire (500ms + buffer).
-	time.Sleep(700 * time.Millisecond)
-
 	secretPath := filepath.Join(dataDir, "config.yml")
 	redactedPath := filepath.Join(appsDir, "_global.yml")
+	// Wait for the debounced write (500ms) to land both files.
+	deadline := time.Now().Add(5 * time.Second)
+	for time.Now().Before(deadline) {
+		_, e1 := os.Stat(secretPath)
+		_, e2 := os.Stat(redactedPath)
+		if e1 == nil && e2 == nil {
+			break
+		}
+		time.Sleep(50 * time.Millisecond)
+	}
 
 	if _, err := os.Stat(secretPath); err != nil {
 		t.Errorf("config.yml missing: %v", err)

internal/gitsync/gitsync.go

@@ -137,6 +137,9 @@ type commitReq struct {
 type syncReq struct {
 	ctx  context.Context
 	done chan<- error
+	// apply applies fetched commits even when AutoApplyEnabled is off
+	// (ApplyPending).
+	apply bool
 }
 
 // CommitInfo is a summary of a single commit for the status endpoint.
@@ -196,7 +199,7 @@ type Syncer struct {
 	commitCh chan commitReq // buffered
 	syncCh   chan syncReq   // buffered
 
-	suppress atomic.Bool // true while import-from-pull is running
+	suppress atomic.Bool // true while a pull's sidecars are being imported
 
 	mu              sync.Mutex
 	headSHA         string
@@ -205,9 +208,16 @@ type Syncer struct {
 	recentConflicts []Conflict
 	dropped         int64
 	commitsBehind   int
-	// pullRewrites holds repo-relative managed paths written while a pull
-	// was being imported and not changed locally since (see pending.go).
+	// pullRewrites holds repo-relative managed paths SimpleDeploy rewrote
+	// after importing a pull and not changed locally since (see pending.go).
 	pullRewrites map[string]bool
+	// rewriteHash maps a pulled sidecar's path to the SHA-256 of the file
+	// SimpleDeploy renders from the DB right after importing it. A later
+	// write with exactly these bytes is that re-render, not a local edit.
+	rewriteHash map[string]string
+	// held are commit requests made while suppress was on, classified when
+	// the import finishes.
+	held []commitReq
 
 	wg     sync.WaitGroup
 	cancel context.CancelFunc
@@ -217,13 +227,6 @@ const (
 	commitChanSize = 32
 	syncChanSize   = 8
 	maxConflicts   = 20
-
-	// suppressTail is how long to keep suppress=true after an import completes.
-	// The configsync debouncer fires up to 500ms after the last ScheduleAppWrite
-	// call; keeping suppress active for 2x that window ensures the debounced
-	// WriteAppSidecar -> callHook -> EnqueueCommit path is also suppressed,
-	// preventing a spurious bot commit after every remote pull.
-	suppressTail = 1200 * time.Millisecond
 )
 
 // New validates config and constructs a Syncer. Does not touch disk or network.
@@ -307,10 +310,16 @@ func (g *Syncer) EnqueueCommit(paths []string, reason string) {
 		return
 	}
 	if g.suppress.Load() {
-		g.notePullRewrites(paths)
+		// Mid-import: classified once the import's own render is known.
+		g.mu.Lock()
+		g.held = append(g.held, commitReq{paths: paths, reason: reason})
+		g.mu.Unlock()
+		return
+	}
+	paths, ok := g.skipPullRewrites(paths)
+	if !ok {
 		return
 	}
-	g.forgetPullRewrites(paths)
 	select {
 	case g.commitCh <- commitReq{paths: paths, reason: reason}:
 	default:
@@ -345,7 +354,13 @@ func (g *Syncer) recentCommits(n int) []CommitInfo {
 	if g.repo.Load() == nil {
 		return nil
 	}
-	logIter, err := g.repo.Load().Log(&git.LogOptions{})
+	// Status runs on API goroutines while the worker uses g.repo, and a
+	// go-git handle is not safe for concurrent use: read through our own.
+	repo, err := git.PlainOpen(g.cfg.AppsDir)
+	if err != nil {
+		return nil
+	}
+	logIter, err := repo.Log(&git.LogOptions{})
 	if err != nil {
 		return nil
 	}
@@ -458,8 +473,11 @@ func (g *Syncer) worker(ctx context.Context) {
 		case req := <-g.commitCh:
 			g.doCommit(ctx, req)
 		case req := <-g.syncCh:
-			err := g.doPull(req.ctx)
-			req.done <- err
+			if req.apply {
+				req.done <- g.applyPending(req.ctx)
+			} else {
+				req.done <- g.doPull(req.ctx)
+			}
 		}
 	}
 }
@@ -866,13 +884,31 @@ func (g *Syncer) commitsAhead() int {
 
 // ApplyPending runs fetchAndInspect + applyFetched regardless of AutoApplyEnabled.
 // Intended for on-demand application of pending remote changes.
+// It runs on the worker goroutine, like every other git operation: the
+// go-git handle is not safe for concurrent use.
 func (g *Syncer) ApplyPending(ctx context.Context) error {
 	if !g.cfg.Enabled {
 		return errors.New("gitsync: not enabled")
 	}
 	if g.repo.Load() == nil {
 		return errors.New("gitsync: repo not initialized")
 	}
+	done := make(chan error, 1)
+	select {
+	case g.syncCh <- syncReq{ctx: ctx, done: done, apply: true}:
+	case <-ctx.Done():
+		return ctx.Err()
+	}
+	select {
+	case err := <-done:
+		return err
+	case <-ctx.Done():
+		return ctx.Err()
+	}
+}
+
+// applyPending fetches and applies whatever origin has, then pushes.
+func (g *Syncer) applyPending(ctx context.Context) error {
 	if _, err := g.fetchAndInspect(); err != nil {
 		return err
 	}
@@ -1028,15 +1064,24 @@ func (g *Syncer) applyFetched(ctx context.Context) error {
 		return nil
 	}
 
-	// Import sidecar changes; suppress new commits during this window.
-	// Keep suppress active for suppressTail after import so that the
-	// configsync debouncer (500ms) cannot fire a WriteAppSidecar ->
-	// callHook -> EnqueueCommit sequence that would produce a spurious
-	// bot commit rebounding the pulled change.
+	// Hold commit requests while the pulled sidecars are imported. The
+	// import rewrites the DB, which re-renders those sidecars (store
+	// mutation hook -> debounced sidecar write). Once it is done, record
+	// what that render looks like, so it is told apart from a local edit by
+	// content however late it lands (see skipPullRewrites).
 	g.suppress.Store(true)
-	defer func() {
-		time.AfterFunc(suppressTail, func() { g.suppress.Store(false) })
-	}()
+	var imported []string
+	importDone := false
+	finishImport := func() {
+		if importDone {
+			return
+		}
+		importDone = true
+		g.recordPullRenders(imported)
+		g.suppress.Store(false)
+		g.releaseHeld()
+	}
+	defer finishImport()
 
 	chk := g.securePulledTree(prevSHA, newSHA, snap)
 	changedPaths := chk.changed
@@ -1069,9 +1114,12 @@ func (g *Syncer) applyFetched(ctx context.Context) error {
 			// Access grants are dashboard-managed: never apply them from a pull.
 			if importErr := g.cs.ImportAppSidecarWithOptions(sidecar, configsync.ImportOptions{SkipAccess: true}); importErr != nil {
 				log.Printf("[gitsync] import app sidecar %s: %v", slug, importErr)
+				continue
 			}
+			imported = append(imported, slug)
 		}
 	}
+	finishImport()
 
 	// A commit of pull rewrites alone is not pushed here; it goes out with
 	// the next push, so a pull does not echo straight back to the remote.

internal/gitsync/gitsync_test.go

@@ -419,23 +419,27 @@ func TestPullConflictServerWins(t *testing.T) {
 	}
 }
 
-// TestSuppressDuringImport: suppress flag blocks EnqueueCommit.
+// TestSuppressDuringImport: requests made while an import runs are held,
+// then classified (and here enqueued) when it finishes.
 func TestSuppressDuringImport(t *testing.T) {
 	s := &Syncer{
 		cfg:      Config{Enabled: true, AutoPushEnabled: true},
 		commitCh: make(chan commitReq, commitChanSize),
 	}
 
 	s.suppress.Store(true)
-	s.EnqueueCommit(nil, "should be dropped")
-
+	s.EnqueueCommit(nil, "held")
 	if len(s.commitCh) != 0 {
-		t.Fatal("expected commit to be dropped when suppress=true")
+		t.Fatal("expected the request to be held while suppress=true")
 	}
 
 	s.suppress.Store(false)
-	s.EnqueueCommit(nil, "should enqueue")
+	s.releaseHeld()
 	if len(s.commitCh) != 1 {
+		t.Fatal("expected the held request to enqueue once the import finished")
+	}
+	s.EnqueueCommit(nil, "direct")
+	if len(s.commitCh) != 2 {
 		t.Fatal("expected commit to enqueue when suppress=false")
 	}
 }
@@ -622,7 +626,7 @@ func TestStopFlushesPending(t *testing.T) {
 }
 
 // TestPullDoesNotRebound: a remote change pulled via SyncNow must not produce
-// a follow-up bot commit. Waits 2x suppressTail after the pull to be sure.
+// a follow-up bot commit. Waits out the sidecar write debounce after the pull.
 func TestPullDoesNotRebound(t *testing.T) {
 	appsDir := makeAppsDir(t)
 	bareDir := makeBareRemote(t)
@@ -676,8 +680,9 @@ func TestPullDoesNotRebound(t *testing.T) {
 		t.Fatalf("SyncNow: %v", err)
 	}
 
-	// Wait longer than suppressTail + debounceDelay to let any spurious commit fire.
-	time.Sleep(suppressTail + 700*time.Millisecond)
+	// Wait well past the 500ms sidecar write debounce to let any spurious
+	// commit fire.
+	time.Sleep(1500 * time.Millisecond)
 
 	// Drain any pending commit work.
 	drainCommits(t, s, 2*time.Second)