fix(backup): resolve the restic repository against the live DataDir (agent-os-9au) - #33
Merged
Merged
Conversation
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
<DataDir>/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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four handlers resolved their
BackupConfigwithservices.ResolveBackupConfig(db), which calledresolveBackupConfig(db, &config.Config{})— an empty config. Withcfg.DataDir == ""the default repository becamefilepath.Join("", "restic-repo"), i.e. the relative pathrestic-repo, resolved against the server's working directory (/app).Every other code path used the service's real config and correctly produced
<DataDir>/restic-repo. So init created a repository at/app/restic-repoand reported success, while every backup looked in/app/data/restic-repoand failed.Observed before the fix
Real container,
DATA_DIR=/app/data, norestic_repositorysetting:Out of the box — no
restic_repositorysetting, noRESTIC_REPOSITORYenv — the UI "Initialise repository" flow could not produce a working backup./appis also the container's writable layer rather than the data volume, so the repository that was created is destroyed on every container recreation.Why it was never caught
The four sites —
repoInit,cloudTest,listSnapshotsViaRestic,previewSnapshotViaRestic— also constructed their managers withservices.NewResticManager/NewRcloneManagerdirectly, bypassingBackupService's factory seam. A test with an injected runner could not reach those paths at all.The pre-existing
TestResolveBackupConfig_ExportedVariantReadsMergedConfigsetrestic_repositoryexplicitly, so it never exercised the default branch. That is precisely how this survived.Fix
Both problems have one fix.
BackupServicegainsResolveConfig,NewResticManagerandNewRcloneManager, which resolve against the live config and honour the factory seam. The handlers call those.The DataDir-less exported
ResolveBackupConfigis removed rather than corrected — any resolver that does not take the live config can reintroduce this silently, so the type system now forbids it. 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):
Plus two resolver-level guards, including one for the default branch the old test skipped.
Gates
go buildandgo vetclean; 667 tests pass in 9 packages (663 baseline + 4 added).Found while running the
agent-os-9e1backup drill, not by reading code. It also clears the way foragent-os-36o's DR drill, which starts from a fresh install and would otherwise hit this immediately.Closes
agent-os-9au.