From 2f08306770932378aff4de8b68811ecaad1d9cd2 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Wed, 18 Feb 2026 08:53:29 +0100 Subject: [PATCH] =?UTF-8?q?TASK.md=20=E2=80=94=20Cross-Drive=20Backup=20Im?= =?UTF-8?q?provements=20(v0.12.6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- TASK.md | 234 ++++++++++++++++++++++++++------------------------------ 1 file changed, 107 insertions(+), 127 deletions(-) diff --git a/TASK.md b/TASK.md index 264285c..92dd5da 100644 --- a/TASK.md +++ b/TASK.md @@ -1,4 +1,4 @@ -# TASK.md — Cross-Drive Backup Validation Fix (v0.12.5) +# TASK.md — Cross-Drive Backup Improvements (v0.12.6) ## Prompt (copy-paste this into Claude Code) @@ -6,7 +6,7 @@ Read TASK.md for the full plan. Apply all code changes described, then build and deploy. After all fixes are done: 1. Run `go build ./...` and `go vet ./...` from the controller/ directory — fix any errors -2. Update CHANGELOG.md with a new entry at the top (session 41, v0.12.5) +2. Update CHANGELOG.md with a new entry at the top (session 42, v0.12.6) 3. Commit, build, and deploy following the workflow in CLAUDE.md ``` @@ -14,167 +14,147 @@ After all fixes are done: ## Context -The cross-drive backup for Immich failed last night with: -``` -Hiba: destination /mnt/hdd_placeholder is not a mount point (0s) -``` +The cross-drive backup for Immich was fixed earlier today (mount-point validation + system-drive +space thresholds). During testing, two more issues were found: -**Root cause:** `ValidateDestination()` in `crossdrive.go` hard-blocked non-mount-point -destinations. The `/mnt/hdd_placeholder` folder is on the internal SSD (not a separate mount), -so the device-ID check in `IsMountPoint()` returned false. +1. **Redundant destination folder nesting** — rsync creates + `backups/rsync/immich/storage/immich/` instead of `backups/rsync/immich/` +2. **DB backups backed up twice** — Immich stores its own DB dumps in + `/mnt/hdd_1/storage/immich/backups/` (~16 MB each). The cross-drive rsync copies these as part of the user data, but the controller already handles DB backups separately via pg_dump. -**Already fixed (in current working tree):** The mount-point check was changed from a hard -block to a logged warning (lines 172–174 of crossdrive.go). The backup will now proceed -for system-drive destinations. -**Remaining work:** Improve the disk space validation to be smarter about system-drive -destinations (don't fill up the OS drive). +## Fix 1: Simplify rsync destination path structure (crossdrive.go) ---- +**File:** `internal/backup/crossdrive.go`, function `runRsyncBackup`, lines ~206–217 -## Fix 1: Smarter space checks in `ValidateDestination` (crossdrive.go) +**Problem:** The path-stripping logic strips only the first 2 segments of the source path +(e.g., `mnt/hdd_1`) and keeps everything else as a relative subpath: -**File:** `internal/backup/crossdrive.go`, lines 161–183 - -**Current code (already patched with the mount-point warning):** ```go -func (r *CrossDriveRunner) ValidateDestination(path string) error { - if path == "" { - return fmt.Errorf("destination path is empty") - } - if _, err := os.Stat(path); os.IsNotExist(err) { - return fmt.Errorf("destination %s does not exist", path) - } - if !system.IsMountPoint(path) { - r.logger.Printf("[WARN] Destination %s is not a separate mount point (system drive) — backup will proceed but data is not protected against drive failure", path) - } - if !system.IsWritable(path) { - return fmt.Errorf("destination %s is not writable", path) - } - di := system.GetDiskUsage(path) - if di != nil && di.AvailGB < 0.1 { - return fmt.Errorf("destination %s has insufficient free space (%.1f GB)", path, di.AvailGB) - } - return nil +parts := strings.SplitN(strings.TrimPrefix(srcMount, "/"), "/", 3) +if len(parts) >= 3 { + rel = parts[2] // "storage/immich" — redundant nesting! } ``` -**Required change:** Replace the flat 100MB space check (lines 178–181) with drive-type-aware logic: +For source `/mnt/hdd_1/storage/immich`, this creates: +``` +backups/rsync/immich/storage/immich/ ← "storage/immich" repeats context +``` + +Expected: +``` +backups/rsync/immich/ ← data goes directly here (single mount) +``` + +**Fix:** Use `filepath.Base()` as the subdirectory name. If the app has only one mount, +rsync directly into the stack folder; if multiple, use basenames to keep them separate. + +Replace the path-stripping block (lines ~206–219) with: ```go -func (r *CrossDriveRunner) ValidateDestination(path string) error { - if path == "" { - return fmt.Errorf("destination path is empty") - } - if _, err := os.Stat(path); os.IsNotExist(err) { - return fmt.Errorf("destination %s does not exist", path) - } - onSystemDrive := !system.IsMountPoint(path) - if onSystemDrive { - r.logger.Printf("[WARN] Destination %s is not a separate mount point (system drive) — backup will proceed but data is not protected against drive failure", path) - } - if !system.IsWritable(path) { - return fmt.Errorf("destination %s is not writable", path) - } - if di := system.GetDiskUsage(path); di != nil { - if onSystemDrive { - // System drive: protect OS stability — require ≥10 GB free and <90% used - if di.AvailGB < 10 { - return fmt.Errorf("destination %s is on the system drive with only %.1f GB free — at least 10 GB required to protect OS stability", path, di.AvailGB) - } - if di.UsedPercent >= 90 { - return fmt.Errorf("destination %s is on the system drive at %.0f%% capacity — maximum 90%% allowed", path, di.UsedPercent) - } - } else { - // External drive: just ensure it's not completely full - if di.AvailGB < 0.1 { - return fmt.Errorf("destination %s has insufficient free space (%.1f GB free)", path, di.AvailGB) - } - } - } - return nil +for i, srcMount := range mounts { + var dstPath string + if len(mounts) == 1 { + // Single mount: rsync directly into the stack folder + dstPath = destDir + } else { + // Multiple mounts: use the leaf directory name as subfolder + // Disambiguate if needed by appending index + leaf := filepath.Base(srcMount) + dstPath = filepath.Join(destDir, leaf) + // Check for duplicate leaf names (unlikely but safe) + if i > 0 { + if _, err := os.Stat(dstPath); err == nil { + dstPath = filepath.Join(destDir, fmt.Sprintf("%s_%d", leaf, i)) + } + } + } + if err := os.MkdirAll(dstPath, 0755); err != nil { + return fmt.Errorf("creating rsync destination: %w", err) + } + + // Ensure trailing slash on source for rsync semantics (copy contents, not the dir itself) + src := strings.TrimRight(srcMount, "/") + "/" + dst := strings.TrimRight(dstPath, "/") + "/" + + cmd := exec.CommandContext(ctx, "rsync", "-a", "--delete", src, dst) + r.logger.Printf("[DEBUG] rsync: %s → %s", src, dst) + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("rsync failed for %s: %v (%s)", srcMount, err, strings.TrimSpace(string(out))) + } } ``` -**Update the function comment (lines 161–164) to match:** -```go -// ValidateDestination checks that the destination path exists, is writable, -// and has sufficient free space. System-drive destinations get stricter limits -// (≥10 GB free, <90% used) to protect OS stability; external drives just need -// ≥100 MB. Non-mount-point destinations are allowed with a logged warning. +Remove the old `rel` variable and the `SplitN` block entirely. Also remove the +`os.MkdirAll(dstPath, 0755)` that was inside the old loop since it's now in the new block. + +**Result after fix:** ``` +/mnt/hdd_placeholder/backups/rsync/immich/ +├── backups/ ← Immich's internal DB dumps (will be excluded in Fix 4) +├── encoded-video/ +├── library/ +├── profile/ +├── thumbs/ +└── upload/ +``` + +**Impact on existing backups:** The first rsync after this change will create the new +flat structure. The old nested `storage/immich/` subfolder inside `backups/rsync/immich/` +will remain orphaned (rsync `--delete` only deletes within the target, not sibling dirs). +This is fine — no data loss, and the old folder can be cleaned up manually. --- -## Fix 2: Align `CheckBackupDestination` thresholds for system drives (mounts_linux.go) +## Fix 2: Exclude app-internal DB backups from rsync (crossdrive.go) -**File:** `internal/system/mounts_linux.go`, lines 134–186 +**File:** `internal/backup/crossdrive.go`, function `runRsyncBackup` -The web UI's `CheckBackupDestination` currently applies the same disk thresholds (90% warn, -95% block) regardless of drive type. For system drives, it should use the same stricter -thresholds as the runner (90% block, 10 GB minimum) so the UI warning matches what the -runner will actually enforce. +**Problem:** Many apps store their own periodic DB dumps inside their data directory: +- Immich: `storage/immich/backups/` (64 MB of daily postgres dumps) +- Other apps may follow similar patterns -**Required change:** In the Tier 4 block (lines 171–183), add system-drive-specific checks -BEFORE the generic percentage checks. The logic should be: +The controller already handles DB backups separately via `pg_dump` (the "Adatbázis mentés" +feature). Copying the app's internal DB dumps via rsync is redundant and wastes space. + +Immich's internal backup path: `/backups/*.sql.gz` (created by Immich itself daily). + +**Fix:** Add `--exclude` flags to the rsync command for common app-internal backup patterns. + +In the rsync `exec.CommandContext` call, add excludes: ```go - // Tier 4: disk usage checks - if di := GetDiskUsage(path); di != nil { - h.UsedPercent = di.UsedPercent - h.FreeGB = di.AvailGB - if h.SystemDrive { - // System drive: stricter limits to protect OS stability - if di.AvailGB < 10 { - h.Warning = fmt.Sprintf("A rendszermeghajtón csak %.1f GB szabad — legalább 10 GB szükséges a rendszer stabilitásához!", di.AvailGB) - h.Blocked = true - h.Severity = "critical" - } else if di.UsedPercent >= 90 { - h.Warning = fmt.Sprintf("A rendszermeghajtó %.0f%%-ban megtelt — maximum 90%% megengedett.", di.UsedPercent) - h.Blocked = true - h.Severity = "critical" - } - // If neither triggers, keep the Tier 3 system-drive warning - } else { - // External drive: original thresholds - if di.UsedPercent >= 95 { - h.Warning = fmt.Sprintf("A mentési meghajtó megtelt (%.0f%% használt)!", di.UsedPercent) - h.Blocked = true - h.Severity = "critical" - } else if di.UsedPercent >= 90 { - h.Warning = fmt.Sprintf("A mentési meghajtó majdnem megtelt (%.0f%% használt).", di.UsedPercent) - h.Severity = "warning" - } - } - } +cmd := exec.CommandContext(ctx, "rsync", "-a", "--delete", + "--exclude", "backups/*.sql.gz", + "--exclude", "backups/*.sql", + "--exclude", "backups/*.dump", + src, dst) ``` -Note: the `else if di.UsedPercent >= 90 && h.Severity == "ok"` condition in the original -was preventing the 90% warning from overriding the system-drive warning. The new code -separates the branches cleanly — system drive gets its own block, external drive gets its own. +This excludes only DB dump files inside `backups/` subdirectories — not the `backups/` +directory itself (which might contain non-DB files), and not any other `*.sql.gz` files +outside of `backups/`. This is conservative and safe. ---- - -## Summary of thresholds - -| Condition | System drive | External drive | -|-----------|-------------|----------------| -| Free space < 10 GB | **Block** | — | -| Usage ≥ 90% | **Block** | Warning | -| Usage ≥ 95% | (caught by 90%) | **Block** | -| Free space < 100 MB | (caught by 10GB) | **Block** | +**Note:** The `.immich` marker file and the `backups/` directory structure itself are +preserved — only the large dump files are excluded. --- ## Files to modify -1. `internal/backup/crossdrive.go` — `ValidateDestination()` (Fix 1) -2. `internal/system/mounts_linux.go` — `CheckBackupDestination()` (Fix 2) +1. `internal/backup/crossdrive.go` — `runRsyncBackup()` (Fix 3 + Fix 4) ## Post-fix checklist - [ ] `go build ./...` passes - [ ] `go vet ./...` passes -- [ ] Update `CHANGELOG.md` — session 41, version **v0.12.5**, describe both fixes +- [ ] Update `CHANGELOG.md` — session 42, version **v0.12.6**, describe ALL fixes: + - Fix 1: ValidateDestination allows non-mount-point destinations with warning + - Fix 2: System-drive space thresholds (10 GB / 90%) in both runner and web UI + - Fix 3: Simplified rsync destination path (flat structure per app) + - Fix 4: Exclude app-internal DB dumps from rsync +- [ ] Update `controller/README.md` backup section with the new architecture - [ ] Commit, build on 192.168.0.180, deploy on 192.168.0.162 - [ ] Verify with `docker ps` and `docker logs` +- [ ] After deploy, run manual Immich backup and verify new folder structure