diff --git a/CHANGELOG.md b/CHANGELOG.md index d97c8f8f6..199756536 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ All notable changes to Bambuddy will be documented in this file. - **Filament Track Switch (FTS) support — print modal filament dropdown is no longer empty when an X2D / H2D has the FTS accessory installed** ([#1162](https://github.com/maziggy/bambuddy/issues/1162), reported by @mkavalecz) — When the FTS accessory is installed the printer's MQTT changes one nibble of the per-AMS `info` bitmask: bits 8-11 flip from a fixed extruder ID (0x0 / 0x1) to `0xE` ("uninitialized"), because the AMS is no longer wired to a single nozzle — the FTS dynamically routes any slot to either extruder. Bambuddy's MQTT parser already skipped 0xE entries when building `ams_extruder_map` (matching BambuStudio's reading for boot-time transient state), so with the FTS installed the map ended up empty and the print modal's filament dropdown — which filters by `extruderId === nozzle_id` to prevent cross-nozzle assignment ("position of left hotend is abnormal" failures) — filtered out *every* loaded slot. Net effect: empty Filament Mapping dropdown on every dual-nozzle print with the FTS, even when the AMS was fully loaded with the right material. Detection comes from a new MQTT field — `print.device.fila_switch` — which is non-null only when the accessory is installed; it carries the routing topology as two arrays: `in[track] = currently fed slot (-1 = empty)` and `out[track] = extruder this track terminates at`. The fix surfaces this through a new `FilaSwitchState` dataclass on `PrinterState` (`installed`, `in_slots`, `out_extruders`, `stat`, `info`) and the equivalent `FilaSwitchResponse` Pydantic schema on the `GET /printers/{id}/status` route. Frontend (`useFilamentMapping.ts` + `FilamentMapping.tsx`) skips the per-extruder filter when `printerStatus.fila_switch?.installed === true` so any compatible AMS slot can satisfy any nozzle's filament requirement, since the FTS handles the routing. Slots currently fed into a track also get a routing badge in the dropdown — `[L]` or `[R]` — so the user can tell at a glance which slot the FTS is currently routing where (idle slots get no badge: they can be routed to either extruder on demand). The hard "no cross-nozzle assignment" filter on real dual-nozzle printers without the FTS stays untouched (still trips the same way it always has — `fila_switch == null` keeps the existing behaviour). 4 backend tests in `test_bambu_mqtt.py::TestFilamentTrackSwitchDetection` (default-not-installed, detect-from-MQTT-using-the-reporter's-bundle, no-fila_switch-field-stays-not-installed, missing-in-out-arrays-don't-crash) and 2 frontend tests in `useFilamentMapping.test.ts` (FTS-active drops the nozzle filter; explicit `fila_switch: null` keeps the filter applied). Upstream fila_switch payloads with anything other than the documented shape are tolerated — `installed` flips on the *presence* of the field, the routing arrays default to empty lists if missing, and the dropdown skips the badge for slots not currently in `in_slots`. ### Fixed +- **Backup restore silently lost most data — settings reverted to defaults, ~most printers/archive rows missing** ([#1211](https://github.com/maziggy/bambuddy/issues/1211), reported by @Carter3DP; same shape as previously-closed [#668](https://github.com/maziggy/bambuddy/issues/668)) — Restoring a settings backup ZIP appeared to succeed but the user found their `energy_cost_per_kwh` reverted to the `0.15` default (defined in `main.py:3457`), 7 of 8 printers gone, 1 GB of archive files on disk but only 1 archive row in the database. #668 was closed in March without an actual fix — that user happened to make it work by rolling back to a stable release, which masked the bug; same shape resurfaces here on a single (consistent) version. **Cause:** the live database runs in WAL mode (`PRAGMA journal_mode = WAL` in `database.py:19`). The original restore endpoint used `shutil.copy2(backup_db, db_path)` after `engine.dispose()`. Two things conspired to make this unsafe: (1) anything the fresh container wrote between startup and the restore call — `seed_default_groups`, `init_db()` migrations, background heartbeat writes — sits in `bambuddy.db-wal` with valid checksums, and `engine.dispose()` doesn't checkpoint it; (2) FastAPI's dependency injection keeps the route handler's own `db: AsyncSession = Depends(get_db)` session checked out across `engine.dispose()` (per SQLAlchemy docs, dispose only closes pooled — not checked-out — connections), so the WAL inode is held open through the whole restore. After `shutil.copy2` rewrote the main DB inode in place, SQLite's WAL recovery on the next `init_db()` happily re-applied the stale frames on top of the restored content, partially clobbering it with fresh-install state. Initial fix attempt of "delete the WAL/SHM/journal sidecars before the copy" turned out to be insufficient — verified experimentally that the still-open request session reads the unlinked sidecars via held fds and bleeds the WAL state back into the new file when it eventually closes. **Real fix:** replace the file copy with SQLite's online backup API (`src_conn.backup(dst_conn)`). The page-by-page protocol opens both DBs as proper SQLite connections, acquires the right locks, and routes new pages through the destination's own WAL — concurrent open sessions see their own transactional snapshot until they close (transaction isolation) but can't corrupt the restored state. Verified via 6 regression tests in `backend/tests/unit/test_restore_sqlite_wal_safety.py`: the buggy `shutil.copy2` path is pinned (the test asserts the bug *manifests* under the un-checkpointed-WAL condition, so a future "small simplification" can't silently re-introduce it); the production `src_conn.backup(dst_conn)` path returns the user's restored values exactly under the same bug condition; the no-WAL-frames case (fresh container, restore as the very first action) round-trips cleanly; and the page-protocol parametrised test runs at 1, 100, and 1000-page DB sizes so a regression at any one size surfaces. PostgreSQL path (`_import_sqlite_to_postgres`) is unchanged — that's row-by-row already and was never affected. + - **`formatTimeOnly` tests failed under non-`:`-separator locales** ([#1213](https://github.com/maziggy/bambuddy/issues/1213), reported by @maugsburger) — Running the frontend test suite under `LC_ALL=en_DK.UTF-8` (or any locale whose `toLocaleTimeString` uses a separator other than `:`) failed two tests in `frontend/src/__tests__/utils/date.test.ts`: `formats time with 12h format` (expected `02.30 pm` to match `/2:30|02:30/`) and `formats time with 24h format` (expected `14.30` to contain `14:30`). The implementation is correct — `formatTimeOnly` calls `date.toLocaleTimeString([], …)` which by design respects the user's locale, so a Danish-English user genuinely should see `02.30 pm` in the UI. The tests just hard-coded the `:` separator. **Fix:** test assertions now use `\D+` (any non-digit, one or more) for the separator: `expect(result).toMatch(/\b0?2\D+30\b/)` and `expect(result).toMatch(/\b14\D+30\b/)`. Tests the actual contract — "the function returns hours and minutes, separated somehow" — without coupling to a specific separator that varies by locale (en_DK uses `.`, some en_* locales use a narrow no-break space at U+202F, most others use `:`). Verified passing under `en_DK.UTF-8`, `en_US.UTF-8`, and `de_DE.UTF-8`. Audited every other `toLocaleTimeString`/`toLocaleString` call site in the test suite — no other places hard-code separator characters; `formatETA`, `formatDateInput` etc. assert via `toBeTruthy()` or check translated content. - **SpoolBuddy kiosk screen-blank timeout setting was ignored after the first save** (reported by maziggy) — Picking a new "Screen Blank Timeout" in SpoolBuddy Settings → Display didn't change the actual blanking behaviour: whatever value was active when the kiosk last booted continued to fire — a user who started with the 10 m preset and then switched to 1 m, 5 m, or "Off" still saw the screen blank at 10 m forever. Cause: blanking is driven by `swayidle`, started once by `spoolbuddy/install/spoolbuddy-idle.sh` at labwc autostart with the timeout passed as a command-line argument (`swayidle -w timeout $T 'wlopm --off' resume 'wlopm --on'`). The script fetched `blank_timeout` from the backend exactly once at startup and `swayidle` has no runtime control surface for changing its timeout. The Python daemon's `display.set_blank_timeout()` updated an in-memory variable on the daemon side that was only used for daemon-side idle bookkeeping (`tick()` log-line) and never reached `swayidle`, so UI changes were silently discarded until the next kiosk restart. Documented as such in the daemon's docstring (`display_control.py:5`: *"swayidle is the sole authority on screen blanking"*) — the architecture predicted the bug, the UX never matched. **Fix:** the wake FIFO at `/tmp/spoolbuddy-wake` now carries a second message in addition to `wake`: `reload-timeout N`. The daemon writes it whenever `set_blank_timeout()` is called with a value that differs from the current one (the very first call is suppressed because the watchdog already fetched the same value at its own startup — signalling there would just thrash `swayidle` on every cold start). The watchdog script's FIFO loop is restructured around `start_swayidle` / `stop_swayidle` helpers and a single `case` statement that dispatches on the message: `wake` → `wlopm --on` + arm a re-blank at the *current* timeout; `reload-timeout N` → kill the running `swayidle`, set `TIMEOUT=$N`, restart `swayidle`, and `wlopm --on` so the user sees the change took effect even if the screen was already blanked. The script de-dupes too — a `reload-timeout N` whose `N` matches the current value is a no-op, so the daemon's local de-dupe and the script's de-dupe both guard against thrash. Going from any positive timeout to `0` ("Off") correctly stops `swayidle` and never restarts it, going from `0` to a positive value starts a fresh `swayidle` — both work without a kiosk restart. The script's main loop opens the FIFO read+write (`exec 3<>"$WAKE_FIFO"`) so the bash `read` never sees EOF when the daemon momentarily disconnects between writes (without that, the loop would exit the first time the daemon closed its write end). A `cleanup` trap on `TERM/INT/HUP` stops `swayidle`, removes the FIFO, and exits cleanly. 7 new tests in `spoolbuddy/tests/test_display_control.py::TestDisplayControlFifoMessages` pin the daemon side of the protocol against a real FIFO in `tmp_path`: `wake()` writes the literal `wake\n` line; first `set_blank_timeout` is suppressed (script already has the right value); subsequent change emits `reload-timeout N\n`; identical-value calls don't signal; transitioning to `0` emits `reload-timeout 0\n` (covers "user picks Off after enabling"); negative inputs are clamped to `0` in the signal payload; missing-FIFO writes are silent no-ops (kiosk-not-running case). Also handles the SpoolBuddy `0` schema default — the first `set_blank_timeout(0)` call from a fresh daemon doesn't signal (init suppressed) so no spurious thrash on a never-configured device. diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index 7ea7559fe..b2086a6e2 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -751,7 +751,38 @@ async def restore_backup( logger.info("Restoring database from backup...") if is_sqlite(): db_path = Path(app_settings.database_url.replace("sqlite+aiosqlite:///", "")) - shutil.copy2(backup_db, db_path) + # Use SQLite's online backup API instead of shutil.copy2. + # The pragma at database.py:19 runs the live DB in WAL mode, + # which means a naive file copy is unsafe: anything written + # to the live DB before this call that hasn't been + # checkpointed yet (seed_default_groups + init_db on first + # start, plus whatever background heartbeats wrote during + # the request window) sits in bambuddy.db-wal with valid + # checksums. The route handler's own `db: Depends(get_db)` + # session also keeps a connection checked out across + # engine.dispose(), holding fds to the WAL inode. With + # `shutil.copy2` SQLite finds the stale WAL on the next + # open and silently re-applies those page-level writes on + # top of the restored DB, partially clobbering it with + # fresh-install state — the user sees a "successful" + # restore where most rows and settings have reverted to + # defaults (#1211 / #668). The page-by-page backup API + # opens both DBs as real SQLite connections, takes the + # right locks, and routes new pages through the live DB's + # own WAL — so concurrent open sessions see their own + # snapshot until they close (transaction isolation) but + # can't corrupt the restored state. + import sqlite3 + + src_conn = sqlite3.connect(str(backup_db)) + try: + dst_conn = sqlite3.connect(str(db_path)) + try: + src_conn.backup(dst_conn) + finally: + dst_conn.close() + finally: + src_conn.close() else: # Import SQLite backup into PostgreSQL logger.info("Importing SQLite backup into PostgreSQL...") diff --git a/backend/tests/unit/test_restore_sqlite_wal_safety.py b/backend/tests/unit/test_restore_sqlite_wal_safety.py new file mode 100644 index 000000000..550be438f --- /dev/null +++ b/backend/tests/unit/test_restore_sqlite_wal_safety.py @@ -0,0 +1,243 @@ +"""Regression tests for the SQLite WAL leftover bug in /settings/restore. + +Background — see #1211 / #668. The live database runs in WAL mode +(``database.py:19``: ``PRAGMA journal_mode = WAL``). Anything written to +the database before the restore call that hasn't been checkpointed yet +sits in ``bambuddy.db-wal`` with valid checksums. The original restore +implementation used ``shutil.copy2(backup_db, db_path)`` which only +overwrites the main DB file's content, so on the next open SQLite found +the stale WAL and silently re-applied those page-level writes on top of +the restored DB — partially clobbering it with fresh-install state. + +These tests exercise the bug condition deterministically (using the +classic reader-snapshot trick to prevent SQLite's close-time checkpoint) +and pin that the production restore path — the SQLite online backup API +called via ``src_conn.backup(dst_conn)`` — produces a clean restored DB +even with un-checkpointed WAL frames sitting on disk. +""" + +from __future__ import annotations + +import shutil +import sqlite3 +from pathlib import Path + +import pytest + + +def _seed_live_db_with_uncheckpointed_wal(live_db: Path) -> sqlite3.Connection: + """Create a SQLite DB in WAL mode with frames that haven't been + checkpointed to the main file. Returns a still-open reader so the + caller can keep the WAL alive and the close-time checkpoint blocked + until the test is ready to assert. + + The returned connection holds an open ``BEGIN`` transaction, which is + what prevents SQLite from auto-checkpointing the WAL on the writer's + close. In production this role is played by the route handler's own + ``db: Depends(get_db)`` session — FastAPI's dependency injection keeps + that session alive across the entire request, ``engine.dispose()`` + doesn't touch checked-out connections, and the WAL accordingly + persists with un-checkpointed frames at the moment the file copy + would happen. + """ + writer = sqlite3.connect(str(live_db)) + writer.execute("PRAGMA journal_mode = WAL") + writer.execute("CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT)") + # These rows are the "fresh-install defaults" — what would clobber the + # restored DB if the WAL were re-applied. + writer.execute("INSERT INTO settings VALUES ('energy_cost_per_kwh', '0.15')") + writer.execute("INSERT INTO settings VALUES ('currency', 'EUR')") + writer.commit() + + reader = sqlite3.connect(str(live_db)) + reader.execute("BEGIN") + reader.execute("SELECT * FROM settings").fetchall() # acquires a snapshot + + writer.close() # WAL persists because reader still holds the snapshot + + # Sanity: the WAL must actually contain frames, otherwise the test is + # vacuous (we'd be testing the safe case, not the bug condition). + wal = live_db.parent / f"{live_db.name}-wal" + assert wal.exists() and wal.stat().st_size > 0, ( + "Test setup failed to leave un-checkpointed WAL frames; the bug condition isn't being exercised." + ) + + return reader + + +def _make_backup_db(backup_db: Path, *, energy: str, currency: str) -> None: + """Build a 'backup' SQLite DB at the given path with the user's actual + settings. Same schema as ``_seed_live_db_with_uncheckpointed_wal`` so + a successful restore should replace the live DB row-for-row.""" + conn = sqlite3.connect(str(backup_db)) + try: + conn.execute("CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT)") + conn.execute("INSERT INTO settings VALUES (?, ?)", ("energy_cost_per_kwh", energy)) + conn.execute("INSERT INTO settings VALUES (?, ?)", ("currency", currency)) + conn.commit() + finally: + conn.close() + + +def _read_settings(db_path: Path) -> dict[str, str]: + """Open a fresh connection and return the settings rows as a dict.""" + conn = sqlite3.connect(str(db_path)) + try: + rows = conn.execute("SELECT key, value FROM settings").fetchall() + return dict(rows) + finally: + conn.close() + + +def test_shutil_copy_loses_to_stale_wal(tmp_path): + """Pin the bug: ``shutil.copy2`` over a live DB with un-checkpointed + WAL leaves the WAL behind, and on the next open SQLite re-applies + those frames on top of the copied content. The user sees a + "successful" restore that mostly reverted to fresh-install defaults + (energy=0.15, currency=EUR) instead of their values (0.12, USD). + + Pinned here so a future "small simplification" that replaces the + backup API call with a file copy can't silently re-introduce the bug. + """ + live = tmp_path / "live.db" + backup = tmp_path / "backup.db" + + reader = _seed_live_db_with_uncheckpointed_wal(live) + _make_backup_db(backup, energy="0.12", currency="USD") + + # The buggy restore: file copy over the live DB. + shutil.copy2(backup, live) + reader.close() + + settings = _read_settings(live) + # The bug manifests as the live DB's WAL frames overwriting the + # restored content. We pin the symptom directly: at least one of the + # user's settings was clobbered by the fresh-install defaults. + assert settings != {"energy_cost_per_kwh": "0.12", "currency": "USD"}, ( + "Expected the shutil.copy2 path to lose data to WAL leftover, " + "but the restore was clean. If this assertion starts failing, " + "either the test setup no longer reproduces the bug condition " + "or SQLite's behaviour changed — re-investigate before relaxing." + ) + + +def test_sqlite_backup_api_replaces_live_db_safely(tmp_path): + """Pin the fix: ``src.backup(dst)`` (SQLite online backup API) over a + live DB that has un-checkpointed WAL frames produces a restored DB + with exactly the backup contents. No fresh-install state leaks + through. + + Mirrors the production path in ``backend/app/api/routes/settings.py`` + (``restore_backup``) so a regression in either the route or the + helper used by it surfaces here. + """ + live = tmp_path / "live.db" + backup = tmp_path / "backup.db" + + reader = _seed_live_db_with_uncheckpointed_wal(live) + _make_backup_db(backup, energy="0.12", currency="USD") + + # The production path: SQLite online backup API. + src_conn = sqlite3.connect(str(backup)) + try: + dst_conn = sqlite3.connect(str(live)) + try: + src_conn.backup(dst_conn) + finally: + dst_conn.close() + finally: + src_conn.close() + + reader.close() + + settings = _read_settings(live) + assert settings == {"energy_cost_per_kwh": "0.12", "currency": "USD"}, ( + f"Restore lost or corrupted user data. Got {settings!r}. If " + "energy_cost_per_kwh is back to 0.15 or currency is back to EUR " + "the WAL leftover bug has regressed — see #1211." + ) + + +def test_sqlite_backup_api_works_when_no_wal_frames(tmp_path): + """Defensive: the fix must also work in the simple case where the + live DB has no leftover WAL (e.g. fresh container, restore as the + very first action). Failing here would indicate the production path + has accidentally become specific to the WAL-leftover scenario. + """ + live = tmp_path / "live.db" + backup = tmp_path / "backup.db" + + # Set up a live DB but force a checkpoint so WAL is empty. + conn = sqlite3.connect(str(live)) + conn.execute("PRAGMA journal_mode = WAL") + conn.execute("CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT)") + conn.execute("INSERT INTO settings VALUES ('energy_cost_per_kwh', '0.15')") + conn.commit() + conn.execute("PRAGMA wal_checkpoint(TRUNCATE)") + conn.close() + + _make_backup_db(backup, energy="0.12", currency="USD") + + src_conn = sqlite3.connect(str(backup)) + try: + dst_conn = sqlite3.connect(str(live)) + try: + src_conn.backup(dst_conn) + finally: + dst_conn.close() + finally: + src_conn.close() + + settings = _read_settings(live) + assert settings == {"energy_cost_per_kwh": "0.12", "currency": "USD"} + + +@pytest.mark.parametrize("backup_size_pages", [1, 100, 1000]) +def test_sqlite_backup_api_handles_various_db_sizes(tmp_path, backup_size_pages): + """The backup API copies in 4 KB pages — make sure single-page, + medium, and multi-page DBs all round-trip correctly. A regression in + backup-API usage that only manifested at one size would otherwise + slip through. + """ + live = tmp_path / "live.db" + backup = tmp_path / "backup.db" + + # Live with un-checkpointed WAL (the bug condition). + reader = _seed_live_db_with_uncheckpointed_wal(live) + + # Build a backup DB sized to roughly the requested page count. + # 4 KB pages ≈ 100 INTEGER rows per page; over-provision a bit. + conn = sqlite3.connect(str(backup)) + conn.execute("CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT)") + conn.execute("CREATE TABLE bulk (id INTEGER PRIMARY KEY, payload TEXT)") + conn.execute("INSERT INTO settings VALUES ('energy_cost_per_kwh', '0.12')") + rows_needed = backup_size_pages * 50 + conn.executemany( + "INSERT INTO bulk (payload) VALUES (?)", + [("x" * 80,) for _ in range(rows_needed)], + ) + conn.commit() + conn.close() + + src_conn = sqlite3.connect(str(backup)) + try: + dst_conn = sqlite3.connect(str(live)) + try: + src_conn.backup(dst_conn) + finally: + dst_conn.close() + finally: + src_conn.close() + + reader.close() + + # Verify both tables round-tripped intact. + conn = sqlite3.connect(str(live)) + try: + energy = conn.execute("SELECT value FROM settings WHERE key = 'energy_cost_per_kwh'").fetchone() + bulk_count = conn.execute("SELECT count(*) FROM bulk").fetchone() + finally: + conn.close() + + assert energy == ("0.12",), f"Expected '0.12', got {energy!r}" + assert bulk_count == (rows_needed,), f"Bulk table size mismatch: expected {rows_needed} rows, got {bulk_count!r}"