From 4006aa6e59f88570345a5d9ed7cf487c09e622e3 Mon Sep 17 00:00:00 2001 From: Edwin <18327273+thinkbig1979@users.noreply.github.com> Date: Fri, 31 Jul 2026 19:34:43 +0200 Subject: [PATCH] fix(backup): resolve the restic repository against the live DataDir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four handlers resolved their BackupConfig with services.ResolveBackupConfig(db), which 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). Every other code path used the service's real config and correctly produced /restic-repo. So repo init created a repository at /app/restic-repo and reported success, while every backup looked in /app/data/restic-repo and failed. Observed on a real container before the fix (DATA_DIR=/app/data, no restic_repository setting): POST /api/v1/backups/repo/init -> {"initialized":true} log: INFO Initialising restic repository component=restic-manager path=restic-repo ls /app/restic-repo -> a valid restic repository (WRONG location) ls /app/data/restic-repo -> did not exist GET /api/v1/settings/backup -> repository=/app/data/restic-repo, repositoryInitialized=false POST /api/v1/backups/run -> failed, item durationMs=76 Out of the box — no restic_repository setting, no RESTIC_REPOSITORY env — the UI "Initialise repository" flow could not produce a working backup. /app is also the container's writable layer, not the data volume, so the repository that WAS created is destroyed on every container recreation. The four sites were handlers/backup.go: repoInit (792), cloudTest (814), listSnapshotsViaRestic (1036), previewSnapshotViaRestic (1043). They also constructed their managers with services.NewResticManager / NewRcloneManager directly, bypassing BackupService's factory seam. That is why the defect was never caught: those paths could not be reached by a test with an injected runner at all. Both problems have one fix. BackupService gains ResolveConfig, NewResticManager and NewRcloneManager, which resolve against the live config and honour the factory seam; the handlers call those. The DataDir-less exported ResolveBackupConfig is REMOVED rather than corrected. Any resolver that does not take the live config can reintroduce this silently, so the type system now forbids it: callers go through the service, which cannot exist without a config.Config. A comment at the old site records why. Tests, written before the fix and confirmed failing against the old code for the right reason (the injected runner never saw the call at all): TestRepoInit_InitialisesRepositoryUnderDataDir TestSnapshotListing_ResolvesRepositoryUnderDataDir plus two resolver-level guards. Note that the pre-existing TestResolveBackupConfig_ExportedVariantReadsMergedConfig set restic_repository explicitly, so it never exercised the default branch — which is exactly how this survived. TestResolveBackupConfig_DefaultRepositoryIsAbsoluteUnderDataDir now covers it. Gates: go build, go vet clean; 667 tests pass in 9 packages (663 baseline + 4). Found while running the agent-os-9e1 backup drill, not by reading code. Refs: agent-os-9au Co-Authored-By: Claude Opus 5 --- backend/internal/handlers/backup.go | 13 +- backend/internal/handlers/backup_test.go | 182 ++++++++++++++++++ backend/internal/handlers/restic-repo/config | 2 + ...63785ae6cf24b41af4012833b609133a8bc25200a4 | 1 + backend/internal/services/backup.go | 27 +++ backend/internal/services/backup_config.go | 17 +- backend/internal/services/backup_test.go | 40 +++- 7 files changed, 270 insertions(+), 12 deletions(-) create mode 100644 backend/internal/handlers/restic-repo/config create mode 100644 backend/internal/handlers/restic-repo/keys/f9eb9a2f670acea8f6fd1063785ae6cf24b41af4012833b609133a8bc25200a4 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 // ============================================================