From b8ce240de678a5460c0065037de4520e4ecaf736 Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:39:40 -0700 Subject: [PATCH] fix(gitsync): tell a pulled file's re-render from a local edit by content, not a time window Every write within 1.2s of a pull import was treated as SimpleDeploy re-rendering the pulled file: a dashboard edit in that window was not committed or pushed, and on a later conflict the remote version won. Right after an import git sync now records the SHA-256 of each imported sidecar as rendered from the DB; only a write with exactly those bytes counts as the re-render, whenever it lands. Writes during the import itself are held and classified when it finishes. Also: ApplyPending runs on the worker goroutine and Status reads recent commits through its own repository handle, since a go-git handle is not safe for concurrent use (race detector). --- internal/configsync/configsync.go | 26 ++++++- internal/configsync/configsync_test.go | 13 +++- internal/gitsync/gitsync.go | 94 ++++++++++++++++++------- internal/gitsync/gitsync_test.go | 21 +++--- internal/gitsync/pending.go | 95 +++++++++++++++++++++++--- internal/gitsync/pullonly_test.go | 59 ++++++++++++++-- 6 files changed, 256 insertions(+), 52 deletions(-) diff --git a/internal/configsync/configsync.go b/internal/configsync/configsync.go index 25b18fee..47e1afa5 100644 --- a/internal/configsync/configsync.go +++ b/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 diff --git a/internal/configsync/configsync_test.go b/internal/configsync/configsync_test.go index e8cfa988..3c3083f1 100644 --- a/internal/configsync/configsync_test.go +++ b/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) diff --git a/internal/gitsync/gitsync.go b/internal/gitsync/gitsync.go index 533229f7..295b7570 100644 --- a/internal/gitsync/gitsync.go +++ b/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,6 +884,8 @@ 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") @@ -873,6 +893,22 @@ func (g *Syncer) ApplyPending(ctx context.Context) error { 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. diff --git a/internal/gitsync/gitsync_test.go b/internal/gitsync/gitsync_test.go index a4764ee0..2c68c23f 100644 --- a/internal/gitsync/gitsync_test.go +++ b/internal/gitsync/gitsync_test.go @@ -419,7 +419,8 @@ 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}, @@ -427,15 +428,18 @@ func TestSuppressDuringImport(t *testing.T) { } 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) diff --git a/internal/gitsync/pending.go b/internal/gitsync/pending.go index 32f3781e..f3e97c00 100644 --- a/internal/gitsync/pending.go +++ b/internal/gitsync/pending.go @@ -10,22 +10,29 @@ package gitsync // them into local bot commits so the rebase can run and the server-wins // conflict policy decides what is kept. // -// Files SimpleDeploy writes while importing a pull (the sidecar write hook -// fires while suppress is on, e.g. a pulled sidecar re-rendered from the -// DB) carry no local edit. They are committed separately under -// pulledRewriteSubject, and on conflict the rebase takes the remote side for -// them, as it does for an access restore (see replayingPullFollowUp). -// Otherwise such a commit, which stays unpushed in pull-only mode, would -// override every later remote edit to that file. The set is kept in memory -// only: after a restart such files are committed as ordinary local changes. +// A pulled sidecar that SimpleDeploy re-renders from the DB after importing +// it (e.g. a hand-formatted simpledeploy.yml saved back in SimpleDeploy's +// format) carries no local edit. Right after an import, git sync records the +// SHA-256 of that render; a write with exactly those bytes is the re-render, +// whenever it lands, and any other write is a local edit (skipPullRewrites). +// Re-renders are committed separately under pulledRewriteSubject, and on +// conflict the rebase takes the remote side for them, as it does for an +// access restore (see replayingPullFollowUp). Otherwise such a commit, which +// stays unpushed in pull-only mode, would override every later remote edit +// to that file. The records are kept in memory only: after a restart such +// files are committed as ordinary local changes. import ( "bytes" + "crypto/sha256" + "encoding/hex" "fmt" "log" "os" "path/filepath" "strings" + + "github.com/vazra/simpledeploy/internal/fsutil" ) const ( @@ -195,8 +202,80 @@ func (g *Syncer) forgetPullRewrites(paths []string) { for _, p := range paths { if rel, ok := g.repoRel(p); ok { delete(g.pullRewrites, rel) + delete(g.rewriteHash, rel) + } + } +} + +// recordPullRenders records, for each app whose sidecar a pull just +// imported, the SHA-256 of that sidecar as SimpleDeploy renders it from the +// DB now. +func (g *Syncer) recordPullRenders(slugs []string) { + if g.cs == nil { + return + } + for _, slug := range slugs { + b, err := g.cs.RenderAppSidecar(slug) + if err != nil { + log.Printf("[gitsync] render pulled sidecar %s: %v", slug, err) + continue + } + sum := sha256.Sum256(b) + g.mu.Lock() + if g.rewriteHash == nil { + g.rewriteHash = map[string]string{} + } + g.rewriteHash[slug+"/simpledeploy.yml"] = hex.EncodeToString(sum[:]) + g.mu.Unlock() + } +} + +// releaseHeld classifies the commit requests made during an import. +func (g *Syncer) releaseHeld() { + g.mu.Lock() + held := g.held + g.held = nil + g.mu.Unlock() + for _, r := range held { + g.EnqueueCommit(r.paths, r.reason) + } +} + +// skipPullRewrites records the paths whose file holds exactly SimpleDeploy's +// render of a pulled sidecar as pull rewrites and returns the others, which +// are local changes. ok is false when nothing is left to commit. nil (stage +// everything) passes through: staging leaves pull rewrites out. +func (g *Syncer) skipPullRewrites(paths []string) ([]string, bool) { + if paths == nil { + return nil, true + } + var keep []string + for _, p := range paths { + if rel, ok := g.repoRel(p); ok && g.matchesPullRender(rel) { + g.notePullRewrites([]string{p}) + continue } + g.forgetPullRewrites([]string{p}) + keep = append(keep, p) + } + return keep, len(keep) > 0 +} + +// matchesPullRender reports whether the file at rel is byte for byte the +// render recorded by recordPullRenders. +func (g *Syncer) matchesPullRender(rel string) bool { + g.mu.Lock() + want, ok := g.rewriteHash[rel] + g.mu.Unlock() + if !ok { + return false + } + b, err := fsutil.ReadRegularFile(filepath.Join(g.cfg.AppsDir, filepath.FromSlash(rel))) + if err != nil { + return false } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]) == want } func (g *Syncer) isPullRewrite(rel string) bool { diff --git a/internal/gitsync/pullonly_test.go b/internal/gitsync/pullonly_test.go index 2fb089a7..c2a1ccb2 100644 --- a/internal/gitsync/pullonly_test.go +++ b/internal/gitsync/pullonly_test.go @@ -263,9 +263,9 @@ func handFormattedAliceSidecar(threshold string) string { // TestPullOnlyRewriteOfPulledSidecarYieldsToRemote: after a pull imports a // hand-written sidecar, the server re-renders it from the DB (store mutation -// hook -> debounced sidecar write) while the import is suppressed. That -// rewrite is committed locally before the next rebase, but it must not win -// over later remote edits to the same file. +// hook -> debounced sidecar write), byte for byte as recorded after the +// import. That rewrite is committed locally before the next rebase, but it +// must not win over later remote edits to the same file. func TestPullOnlyRewriteOfPulledSidecarYieldsToRemote(t *testing.T) { var app *store.App e := newSecEnv(t, func(e *secEnv) { app = seedAliceApp(t, e) }, pullOnly) @@ -307,13 +307,13 @@ func TestPullOnlyRewriteOfPulledSidecarYieldsToRemote(t *testing.T) { b, _ := os.ReadFile(sidecar) return !strings.Contains(string(b), "# app1 settings") }) - waitFor(t, 5*time.Second, "import suppress window to end", func() bool { return !e.gs.suppress.Load() }) + // The write hook records the rewrite just after the file lands. + waitFor(t, 5*time.Second, "re-render recorded as a rewrite of pulled config", func() bool { + return e.gs.isPullRewrite("app1/simpledeploy.yml") + }) if got := readFile(t, sidecar); !strings.Contains(got, "threshold: "+want) { t.Errorf("re-rendered sidecar lost threshold %s:\n%s", want, got) } - if !e.gs.isPullRewrite("app1/simpledeploy.yml") { - t.Error("re-rendered sidecar not recorded as a rewrite of pulled config") - } if hasConflict(e.gs, "app1/simpledeploy.yml") { t.Errorf("rewrite of pulled config reported as a conflict: %+v", e.gs.Status().RecentConflicts) } @@ -467,3 +467,48 @@ func TestPullOnlyLocalDeletionWinsOverRemoteEdit(t *testing.T) { t.Errorf("pull-only mode pushed: remote main %s, want %s", got, remote) } } + +// A dashboard edit made right after a pull is applied is a local change, not +// a re-render of the pulled file: it is committed and pushed like any other +// edit (it used to be swallowed by a time window after each import). +func TestDashboardEditRightAfterPullIsPushed(t *testing.T) { + var app *store.App + e := newSecEnv(t, func(e *secEnv) { app = seedAliceApp(t, e) }, nil) + e.st.SetMutationHook(func(scope store.MutationScope, slug string) { + if scope == store.ScopeApp && slug != "" { + e.cs.ScheduleAppWrite(slug) + } + }) + t.Cleanup(func() { e.st.SetMutationHook(nil) }) + e.cs.SetSidecarWriteHook(func(path, reason string) { + if path == "" { + e.gs.EnqueueCommit(nil, reason) + return + } + e.gs.EnqueueCommit([]string{path}, reason) + }) + + pushFromClone(t, e.bareDir, func(dir string) { + writeFile(t, filepath.Join(dir, "app1", "simpledeploy.yml"), handFormattedAliceSidecar("90")) + }) + if err := e.gs.SyncNow(e.ctx); err != nil { + t.Fatalf("SyncNow: %v", err) + } + rules, err := e.st.ListAlertRules(&app.ID) + if err != nil || len(rules) != 1 || rules[0].Threshold != 90 { + t.Fatalf("alert rules after pull = %+v (err %v)", rules, err) + } + + // Dashboard edit while the import's own re-render is still pending. + rules[0].Threshold = 42 + if err := e.st.UpdateAlertRule(&rules[0]); err != nil { + t.Fatal(err) + } + + waitFor(t, 10*time.Second, "dashboard edit pushed to the remote", func() bool { + return strings.Contains(gitOut(t, e.bareDir, "show", "main:app1/simpledeploy.yml"), "threshold: 42") + }) + if e.gs.isPullRewrite("app1/simpledeploy.yml") { + t.Error("dashboard edit recorded as a rewrite of pulled config") + } +}