admin/deploy-felhom-compose
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix data loss on container restart (v0.15.2)

Browse Source
This commit is contained in:
authentik Default Admin
2026-02-19 08:41:59 +01:00
parent 2befa6877b
commit ea7105d468
1 changed files with 89 additions and 302 deletions
Download Patch File Download Diff File Expand all files Collapse all files
TASK.md
+89 -302
View File
@@ -1,339 +1,126 @@
# TASK.md — Bug Fixes from v0.12.4–v0.13.1 Code Review
# TASK: Fix data loss on container restart (v0.15.2)
**Version:** v0.14.1 → v0.14.2
**Type:** Bug fixes identified by thorough code review of today's changes (sessions 40–48)
**Scope:** Controller Go code — `internal/backup/`, `internal/web/`
Two pieces of data are lost when the felhom-controller container restarts:
1. Snapshot history delta stats (HOZZÁADOTT, ÚJ FÁJL, VÁLTOZOTT columns show 0)
2. DB validation info (ÉRVÉNYESÍTÉS column shows "–" instead of table counts)
---
## Bug 1: Snapshot history delta stats lost on restart
## Bug Summary
### Root cause
| # | Severity | File | Description |
|---|----------|------|-------------|
| 1 | **HIGH** | `crossdrive.go` | rsync `--delete` destroys `_db/` and `_config/` directories on every single-mount run |
| 2 | **MEDIUM** | `backup.go` | Scheduled backups don't set `m.running` flag — restore/manual backup can overlap |
| 3 | **MEDIUM** | `crossdrive.go` | `ValidateDestination` silently succeeds when disk usage can't be read |
| 4 | **MEDIUM** | `backup.go` | Empty `systemDataPath` silently produces relative dump paths |
`LoadSnapshotHistory()` in `controller/internal/backup/backup.go` (~line 788) loads snapshots from restic repos on startup, but sets `HasStats: false` because restic's `snapshots` command only returns ID/time/paths/tags — NOT the delta stats (files_new, files_changed, data_added) which are only available in the `restic backup` output during the actual backup run.
---
## Bug 1 (HIGH): rsync `--delete` destroys `_db/` and `_config/` directories
### File
`internal/backup/crossdrive.go`, function `runRsyncBackup`, lines 242–276
### Root Cause
When an app has exactly one HDD mount (`len(mounts) == 1`), rsync copies directly into `destDir/` with the `--delete` flag. The `_db/` and `_config/` subdirectories — created by the DB dump copy (lines 278–286) and config rsync (lines 288–303) from the **previous** backup run — don't exist in the source mount, so `--delete` removes them from the destination **before** the current run re-creates them.
### Impact
1. **Brief window of incomplete backup** on every run — between rsync completion and the subsequent `_db`/`_config` copy steps, the backup destination is missing DB dumps and config
2. **Data loss if interrupted** — if the process is killed (OOM, power loss, context cancellation) between the rsync step and the copy steps, `_db/` and `_config/` are gone until the next successful complete run
3. **Wasted I/O** — every run deletes and re-creates these directories unnecessarily
### Reproduction
1. Deploy Immich with a single HDD mount to `/mnt/hdd_1/storage/immich`
2. Configure cross-drive rsync backup to `/mnt/sys_drive`
3. Run backup twice
4. After first run: `ls /mnt/sys_drive/backups/secondary/immich/rsync/` shows `_db/`, `_config/`, and app data
5. During second run's rsync phase: `_db/` and `_config/` disappear from dest
6. After second run completes: `_db/` and `_config/` reappear (re-created)
### Fix Instructions
In `runRsyncBackup()`, add an `--exclude` flag with pattern `_*` to the rsync command. This prevents `--delete` from touching any controller-managed directories (which all use underscore prefix: `_db`, `_config`, and any future ones) while still cleaning up stale user data.
**Change the rsync command construction (around line 267) from:**
```go
cmd := exec.CommandContext(ctx, "rsync", "-a", "--delete",
"--exclude", "backups/*.sql.gz",
"--exclude", "backups/*.sql",
"--exclude", "backups/*.dump",
src, dst)
The template in `controller/internal/web/templates/backups.html` renders 0 for `HasStats: false`:
```html
<td class="mono">{{if .HasStats}}+{{.DataAdded}}{{else}}0{{end}}</td>
```
**To:**
```go
cmd := exec.CommandContext(ctx, "rsync", "-a", "--delete",
"--exclude", "_*",
"--exclude", "backups/*.sql.gz",
"--exclude", "backups/*.sql",
"--exclude", "backups/*.dump",
src, dst)
```
### Fix
**Why `_*` instead of listing `_db` and `_config` individually:**
- Future-proof — if we ever add another controller-managed subdirectory (e.g., `_metadata`), it's automatically protected
- The underscore prefix is a convention for controller-managed dirs; no real app data directory starts with `_`
- Simpler to maintain (one pattern vs growing list)
Persist the `snapshotHistory` ring buffer to a JSON file on disk so delta stats survive restarts.
**Why this is safe:** The `_db` and `_config` directories are managed exclusively by the controller (lines 278–303), never by the user's app. No user data directory starts with `_`. The underscore prefix convention was specifically chosen to avoid collision with app data paths.
**Implementation steps:**
**Note on `_infra`:** The `_infra/` directory lives at `<dest>/backups/secondary/_infra/` (a sibling of app directories, NOT inside any app's rsync target directory). So it's not affected by `--delete` regardless. The `_*` exclude is still good practice in case the directory structure changes later.
1. **Add persistence methods to Manager** in `controller/internal/backup/backup.go`:
- Add a `snapshotHistoryFile` field to `Manager` struct (set to e.g. `<dataPath>/snapshot-history.json`)
- Add `saveSnapshotHistory()` — marshals `snapshotHistory` to JSON, writes atomically (write to .tmp, rename)
- Add `loadSnapshotHistory()` — reads JSON file, unmarshals into `snapshotHistory`
- Call `saveSnapshotHistory()` at the end of `appendSnapshotRecord()` (called after each backup)
**For the multi-mount case** (`len(mounts) > 1`): rsync goes into per-leaf subdirectories, not the root `destDir`, so `_db/` and `_config/` are siblings (not children) of the rsync targets. The `--delete` flag only affects the target subtree, so they're safe in the multi-mount case. Adding the exclude universally doesn't hurt.
2. **Update `LoadSnapshotHistory()`** in same file:
- First try `loadSnapshotHistory()` from the JSON file
- If the file exists and loads successfully, merge with restic repo snapshots:
- Build a map of persisted records by SnapshotID
- For any restic snapshot NOT in the persisted map, add it with `HasStats: false`
- This preserves delta stats for recent snapshots while still picking up very old ones from restic
- If the file doesn't exist (first run), fall back to the current restic-only loading
---
3. **Wire up the file path** in `NewManager()`:
- The data path can be derived from `cfg.Paths.SystemDataPath` or use a fixed path like `/opt/docker/felhom-controller/data/snapshot-history.json`
- The `data/` directory is a Docker volume, so it persists across container recreations
- Check what `config.Config` has available. See `controller/internal/config/config.go` for the config struct. The controller's data directory is typically `/opt/docker/felhom-controller/data/` which is a mounted volume (check `cmd/controller/main.go` for the data path).
## Bug 2 (MEDIUM): Scheduled backups don't set `m.running` flag
**Key file paths to check:**
- `controller/internal/backup/backup.go` — Manager struct, LoadSnapshotHistory, appendSnapshotRecord
- `controller/internal/config/config.go` — Config struct for data paths
- `controller/cmd/controller/main.go` — where NewManager is called, what paths are available
### File
`internal/backup/backup.go`, functions `RunBackup()` (line 272) and `RunDBDumps()` (line 181)
## Bug 2: DB validation missing after restart
### Root Cause
Only `RunFullBackup()` (line 454) sets `m.running = true` before running. The scheduler in `main.go` calls `RunBackup()` and `RunDBDumps()` directly (not via `RunFullBackup()`), so `m.running` is never set during nightly scheduled runs.
### Root cause
Code that checks `m.running`:
- `RestoreApp()` in `restore.go:25` — blocks if running
- `RunFullBackup()` in `backup.go:456` — blocks if running
- `IsRunning()` in `backup.go:496` — used by API and UI to show status
### Impact
1. **UI shows "not running" during nightly backups** — the dashboard backup card and backup page don't indicate that a backup is in progress
2. **Restore can overlap with nightly backup** — a user triggering "Restore" at 03:05 while the nightly restic backup is running won't see a "backup in progress" error. Both operations compete for the restic repo lock; one will fail with an opaque error
3. **Manual "Backup now" can overlap with scheduled backup** — `RunFullBackup()` checks `m.running`, but since the scheduled `RunBackup()` doesn't set it, the manual trigger proceeds and both run concurrently
### Fix Instructions
Add `m.running` guard to both `RunBackup()` and `RunDBDumps()`. The pattern is the same as in `RunFullBackup()`:
**In `RunBackup()` (line 272), add at the beginning of the function:**
```go
func (m *Manager) RunBackup(ctx context.Context) error {
m.mu.Lock()
if m.running {
m.mu.Unlock()
return fmt.Errorf("backup already in progress")
}
m.running = true
m.mu.Unlock()
defer func() {
m.mu.Lock()
m.running = false
m.mu.Unlock()
}()
start := time.Now()
// ... rest of existing function
```
**In `RunDBDumps()` (line 181), add the same guard:**
```go
func (m *Manager) RunDBDumps(ctx context.Context) error {
m.mu.Lock()
if m.running {
m.mu.Unlock()
return fmt.Errorf("backup already in progress")
}
m.running = true
m.mu.Unlock()
defer func() {
m.mu.Lock()
m.running = false
m.mu.Unlock()
}()
start := time.Now()
// ... rest of existing function
```
**IMPORTANT:** `RunFullBackup()` calls `RunDBDumps()` then `RunBackup()` internally. After adding the guards above, `RunFullBackup()` would deadlock on itself (it sets `m.running=true`, then calls `RunDBDumps()` which also tries to set it). Fix by either:
**Option A (recommended):** Extract the logic into internal methods that DON'T check the flag:
```go
// Public methods — set the running guard
func (m *Manager) RunDBDumps(ctx context.Context) error {
if err := m.acquireRunning(); err != nil {
return err
}
defer m.releaseRunning()
return m.runDBDumpsInternal(ctx)
}
func (m *Manager) RunBackup(ctx context.Context) error {
if err := m.acquireRunning(); err != nil {
return err
}
defer m.releaseRunning()
return m.runBackupInternal(ctx)
}
func (m *Manager) RunFullBackup(ctx context.Context) error {
if err := m.acquireRunning(); err != nil {
return err
}
defer m.releaseRunning()
if err := m.runDBDumpsInternal(ctx); err != nil {
m.logger.Printf("[WARN] DB dump had errors, continuing with backup anyway")
}
return m.runBackupInternal(ctx)
}
// Helper methods
func (m *Manager) acquireRunning() error {
m.mu.Lock()
defer m.mu.Unlock()
if m.running {
return fmt.Errorf("backup already in progress")
}
m.running = true
return nil
}
func (m *Manager) releaseRunning() {
m.mu.Lock()
m.running = false
m.mu.Unlock()
}
// Internal methods — no guard, caller must hold running flag
func (m *Manager) runDBDumpsInternal(ctx context.Context) error {
// ... current RunDBDumps body (without the running guard)
}
func (m *Manager) runBackupInternal(ctx context.Context) error {
// ... current RunBackup body (without the running guard)
}
```
**Option B (simpler):** Only add the guard to `RunBackup()` and `RunDBDumps()`, but make `RunFullBackup()` NOT call them — instead inline the logic. This duplicates code but avoids the refactor.
**Option A is recommended** — it's cleaner and ensures all entry points are guarded.
---
## Bug 3 (MEDIUM): `ValidateDestination` silently succeeds when disk usage can't be read
### File
`internal/backup/crossdrive.go`, function `ValidateDestination`, line 215
### Root Cause
```go
if di := system.GetDiskUsage(path); di != nil {
// Space checks only execute if di != nil
...
}
return nil // ← Success even when di was nil (space unknown)
```
If `system.GetDiskUsage(path)` returns `nil` (e.g., unsupported filesystem like FUSE/NFS, permission issue, or a path on a virtual filesystem), all space checks are skipped and validation passes without any space verification.
### Impact
A destination with zero free space could pass validation, leading to a backup failure mid-operation. The rsync or restic process would fail with "no space left on device", but only after partial work, leaving an incomplete backup.
### Fix Instructions
After the `GetDiskUsage` check, add a warning log and optionally block:
In `GetFullStatus()` in `controller/internal/backup/backup.go` (~line 973-992), when `LastDBDump` is nil (always nil after restart), a synthetic `LastDBDump` is created from `DumpFiles`. But the synthesized `DumpResult` does NOT copy the `Validation` field from `DumpFileInfo`:
```go
di := system.GetDiskUsage(path)
if di == nil {
r.logger.Printf("[WARN] Cannot determine disk usage for %s — proceeding without space verification", path)
return nil
}
// Current code — BUG: missing Validation
results = append(results, DumpResult{
DB: DiscoveredDB{StackName: f.StackName, DBType: f.DBType, ContainerName: f.StackName},
FilePath: f.FileName,
Size: f.Size,
})
```
This preserves backward compatibility (doesn't block) but makes the situation visible in logs. If you want to be stricter, return an error instead:
The `DumpResult` struct has a `Validation DumpValidation` field (see `controller/internal/backup/dbdump.go:41`), but it's left zero-valued (`Valid: false, Error: ""`). The template then renders "–" for this case.
Meanwhile, `ListDumpFiles()` in `dbdump.go:342` DOES call `ValidateDump()` on each file and populates `DumpFileInfo.Validation` correctly. The data is there — it's just not copied into the synthesized `DumpResult`.
### Fix
This is a one-line fix. In `GetFullStatus()` (~line 978), add `Validation: f.Validation` to the synthesized `DumpResult`:
```go
if di == nil {
return fmt.Errorf("destination %s: cannot determine disk usage", path)
}
results = append(results, DumpResult{
DB: DiscoveredDB{StackName: f.StackName, DBType: f.DBType, ContainerName: f.StackName},
FilePath: f.FileName,
Size: f.Size,
Validation: f.Validation, // <-- ADD THIS LINE
})
```
**Recommended:** Use the warning-only approach for now. A destination that can't report disk usage is unusual and worth logging, but blocking it could break setups with exotic filesystems (CIFS mounts, etc.).
**File:** `controller/internal/backup/backup.go`, inside `GetFullStatus()`, in the "Synthesize LastDBDump from DumpFiles" block.
---
## Build & Deploy
## Bug 4 (MEDIUM): Empty `systemDataPath` produces relative dump paths
Version: **v0.15.2**
### File
`internal/backup/backup.go`, function `DumpStackDB`, line 570
### Root Cause
```go
drivePath := m.GetAppDrivePath(stackName) // Returns "" if stackProvider is nil AND systemDataPath is ""
dumpDir := AppDBDumpPath(drivePath, stackName) // AppDBDumpPath("", "mealie") = "backups/primary/mealie/db-dumps" (RELATIVE!)
### Build workflow
```bash
SSH=/c/Windows/System32/OpenSSH/ssh.exe
# 1. Commit & push
cd e:/git/deploy-felhom-compose
git add -A && git commit -m "v0.15.2: Fix snapshot stats and DB validation loss on restart" && git push
# 2. Build
$SSH kisfenyo@192.168.0.180 "cd ~/build/felhom-controller && git -C ~/git/deploy-felhom-compose pull && ./build.sh v0.15.2 --push"
# 3. Deploy
$SSH kisfenyo@192.168.0.162 "cd /opt/docker/felhom-controller && sudo docker pull gitea.dooplex.hu/admin/felhom-controller:v0.15.2 && sudo sed -i 's|image: gitea.dooplex.hu/admin/felhom-controller:.*|image: gitea.dooplex.hu/admin/felhom-controller:v0.15.2|' docker-compose.yml && sudo docker compose up -d"
# 4. Verify
$SSH kisfenyo@192.168.0.162 "docker ps --filter name=felhom-controller --format '{{.Image}} {{.Status}}'"
```
If `systemDataPath` is empty (misconfiguration or missing `system_data_path` in controller.yaml) and the stack has no HDD_PATH, `GetAppDrivePath()` returns an empty string. `AppDBDumpPath("", stackName)` then produces a **relative path** instead of an absolute one. The `DumpOne()` function creates directories relative to the process's working directory (typically `/opt/docker/felhom-controller/`), orphaning the dumps where nothing else looks for them.
### Compile check
Always run `go build ./...` in `controller/` before committing to ensure no compile errors.
The same issue exists in `RunDBDumps()` (line 212) and `RunBackup()` (line 308, 321).
## Documentation
### Impact
- DB dumps written to unexpected location
- Restic backup doesn't find them (looks in the expected absolute path)
- Backup appears to succeed but DB dumps are orphaned
- Restore would not have fresh DB dumps
Add a CHANGELOG.md entry at the top (under `## Changelog`). Read the first 30 lines to see the format, then insert a new entry. Example:
### Fix Instructions
Add a validation check in `GetAppDrivePath()`:
```markdown
### What was just completed (2026-02-19 session 52)
- **v0.15.2 — Fix data loss on container restart (2 bugs):**
```go
func (m *Manager) GetAppDrivePath(stackName string) string {
if m.stackProvider != nil {
if hddPath := m.stackProvider.GetStackHDDPath(stackName); hddPath != "" {
return hddPath
}
}
if m.systemDataPath == "" {
m.logger.Printf("[ERROR] systemDataPath is empty — cannot determine drive for %s", stackName)
}
return m.systemDataPath
}
**Bug 1:** Snapshot history delta stats (HOZZÁADOTT, ÚJ FÁJL, VÁLTOZOTT) showed 0 after container restart because restic doesn't store these stats — they were only in memory. Fixed by persisting the snapshot history ring buffer to `data/snapshot-history.json`. On startup, persisted stats are merged with restic repo snapshots.
**Bug 2:** DB validation (ÉRVÉNYESÍTÉS column) showed "–" after restart because the synthesized `LastDBDump.Results` didn't copy `Validation` from `DumpFileInfo`. One-line fix: added `Validation: f.Validation` to the synthesized `DumpResult` in `GetFullStatus()`.
**Files modified:** `internal/backup/backup.go`
```
Also add a startup validation in `NewManager()`:
Update version in `C:\Users\User\.claude\projects\e--git\memory\MEMORY.md` to `v0.15.2`.
```go
func NewManager(cfg *config.Config, pinger *monitor.Pinger, sett *settings.Settings, logger *log.Logger) *Manager {
if cfg.Paths.SystemDataPath == "" {
logger.Printf("[WARN] SystemDataPath is empty in config — SSD-only apps will not have correct backup paths")
}
return &Manager{
// ... existing fields
}
}
```
## Verification
And in `DumpStackDB()`, add a guard before using the path:
```go
drivePath := m.GetAppDrivePath(stackName)
if drivePath == "" || !filepath.IsAbs(drivePath) {
return fmt.Errorf("cannot determine absolute drive path for %s (systemDataPath not configured?)", stackName)
}
dumpDir := AppDBDumpPath(drivePath, stackName)
```
---
## Testing Checklist
After fixing all bugs, verify:
- [ ] **Bug 1:** Run cross-drive rsync backup twice for a single-mount app → `_db/` and `_config/` persist between runs (not deleted by `--delete`)
- [ ] **Bug 1:** Run cross-drive rsync backup for a multi-mount app → behavior unchanged
- [ ] **Bug 2:** During nightly scheduled backup, UI shows "Mentés folyamatban" on dashboard
- [ ] **Bug 2:** During nightly scheduled backup, "Visszaállítás" button shows "already in progress" error
- [ ] **Bug 2:** `RunFullBackup()` still works correctly (calls internal methods, doesn't deadlock)
- [ ] **Bug 2:** Scheduled `db-dump` and `backup` jobs don't deadlock with each other (they run at different times, but if one overruns, the next should get "already in progress" error)
- [ ] **Bug 3:** Cross-drive backup to a path where `GetDiskUsage` returns nil → logs a warning (not a silent pass)
- [ ] **Bug 4:** With `system_data_path: ""` in config → startup log warning + `DumpStackDB()` returns error instead of using relative path
- [ ] Build succeeds: `go build ./...`
---
## Implementation Notes
- **Do NOT change** any template files, CSS, or UI text — only Go backend files
- **Do NOT change** any function signatures that are part of the public API (other packages import them)
- **Do NOT change** the scheduler wiring in `main.go` — the fix should be in the backup package
- **Run `go vet ./...` and `go build ./...`** after all changes to verify no compilation errors
- Keep all log messages in English (UI text is Hungarian, but log messages are English)
---
## Files to Modify
| File | Bug(s) | Changes |
|------|--------|---------|
| `internal/backup/crossdrive.go` | 1, 3 | Add rsync `--exclude` flags; add nil check for GetDiskUsage |
| `internal/backup/backup.go` | 2, 4 | Extract `acquireRunning`/`releaseRunning` + internal methods; add systemDataPath validation |
After deploying v0.15.2:
1. Navigate to /backups — verify Pillanatképek shows actual values (if a backup has been run before the restart)
2. Restart the container: `sudo docker compose restart`
3. Refresh /backups — Pillanatképek should still show the stats from before restart
4. DB Adatbázisok table should show table counts (e.g., "42 tábla") in ÉRVÉNYESÍTÉS column, not "–"