diff --git a/backend/internal/handlers/backup.go b/backend/internal/handlers/backup.go index fc160c3..c26f793 100644 --- a/backend/internal/handlers/backup.go +++ b/backend/internal/handlers/backup.go @@ -789,8 +789,7 @@ func (h *BackupHandler) repoInit(c *gin.Context) { return } - bc := services.ResolveBackupConfig(h.db) - restic := services.NewResticManager(bc, h.logger) + restic := h.svc.NewResticManager() if err := restic.EnsureRepository(ctx); err != nil { h.internalError(c, "Failed to initialise repository", err) @@ -811,7 +810,7 @@ func (h *BackupHandler) cloudTest(c *gin.Context) { return } - bc := services.ResolveBackupConfig(h.db) + bc := h.svc.ResolveConfig() if bc.RcloneRemote == "" { c.JSON(http.StatusBadRequest, models.NewAppError( @@ -825,7 +824,7 @@ func (h *BackupHandler) cloudTest(c *gin.Context) { ctx, cancel := context.WithTimeout(c.Request.Context(), 30*time.Second) defer cancel() - rclone := services.NewRcloneManager(bc, h.logger) + rclone := h.svc.NewRcloneManager() if err := rclone.TestConnectivity(ctx, bc.RcloneRemote); err != nil { c.JSON(http.StatusOK, gin.H{"ok": false, "error": err.Error()}) return @@ -1033,15 +1032,13 @@ func (h *BackupHandler) internalError(c *gin.Context, msg string, err error) { // listSnapshotsViaRestic builds a ResticManager and returns all snapshots for // the given tag (empty = all). func (h *BackupHandler) listSnapshotsViaRestic(ctx context.Context, stackID string) ([]models.BackupSnapshot, error) { - bc := services.ResolveBackupConfig(h.db) - restic := services.NewResticManager(bc, h.logger) + restic := h.svc.NewResticManager() return restic.ListSnapshots(ctx, stackID, 0) } // previewSnapshotViaRestic runs restic ls and collects the output lines. func (h *BackupHandler) previewSnapshotViaRestic(ctx context.Context, snapshotID string) ([]string, error) { - bc := services.ResolveBackupConfig(h.db) - restic := services.NewResticManager(bc, h.logger) + restic := h.svc.NewResticManager() out := make(chan services.StreamLine, 256) done := make(chan error, 1) diff --git a/backend/internal/handlers/backup_test.go b/backend/internal/handlers/backup_test.go index d548d9a..b61d6aa 100644 --- a/backend/internal/handlers/backup_test.go +++ b/backend/internal/handlers/backup_test.go @@ -4,10 +4,13 @@ import ( "bytes" "context" "encoding/json" + "errors" "log/slog" "net/http" "net/http/httptest" + "path/filepath" "strings" + "sync" "testing" "time" @@ -1220,3 +1223,182 @@ func TestRegistry_PanicInExec_RunTerminatesAsFailed(t *testing.T) { assert.NotNil(t, dbRun.FinishedAt, "FinishedAt must be set after recoverExec finalises the run") } + +// ───────────────────────────────────────────── +// Repository path resolution (agent-os-9au) +// ───────────────────────────────────────────── + +// recordingResticRunner is a fake CommandRunner that records every invocation +// with the environment it was given, and fails `restic snapshots` so callers +// treat the repository as not yet initialised and proceed to `restic init`. +type recordingResticRunner struct { + mu sync.Mutex + calls []recordedResticCall + + // failRepoProbe makes `restic snapshots --quiet` fail, which is how + // BackupService.CheckRepository decides a repository is not reachable. + // Set it when the code under test should proceed to `restic init`; leave it + // false when the repository must look reachable (e.g. snapshot listing, + // which returns an empty list early if the probe fails). + failRepoProbe bool +} + +type recordedResticCall struct { + args []string + env []string +} + +func (r *recordingResticRunner) record(args, env []string) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls = append(r.calls, recordedResticCall{args: args, env: env}) +} + +// repoFor returns the RESTIC_REPOSITORY the runner was given for the first +// invocation whose first argument is subcommand and whose argument list +// contains every string in mustContain. ok is false if no such invocation was +// recorded. +// +// mustContain matters: BackupService.CheckRepository also shells out to +// `restic snapshots --quiet`, and it already resolves its config correctly. A +// test that matched on the subcommand alone would observe that call and pass +// regardless of the bug. Matching on `--json` pins the listing path instead. +func (r *recordingResticRunner) repoFor(subcommand string, mustContain ...string) (repo string, ok bool) { + r.mu.Lock() + defer r.mu.Unlock() + for _, c := range r.calls { + if len(c.args) == 0 || c.args[0] != subcommand { + continue + } + if !argsContainAll(c.args, mustContain) { + continue + } + for _, e := range c.env { + if strings.HasPrefix(e, "RESTIC_REPOSITORY=") { + return strings.TrimPrefix(e, "RESTIC_REPOSITORY="), true + } + } + return "", true + } + return "", false +} + +// argsContainAll reports whether args contains every string in want. +func argsContainAll(args, want []string) bool { + for _, w := range want { + found := false + for _, a := range args { + if a == w { + found = true + break + } + } + if !found { + return false + } + } + return true +} + +func (r *recordingResticRunner) Run( + _ context.Context, + _ string, + args []string, + env []string, + _ chan<- services.StreamLine, +) error { + r.record(args, env) + // BackupService.CheckRepository probes with `restic snapshots --quiet`. + // `snapshots --json` (the listing path) is a different invocation and must + // never be failed by this probe. + if r.failRepoProbe && argsContainAll(args, []string{"snapshots", "--quiet"}) { + return errors.New("repository does not exist") + } + return nil +} + +func (r *recordingResticRunner) Output( + _ context.Context, + _ string, + args []string, + env []string, +) ([]byte, error) { + r.record(args, env) + return []byte(`[]`), nil +} + +// TestRepoInit_InitialisesRepositoryUnderDataDir pins the fix for agent-os-9au. +// +// repoInit used to resolve its BackupConfig with services.ResolveBackupConfig(db), +// which passed an EMPTY &config.Config{}. With no DataDir the default repository +// became filepath.Join("", "restic-repo") — the RELATIVE path "restic-repo", +// resolved against the server's working directory. Every other code path used +// the service's config and correctly produced /restic-repo, so init +// created a repository somewhere the backups never looked, reported success, and +// every subsequent backup failed with "repository does not exist". +// +// Observed on a real container before the fix: init logged path=restic-repo and +// created /app/restic-repo, while GET /settings/backup reported +// /app/data/restic-repo and repositoryInitialized=false. +// +// This test fails against the old code for the right reason: repoInit built its +// own ResticManager with services.NewResticManager, bypassing the service +// factory entirely, so the injected runner never saw the `init` call at all. +func TestRepoInit_InitialisesRepositoryUnderDataDir(t *testing.T) { + // Not parallel — injects a manager factory on the service. + + db := newBackupHandlerDB(t) + require.NoError(t, db.SetSetting("restic_password", "test-repo-password")) + + svc := buildBackupSvc(t, db, true, false) + // The repository must look uninitialised so repoInit proceeds to `init`. + runner := &recordingResticRunner{failRepoProbe: true} + svc.SetResticMgrFactory(func(bc services.BackupConfig) *services.ResticManager { + return services.NewResticManagerForTest(bc, runner, slog.Default()) + }) + + h := NewBackupHandler(svc, db, slog.Default()) + r := newBackupRouter(h) + + w := httptest.NewRecorder() + r.ServeHTTP(w, jsonReq(t, http.MethodPost, "/api/backups/repo/init", map[string]interface{}{})) + + require.Equal(t, http.StatusOK, w.Code, "repo init should succeed against the injected runner") + + repo, ok := runner.repoFor("init") + require.True(t, ok, + "repoInit must build its ResticManager through the service factory; "+ + "constructing one directly bypasses both the injected runner and the live config") + + wantRepo := filepath.Join(svc.Config().DataDir, "restic-repo") + assert.Equal(t, wantRepo, repo, + "restic init must target /restic-repo, not a path relative to the process working directory") + assert.True(t, filepath.IsAbs(repo), "the resolved repository must be an absolute path") +} + +// TestSnapshotListing_ResolvesRepositoryUnderDataDir covers the same defect on +// the snapshot-listing path, which shared the DataDir-less resolver. +func TestSnapshotListing_ResolvesRepositoryUnderDataDir(t *testing.T) { + // Not parallel — injects a manager factory on the service. + + db := newBackupHandlerDB(t) + require.NoError(t, db.SetSetting("restic_password", "test-repo-password")) + + svc := buildBackupSvc(t, db, true, false) + runner := &recordingResticRunner{} + svc.SetResticMgrFactory(func(bc services.BackupConfig) *services.ResticManager { + return services.NewResticManagerForTest(bc, runner, slog.Default()) + }) + + h := NewBackupHandler(svc, db, slog.Default()) + r := newBackupRouter(h) + + w := httptest.NewRecorder() + r.ServeHTTP(w, jsonReq(t, http.MethodGet, "/api/backups/snapshots", nil)) + + repo, ok := runner.repoFor("snapshots", "--json") + require.True(t, ok, "snapshot listing must build its ResticManager through the service factory") + + assert.Equal(t, filepath.Join(svc.Config().DataDir, "restic-repo"), repo, + "restic snapshots must target /restic-repo") +} diff --git a/backend/internal/handlers/restic-repo/config b/backend/internal/handlers/restic-repo/config new file mode 100644 index 0000000..d4388e3 --- /dev/null +++ b/backend/internal/handlers/restic-repo/config @@ -0,0 +1,2 @@ +s 5>*.pGs9lKxrd-A{-X(]B!I#j'X!l2W3Cwp7V%Y %0&-`C +:_@ۙ=nbZ ItkkbBc,,}Z>j N Q' \ No newline at end of file diff --git a/backend/internal/handlers/restic-repo/keys/f9eb9a2f670acea8f6fd1063785ae6cf24b41af4012833b609133a8bc25200a4 b/backend/internal/handlers/restic-repo/keys/f9eb9a2f670acea8f6fd1063785ae6cf24b41af4012833b609133a8bc25200a4 new file mode 100644 index 0000000..f2c89ea --- /dev/null +++ b/backend/internal/handlers/restic-repo/keys/f9eb9a2f670acea8f6fd1063785ae6cf24b41af4012833b609133a8bc25200a4 @@ -0,0 +1 @@ +{"created":"2026-07-31T19:30:44.901279811+02:00","username":"edwin","hostname":"HP-Elitebook-G8-Linux","kdf":"scrypt","N":32768,"r":8,"p":5,"salt":"51/2EsIPp3r958K9RQ5WgtOzXTIDsptB3RBjlqx/CSGuPzxPPmN0u5m/9EBVlR4CO2A5Or0h2BfQybfhzyK7Lw==","data":"yzhqGkLQTJHij/c1N4IvDYUUdvk7gVKvTlLSTmpw7mfFuaUca+yZZcfjRKtIgbZkf+zCUivLyCoSoQGEBXXPJuV/T9J0YZ+uS2Zue2tyDHlQnFiZhAxTS35i8nBSKivU2VaCk69ah2RyDNR3mpAk3ORC6Ooob2E85N1AYVFLT/12tzY56i43BZQiWIASONhNP03skQw7FcMxZqU9z4hb2Q=="} \ No newline at end of file diff --git a/backend/internal/services/backup.go b/backend/internal/services/backup.go index c1f12c7..baa6c72 100644 --- a/backend/internal/services/backup.go +++ b/backend/internal/services/backup.go @@ -150,6 +150,33 @@ func (s *BackupService) Config() *config.Config { return s.cfg } +// ResolveConfig merges DB settings over the live config.Config and returns the +// effective backup configuration. +// +// Callers outside this package MUST use this rather than resolving on their own. +// The previous exported helper took only a *database.DB and filled in an empty +// config.Config, so cfg.DataDir was "" and the default repository resolved to +// the RELATIVE path "restic-repo" instead of /restic-repo — see +// agent-os-9au. Routing every caller through the service makes that class of +// mistake unrepresentable: there is no longer a way to resolve without the live +// config. +func (s *BackupService) ResolveConfig() BackupConfig { + return resolveBackupConfig(s.db, s.cfg) +} + +// NewResticManager builds a ResticManager from the effective backup config, +// honouring the test factory seam. Handlers use this instead of constructing a +// manager directly, which bypassed both the live config and the seam. +func (s *BackupService) NewResticManager() *ResticManager { + return s.newResticMgr(s.ResolveConfig()) +} + +// NewRcloneManager builds an RcloneManager from the effective backup config, +// honouring the test factory seam. See NewResticManager. +func (s *BackupService) NewRcloneManager() *RcloneManager { + return s.newRcloneMgr(s.ResolveConfig()) +} + // SetBins overrides the cached binary paths resolved at construction time. // This is used by tests (including tests in external packages) to control // availability without requiring real restic/rclone binaries on the host. diff --git a/backend/internal/services/backup_config.go b/backend/internal/services/backup_config.go index 24fa036..0dd4fc2 100644 --- a/backend/internal/services/backup_config.go +++ b/backend/internal/services/backup_config.go @@ -129,9 +129,20 @@ func resolveBackupConfig(db *database.DB, cfg *config.Config) BackupConfig { // ResolveBackupConfig is the exported variant of resolveBackupConfig for use // by the BackupHandler. It builds the effective BackupConfig from DB settings, // using an empty Config struct as the env-var fallback layer (DB values win). -func ResolveBackupConfig(db *database.DB) BackupConfig { - return resolveBackupConfig(db, &config.Config{}) -} +// REMOVED (agent-os-9au): ResolveBackupConfig(db *database.DB). +// +// It called resolveBackupConfig(db, &config.Config{}) — an EMPTY config. With +// cfg.DataDir == "" the default repository became filepath.Join("", "restic-repo"), +// i.e. the RELATIVE path "restic-repo", resolved against the server's working +// directory (/app). Four handlers used it, so repo init, cloud test, snapshot +// listing and snapshot preview all pointed at /app/restic-repo while every +// other path correctly used /restic-repo. Init reported success and +// backups then failed with "repository does not exist". +// +// Deliberately not replaced with a fixed version: any resolver that does not +// take the live config can reintroduce the same defect silently. Callers go +// through BackupService.ResolveConfig / NewResticManager / NewRcloneManager, +// which cannot be constructed without a config.Config. // ResolveBackupConfigWithCfg is like ResolveBackupConfig but accepts a // config.Config so env-var fallbacks are applied. Use this when the caller diff --git a/backend/internal/services/backup_test.go b/backend/internal/services/backup_test.go index 493bb0a..7979cbe 100644 --- a/backend/internal/services/backup_test.go +++ b/backend/internal/services/backup_test.go @@ -1660,10 +1660,48 @@ func TestResolveBackupConfig_ExportedVariantReadsMergedConfig(t *testing.T) { db := newBackupTestDB(t) require.NoError(t, db.SetSetting("restic_repository", "/exported/repo")) - bc := ResolveBackupConfig(db) + bc := ResolveBackupConfigWithCfg(db, &config.Config{DataDir: t.TempDir()}) assert.Equal(t, "/exported/repo", bc.ResticRepository) } +// TestResolveBackupConfig_DefaultRepositoryIsAbsoluteUnderDataDir is the +// regression guard for agent-os-9au. +// +// The test above sets restic_repository explicitly, so it never exercised the +// default branch — which is precisely why the bug survived. When neither the DB +// nor the environment supplies a repository, the default is computed as +// filepath.Join(cfg.DataDir, "restic-repo"). Resolving with an empty +// config.Config made that filepath.Join("", "restic-repo") == "restic-repo", a +// RELATIVE path resolved against the server's working directory. +func TestResolveBackupConfig_DefaultRepositoryIsAbsoluteUnderDataDir(t *testing.T) { + t.Parallel() + + db := newBackupTestDB(t) + dataDir := t.TempDir() + + bc := ResolveBackupConfigWithCfg(db, &config.Config{DataDir: dataDir}) + + assert.Equal(t, filepath.Join(dataDir, "restic-repo"), bc.ResticRepository, + "the default repository must sit under DataDir") + assert.True(t, filepath.IsAbs(bc.ResticRepository), + "the default repository must be absolute; a relative path resolves against the "+ + "server's working directory and lands outside the data volume") +} + +// TestBackupService_ResolveConfig_UsesLiveDataDir pins the replacement entry +// point. Callers outside this package can only resolve through the service, so +// there is no longer a way to resolve without the live config. +func TestBackupService_ResolveConfig_UsesLiveDataDir(t *testing.T) { + t.Parallel() + + db := newBackupTestDB(t) + dataDir := t.TempDir() + cfg := &config.Config{DataDir: dataDir} + svc := NewBackupService(cfg, db, nil, NewOperationLock(), NewActionLogger(db)) + + assert.Equal(t, filepath.Join(dataDir, "restic-repo"), svc.ResolveConfig().ResticRepository) +} + // ============================================================ // multiCallRunner — helper for multi-step test scenarios // ============================================================