dbtool-gui: managed profile backups (backup all / restore with target picker) - #523
Open
Yaraslaut wants to merge 47 commits into
Open
dbtool-gui: managed profile backups (backup all / restore with target picker)#523Yaraslaut wants to merge 47 commits into
Yaraslaut wants to merge 47 commits into
Conversation
Yaraslaut
force-pushed
the
feature/dbtool-gui-managed-backups
branch
from
July 28, 2026 12:40
f52d006 to
2e41d7e
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n string) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ResolveConnectionString returned profile.connectionString as-is before ever consulting secretRef, so a profile combining both (per ProfileStore's contract that secretRef "applies to both forms") never got its password appended. Delegate to Profile::ToConnectInfo for both the raw-string and DSN forms and serialize its SqlConnectInfo result; ToConnectInfo already returns a raw string unmodified when no password was resolved, so the existing "used as-is" behavior is preserved with no extra branching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implement QAbstractListModel subclass for backup status display. Provides rows per configured profile with role-based access to archive metadata (exists/size/mtime) and live run state (idle/queued/running/ok/failed). Used by ManagedBackupController to update the UI during backup operations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds ManagedBackupController: the QObject that owns the managed backup folder setting (persisted via QSettings), computes the effective/default folder, feeds BackupStatusListModel from ManagedBackupCore's pure scan/plan logic, and surfaces folderProblem when the configured path is unusable. Backup/restore invokables land in later tasks; this slice only covers construction, the folder property, setProfiles/setBusyProbe, and refreshStatus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… first
CreateMetadata() and three Restore.cpp helpers (RestoreIndexes,
ApplyDatabaseConstraints, RecreateDatabaseSchema) each declared
`SqlConnection conn;` before immediately calling `conn.Connect(connectionString)`
with the real connection string. The default SqlConnection constructor
auto-connects using the process-global DefaultConnectionString() and throws
on failure — so whenever that global default was never set (any real
embedder that doesn't call SqlConnection::SetDefaultConnectionString(), e.g.
a GUI backing multiple named profiles), these helpers threw before ever
reaching the intended Connect() call, surfacing as a generic
"IM002 ... Data source name not found" error.
The existing SqlBackup/Restore test suite masked this because the Catch2
test binary always calls SetDefaultConnectionString() from --test-env,
making the accidental first connect happen to succeed against the same
database. Switch all four sites to `SqlConnection conn { std::nullopt }`
so only the explicit connectionString parameter is ever used, matching the
existing pattern already used by Backup()'s mainConn and ConnectionPool.
Verified via LightweightTest (sqlite3 in-process: 1240/1241 passed, 1
pre-existing skip; SqlBackup subset also green against mssql2022 via
Docker). No API/behavior change for callers that do set a global default.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds ManagedBackupController::backupAll()/backupProfile(name): a shared StartRunGuard() refuses (with a logged Warning/Error line) when a run is already in progress, the busy probe vetoes, or the managed folder is unusable, then RunBackups() sequences the SqlBackup::Backup() calls on the existing single-thread QThreadPool, publishing per-profile status through BackupStatusListModel via QMetaObject::invokeMethod(..., QueuedConnection) so the model is only ever touched from the GUI thread. Each profile backs up to a `.tmp` sibling first and only replaces the final `.zip` via ManagedBackup::CommitArchive() on success, so a crash or failure mid-run never corrupts a previous good archive; failures are recorded per-profile and the run continues to the next profile. FunctionTask and EmittingProgressManager are copied from BackupRunner.cpp into an anonymous namespace (same worker-closure and progress-forwarding pattern). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RunRestore now takes (archiveProfile, targetProfileName, rawConnectionString, schema): the busy/folder guard runs first so an in-progress run always beats an unknown-target-profile diagnostic, and a looked-up target profile's secret is resolved inside the pool-thread worker (mirroring RunBackups) instead of synchronously in the Q_INVOKABLE restoreArchive body, so a "stdin:" secretRef can no longer block the Qt main thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wire ManagedBackupController into AppController: a managedBackups Q_PROPERTY/accessor/member, log forwarding into the unified feed, a busy probe that defers to the migration and backup runners, and a setProfiles call in loadProfiles so the managed-backup status model stays in sync with the loaded profile store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Backup/restore is graduating from experimental to a regular feature. Removes the --enable-backup-restore CLI option, AppController's backupRestoreEnabled property/SeedBackupRestoreEnabled/_backupRestoreEnabled, and BackupRunner's setEnabled/_enabled gate (including the early-return warning blocks in runBackup/runRestore). Updates the QML bindings in BackupRestoreDialog.qml, SimpleView.qml, and ToolBar.qml that referenced the now-removed backupRestoreEnabled property, and drops the --enable-backup-restore arg from the GUI smoke test harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add BackupsPage.qml: a full-page managed-backups view (backup folder card with Settings link + folderProblem banner, per-profile status rows with Backup/Restore actions and a last-run summary, and the ported custom-archive back up/restore controls). Restore is confirmed through an in-page dialog implementing the restore-confirm mockups: a target picker over all profiles (source preselected) plus a custom connection-string entry, with a cross-target warning. Wire navigation in Main.qml (showBackups flag, StackLayout index 3, toolbar Backup/Restore now refreshes status then opens the page; openSettings routes to the Settings page). Delete the obsolete BackupRestoreDialog.qml and update CMake QML file list. StatusPill gains an optional `label` to decouple the visible caption from its palette key. New tst_backups_page.qml load test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Docs: replace the experimental backup/restore mentions in the dbtool-gui README with a "Managed backups" section, and cross-link it from docs/dbtool.md. Update the README's test-layer table and Layout tree for ManagedBackupCore/ManagedBackupController/BackupStatusListModel and their test files. clang-tidy (scoped to the three new .cpp files via `clang-tidy -p out/build/clang-release`, since dbtool-gui-lib opts out of clang-tidy project-wide): fixed bugprone-empty-catch (log instead of silently swallowing in FunctionTask::run()), readability-avoid-nested-conditional- operator (extracted LevelForState()), cppcoreguidelines-owning-memory x2 (unique_ptr::release() into QThreadPool::start()), and performance-enum-size for Phase/Role. Left cppcoreguidelines-use-enum-class for BackupStatusListModel::Role as an accepted match to five pre-existing sibling Model classes using the identical unscoped-enum-role idiom (same carve-out already granted for Qt identifier-naming). Coverage: dbtool-gui-lib had no enable_coverage_for_target() wiring, so `clang-coverage` builds never actually instrumented any GUI code; added the missing call. Closed reachable gaps with new unit tests (UTF-8 multi-byte SanitizeArchiveName cases, CheckWritableFolder failure branches, BackupStatusListModel invalid-index/role handling, and several ManagedBackupController guard/collision/resolve-failure paths). ManagedBackupCore.cpp now at 100% lines/functions; BackupStatusListModel.cpp 98.2%; ManagedBackupController.cpp 95.8% (up from 90.5%), with the residual gaps being either gcov closing-brace/header-inline tooling artifacts or genuinely hard-to-reach defensive/library-boundary paths (documented in .superpowers/sdd/task-10-report.md). Regression: full clang-release rebuild + ctest (all 5 labels) green against SQLite; LightweightTest also re-run directly against the running MSSQL 2022 Docker container (1238 passed, 3 pre-existing skips, 0 failed). PostgreSQL skipped — no ODBC driver installed on this machine (pre-existing environment limitation, not specific to this change). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CreateMetadata wrote the raw connection string into every archive's metadata.json. With the managed-backup path resolving a profile's secretRef, that embedded plaintext passwords into the archive. Nothing reads the field at restore time (verified by grep) — it is diagnostic only — so redact PWD=/Password= values to *** before writing, at the library level so dbtool benefits too. Adds MetadataRedactionTests (SQLite ignores PWD, so a fake password keeps the connect valid) asserting the plaintext is gone and the value masked. Documents the redaction in sql-backup-format.md and the dbtool-gui README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collision detection previously ran only inside backupAll over the profiles it was handed, so a single-profile backup (one-entry plan, never collides) and RunRestore (independent SanitizeArchiveName) could silently overwrite another profile's archive or restore the wrong data when two names sanitize to the same file. Add PlanEntryFor(name) which plans over the FULL profile list and returns the entry, then honor its collision flag in backupProfile, RunBackups (single-profile path) and RunRestore (refuse before any destructive work). refreshStatus already planned over all names. Also fix the EmittingProgressManager use-after-scope in both this file and BackupRunner: the queued lambda captured the stack-local manager's `this` and dereferenced it on the GUI thread after Backup()/Restore() had returned and destroyed it. Replace with a single queued invoke that reads the target pointer now and copies the args into the event. Adds tests for the refused-collision backup/restore paths (red-first), keeping the earlier owner of the name working. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ManagedBackupController.hpp: document that _pool must be declared first (destroyed last) so ~QThreadPool joins the worker before the members it dereferences are gone. - ManagedBackupCore ScanArchive: check the error_code after file_size / last_write_time and report not-exists on error so a TOCTOU'd file never surfaces uintmax_t(-1) bytes; comment why CheckWritableFolder discards the probe-removal ec. - ManagedBackupCore.hpp: correct the ResolveConnectionString doc — it does NOT mirror connectToProfile; it always resolves secretRef and fails when it cannot. - AppController: refresh the migration model after a successful managed restore into the connected profile (extracted the runner's post-run refresh into RefreshMigrationModelsAfterRun and reused it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Qt's qt_add_executable finalizer embeds a com.yourcompany manifest through lld-link's /MANIFEST:EMBED merge, which re-serializes requestedExecutionLevel into the asm.v2 namespace (ms_asmv2:level). The Windows SxS loader only honors that attribute in asm.v3, so it reports the level attribute as missing and refuses to start the GUI. Ship a hand-written manifest and embed it verbatim via an RT_MANIFEST .rc resource (no XML re-serialization). Its presence also trips Qt's user-manifest skip branch so Qt stops generating the broken one. Windows/MSVC-linker only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…aunch ManagedBackupCoreTests.cpp used ::getpid() unconditionally, which clang-cl rejects on Windows (only _getpid() from <process.h> exists there); added a portable CurrentProcessId() shim. dbtool-gui-tests and dbtool-gui-qmltest hit the same lld-link manifest-merge SxS bug already fixed for the dbtool-gui executable (commit f60871f) - both targets now embed the shared ../dbtool-gui.manifest via a new dbtool-gui-tests.rc RT_MANIFEST resource instead of letting Qt/lld-link generate and corrupt one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
resetProfiles() left the previous rows' BackupTableListModel children parented to the status model but unreachable; release them explicitly before rebuilding _rows. Adds a QPointer regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…models Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…panel _pillStatus mapped the "warning" state to the neutral "empty" palette, so a table that finished with a warning looked identical to an untouched row. Map it to the warn-coloured "pending" key. Also introduced a `hasTotal` property on the table-row delegate to dedupe the repeated `totalRows > 0` check shared by the meta label and progress bar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…required The detail panel's empty-state binding depends on the model's non-reactive rowCount(); the `_detailTablesModel = null` pass-through forces the property change that makes the panel re-read it. Flagged by the whole-branch review as fragile-if-removed. Comment only, no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The live per-table details were rendered by a GridLayout nested inside the Profiles Card's plain Column, where Layout.preferredWidth is ignored (see Card.qml's own note) — so the profile list and the details panel both drew at full width and overlapped, and the details were unreadable. Restructure the Backups page into a real page-level GridLayout: a fixed-width left rail (backup folder, profile master-list, custom archive) and a flexible right region that gives the live details their own column. Collapses to a single stacked column on a narrow window. Widen the centred content to 1180px to make room for the split. Also fix a latent binding bug in BackupDetailPanel: the empty-state and the table ListView keyed their visibility on the model's rowCount — but rowCount is an invokable METHOD on QAbstractListModel, so that expression is a function object, never == 0 or > 0. The result: the empty state only showed for a null model and the live table list would NEVER appear during a run. Key both on the ListView's reactive count instead. The panel is now transparent (it fills a host Card) with a header divider, a workers chip, and a proper empty state. Verified: full build clean, dbtool-gui-tests 240/66 and dbtool-gui-qmltest pass; the running GUI renders the two-region layout with no overlap and shows the empty state (screenshotted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two fixes to the managed-backups master-detail page, both from user feedback on a run with ~300 profiles and hundreds of tables: 1. Independent scrolling. The whole page previously lived inside one ScrollView, so scrolling the 300-profile list scrolled the details region away with it — to see live progress you had to scroll back to the top. The page is now a fixed header + a fill-height master-detail body. The profile master-list is its own ListView with its own scrollbar, and the detail panel's active-table ListView has another; neither scrolls the other. 2. Summary + active-only detail. The detail panel no longer lists every table. It shows a compact summary — "142 / 700 tables done", an aggregate progress bar, and per-state chips (running / queued / error / warning) — then a live list of ONLY the in-flight tables (running, error, warning). Completed and queued tables are filtered out so the handful actually in progress are never buried. Implementation: - BackupTableListModel gains cached per-state tallies (totalCount, doneCount, runningCount, queuedCount, errorCount, warningCount) exposed as NOTIFY-backed Q_PROPERTYs, recomputed on every applyProgress/clear. The summary reads these instead of scanning rows in QML. - New BackupActiveTablesModel (QSortFilterProxyModel) surfaces only running/error/warning rows and re-filters automatically as tables transition state — done in C++ rather than fragile zero-height delegate collapsing in QML. The detail panel owns one and points it at the selected profile's table model. - New StatChip.qml — compact count badge (coloured dot + count) for the summary chips, keyed by state. - BackupsPage.qml: page-level fixed-header + fill-height GridLayout; the Profiles master-list is a bordered fill-height panel wrapping a ListView (a Card is content-sized and can't host a scrolling list); the detail region is likewise a fill-height panel so its inner list can scroll. Tests: - BackupTableListModelTests: summary tallies (by state, in-place transitions, unknown-state bucket) + summaryChanged signal firing. - BackupActiveTablesModelTests: active-only filtering, live re-filter on state transition, roleNames pass-through, source reset. Verified: dbtool-gui-tests 283 assertions / 76 cases pass; dbtool-gui-qmltest 18/18 pass; GUI launches clean; running-state layout confirmed by screenshot (summary + 6 active rows, 140 done rows hidden; both regions scroll independently). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two backups-page fixes from user feedback on a run with 325 profiles: 1. Profile search. The profile master-list had no way to jump to a profile short of scrolling hundreds of rows. Add a search field above the list that filters by name (case-insensitive substring), with a clear affordance and a "no matches" placeholder. Filtering is done in C++ by a new BackupProfileFilterModel (QSortFilterProxyModel over the status model), mirroring BackupActiveTablesModel — the ListView binds to the proxy while selection / auto-follow still resolve against the unfiltered status model, so filtering never changes which profile's details are shown. Whitespace is trimmed so a stray space does not hide every row, and the caption switches to "X of Y match" while filtering. 2. Correct the worker-count tooltip. It claimed "MS SQL Server is limited to 1 worker by the driver", which is false: SqlBackup::Backup spawns `concurrency` worker threads for every DBMS (SqlBackup.cpp, iota over 0..concurrency) with no MSSQL special-casing — concurrency is only floored at max(1U, …), never capped to 1 for SQL Server. The GUI passes BackupConcurrency() (min(hw, 8)) for all backends alike. Tooltip now reads "Tables are backed up in parallel by up to N worker thread(s)." Verified against a live MSSQL run — all 8 workers active. Tests: - BackupProfileFilterModelTests: empty source, empty-text pass-through, case-insensitive substring match, no-match, clear, whitespace trim, filterTextChanged fires only on a real change, re-filter on source reset. Verified: dbtool-gui-tests 301 assertions / 84 cases pass; dbtool-gui-qmltest 18/18 pass; GUI smoke test alive; manually confirmed by the user (search + parallel MSSQL execution both working). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Yaraslaut
force-pushed
the
feature/dbtool-gui-managed-backups
branch
from
July 28, 2026 13:05
2e41d7e to
56e80eb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The comment above `_pool` claimed it "MUST be declared first so it is destroyed last", which is backwards: non-static members are destroyed in *reverse* declaration order, so declaring `_pool` first destroyed it last — i.e. `_status`, `_profiles` and `_busyProbe` were already gone by the time `~QThreadPool` joined the worker. That is precisely the use-after-free the comment said it was preventing. Benign today only because the worker publishes everything through queued `invokeMethod`, which a destroyed QObject drops. Move `_pool` to the end of the member list, where reverse-order destruction really does join the pool before anything the worker touches goes away, and rewrite the comment to describe what the language actually does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`std::filesystem::path(qstring.toStdString())` was used to turn the configured backup folder into a path. `QString::toStdString()` produces UTF-8, but MSVC's narrow `fs::path` constructor decodes its argument in the process' active code page — so `C:\Users\Müller\Sicherungen` became mojibake and the archives landed somewhere the user never picked. The reverse conversion (`path::string()`, used in log lines and in ManagedBackupCore's error messages) has the same defect in the other direction. Route both directions through UTF-16 / UTF-8 instead: - `ToPath()` / `FromPath()` (controller-local) convert via `toStdU16String()` / `fromStdU16String()`, which is lossless on every platform Qt supports. - `ManagedBackup::PathToUtf8()` renders a path for the `std::string` error messages the core layer produces, replacing four `.string()` calls that would otherwise mangle a non-ASCII folder name in the very message that names it. Covered by two new ManagedBackupCore tests: a UTF-8 round-trip and a `CheckWritableFolder` failure whose message must still spell "Müller". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BLOCKER (backup). `SqlBackup::Backup()` does not throw when individual tables fail: `ChunkWorker` catches the per-chunk exception, marks the chunk failed, reports `Progress::State::Error`, and returns normally. The established way to notice is `ProgressManager::ErrorCount()` — exactly what the CLI gates its exit code on (`return pm->ErrorCount() > 0 ? EXIT_FAILURE : EXIT_SUCCESS`). `EmittingProgressManager` derived from `ProgressManager`, whose `ErrorCount()` is a hard-coded `0`, and nothing called it. So a run that silently dropped tables still reached `CommitArchive(tmp, final)` — the atomic rename replaced the last good archive with the partial one — and the profile was marked "ok". The only copy of the data the user had was destroyed by a backup that reported success. BLOCKER (restore). Same shape: `CreateTablesInOrder` drops each archived table, and when the `CREATE TABLE` fails it reports `State::Error` and omits the table from `createdTables` without throwing. `RunRestore` caught only `std::exception`, so `ok` stayed true and it emitted `finished(true, ...)` — a green banner over a database that had just lost the tables it failed to recreate. Fix, for both paths: - `EmittingProgressManager` now derives from `ErrorTrackingProgressManager` and chains its `Update()`, so `ErrorCount()` is real. - Backup: a non-zero count aborts before the commit. The temporary archive is deleted, the previous archive is left untouched, the profile is marked `failed` with the reason, and the run summary counts it as a failure. An error log line names the profile. - Restore: a non-zero count (or a thrown exception) reports the run as failed *and* states that a restore is not transactional — tables are dropped and recreated one at a time, so the target may be left incomplete and must not be used before a successful re-run. Testing this needs a run that reports table errors without throwing, which no live SQLite database will do on demand, so the two SqlBackup calls become injectable operations (`setBackupOperation` / `setRestoreOperation`, defaulting to the real API and restored by passing an empty callable). The operation is copied into the worker task on the GUI thread, so the worker never races a replacement. New tests, all three of which fail without this change: - a backup whose tables failed is reported as failed and never committed - a failed backup leaves the previous good archive byte-identical - a restore whose tables failed is reported as failed and says the target may be INCOMPLETE plus a counter-example proving a warning-only run still commits, and that an empty callable restores the real backup path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AppController` installed a busy probe on `_managedBackups` only. `MigrationRunner::applyUpTo` and `BackupRunner::runBackup/runRestore` checked nothing but their own `_phase`, and no QML binding disabled them on `managedBackups.phase` — the custom-archive buttons gated on `BackupRunner.Idle` alone, and SimpleView's "Run migrations" had no `enabled` condition at all. So the guard was one-way: a migration, or an ad-hoc restore, could start *during* a managed backup. They share the same databases, so the archive that run was about to atomically commit — and mark "ok" — would be torn. Make it mutual, in both layers: - `MigrationRunner::setBusyProbe()` gates the mutating entry points (`applyUpTo`, `applySelected`, `rollbackToRelease`) through a shared `CanStartMutatingRun()`. Dry-runs are read-only and stay ungated. - `BackupRunner::setBusyProbe()` gates `runBackup` / `runRestore` through `CanStartRun()`. - Both log a warning on refusal, so a click that does nothing is explained rather than silently swallowed. - `AppController` wires all three probes to each other. - QML `enabled` bindings mirror the same condition on the Backups page, ActionsPanel (apply + rollback) and SimpleView (Run migrations, Retry), so a blocked action reads as disabled instead of dead. Covered by RunnerBusyGuardTests (both runners refuse and log while the probe reports busy; an unwired probe changes nothing) and an AppController test that starts a managed run and asserts the ad-hoc backup, the ad-hoc restore and a migration are all refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Backups page's "Restore…" button next to the custom archive path called `BackupRunner.runRestore` directly — one click, no confirmation, on an action that drops and recreates every table of the *connected* database. The trailing ellipsis promised a dialog, the adjacent per-profile restore has one, and this surface used to sit behind `--enable-backup-restore`, which this branch removed — so it is now on by default, roughly 6px from "Backup". Route it through a confirmation dialog with the same destructive warning, the archive path, and the target it will overwrite. The warning banner itself moves into a shared `DestructiveWarningBanner` component used by both restore dialogs, so their wording and prominence cannot drift apart. Covered by two QML tests: clicking the button opens the dialog and leaves the runner idle, and the shared banner carries the destructive wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`RedactConnectionStringSecrets()` scanned for the next `;` and ignored
ODBC's `{...}` quoting, so a password that legitimately contains a
semicolon was only half masked: `PWD={pa;ss}` was written into an
archive's `metadata.json` as `PWD=***;ss}` — cleartext credential
material inside a file that gets copied around. The complementary
`connectionString[pos - 1] != ';'` boundary check also meant a separator
followed by whitespace (`...; PWD=secret`) escaped redaction entirely,
even though driver managers accept that spelling.
Parse the string attribute-wise instead:
- an attribute value that starts with `{` runs to its matching `}`
(with `}}` treated as an escaped brace) and is masked whole, so an
embedded `;` neither ends the value nor starts a new attribute;
- the key may be preceded by whitespace after the separator, and
followed by whitespace before the `=`;
- key matching stays case-insensitive and whole-attribute only, so
`MyPWD=` and `Database=PasswordVault` are still left alone.
Adds coverage for all of it to MetadataRedactionTests, which previously
exercised neither brace quoting nor whitespace, plus the malformed
segments the new attribute walk has to step over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records in the README what the preceding commits changed (partial runs are failures, restores warn about a possibly incomplete target, the mutual busy guard, restores are always confirmed) and adds a "Known limitations" section for the issues found in review that are deliberately out of scope here: no archive retention/generations, no cancellation for a running backup-all, and the live-details cosmetic inaccuracies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ProgressManager` had no way to learn how many tables an operation would process, so a consumer rendering "n / total tables" could only count the distinct table names it had seen so far. Rows appear lazily, on each table's first progress report, so that total climbed for the whole run: the GUI's per-table panel showed "12 / 12", then "12 / 47", then "12 / 700" on a single backup, which reads as the job growing rather than as progress being made. Both paths already know the answer before any data moves — Backup after its schema scan enumerates every table, Restore after building its per-table progress map from the archive manifest. Add a `SetTotalTables()` hook and call it at those two points. The hook has a default no-op body, so every existing ProgressManager (dbtool's CLI reporters included) keeps compiling and behaving unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`BackupTableListModel::totalCount()` returned `_rows.size()`, and rows are
created lazily on each table's first progress report. The denominator of
the detail panel's "n / total tables" readout was therefore a count of
tables seen so far, and climbed throughout the run instead of standing
still — the panel showed "12 / 12", then "12 / 47", then "12 / 700" on one
backup.
Route SqlBackup's new `SetTotalTables()` announcement through to the
model: the worker-thread progress manager emits `tableTotalKnown` (queued,
like every other signal it raises, because the manager is a stack local
that may die the moment Backup()/Restore() returns), the controller routes
it to the matching profile, and `totalCount()` now prefers that announced
value.
Three details that keep the readout honest:
* falls back to counting rows when nothing was announced, so a progress
source predating the hook still shows something sensible;
* takes max(announced, seen) so the denominator can never come out below
the numerator if more tables report than were announced;
* cleared by clearTables(), so a new run cannot inherit the previous
run's denominator.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The GUI used emoji as inline icons. Emoji are drawn from the platform's
colour-emoji font, which makes them unfit for this purpose:
* they ignore `color`, so they cannot follow the light/dark Theme
palette. The destructive-restore banner is the clearest case: its
warning sign rendered as a yellow-and-black sticker on the red error
surface, contradicting the banner it sat in.
* they differ per platform (Segoe UI Emoji, Apple Color Emoji, Noto), so
metrics and weight shift across the platforms CI builds for.
* a missing face draws tofu.
Add `Glyph.qml`: eight icons (folder, archive, search, workers, warning,
check, cross, chevron) drawn with QtQuick.Shapes from strokes and fills
that take `color`, on a 12x12 authoring grid scaled by `size`. Selection
is done with a Loader per glyph rather than a `visible:` binding per path,
because `ShapePath` is not an `Item` and has no `visible` property —
gating paths that way makes the whole type silently fail to load.
Also add two supporting pieces and the tokens they need:
* `ProgressTrack.qml` — the stock QtQuick.Controls ProgressBar renders
through the platform style, so its height and colours came from the
system rather than from Theme (a chunky Fluent bar ignoring the app
accent), and its indeterminate animation could not be tinted at all.
* `PageHeader.qml` — eyebrow / title / context stack plus right-aligned
actions, so a page's identity strip is not re-hand-rolled per page.
* Theme gains a named radius scale and one elevation, replacing radii
that were hard-coded per call site and had drifted.
Requires Qt6::QuickShapes for `import QtQuick.Shapes`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The page presented per-profile data in a way that made the profile list —
the one surface whose job is comparing profiles — unusable, and put a
destructive action six pixels from a safe one.
Profile rows are now their own component (`ProfileRow.qml`), laid out as
the name on its own full-width line with status, archive metadata and the
actions on a second line beneath it:
* The name is no longer elided. Profile names run to ~50 characters and
differ in their tails ("...-eu-west-1" vs "...-eu-west-2"), so
truncating to fit a pill and two buttons on one line removed precisely
the part that tells two rows apart, and the list showed several
visually identical entries. It wraps instead (WrapAnywhere: these are
single unbroken tokens, so word-boundary wrapping alone cannot break
them). Rows are variable-height as a result, which the delegate must
opt into with an explicit `height` — a ListView positions by `height`
and does not adopt an `implicitHeight`, so without it wrapped rows
overlap their neighbours.
* The status pill was vanishing entirely. StatusPill is a plain
Rectangle carrying only `implicitWidth`, and a RowLayout will shrink
any child below its implicit size unless told otherwise — so the
fillWidth name Label beside it claimed the row and squeezed the pill
to zero width. Moving it off the name's line removes the contention.
* Per-row archive metadata (mtime, size, or the error text) is back on
each row. It had been hoisted into a single shared line under the list
showing only the selected profile's values, which is what made every
row look alike.
* Row actions reveal on hover or selection, and are disabled while
hidden so a click cannot land on an invisible "Restore...".
Elsewhere on the page:
* The custom-archive "Restore..." is separated from "Backup" into its own
band that names what it destroys. The confirmation dialog remains the
real guard; this stops the two from looking interchangeable.
* The backup folder moves from a whole Card into the page header's
context line, so the path stays visible (it determines what "Backup
all" overwrites) without costing the list vertical space. Only the
folder *problem* still takes page room, since it blocks every action.
* Nothing interpolates a profile name into a fixed-width dialog title or
a button label any more: "Restore into <50 chars>" produced a button
wider than its own 460px dialog, and a dialog title bar does not
elide. The archive row splits name and size onto two lines so eliding
can no longer discard the size, which is the value being checked.
* Per-table row counts are abbreviated ("203M / 320T", exact figures on
hover). Printed raw they read "203123123 / 320492304592345", which
overflowed a 76px column and is unreadable regardless.
Add 12 QML tests asserting measured geometry, not just instantiation —
every bug above passes a "does it load" check. Two notes for whoever edits
them: `item.visible` is useless here (it is inherited, and the test parent
is not shown, so every child reports false — assert width/height instead),
and text cannot be pixel-diffed under the offscreen platform CI uses
because no fonts are deployed there and glyphs rasterise to nothing, so
the name assertions go through Qt's text metrics.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a managed backup workflow to dbtool-gui: configure a backup folder once (Settings), press Backup all to back up every profile's database into it — one safely-overwritten
<profile>.zipeach — and restore any archive into its own profile, a different profile, or a custom connection string. Backup/restore is promoted out of the experimental--enable-backup-restoreflag.Design doc:
docs/superpowers/specs/2026-07-11-dbtool-gui-managed-backups-design.md(visual mockups maintained in the Claude Design project dbtool-gui).What's new
ManagedBackupCore— pure, Qt-free decision logic (archive naming/sanitization + collision detection, folder scan, atomic tmp→rename overwrite, ProfileStore+SecretResolver connection-string resolution).ManagedBackupController+BackupStatusListModel— sequential per-profile orchestration on a worker thread; per-row status (queued/running/ok/failed + archive size/date); guards against concurrent migration/backup runs; secrets resolved off the GUI thread.BackupsPage.qml— replaces the bareBackupRestoreDialog; profile list with status pills, per-row Backup/Restore, destructive-restore confirmation with a Restore to target picker (cross-target warning), and a Custom-archive section for ad-hoc paths.backup/folder).SqlBackup/Restorehelpers no longer connect via the unset global default connection string;metadata.jsonnow redactsPWD=/Password=values instead of embedding resolved plaintext credentials.--enable-backup-restore,AppController::backupRestoreEnabled,BackupRunner::setEnableddeleted.Performance
Backup/restore run with
jobs = min(8, hardware_concurrency)and zstd (fallback deflate). Benchmarked on an 87 MB / 800k-row database: restore ~2.4–3 s (MSSQL 2022 jobs=8 / SQLite) vs ~6 s at dbtool's CLI default--jobs 1on MSSQL.Risk assessment
BackupRunner) was fixed in both copies.SqlConnection{std::nullopt}fix is behavior-preserving for all existing callers (every site immediately callsConnect(...)); metadata redaction is write-only data no restore path consumes.fs::pathconversions (pre-existing pattern) may mishandle non-ASCII folder names — follow-up candidate.Code coverage (clang-coverage, dbtool-gui label)
ManagedBackupCore.cpp100% ·BackupStatusListModel.cpp98.2% ·ManagedBackupController.cpp95.8% (residual: gcov brace artifacts + library-boundary error paths, documented).Databases tested
LightweightTest(1242 passed) + all dbtool-gui suites (204 assertions / 56 cases, qmltest, smoke).LightweightTest [SqlBackup]2558 assertions / 170 cases + end-to-end dbtool backup verifyingPWD=***in archive metadata.Update — live per-table backup "Details" panel
Adds a live master-detail view to the Backups page so a running backup/restore shows real-time per-table progress, and fixes a Windows launch blocker.
Design doc:
docs/superpowers/specs/2026-07-11-backup-live-details-design.md· plan:docs/superpowers/plans/2026-07-11-backup-live-details.md.What's new
BackupTableListModel— GUI-thread-confined per-table model (one per profile), keyed by table name: rows create-on-first-sight and update-in-place, tolerating the up-to-8 concurrent table workers reporting in any order. Cleared at each run start; rows persist after a run so a finished profile still shows its last per-table results.ManagedBackupControllergains a queuedtableProgress(profile, table, current, total, state, message)signal;EmittingProgressManagernow carries the profile name and emits it alongside the existinglogLine. A GUI-thread slot routes each update into the right profile's table model. Restore progress routes under the archive profile too.BackupDetailPanel.qml— the live pane: per-table progress bar (indeterminate until the row total is known), a state badge (queued/running/done/error/warning), and the latest message, plus an "N workers" caption surfacing the existing 8-thread table parallelism (see the later follow-up correcting the MSSQL worker claim).BackupsPage.qmlmaster-detail split — the Profiles card becomes a profile list + detail pane (side-by-side when wide, stacked when narrow). Selection auto-follows the running profile and a click pins a specific one.dbtool-gui.exe(and the test exes) failed to start with an SxS "requestedExecutionLevel level missing" error: lld-link's/MANIFEST:EMBEDmerge rewrote the manifest into the wrong XML namespace. Now a hand-written manifest is embedded verbatim via an.rcRT_MANIFESTresource. Also fixed a::getpidportability break in the GUI test target on clang-cl.Parallelism
No engine change — backups already run tables with
jobs = min(8, hardware_concurrency). This change makes that concurrency visible in the UI.Risk assessment
invokeMethodcalls (args copied into the event, nothis/manager capture). A whole-branch review traced the worker->GUI hand-off, clear-before-progress FIFO ordering, and mid-run profile-reload lifetime — no data race or use-after-free.deleteLater()'d on profile reload (a leak found in review and fixed with aQPointerregression test)..rcchange (no-op elsewhere); DBMS-agnostic (Progresscontract is per-DBMS-neutral).Tests
dbtool-gui-tests: 240 assertions / 66 cases pass (was 204/56) — new per-table-model tests, the reload-leak regression test, and a controller test that runs a real SQLite backup and assertstableProgressfires and the profile's model is populated.dbtool-gui-qmltest: 18/18 (adds aBackupDetailPanelsmoke test).Layout fix — details as a dedicated master-detail region
The first cut nested the live details inside the Profiles card's
Column, whereLayout.*sizing is ignored, so the list and the details overlapped. Reworked into a page-levelGridLayout: a fixed-width left rail (folder + profile master-list + custom archive) and a flexible right region that gives the details their own column (stacks on a narrow window). Also fixed a latent bug where the panel keyed visibility ontablesModel.rowCount— an invokable method, so the table list would never have appeared during a run — now keyed on the ListView's reactivecount. Verified in the running GUI (no overlap; correct empty state).🤖 Generated with Claude Code
Follow-up — independent scrolling + summary/active-only details
Two more fixes from real-fleet feedback (~300 profiles, hundreds of tables):
ScrollView, so scrolling the 300-profile master-list scrolled the details away — you had to scroll back to the top to watch progress. It's now a fixed header + a fill-height master-detail body, where the profile list and the detail panel's active-table list each own their own scrollbar and scroll independently. (The Profiles master-list moved out of a content-sizedCardinto a fill-height bordered panel so its innerListViewcan take the slack.)BackupActiveTablesModel(QSortFilterProxyModel) that re-filters automatically as tables transition state — done in C++, not via fragile zero-height QML delegates. NewStatChip.qmlrenders the summary chips;BackupTableListModelgains NOTIFY-backed per-state count properties feeding the summary.summaryChangedfiring (BackupTableListModelTests), and active-only filtering incl. live re-filter on state transition, roleNames pass-through, and source reset (BackupActiveTablesModelTests). Suite now 283 assertions / 76 cases; qmltest 18/18. Running-state layout verified by screenshot (summary + 6 active rows, 140 done rows hidden; both regions scroll independently).Follow-up — profile search + corrected MSSQL worker claim
Two more fixes from a run with 325 profiles:
BackupProfileFilterModel(QSortFilterProxyModelover the status model), mirroringBackupActiveTablesModel: theListViewbinds to the proxy while selection / auto-follow still resolve against the unfiltered status model, so filtering never changes which profile's details are shown. Search text is whitespace-trimmed (a stray space won't hide every row) and the list caption switches to "X of Y match" while filtering.SqlBackup::Backupspawnsconcurrencyworker threads for every DBMS (iota over0..concurrency) with no MSSQL special-casing;concurrencyis only floored atmax(1U, …), never capped to 1 for SQL Server, and the GUI passesBackupConcurrency()(min(hw, 8)) for all backends alike. Tooltip now reads "Tables are backed up in parallel by up to N worker thread(s)." Confirmed against a live MSSQL run — all workers active.BackupProfileFilterModelTests— empty source, empty-text pass-through, case-insensitive match, no-match, clear, whitespace trim,filterTextChangedfires only on a real change, and re-filter on source reset. Suite now 301 assertions / 84 cases; qmltest 18/18. Search + parallel MSSQL execution both manually confirmed by the maintainer.Follow-up — review fixes: two data-loss defects + four smaller ones
A review of this branch turned up two ways the feature could destroy data while
reporting success, plus four smaller defects. All six are fixed here (7 commits,
c2219e4d..669de920).🔴 BLOCKER — a backup that lost tables was committed over the last good archive and reported "ok"
SqlBackup::Backup()does not throw when individual tables fail:ChunkWorkercatches the per-chunk exception, marks the chunk failed, reportsProgress::State::Error, and returns normally. The established way to notice isProgressManager::ErrorCount()— exactly what the CLI gates its exit code on(
return pm->ErrorCount() > 0 ? EXIT_FAILURE : EXIT_SUCCESS,dbtool/main.cpp).EmittingProgressManagerderived fromProgressManager, whoseErrorCount()is a hard-coded
0, and nothing called it. So a run that silently droppedtables still reached
CommitArchive(tmp, final)— the atomic rename replacedthe last good archive with the partial one — and the profile was marked
"ok".Since there is exactly one archive per profile, the only copy of the data the
user had was destroyed by a backup that reported success.
Fix:
EmittingProgressManagernow derives fromErrorTrackingProgressManagerand chains itsUpdate(). A non-zeroErrorCount()aborts before the commit: the temporary archive is deleted, theprevious archive is left untouched, the profile is marked
failedwith thereason, an error line is logged, and the run summary counts it as a failure.
🔴 BLOCKER — restore reported success after dropping tables it then failed to recreate
Same shape on the restore side:
CreateTablesInOrderdrops each archived tableand, when the
CREATE TABLEfails, reportsState::Errorand omits the tablefrom
createdTableswithout throwing.RunRestorecaught onlystd::exception, sookstayedtrueand it emittedfinished(true, …)— agreen success banner over a database that had just lost the tables it failed to
recreate.
Fix: same
ErrorCount()mechanism. A failed restore additionally statesthat a restore is not transactional — tables are dropped and recreated one
at a time and data is loaded by concurrent workers — so the target may be left
incomplete and must not be used before a successful re-run. The thrown-exception
path carries the same warning.
Because no live SQLite database will report table errors without throwing on
demand, the two
SqlBackupcalls became injectable operations(
setBackupOperation/setRestoreOperation, defaulting to the real API, anempty callable restoring the default). The operation is copied into the worker
task on the GUI thread, so the worker never races a replacement.
🟠 HIGH — the busy guard was one-way
AppControllerinstalled a busy probe on_managedBackupsonly.MigrationRunner::applyUpToandBackupRunner::runBackup/runRestorecheckednothing but their own
_phase, and no QML binding disabled them onmanagedBackups.phase(SimpleView's "Run migrations" had noenabledcondition at all). A migration — or an ad-hoc restore — could therefore run
during a managed backup, producing a torn archive that was then committed and
marked "ok".
Fix: mutual, in both layers.
MigrationRunnerandBackupRunnereach gaineda
setBusyProbe(), consulted by a shared entry guard that logs a warning onrefusal;
AppControllerwires all three probes to each other; and the QMLenabledbindings on the Backups page, ActionsPanel and SimpleView mirror thesame condition. Dry-runs are read-only and stay ungated, matching the C++ side.
🟠 HIGH — the destructive ad-hoc restore had no confirmation
The custom-archive "Restore…" button called
BackupRunner.runRestoredirectly: one click, no confirmation, on an action that drops and recreates every
table of the connected database. The ellipsis promised a dialog, the adjacent
per-profile restore has one, and this surface used to sit behind
--enable-backup-restore— removed in this PR, so it is now on by default,~6px from "Backup".
Fix: routed through a confirmation dialog showing the archive path and the
target it will overwrite. The red warning banner moved into a shared
DestructiveWarningBanner.qmlused by both restore dialogs so their wording andprominence cannot drift apart.
🟡 MEDIUM — inverted destruction-order comment (and a violated invariant)
ManagedBackupController.hppclaimed_pool"MUST be declared first so it isdestroyed last". Non-static members are destroyed in reverse declaration
order, so declaring
_poolfirst destroyed it last —_status,_profilesand
_busyProbewere gone before~QThreadPooljoined the worker, which isprecisely the use-after-free the comment said it prevented. Benign today only
because the worker publishes everything through queued
invokeMethod._poolnow really is declared last, and the comment describes what the language does.
🟡 MEDIUM —
RedactConnectionStringSecretsleaked brace-quoted passwordsThe scan looked for the next
;and ignored ODBC{…}quoting, soPWD={pa;ss}was written into an archive'smetadata.jsonasPWD=***;ss}—half the password in cleartext, inside a file that gets copied around. The
connectionString[pos-1] != ';'boundary check also let…; PWD=secret(aseparator followed by whitespace, which driver managers accept) escape redaction
entirely.
Fix: the string is parsed attribute-wise. A value starting with
{runs toits matching
}(}}= escaped brace) and is masked whole, so an embedded;neither ends the value nor starts a new attribute; whitespace is tolerated around
the key; matching stays case-insensitive and whole-attribute only, so
MyPWD=and
Database=PasswordVaultare still untouched.MetadataRedactionTestscovered neither case before and now covers both, plus malformed segments.
🟡 MEDIUM — Windows non-ASCII backup folder was mangled
std::filesystem::path(qstring.toStdString())fed UTF-8 into MSVC's narrowfs::pathconstructor, which decodes in the active code page — soC:\Users\Müller\Sicherungenbecame mojibake and archives landed somewhere theuser never picked.
path::string()has the same defect in reverse, including inthe error messages that name the folder.
Fix:
ToPath()/FromPath()convert via UTF-16 (toStdU16String()/fromStdU16String()), andManagedBackup::PathToUtf8()replaces four.string()calls in the core layer's messages.Known limitations (confirmed in review, deliberately not addressed here)
and a successful run replaces it. The failure gate above means a bad run can
no longer destroy the only copy, but there is still no history to roll back to.
~QThreadPoolblocks processexit until the current run finishes.
clearTables()orsetRunState("running"); a cross-target restore animates the sourceprofile's row; the details header's worker count is hard-coded to
8in QMLwhile C++ uses
min(hardware_concurrency, 8);restoreDialog.openFordefaults to
currentIndex = 0when the archive's own profile is not in thetarget list.
These are recorded in
src/tools/dbtool-gui/README.mdunder "Known limitations".Tests added
a backup whose tables failed is reported as failed and never committed.tmp, run reported as failure, profilefaileda failed backup leaves the previous good archive in placea restore whose tables failed is reported as failed and warns about the targetfinished(false, …)+ "INCOMPLETE" in the summary and the loga clean injected run still commits and reports successRunnerBusyGuardTests(4 cases)the busy guard between the three runners is mutualtst_backups_page(2 new)PathToUtf8+CheckWritableFoldernon-ASCII (2 cases)MetadataRedactionTests(4 new cases);inside another quoted value, whitespace after the separator, malformed segmentsThe three failure-semantics tests were verified to fail against the
pre-fix code (the partial archive overwrote the good one).
dbtool-gui-tests: 370 assertions / 98 cases (was 301/84).dbtool-gui-qmltest: 20/20.Verification
clang-debug(ASan + UBSan + clang-tidy,-Werror)gcc-releasetest_dbtool.py(clang-debug)dbtool-guitargets built andctest -L dbtool-guigreen (3/3: Catch2,qmltestrunner, process smoke).
clang-tidyclean — noNOLINTadded.clang-formatapplied to every changed file.Risk: the failure gate makes previously-"successful" partial backups report
as failures — that is the intended behaviour change and the only user-visible
regression risk. The mutual busy guard can disable a button that used to be
clickable while another run is in flight; the "back up first, then migrate" flow
in SimpleView is unaffected because the backup runner is already
Idlewhen itsfinishedsignal fires. No library ABI change beyondRedactConnectionStringSecrets's (unchanged) signature; no per-DBMS behaviourchange. Performance impact: none — the added work is one integer comparison per
profile per run.