From 5cc19255760e5e2e2d0eed8f35577f8f0157da05 Mon Sep 17 00:00:00 2001 From: Jer Miller Date: Fri, 3 Jul 2026 18:18:03 -0600 Subject: [PATCH] fix(backup): report restore integrity honestly instead of faking success A post-restore `restic check` failure was reported to every surface (web async op, CLI) as a clean success. Introduce a distinct RestoreResult status "degraded" that still runs the persist/scan/resumable tail but no longer masquerades as "ok". A single `_run_long_op` branch maps it to a new terminal phase "degraded" (added to both TERMINAL_PHASES and the front-end terminalPhases set); the byo and hosted paths forward it verbatim. Distinguish couldn't-verify (check returncode 11/124 -> integrity_unverified) from damaged (-> integrity_failed) with truthful owner-facing copy, and make the CLI exit non-zero with reason-aware wording that never claims a bare "restore failed". Co-Authored-By: Claude Opus 4.8 (1M context) --- solstone/apps/backup/copy.py | 3 + solstone/apps/backup/routes.py | 5 +- solstone/apps/backup/static/backup.js | 2 +- solstone/apps/backup/tests/test_routes.py | 39 ++++++++++++ solstone/think/backup/restore.py | 20 ++++-- solstone/think/backup_cli.py | 14 ++++- tests/test_backup_cli.py | 62 +++++++++++++++++- tests/test_backup_restore.py | 76 ++++++++++++++++++++--- 8 files changed, 200 insertions(+), 21 deletions(-) diff --git a/solstone/apps/backup/copy.py b/solstone/apps/backup/copy.py index 1126d213b..7aeb5766b 100644 --- a/solstone/apps/backup/copy.py +++ b/solstone/apps/backup/copy.py @@ -66,6 +66,7 @@ PHASE_LABELS = { "rotating": "making a new recovery key…", "tearing_down": "turning off…", "done": "done", + "degraded": "restored, but not verified", "error": "couldn't finish", "loading": "loading…", "empty": "not set up yet", @@ -96,6 +97,8 @@ OPERATION_REASON_LABELS = { "timeout": "the destination took too long to respond. try again shortly.", "failed": "i couldn't finish the backup action. check the recovery key and destination, then try again.", "incomplete": "the backup action didn't finish. you can try again.", + "integrity_failed": "your journal was restored to this device, but the backup copy failed its integrity check and may be damaged.", + "integrity_unverified": "your journal was restored to this device, but the integrity check couldn't run (the backup was busy or timed out), so the backup copy is unverified.", "missing_required_field": "fill in the required fields, then try again.", "recovery_key_mismatch": "that didn't match your recovery key. re-enter the key from your saved copy.", "expired": "the approval took too long. try again.", diff --git a/solstone/apps/backup/routes.py b/solstone/apps/backup/routes.py index b249e81c2..226340c72 100644 --- a/solstone/apps/backup/routes.py +++ b/solstone/apps/backup/routes.py @@ -79,7 +79,7 @@ DESTINATION_PROBE_TIMEOUT = 30.0 RETENTION_KEYS = ("hourly", "daily", "weekly", "monthly") S3_REQUIRED = ("access_key_id", "secret_access_key") B2_REQUIRED = ("account_id", "account_key") -TERMINAL_PHASES = frozenset({"done", "needs_subscription", "error"}) +TERMINAL_PHASES = frozenset({"done", "needs_subscription", "error", "degraded"}) @dataclass @@ -176,6 +176,9 @@ def _run_long_op(entry: OperationEntry, thunk: Callable[[], OpOutcome]) -> None: elif outcome.status in {"ok", "done", "skipped"}: entry.phase = "done" entry.reason_code = None + elif outcome.status == "degraded": + entry.phase = "degraded" + entry.reason_code = outcome.reason_code else: entry.phase = "error" entry.reason_code = outcome.reason_code or "failed" diff --git a/solstone/apps/backup/static/backup.js b/solstone/apps/backup/static/backup.js index d0770174b..92df354e1 100644 --- a/solstone/apps/backup/static/backup.js +++ b/solstone/apps/backup/static/backup.js @@ -17,7 +17,7 @@ const statusLabels = (copy.management && copy.management.status_labels) || {}; const hostedCopy = copy.hosted || {}; - const terminalPhases = new Set(['done', 'error', 'needs_subscription']); + const terminalPhases = new Set(['done', 'error', 'needs_subscription', 'degraded']); function panel(name) { return root.querySelector(`[data-backup-panel="${name}"]`); diff --git a/solstone/apps/backup/tests/test_routes.py b/solstone/apps/backup/tests/test_routes.py index 929da0198..dc5772fb7 100644 --- a/solstone/apps/backup/tests/test_routes.py +++ b/solstone/apps/backup/tests/test_routes.py @@ -21,6 +21,7 @@ from solstone.think.backup.hosted import ( HostedCredentials, HostedCredsUnavailable, ) +from solstone.think.backup.restore import RestoreResult CONSENT_URL = "https://services.test/enable/backup?nonce=NONCE" SUBSCRIBE_URL = "https://services.test/plan" @@ -236,6 +237,44 @@ def test_rotate_restore_and_teardown_routes_call_engine_hooks( assert restore_journal.call_args.args[1] == "A" * 64 +def test_restore_route_reports_degraded_as_terminal_phase( + backup_env, + monkeypatch, + wait_until_helper, +) -> None: + env = backup_env() + restore_journal = Mock( + return_value=RestoreResult( + status="degraded", + reason_code="integrity_failed", + integrity_ok=False, + resumable=True, + bytes_restored=123, + ) + ) + monkeypatch.setattr(backup_routes, "restore_journal", restore_journal) + + response = env.client.post( + "/app/backup/restore", + json={ + "repository": "b2:bucket:path", + "backend": "b2", + "credentials": { + "account_id": "key-id", + "account_key": "application-key", + }, + "recovery_key": "A" * 64, + }, + ) + final = _wait_for_phase(env, wait_until_helper, "degraded") + js_text = Path("solstone/apps/backup/static/backup.js").read_text(encoding="utf-8") + + assert response.status_code == 202 + assert final["reason_code"] == "integrity_failed" + assert "degraded" in backup_routes.TERMINAL_PHASES + assert "new Set(['done', 'error', 'needs_subscription', 'degraded'])" in js_text + + def test_single_slot_concurrent_operation_returns_backup_busy( backup_env, monkeypatch, diff --git a/solstone/think/backup/restore.py b/solstone/think/backup/restore.py index 2fd90fb3d..558908993 100644 --- a/solstone/think/backup/restore.py +++ b/solstone/think/backup/restore.py @@ -149,11 +149,20 @@ def _run_restore( timeout=RESTORE_CHECK_TIMEOUT_SECONDS, ) integrity_ok = check.returncode == 0 - if not integrity_ok: + if integrity_ok: + status = "ok" + reason_code = None + else: + status = "degraded" + reason_code = ( + "integrity_unverified" + if check.returncode in {11, 124} + else "integrity_failed" + ) logger.warning( "backup restore integrity check completed returncode=%s reason_code=%s", check.returncode, - reason_for_returncode(check.returncode), + reason_code, ) persist(canonical) @@ -163,12 +172,13 @@ def _run_restore( scan_journal(str(journal), full=True) logger.info( - "backup restore completed returncode=%s reason_code=ok", + "backup restore completed returncode=%s reason_code=%s", restore.returncode, + reason_code or "ok", ) return RestoreResult( - status="ok", - reason_code=None, + status=status, + reason_code=reason_code, integrity_ok=integrity_ok, resumable=resumable, bytes_restored=restored_size, diff --git a/solstone/think/backup_cli.py b/solstone/think/backup_cli.py index f8bbd464b..0f7f28e24 100644 --- a/solstone/think/backup_cli.py +++ b/solstone/think/backup_cli.py @@ -352,13 +352,23 @@ def restore() -> None: result = restore_journal(destination, recovery_key.strip()) if result.status == "error": _die(f"Restore failed: {result.reason_code}.") + if result.status == "degraded": + if result.reason_code == "integrity_unverified": + detail = ( + "integrity verification could not run " + "(the repository was busy or timed out)" + ) + else: + detail = "integrity verification failed — the backup copy may be damaged" + _die( + f"Restored {result.bytes_restored} bytes and saved the recovery key, " + f"but {detail} (reason_code={result.reason_code})." + ) if result.status == "ok": typer.echo( f"Restore complete: {result.bytes_restored} bytes, " f"integrity_ok={result.integrity_ok}, resumable={result.resumable}." ) - if not result.integrity_ok: - typer.echo("Warning: integrity check did not pass.") return raise RuntimeError(f"unknown restore status: {result.status}") diff --git a/tests/test_backup_cli.py b/tests/test_backup_cli.py index 3b513e6d2..20ccfd52e 100644 --- a/tests/test_backup_cli.py +++ b/tests/test_backup_cli.py @@ -551,7 +551,7 @@ def test_restore_reads_secret_from_stdin_and_reports_result( return RestoreResult( status="ok", reason_code=None, - integrity_ok=False, + integrity_ok=True, resumable=True, bytes_restored=123, ) @@ -578,7 +578,65 @@ def test_restore_reads_secret_from_stdin_and_reports_result( assert captured["recovery_key"] == "RECOVERYSECRET" assert "RECOVERYSECRET" not in " ".join(captured["argv"]) assert "RECOVERYSECRET" not in result.output - assert "Warning: integrity check did not pass." in result.output + assert ( + "Restore complete: 123 bytes, integrity_ok=True, resumable=True." + in result.output + ) + + +@pytest.mark.parametrize( + ("reason_code", "detail"), + [ + ( + "integrity_unverified", + "integrity verification could not run " + "(the repository was busy or timed out)", + ), + ( + "integrity_failed", + "integrity verification failed — the backup copy may be damaged", + ), + ], +) +def test_restore_maps_degraded( + monkeypatch: pytest.MonkeyPatch, + reason_code: str, + detail: str, +) -> None: + monkeypatch.setattr( + backup_cli, + "restore_journal", + lambda *_args: RestoreResult( + status="degraded", + reason_code=reason_code, + integrity_ok=False, + resumable=True, + bytes_restored=123, + ), + ) + payload = { + "repository": "s3:safe-bucket/path", + "backend": "s3", + "credentials": { + "access_key_id": "access-key", + "secret_access_key": "secret-key", + }, + "recovery_key": "RECOVERYSECRET", + } + + result = CliRunner().invoke( + backup_cli.app, + ["restore"], + input=json.dumps(payload), + ) + + assert result.exit_code == 1 + assert ( + f"Restored 123 bytes and saved the recovery key, but {detail} " + f"(reason_code={reason_code})." + ) in result.output + assert "Restore failed" not in result.output + assert "RECOVERYSECRET" not in result.output def test_restore_maps_error(monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/tests/test_backup_restore.py b/tests/test_backup_restore.py index d7caaf5c5..9a3cb131a 100644 --- a/tests/test_backup_restore.py +++ b/tests/test_backup_restore.py @@ -302,32 +302,88 @@ def test_restore_malformed_snapshots_json_returns_failed( assert result.reason_code == "failed" -def test_restore_check_failure_is_nonfatal( +@pytest.mark.parametrize( + ("check_returncode", "reason_code"), + [ + (11, "integrity_unverified"), + (1, "integrity_failed"), + ], +) +def test_restore_check_failure_reports_degraded_and_keeps_side_effects( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + check_returncode: int, + reason_code: str, ) -> None: monkeypatch.setenv("SOLSTONE_JOURNAL", str(tmp_path)) _write_config(tmp_path, {"backup": {"daily_key": "daily-secret"}}) + destination = _destination() + recovery_key = "A" * 64 responses = iter( [ _result(0, [{"paths": ["/old/journal"]}]), _result(0, {"message_type": "summary", "bytes_restored": 5}), - _result(11), + _result(check_returncode), ] ) + order: list[str] = [] + calls: list[tuple[list[str], dict[str, Any]]] = [] + + def fake_run_restic(args: list[str], **kwargs: Any) -> ResticResult: + order.append(args[0]) + calls.append((args, kwargs)) + return next(responses) + + def fake_set_destination(value: Destination) -> None: + order.append("set_destination") + assert value == destination + + def fake_set_recovery_key(value: str) -> None: + order.append("set_recovery_key") + assert value == recovery_key + + def fake_set_recovery_key_confirmed(value: bool) -> None: + order.append("set_recovery_key_confirmed") + assert value is True + + def fake_scan_journal(journal: str, **kwargs: Any) -> bool: + order.append("scan_journal") + assert journal == str(tmp_path) + assert kwargs == {"full": True} + return True + monkeypatch.setattr(restore, "ensure_restic", lambda: Path("/restic")) - monkeypatch.setattr(restore, "run_restic", lambda args, **kwargs: next(responses)) - monkeypatch.setattr(restore, "set_destination", lambda destination: None) - monkeypatch.setattr(restore, "set_recovery_key", lambda key: None) - monkeypatch.setattr(restore, "set_recovery_key_confirmed", lambda confirmed: None) + monkeypatch.setattr(restore, "run_restic", fake_run_restic) + monkeypatch.setattr(restore, "set_destination", fake_set_destination) + monkeypatch.setattr(restore, "set_recovery_key", fake_set_recovery_key) + monkeypatch.setattr( + restore, + "set_recovery_key_confirmed", + fake_set_recovery_key_confirmed, + ) monkeypatch.setattr(restore, "get_backup_config", lambda: {"daily_key": "daily"}) - monkeypatch.setattr(restore, "scan_journal", lambda journal, **kwargs: True) + monkeypatch.setattr(restore, "scan_journal", fake_scan_journal) - result = restore.restore_journal(_destination(), "A" * 64) + result = restore.restore_journal(destination, recovery_key) - assert result.status == "ok" - assert result.reason_code is None + assert result == restore.RestoreResult( + status="degraded", + reason_code=reason_code, + integrity_ok=False, + resumable=True, + bytes_restored=5, + ) assert result.integrity_ok is False + assert order == [ + "snapshots", + "restore", + "check", + "set_destination", + "set_recovery_key", + "set_recovery_key_confirmed", + "scan_journal", + ] + assert calls[2][0] == ["check"] def test_restore_missing_daily_key_is_not_resumable( -- 2.54.0