rbrooks/Quest-Board

Watch

Fork 0

[Backend] A backup shipped off-host cannot be read back: its local path is overwritten by the rclone spec #753

New issue

Closed

opened 2026-09-22 16:11:50 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-09-22 16:11:50 +00:00

Collaborator

Found while building the first-run setup wizard's backup step (#728, lane 2, IS-252), which reads a backup back after taking it.

What happens

backup_logs.destination carries two different things depending on configuration, and only one of them is a path this server can open.

run_scheduled_backup (webapp/backend/app/tasks/reminder_tasks.py, ~line 5643) computes the local path and then throws it away when a remote is configured:

destination = str(dest_path)
if config.remote:
    destination = ship_to_remote(artefacts, config.remote, ...)
...
log.destination = destination

ship_to_remote returns the redacted rclone destination, so on an instance with BackupConfig.remote set, every row records something like s3-backups:…/questboard and the timestamped filename of the dump is gone.

The dump itself is still on disk — the code says so two lines above, and means it: "The local files are kept regardless — the next run copies them too, since rclone copy is additive." Nothing knows what it is called any more.

Why it matters

The obvious consequence is that the new POST /api/admin/backup/verify reports "No local copy of this backup was found to read back" for exactly the instances that took off-host backups — which is to say, the ones that took the trouble to do it properly. #414's whole point is that a dump living only on the machine it backs up is not a backup, so a verification that works only in the case #414 considers insufficient is the wrong way round.

The broader consequence is that the row cannot answer "where is this backup?" for anything else that asks later either — a restore helper, a prune that wants to check before deleting, an operator reading the Admin list.

What I would do about it

Record both, rather than overloading one column: keep the local path and add the remote spec beside it. That is a column and a migration rather than a behaviour change, and it is backward-compatible — an old row keeps whatever single value it has.

Deliberately not doing it as part of lane 2: it is a defect in the backup feature, not in the wizard, and the wizard behaves correctly in its presence (verified: false with a sentence saying the local copy was not found, rather than a lie or a crash). Filing rather than fixing so it is not buried in a redesign PR.

Where it lives in the redesign

No new surface. The verification result is drawn on PUB-8's backup step (lane 2) and the same data belongs on ADMIN-14, Instance health (lane 9), where an operator would look for it. Admin-only either way; not used on a phone.

Related: #414 (scheduled backups, on by default), #429 (backup_client_service refuses to write a dump this server's pg_restore could not read), #439 / #728 (the first-run wizard).

🤖 Generated with Claude Code

Found while building the first-run setup wizard's backup step (#728, lane 2, `IS-252`), which reads a backup back after taking it. ## What happens `backup_logs.destination` carries two different things depending on configuration, and only one of them is a path this server can open. `run_scheduled_backup` (`webapp/backend/app/tasks/reminder_tasks.py`, ~line 5643) computes the local path and then throws it away when a remote is configured: ```python destination = str(dest_path) if config.remote: destination = ship_to_remote(artefacts, config.remote, ...) ... log.destination = destination ``` `ship_to_remote` returns the **redacted rclone destination**, so on an instance with `BackupConfig.remote` set, every row records something like `s3-backups:…/questboard` and the timestamped filename of the dump is gone. The dump itself is still on disk — the code says so two lines above, and means it: *"The local files are kept regardless — the next run copies them too, since rclone copy is additive."* Nothing knows what it is called any more. ## Why it matters The obvious consequence is that the new `POST /api/admin/backup/verify` reports "No local copy of this backup was found to read back" for exactly the instances that took off-host backups — which is to say, the ones that took the trouble to do it properly. #414's whole point is that a dump living only on the machine it backs up is not a backup, so a verification that works only in the case #414 considers insufficient is the wrong way round. The broader consequence is that the row cannot answer "where is this backup?" for anything else that asks later either — a restore helper, a prune that wants to check before deleting, an operator reading the Admin list. ## What I would do about it Record both, rather than overloading one column: keep the local path and add the remote spec beside it. That is a column and a migration rather than a behaviour change, and it is backward-compatible — an old row keeps whatever single value it has. Deliberately **not** doing it as part of lane 2: it is a defect in the backup feature, not in the wizard, and the wizard behaves correctly in its presence (`verified: false` with a sentence saying the local copy was not found, rather than a lie or a crash). Filing rather than fixing so it is not buried in a redesign PR. ## Where it lives in the redesign No new surface. The verification result is drawn on **PUB-8**'s backup step (lane 2) and the same data belongs on **ADMIN-14**, Instance health (lane 9), where an operator would look for it. Admin-only either way; not used on a phone. Related: #414 (scheduled backups, on by default), #429 (`backup_client_service` refuses to write a dump this server's `pg_restore` could not read), #439 / #728 (the first-run wizard). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot referenced this issue

