feat(update): add pre-update rollback snapshot with auto-restore on failure#358
feat(update): add pre-update rollback snapshot with auto-restore on failure#358buddy0323 wants to merge 3 commits intoLight-Heart-Labs:mainfrom
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
Sound design — snapshot before update with auto-restore on failure is the right approach. But several issues need fixing before merge.
Blocking: _update_rollback swallows restore failures
_update_rollback never checks the return value of _restore_snapshot. If restore fails (permissions, corrupt snapshot), the user sees "Rollback complete" even though the system is broken. Must check return value and emit log_error with manual recovery instructions if it fails.
Blocking: || true / 2>/dev/null violates CLAUDE.md
CLAUDE.md rule 4: "Never || true or 2>/dev/null." The PR introduces many new instances:
(( files_saved++ )) || true(4 times) — fix:files_saved=$(( files_saved + 1 ))avoids the exit-code issuecmd_health &>/dev/nullinwait_for_healthy— completely silences health output, making debugging impossible- Docker fallback chains with
|| true— use|| { log_warn "docker compose v2 failed, trying v1"; docker-compose ...; }instead
High: No snapshot integrity validation
A disk-full-mid-copy produces a partial snapshot that looks valid (has snapshot.json) but is missing files. _restore_snapshot only checks [[ -d "$snap_dir" ]]. Add verification: confirm snapshot.json is valid JSON and critical files (.env, .version) exist before proceeding.
High: No timestamp input validation
snapshot_pre_update interpolates the timestamp directly into a path. Validate format: [[ "$timestamp" =~ ^[0-9]{8}-[0-9]{6}$ ]] || { log_error "Invalid timestamp"; return 1; }
Medium: rm -rf in pruning needs safety guard
If ROLLBACK_DIR is misconfigured (empty string), find searches CWD. Add: [[ -n "$ROLLBACK_DIR" && "$ROLLBACK_DIR" == */data/backups ]]
Medium: Function too large
snapshot_pre_update is 60+ lines (CLAUDE.md threshold: 30). Extract pruning into _prune_rollback_snapshots.
Medium: Inconsistent health checking
cmd_update uses the new wait_for_healthy but cmd_rollback still uses sleep 10; cmd_health. Should be consistent.
Low: Nested function relies on dynamic scoping
_update_rollback captures $snap_dir and $compose_flags by closure. Bash doesn't have real closures. Pass them as explicit parameters.
🤖 Reviewed with Claude Code
|
All review items addressed: Blocking fixes
High fixes
Medium fixes
Low fix
|
Summary
dream-update.sh updaterun, a rollback snapshot is written todata/backups/pre-update-<timestamp>/containing.env, all activedocker-composeoverlays, per-extension config dirs (config/{litellm,n8n,openclaw,searxng}/), and.versiondream-update.sh rollbacknow prefers the most recentpre-update-*snapshot over general backups, and accepts a bare timestamp as a target (e.g.rollback 20260317-120000)dream-update.sh statusnow shows rollback snapshot count, storage path, and the last recorded snapshot pathMAX_BACKUPS(default: 10)Files changed
dream-update.sh— addedROLLBACK_DIR,snapshot_pre_update(),_restore_snapshot(),wait_for_healthy(); rewiredcmd_update()with 6-step rollback-aware flow; extendedcmd_rollback()andcmd_status()Test plan
dream-update.sh updateon a clean install — confirmdata/backups/pre-update-<timestamp>/is created with.env, compose files, and config dirsHEALTH_TIMEOUT=15and break a service — confirm auto-restore triggers after timeoutdream-update.sh rollbackwith no argument — confirm it picks the most recentpre-update-*snapshotdream-update.sh rollback <timestamp>— confirm it resolves the correct snapshotdream-update.sh status— confirm rollback snapshot count and path are displayedMAX_BACKUPS