TASK.md — Cross-Drive Backup Improvements (v0.12.6)

2026-02-18 08:53:29 +01:00
parent 1a5e96e998
commit 2f08306770
1 changed files with 107 additions and 127 deletions
@@ -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/<data>` instead of `backups/rsync/immich/<data>`
+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: `<data>/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