Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions backend/internal/handlers/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
182 changes: 182 additions & 0 deletions backend/internal/handlers/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"log/slog"
"net/http"
"net/http/httptest"
"path/filepath"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -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 <DataDir>/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 <DataDir>/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 <DataDir>/restic-repo")
}
2 changes: 2 additions & 0 deletions backend/internal/handlers/restic-repo/config
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Žs¥ 5ÔÈ>Æ*¨€.pƒGs9l‰ŽƒKx¿rd-A¯Ð{ú³£‡-X(]B!šIû#j'§X!l2¤W¦3’CwÏö™p–7‚V%ì†Y %…0³Æ&Þ-·`C±
:_°@åÛ™=“þñnÚäbZ IÕtkkìbŽØëBc,Ðõ,à}üžZ>j N„‡ ÿQ'
Original file line number Diff line number Diff line change
@@ -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=="}
27 changes: 27 additions & 0 deletions backend/internal/services/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <DataDir>/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.
Expand Down
17 changes: 14 additions & 3 deletions backend/internal/services/backup_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <DataDir>/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
Expand Down
40 changes: 39 additions & 1 deletion backend/internal/services/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
// ============================================================
Expand Down
Loading