2026-09-22 16:34:48 +00:00

Lane 2: first-run setup's own state, and a backup read-back (#728) #754

rbrooks referenced this issue from a commit

2026-09-22 20:42:05 +00:00

feat(frontend): setting up a new server, and what it leaves waiting (#728, lane 2)

claude-bot referenced this issue

2026-09-22 20:42:34 +00:00

Lane 2: setting up a new server, and what it leaves waiting (#728) #759

claude-bot referenced this issue

2026-09-23 21:13:32 +00:00

Build lane 3: the campaign core and Settings (CAMP-1 to CAMP-17, SH-7) #729

rbrooks referenced this issue from a commit

2026-09-23 22:39:30 +00:00

fix(backend): a shipped backup keeps its local path, and can be read back (#753)

claude-bot referenced this issue from a pull request that will close it,

2026-09-23 22:39:47 +00:00

Backups: a shipped backup can be read back, retention stops deleting fresh dumps, and remotes are redacted properly (#753) #790

claude-bot closed this issue

2026-09-23 23:19:55 +00:00

claude-bot referenced this issue from a commit

2026-09-23 23:19:55 +00:00

Merge PR #790: backups — a shipped backup can be read back, retention stops deleting fresh dumps, remotes redacted properly (#753)

claude-bot commented

2026-09-23 23:20:01 +00:00

Author

Collaborator

Fixed in #790, merged 2026-09-23.

backup_logs.destination now only ever holds the local path. It's written as soon as pg_dump succeeds, so a row whose shipping failed still says where its good dump is. The redacted remote spec goes in a new remote_destination. Migration d0e1f2a3b4c5 moves old remote-only values across; their local path was never stored, and verify says so plainly.
Verify and the setup wizard's read-back find the local file on an instance that ships off-host.

Fixing this turned up two worse bugs, both fixed in the same PR:

Retention deleted the dump it had just written, and recorded success. Its glob also matched make backup-now's questboard_manual_*.dump, which always sorted as newest. It now prunes only the exact names it writes, and keeps at least one.
Remote credentials leaked through redaction for named remotes with parameters, and for quoted inline parameters.

🤖 Generated with Claude Code

Fixed in #790, merged 2026-09-23. - `backup_logs.destination` now only ever holds the local path. It's written as soon as `pg_dump` succeeds, so a row whose *shipping* failed still says where its good dump is. The redacted remote spec goes in a new `remote_destination`. Migration `d0e1f2a3b4c5` moves old remote-only values across; their local path was never stored, and verify says so plainly. - Verify and the setup wizard's read-back find the local file on an instance that ships off-host. Fixing this turned up two worse bugs, both fixed in the same PR: - **Retention deleted the dump it had just written, and recorded success.** Its glob also matched `make backup-now`'s `questboard_manual_*.dump`, which always sorted as newest. It now prunes only the exact names it writes, and keeps at least one. - **Remote credentials leaked through redaction** for named remotes with parameters, and for quoted inline parameters. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot referenced this issue

2026-09-24 01:57:16 +00:00

Backups: write-only off-site credentials, redaction that doesn't depend on rclone's wording, and the audio archive's path #795