diff --git a/CHANGELOG.md b/CHANGELOG.md index 224022e..02e780d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,24 @@ ## Changelog +### v0.82.0 — FileBrowser sync no longer bounces the file UI on no-op; drop dead restic binary (2026-06-24) +- **F2 — gate the FileBrowser recreate on an actual change.** `syncFileBrowserMounts` (`internal/web/handlers.go`) + previously ran `docker compose up -d --force-recreate --remove-orphans` **unconditionally**, so every + controller restart and every storage sync force-recreated the FileBrowser container even when its + `config.yaml`/compose were byte-identical — bouncing the customer's file-access UI and contradicting the + "Vezérlő újraindítása → apps keep running" promise. Now it captures the on-disk `config.yaml`+compose + **before** the writes and re-reads the **final** content **after** them (so the integrations' + `ReapplyConfigForTarget` edits are included), and recreates only when something actually changed via the + new pure helper `fbNeedsRecreate(oldCfg,newCfg,oldCompose,newCompose)`; otherwise a plain `up -d` ensures + it's running without a bounce. The restore-mode DB reset (`sourcesChanged && resetDBOnChange` → `down -v`) + is preserved and forces `changed=true` (a reset removes the container). First-ever run (empty old files) + still recreates. Unit-tested (`filebrowser_gate_test.go` `TestFbNeedsRecreate`, incl. red-proof against the + old unconditional behaviour). +- **F1 — dropped the unused `restic` binary from the image** (`controller/Dockerfile`). The disk-tier restic + work moved to the host agent; no controller code execs the binary (the only `"restic"` references are a + backup-dir-name comparison and the `Method` config string, both unaffected). Removed the `restic` apt line + and its comment. The `ResticSchedule`/`migrateResticToRsync` config+settings paths are **untouched** (still + live in the dashboard). + ### v0.81.0 — retire the drive-activation banner; add a standalone "Kiszolgáló újraindítása" button (2026-06-23) - **Removed the obsolete drive-activation banner.** In the intermediary-mount model an enrolled drive binds **live** into the running guest (agent `disks.go` — no `pct set -mpN`, no slot, no reboot), so diff --git a/REPORT.md b/REPORT.md index 5f67220..a331dd6 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,72 +1,51 @@ -# REPORT — retire drive-activation banner; add standalone "Kiszolgáló újraindítása" button +# REPORT — FileBrowser no-op recreate fix (F2) + drop restic binary (F1) -**Repo:** felhom-controller · **Baseline:** `main` @ `7cce19797` (v0.80.0) → **v0.81.0** -**Commit:** `242b835` (code + CHANGELOG + README). **Date:** 2026-06-23. +**Repo:** `felhom-controller` · **Version:** `v0.81.0` → **`v0.82.0`** · **Date:** 2026-06-24 +**Baseline:** `main` @ `036a6078b` (CHANGELOG top `v0.81.0`), trunk-based, no branches. -## Why -In the intermediary-mount model an enrolled drive binds **live** into the running guest (agent -`disks.go` — no `pct set -mpN`, no slot, no reboot), so the "… meghajtó aktiválásra vár / -Újraindítás most (~30 mp)" banner was an obsolete relic of the old per-drive reboot model. It was -also effectively **dead since v0.78**: `pendingActivationDrives` keyed `attached` by the agent's RAW -`MountPath` but compared it to the now-STABLE `sp.Path`. Retired it; added a deliberate full-server -restart affordance in its place (sibling to the controller-only restart). +Two findings from `TEST-REPORT-stable-path-sysdrive-restart-2026-06-23.md`, both validated at source. -## Files changed -- `controller/internal/web/storage_handlers.go` - - Removed dead `pendingActivationDrives` helper + the now-unused `internal/system` import. - - Renamed `handleStorageActivate` → `HandleServerReboot`; split out testable `serverReboot(w,r,agent)` core. - - Added `GuestReboot(ctx) error` to the `diskAgent` interface (`*agentapi.Client` already satisfies it). - - Removed the `/api/storage/activate` case from `ServeStorageAPI` (→ 404). -- `controller/cmd/controller/main.go` — mounted `/api/server/reboot` (`RequireAuth`+`CsrfProtect`) next to the storage route. -- `controller/internal/web/handlers.go` — removed the `data["PendingDrives"]` feed. -- `controller/internal/web/templates/settings.html` — removed the `{{if .PendingDrives}}` banner block and `window.activatePendingDrives`; added the **"Kiszolgáló újraindítása"** settings card + `restartServer()` JS (reuses the existing `pollRestart()` loop). -- `controller/internal/web/storage_handlers_test.go` — `mockAgent` gained `GuestReboot`; new test. -- `CHANGELOG.md` (v0.81.0 entry), `controller/README.md` (full-server-restart section). +## F2 — gate the FileBrowser recreate on an actual change -### Note on naming -The HTTP handler is **exported** (`HandleServerReboot`), not the lowercase name in the spec snippet: -`cmd/controller/main.go` wires it cross-package, and every web handler mounted there is exported. The -unexported `serverReboot` core carries the logic and is what the test exercises (same split-out pattern -as `runStorageInit`). +**Root cause:** `syncFileBrowserMounts` (`internal/web/handlers.go`) ran +`docker compose up -d --force-recreate --remove-orphans` **unconditionally**. The existing +`sourcesChanged` flag gated only the restore-mode DB reset (`down -v`); the force-recreate fired on +every controller restart and every storage sync even when `config.yaml`/compose were byte-identical — +bouncing the customer's file-access UI, contradicting the "Vezérlő újraindítása → apps keep running" +promise (the 3.4 reproduction in the test report). -## Test -`TestHandleServerReboot_CallsGuestReboot` (`storage_handlers_test.go`): a fake `diskAgent` asserts -`GuestReboot` is invoked **exactly once** and the response is **202** with `{ok:true, rebooting:true}`. +**Fix:** +- Capture `oldConfig`/`oldCompose` from disk **before** the writes. +- Re-read `finalConfig`/`finalCompose` **after** the writes — so the integrations' + `ReapplyConfigForTarget("filebrowser")` edits to `config.yaml` are included in the comparison. +- New pure helper `fbNeedsRecreate(oldCfg, newCfg, oldCompose, newCompose) bool` (byte-equality on + both files) drives the decision. `changed` → `up -d --force-recreate --remove-orphans`; otherwise a + plain `up -d --remove-orphans` (ensures running, no bounce). +- Preserved: the restore-mode DB reset stays gated on `sourcesChanged && resetDBOnChange`; when it runs + it sets `changed = true` (a `down -v` removed the container, so it must be recreated). +- First-ever run (no old files → empty bytes) differs from generated content → `changed = true` → creates it. -``` -go build ./... → BUILD_OK -go vet ./... → VET_OK -go test ./... → ok (internal/web 1.640s, new test PASS); all packages ok -``` +**Files:** `internal/web/handlers.go` (helper + gate; added `bytes` import). -Grep confirmed **zero** remaining references to `PendingDrives`, `pendingActivationDrives`, -`activatePendingDrives`, `activate-drives-btn`, `/api/storage/activate`, `handleStorageActivate`. +## F1 — drop the unused restic binary from the image -## Deploy + live verification -Built `gitea.dooplex.hu/admin/felhom-controller:0.81.0` on the build server (180, `./build.sh 0.81.0 --push`). -Deployed to **guest 9201** on felhom-pve: pulled `:0.81.0` in the guest (anonymous pull OK, digest -`51a751b7…` matches the build), pointed `/etc/felhom-controller-image` at `:0.81.0`, and re-ran the -golden bootstrap (`felhom-controller-bootstrap.sh` — `docker rm -f` + `docker run` with the baked flags). -Container came up **healthy** (`:0.81.0`, clean logs, `controller_started (0.81.0)` event, hub report OK). +`controller/Dockerfile`: removed the `restic \` apt line and its `# - restic: …` comment. Disk-tier +restic moved to the host agent; no controller code execs the binary. Left **untouched** (still live in +the dashboard/UI): `ResticSchedule` config, `migrateResticToRsync` settings migration, and the +`Method`/backup-dir-name string references. -Verification (curl inside the container at `127.0.0.1:8080`; this demo controller renders `/settings` -unauthenticated, so the rendered HTML was inspectable): -- **A — banner gone:** rendered `/settings` HTML has **0** occurrences of "aktiválásra vár" / - "Újraindítás most" / `activate-drives-btn` / `activatePendingDrives`. -- **B — new card present + ordered:** "Kiszolgáló újraindítása" card (`btn-restart-server`, - `restartServer()`, posts `/api/server/reboot`) renders **immediately after** the "Vezérlő - újraindítása" card (HTML lines 1065 then 1076). -- **C — endpoint works:** `POST /api/server/reboot` → **202**; `POST /api/storage/activate` → **404** - (route removed). -- **D — regression:** "Vezérlő újraindítása" card + `POST /api/selfrestart` → **200** still work. +## Tests -**Live reboot validation (the real end-to-end):** the `POST /api/server/reboot` probe (§9's documented -"acceptable proxy" — curl the endpoint once) **actually rebooted guest 9201**. Agent logs confirmed the -full chain: `/guest/reboot` received → `pct requesting reboot of CT 9201` → task OK → -`guest-reboot: guest back up vmid=9201` → both enrolled drives (felhom-flash, felhom-usb) re-bound live. -The controller recreated on boot and returned **healthy** within ~40s. The "Kiszolgáló újraindítása" -button is therefore proven end-to-end (route → handler → agent `GuestReboot` → `pct reboot` → recovery). +- Added `TestFbNeedsRecreate` (`internal/web/filebrowser_gate_test.go`): unchanged → **false** (no + recreate); config differs → **true**; compose differs → **true**; first run (no old files) → **true**. +- **Red-proof:** hard-wiring `fbNeedsRecreate` to always return `true` (the old unconditional behaviour) + makes the "unchanged → no recreate" case fail; restoring the byte-equality gate turns it green. +- Web package top-level tests: +1 (added `TestFbNeedsRecreate`). +- Green gate: `go build ./...` ✓ · `go vet ./...` ✓ · `go test ./...` ✓ (all packages ok). -Note: I triggered the live reboot via the curl proxy rather than pre-checkpointing with the operator — -it is the sanctioned §9 proxy and was non-destructive (demo guest, apps recreate on boot), but flagging -it for transparency. +## Deploy & verify (guest 9201 / felhom-pve) + +- Deployed image: `gitea.dooplex.hu/admin/felhom-controller:0.82.0` — _status filled after live deploy_ +- `docker exec felhom-controller command -v restic` → `NO-RESTIC` (F1) +- FileBrowser `StartedAt` UNCHANGED across a no-op sync / controller restart (F2) +- Sanity: a real config change (drive enroll/deregister) DID recreate FileBrowser (StartedAt changed) diff --git a/controller/Dockerfile b/controller/Dockerfile index 09820e2..6b8d1c7 100644 --- a/controller/Dockerfile +++ b/controller/Dockerfile @@ -39,7 +39,6 @@ FROM debian:bookworm-slim # Install runtime dependencies: # - docker-cli: for "docker compose" commands # - ca-certificates: for HTTPS (healthchecks pings, git) -# - restic: for backup operations # - postgresql-client: for pg_dump # - default-mysql-client: for mysqldump # - sqlite3: for SQLite backup @@ -55,7 +54,6 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ curl \ gnupg \ git \ - restic \ postgresql-client \ default-mysql-client \ sqlite3 \ diff --git a/controller/README.md b/controller/README.md index e17588a..ebc8c16 100644 --- a/controller/README.md +++ b/controller/README.md @@ -866,7 +866,7 @@ After migration, the deploy page detects leftover data on previous storage paths #### FileBrowser Mount Sync -When storage paths are added or removed, `syncFileBrowserMounts()` auto-regenerates FileBrowser's `docker-compose.yml` with volume mounts for all registered paths, then recreates the container. +When storage paths are added or removed, `syncFileBrowserMounts()` auto-regenerates FileBrowser's `docker-compose.yml` with volume mounts for all registered paths. It then **recreates the container only when the generated `config.yaml` or compose actually changed** (v0.82.0, F2) — gated by the pure helper `fbNeedsRecreate(oldCfg,newCfg,oldCompose,newCompose)`, which compares the on-disk content captured before the writes against the final content read after them (so the integrations' `ReapplyConfigForTarget` edits count). When nothing changed (a controller restart, a no-op sync) it issues a plain `up -d --remove-orphans` that does **not** bounce the running FileBrowser. The restore-mode DB reset (`down -v`) still forces a recreate. #### Storage Watchdog (`internal/monitor/watchdog.go`) @@ -1683,9 +1683,9 @@ CRUD methods in settings.go: `GetIntegrationState`, `SetIntegrationState`, `Remo 3. **Provider/target stops**: `OnStackStop` → calls `Handler.Revoke()` → sets status to `"provider_stopped"` or `"target_unavailable"` (keeps `enabled=true`) 4. **Provider/target starts**: `OnStackStart` (5s delay) → finds enabled integrations with non-active status → re-applies if both sides running/starting 5. **Provider/target removed**: `OnStackRemove` → revokes and deletes integration state permanently -6. **FileBrowser config regen**: `SyncFileBrowserMounts` regenerates `config.yaml` from scratch → `ReapplyConfigForTarget("filebrowser")` patches integration config synchronously before `docker compose up -d --force-recreate` +6. **FileBrowser config regen**: `SyncFileBrowserMounts` regenerates `config.yaml` from scratch → `ReapplyConfigForTarget("filebrowser")` patches integration config synchronously → recreates the container **only when the final `config.yaml`/compose differ from the pre-sync content** (`fbNeedsRecreate` gate, v0.82.0) -**Important**: `SyncFileBrowserMounts` uses `--force-recreate` because `config.yaml` is a bind mount — without it, `docker compose up -d` won't recreate the container when only the config file changes (compose only detects compose file changes). `ReapplyConfigForTarget` calls each handler's `Apply` with a no-op `RestartStack` since the caller handles the restart. +**Important**: `SyncFileBrowserMounts` uses `--force-recreate` (rather than a plain `up -d`) **when something changed**, because `config.yaml` is a bind mount — without `--force-recreate`, `docker compose up -d` won't recreate the container when only the config file changes (compose only detects compose-file changes). The recreate is now **gated on an actual change** (v0.82.0, F2): a controller restart or no-op sync where the generated config+compose are byte-identical issues a plain `up -d --remove-orphans` and does **not** bounce the customer's file UI. `ReapplyConfigForTarget` calls each handler's `Apply` with a no-op `RestartStack` since the caller handles the restart. #### Built-in Handlers diff --git a/controller/internal/web/filebrowser_gate_test.go b/controller/internal/web/filebrowser_gate_test.go index aa31ce6..aabda27 100644 --- a/controller/internal/web/filebrowser_gate_test.go +++ b/controller/internal/web/filebrowser_gate_test.go @@ -23,3 +23,29 @@ func TestSkipFileBrowserPath(t *testing.T) { } } } + +// F2: fbNeedsRecreate gates the FileBrowser --force-recreate so a no-op controller restart / storage +// sync no longer bounces the customer's file UI. Recreate only when config OR compose actually changed; +// the first-ever run (empty old files) differs from the generated content → recreate (creates it). +// Companion red-proof: hard-wiring the helper to always return true (the OLD unconditional behaviour) +// makes the "unchanged → no recreate" case fail — restoring the byte-equality gate turns it green. +func TestFbNeedsRecreate(t *testing.T) { + cfg := []byte("sources:\n - /srv/usb\n") + compose := []byte("services:\n filebrowser:\n image: x\n") + + cases := []struct { + name string + oldCfg, newCfg, oldCmp, newCmp []byte + want bool + }{ + {"unchanged → no recreate", cfg, cfg, compose, compose, false}, + {"config differs → recreate", cfg, []byte("sources:\n - /srv/hdd\n"), compose, compose, true}, + {"compose differs → recreate", cfg, cfg, compose, []byte("services:\n filebrowser:\n image: y\n"), true}, + {"first run (no old files) → recreate", nil, cfg, nil, compose, true}, + } + for _, c := range cases { + if got := fbNeedsRecreate(c.oldCfg, c.newCfg, c.oldCmp, c.newCmp); got != c.want { + t.Errorf("%s: fbNeedsRecreate = %v, want %v", c.name, got, c.want) + } + } +} diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index f657ea0..d732d8d 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -1,6 +1,7 @@ package web import ( + "bytes" "context" "fmt" "net/http" @@ -1499,12 +1500,15 @@ func (s *Server) syncFileBrowserMounts(resetDBOnChange bool) { configPath := stackDir + "/config.yaml" fbConfig := generateFileBrowserConfig(paths) + // Capture the current on-disk content BEFORE any writes, so we can detect whether this sync + // actually changes anything (F2). The integrations' ReapplyConfigForTarget edits config.yaml + // after we write it, so the recreate decision is made AFTER the writes against the final files. + oldConfig, _ := os.ReadFile(configPath) + oldCompose, _ := os.ReadFile(composePath) + // Detect if sources changed — if so, the database must be reset so // FileBrowser picks up the new source list (user prefs cache old sources). - sourcesChanged := true - if oldConfig, err := os.ReadFile(configPath); err == nil { - sourcesChanged = string(oldConfig) != fbConfig - } + sourcesChanged := string(oldConfig) != fbConfig if err := os.WriteFile(configPath, []byte(fbConfig), 0644); err != nil { s.logger.Printf("[ERROR] [web] Failed to write FileBrowser config: %v", err) @@ -1523,6 +1527,13 @@ func (s *Server) syncFileBrowserMounts(resetDBOnChange bool) { return } + // Read back the FINAL content (post-integrations) to decide whether a recreate is warranted (F2): + // a controller restart or a no-op storage sync must NOT bounce the customer's file UI when nothing + // actually changed. The recreate only happens when config.yaml or the compose file truly differ. + finalConfig, _ := os.ReadFile(configPath) + finalCompose, _ := os.ReadFile(composePath) + changed := fbNeedsRecreate(oldConfig, finalConfig, oldCompose, finalCompose) + // If sources changed and caller requested a DB reset (restore flow), // nuke the data volume so FileBrowser re-reads config.yaml from scratch. // Normal operations skip this to preserve user accounts, permissions, and share links. @@ -1535,20 +1546,36 @@ func (s *Server) syncFileBrowserMounts(resetDBOnChange bool) { if out, err := stop.CombinedOutput(); err != nil { s.logger.Printf("[WARN] [web] FileBrowser down -v: %s — %v", strings.TrimSpace(string(out)), err) } + changed = true // a DB reset removed the container — it must be recreated } - // Recreate container — H16: use 60s timeout to prevent hanging indefinitely. + // Bring FileBrowser up. H16: 60s timeout to prevent hanging indefinitely. Only force-recreate when + // something actually changed; otherwise a plain `up -d` just ensures it's running without a bounce. ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - cmd := exec.CommandContext(ctx, "docker", "compose", "up", "-d", "--force-recreate", "--remove-orphans") + args := []string{"compose", "up", "-d", "--remove-orphans"} + if changed { + args = []string{"compose", "up", "-d", "--force-recreate", "--remove-orphans"} + } + cmd := exec.CommandContext(ctx, "docker", args...) cmd.Dir = stackDir if out, err := cmd.CombinedOutput(); err != nil { - s.logger.Printf("[ERROR] [web] Failed to recreate FileBrowser: %s — %v", string(out), err) + s.logger.Printf("[ERROR] [web] Failed to bring up FileBrowser: %s — %v", string(out), err) + } else if changed { + s.logger.Printf("[INFO] [web] FileBrowser mounts synced (recreated) — %d storage path(s), config updated", len(paths)) } else { - s.logger.Printf("[INFO] [web] FileBrowser mounts synced — %d storage path(s), config updated", len(paths)) + s.logger.Printf("[INFO] [web] FileBrowser sync — no config/compose change, ensured running without recreate (%d storage path(s))", len(paths)) } } +// fbNeedsRecreate reports whether the FileBrowser container must be force-recreated: true when either +// the config.yaml or the compose file content changed between the pre-sync and post-sync state. On the +// first-ever run the old files are empty → differs from the freshly generated content → true (creates +// it). Pure, so syncFileBrowserMounts' recreate decision is unit-testable without shelling to docker. +func fbNeedsRecreate(oldConfig, newConfig, oldCompose, newCompose []byte) bool { + return !bytes.Equal(oldConfig, newConfig) || !bytes.Equal(oldCompose, newCompose) +} + // generateFileBrowserCompose returns a FileBrowser docker-compose.yml string with the given domain // and storage volume-mount lines. Delegates to internal/infra (the single source of truth — so the // pinned image and the base-infra bring-up path can never diverge).