diff --git a/controller/internal/sync/sync.go b/controller/internal/sync/sync.go index 97ccf4d..ace9db0 100644 --- a/controller/internal/sync/sync.go +++ b/controller/internal/sync/sync.go @@ -2,6 +2,7 @@ package sync import ( "bytes" + "context" "crypto/sha256" "encoding/hex" "fmt" @@ -18,6 +19,13 @@ import ( "gitea.dooplex.hu/admin/felhom-controller/internal/config" ) +// gitCmdTimeout bounds each individual git subprocess (campaign finding F3, 2026-07-06): +// without a deadline, a hung remote parks the sync goroutine inside cmd.Run() — the doSync +// defer never runs, `syncing` stays true, and every manual + periodic sync is refused with +// "Szinkronizálás már folyamatban" until a controller restart. 120s is generous for the +// shallow catalog clone/fetch. +const gitCmdTimeout = 120 * time.Second + // Syncer handles periodic git sync of the app catalog to the local stacks directory. type Syncer struct { cfg *config.Config @@ -270,10 +278,10 @@ func (s *Syncer) gitCloneOrPull() error { if s.isDebug() { s.logger.Printf("[DEBUG] [sync] git fetch --depth 1 origin %s in %s", s.cfg.Git.Branch, s.cacheDir) } - if err := s.runGitInDir(s.cacheDir, "fetch", "--depth", "1", "origin", s.cfg.Git.Branch); err != nil { + if err := s.gitCmd(s.cacheDir, "fetch", "--depth", "1", "origin", s.cfg.Git.Branch); err != nil { return fmt.Errorf("git fetch: %w", err) } - if err := s.runGitInDir(s.cacheDir, "reset", "--hard", "origin/"+s.cfg.Git.Branch); err != nil { + if err := s.gitCmd(s.cacheDir, "reset", "--hard", "origin/"+s.cfg.Git.Branch); err != nil { return fmt.Errorf("git reset: %w", err) } return nil @@ -414,14 +422,22 @@ func copyIfChanged(src, dst string) (bool, error) { return true, nil } -// runGit executes a git command with the given args. +// runGit executes a git command with the given args under the standard per-command deadline. func (s *Syncer) runGit(args ...string) error { - return s.runGitInDir("", args...) + return s.gitCmd("", args...) } -// runGitInDir executes a git command in the specified directory. -func (s *Syncer) runGitInDir(dir string, args ...string) error { - cmd := exec.Command("git", args...) +// gitCmd runs one git command in dir under a fresh gitCmdTimeout deadline (per-command, +// not per-sync — a multi-step sync gets a full budget for each subprocess). +func (s *Syncer) gitCmd(dir string, args ...string) error { + ctx, cancel := context.WithTimeout(context.Background(), gitCmdTimeout) + defer cancel() + return s.runGitInDir(ctx, dir, args...) +} + +// runGitInDir executes a git command in the specified directory, killed when ctx expires. +func (s *Syncer) runGitInDir(ctx context.Context, dir string, args ...string) error { + cmd := exec.CommandContext(ctx, "git", args...) if dir != "" { cmd.Dir = dir } @@ -433,7 +449,13 @@ func (s *Syncer) runGitInDir(dir string, args ...string) error { s.logger.Printf("[DEBUG] [sync] Running: git %s", maskRepoURL(strings.Join(args, " "))) if err := cmd.Run(); err != nil { - return fmt.Errorf("git %s: %w\nstderr: %s", strings.Join(args, " "), err, stderr.String()) + if ctxErr := ctx.Err(); ctxErr != nil { + // Subprocess was killed by the deadline/cancellation — name it explicitly so the + // failure is diagnosable from SyncResult/logs (never a silent hang). + return fmt.Errorf("git %s: %v (subprocess killed at the %s deadline): %w\nstderr: %s", + maskRepoURL(strings.Join(args, " ")), ctxErr, gitCmdTimeout, err, stderr.String()) + } + return fmt.Errorf("git %s: %w\nstderr: %s", maskRepoURL(strings.Join(args, " ")), err, stderr.String()) } return nil } diff --git a/controller/internal/sync/sync_test.go b/controller/internal/sync/sync_test.go new file mode 100644 index 0000000..583d92e --- /dev/null +++ b/controller/internal/sync/sync_test.go @@ -0,0 +1,105 @@ +package sync + +import ( + "context" + "log" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" +) + +func newTestSyncer(t *testing.T, repoURL string) *Syncer { + t.Helper() + dir := t.TempDir() + cfg := &config.Config{} + cfg.Git.RepoURL = repoURL + cfg.Git.Branch = "main" + cfg.Git.SyncInterval = "15m" + cfg.Paths.DataDir = filepath.Join(dir, "data") + cfg.Paths.StacksDir = filepath.Join(dir, "stacks") + if err := os.MkdirAll(cfg.Paths.StacksDir, 0o755); err != nil { + t.Fatal(err) + } + logger := log.New(os.Stderr, "", 0) + return New(cfg, logger, func() error { return nil }, nil) +} + +// T-C1 (campaign F3): a git subprocess must die with its context — the pre-fix +// exec.Command shape ignored the context entirely, so a hung remote parked the sync +// goroutine in cmd.Run() forever, holding `syncing=true` until a controller restart. +// Effect asserted: the returned error NAMES the context cause (not a generic git error), +// and the call returns promptly instead of running the command to completion. +func TestRunGitInDir_CancelledContextKillsSubprocess(t *testing.T) { + s := newTestSyncer(t, "https://example.invalid/repo.git") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // already cancelled — the subprocess must not be allowed to run to completion + + start := time.Now() + err := s.runGitInDir(ctx, "", "--version") + elapsed := time.Since(start) + + if err == nil { + t.Fatalf("expected an error from a cancelled context, got nil (git --version ran to completion — the pre-fix behavior)") + } + if !strings.Contains(err.Error(), context.Canceled.Error()) { + t.Fatalf("error must name the context cause; got: %v", err) + } + if !strings.Contains(err.Error(), "deadline") && !strings.Contains(err.Error(), "killed") { + t.Fatalf("error must say the subprocess was killed at the deadline; got: %v", err) + } + if elapsed > 5*time.Second { + t.Fatalf("cancelled-context git call took %v — subprocess not killed promptly", elapsed) + } +} + +// T-C2 (campaign F3): a failed sync must release the single-flight `syncing` flag and a +// later TriggerSync (past the debounce) must EXECUTE — the campaign symptom was every +// subsequent sync refused with "Szinkronizálás már folyamatban". Uses a nonexistent local +// repo_url so git fails fast with no network. +func TestTriggerSync_FailureReleasesSyncingAndAllowsRetry(t *testing.T) { + s := newTestSyncer(t, filepath.Join(t.TempDir(), "no-such-repo")) + + res := s.TriggerSync() + if res.OK { + t.Fatalf("sync against a nonexistent repo must fail; got OK with message %q", res.Message) + } + if !strings.Contains(res.Message, "Git hiba") { + t.Fatalf("failure must carry the git error to the caller (no silent swallow); got %q", res.Message) + } + + st := s.Status() + if st.Syncing { + t.Fatalf("`syncing` still true after a failed sync — the single-flight flag leaked (the F3 lockup)") + } + if st.LastStatus != "error" || st.LastError == "" { + t.Fatalf("failed sync must be recorded: LastStatus=%q LastError=%q", st.LastStatus, st.LastError) + } + + // Debounce window: an immediate retry is refused (existing behavior, unchanged). + res2 := s.TriggerSync() + if res2.OK || !strings.Contains(res2.Message, "Túl gyakori") { + t.Fatalf("immediate retry should hit the 30s debounce; got OK=%v message=%q", res2.OK, res2.Message) + } + + // Past the debounce (rewind lastSync directly — same-package seam), the retry must + // EXECUTE: it reaches git again and fails with the git error, not a refusal message. + s.mu.Lock() + s.lastSync = time.Now().Add(-time.Minute) + s.mu.Unlock() + + res3 := s.TriggerSync() + if res3.OK { + t.Fatalf("retry against the same nonexistent repo should still fail: %q", res3.Message) + } + if !strings.Contains(res3.Message, "Git hiba") { + t.Fatalf("retry past the debounce must EXECUTE (git error expected); got refusal/other: %q", res3.Message) + } + if s.Status().Syncing { + t.Fatal("`syncing` leaked after the retry") + } +}