fix: restore by one rule for every error type, and never overwrite a backup

CHANGELOG.md

@@ -79,6 +79,15 @@ The design brain, plus the fixes found reviewing it.
   stays registered whatever the sign-in method. Both now say what is true either way:
   Chartwright signs in with a Superset account's own password (database or LDAP) or
   Preset API tokens, never through SSO or OAuth.
+- A failed build now puts the backup back by the same rule whatever the error. A chart
+  update that Superset rejected after the import left the dashboard half-updated, while a
+  connection drop at the same step restored it; both now restore. An error that wasn't a
+  Superset error (a bug in Chartwright) skipped the restore and lost the backup path;
+  it now follows the same rule and returns the normal report. Failures after the chart
+  updates (linkage, filter scopes, chart queries) still leave the new version live.
+- Two applies to the same dashboard within one second wrote the same backup name, and
+  the second overwrote the first. Backup names now go to the microsecond
+  (`20261003T141502.123456.zip`) and are never overwritten.
 - When an automatic restore failed, the suggested `chartwright restore <zip>` command
   left out the required `--profile`, so pasting it gave a usage error. It now includes it.
 - `docs/VERIFICATION.md` said CI runs on every push, and that the live check covers a

chartwright/apply.py

@@ -238,6 +238,26 @@ def backup_dir_for(profile: str, slug: str) -> "Path":
     return base / profile / slug
 
 
+def write_backup(backup_dir: "Path", data: bytes) -> "Path":
+    """Write a backup zip under a new name; never overwrite an earlier one.
+
+    Names are local time to the microsecond (``20261003T141502.123456.zip``),
+    fixed width, so they sort oldest to newest. Two applies within one second
+    used to share a name and the second overwrote the first. Exclusive create
+    makes a collision a retry, never a silent overwrite."""
+    import datetime
+
+    while True:
+        stamp = datetime.datetime.now().strftime("%Y%m%dT%H%M%S.%f")
+        path = backup_dir / f"{stamp}.zip"
+        try:
+            with open(path, "xb") as fh:
+                fh.write(data)
+            return path
+        except FileExistsError:
+            continue
+
+
 def restore_bundle(zip_bytes: bytes, slug: str, client: SupersetClient) -> ApplyReport:
     """Restore a backup bundle COMPLETELY, not just import it. The importer
     never overwrites existing charts (docs/CONTRACTS.md), so surviving
@@ -321,7 +341,6 @@ def apply(spec: DashboardSpec, client: SupersetClient, profile: str = "default")
     if existing is not None:
         # Last-known-good insurance before we mutate anything: the previous
         # owned state, restorable with `chartwright restore <zip> --profile ...`.
-        import datetime
         import os
 
         backup_dir = backup_dir_for(profile, spec.dashboard.slug)
@@ -330,11 +349,8 @@ def apply(spec: DashboardSpec, client: SupersetClient, profile: str = "default")
             # Zips hold dashboard/dataset metadata; gate the default tree to
             # the owner. (No-op on Windows; custom dirs are the user's to manage.)
             os.chmod(backup_dir.parent.parent, 0o700)
-        stamp = datetime.datetime.now().strftime("%Y%m%dT%H%M%S")
-        backup_path = backup_dir / f"{stamp}.zip"
         backup_bytes = client.export_dashboard(existing["id"])
-        backup_path.write_bytes(backup_bytes)
-        report.backup = str(backup_path)
+        report.backup = str(write_backup(backup_dir, backup_bytes))
 
     def _auto_restore(reason: str) -> None:
         """Import failed mid-mutation: put the previous state back rather
@@ -399,7 +415,11 @@ def _auto_restore(reason: str) -> None:
             updated, update_errors = _update_owned_charts_in_place(
                 spec, client, bundle, existing_by_uuid, resolution)
             if update_errors:
+                # Still the import step: some charts may carry the new params
+                # and others the old. Same outcome as a connection drop here,
+                # which already restored (SupersetAPIError while stage is import).
                 report.import_detail = "; ".join(update_errors)
+                _auto_restore("updating charts in place failed after the import")
                 return report
             if updated:
                 report.warnings.append(
@@ -441,6 +461,13 @@ def _auto_restore(reason: str) -> None:
         if report.stage in ("prepare", "import"):
             _auto_restore(f"apply failed during {report.stage}: {e}")
         return report
+    except Exception as e:  # noqa: BLE001 - a bug mid-mutation must still restore and report
+        # Not a Superset error: most likely a bug here. Same rule as above, and
+        # the report keeps the backup path, which a traceback would lose.
+        report.import_detail = f"unexpected error during {report.stage}: {type(e).__name__}: {e}"
+        if report.stage in ("prepare", "import"):
+            _auto_restore(f"apply failed during {report.stage}: {type(e).__name__}: {e}")
+        return report
 
     report.stage = "done"
     report.ok = True

docs/VERIFICATION.md

@@ -35,7 +35,7 @@ injection.
 
 | Layer | Proves | Where it runs |
 |---|---|---|
-| Offline suite (343 tests, 35 modules) | Contract, determinism, round-trips, credentials | every PR and push to main, Linux + Windows, mcp 1 and 2 |
+| Offline suite (347 tests, 36 modules) | Contract, determinism, round-trips, credentials | every PR and push to main, Linux + Windows, mcp 1 and 2 |
 | Chart-option contract | Every emitted chart option is declared by each version's plugin source | every PR and push to main |
 | Live guarantee check | 15-chart apply, per-chart data check, ids stable across re-apply | every PR and push to main, all 3 versions |
 | Lifecycle soak | 500 randomized edit cycles with invariants held | 500 cycles on 6.1.0 and 4.1.4 before release; 25 cycles per version on every PR and push to main |
@@ -159,15 +159,18 @@ All four scenarios pass on 4.1.4, 5.0.0, and 6.1.0.
 ## 6. Fault injection (`tools/faultline.py`)
 
 A wrapping client trips a chosen method with the same typed error a real
-network drop produces, or a fake HTTP 500 for the importer. A fault at
-every stage boundary (the dataset round-trip and stale-chart deletion
-during preparation, an importer 500 and a mid-import drop, the post-import
-link verification, the data check) must produce a typed report naming the
+network drop produces, a fake HTTP 500 for a rejected request, or a plain
+`RuntimeError` standing in for a bug in the tool itself. A fault at every
+stage boundary (the dataset round-trip and stale-chart deletion during
+preparation; an importer 500, a mid-import drop and an import-time bug;
+a rejected or dropped in-place chart update; the post-import link
+verification; the data check) must produce a typed report naming the
 stage, never a traceback, and a recorded backup whenever mutation had
 begun. Then:
 
-- for faults during preparation or import, the automatic restore fires,
-  and `plan` against the pre-apply spec comes back clean: the dashboard is
+- for faults during preparation or import, in-place chart updates
+  included, the automatic restore fires whatever the error type, and
+  `plan` against the pre-apply spec comes back clean: the dashboard is
   back, not half-updated;
 - for faults after import, a clean re-apply converges.
 

tests/test_apply_rollback.py

@@ -0,0 +1,119 @@
+"""When apply fails partway, the backup goes back on the same rule for every
+error type: a failure in the prepare or import step (chart updates included)
+restores it; a later failure leaves the new version live. Either way the
+report carries the backup path. Live coverage of the same rule, on all three
+Superset releases, is tools/faultline.py."""
+
+import pytest
+
+import chartwright.apply as ap
+from chartwright.resolver import Resolution
+from chartwright.spec import load_spec
+
+SPEC = {
+    "spec_version": "1",
+    "dashboard": {"title": "Rollback", "slug": "rollback"},
+    "charts": [{"name": "Orders", "type": "big_number_total", "metric": "COUNT(*)",
+                "dataset": {"database": "warehouse", "table": "orders"}}],
+    "layout": {"rows": [["Orders"]]},
+}
+
+
+class StubClient:
+    """Just enough of SupersetClient for apply's flow, with an existing dashboard."""
+
+    base_url = "http://superset.test"
+
+    def __init__(self, crash_on: str | None = None):
+        self.crash_on = crash_on
+
+    def _maybe_crash(self, name):
+        if self.crash_on == name:
+            raise RuntimeError(f"bug in {name}")
+
+    def find_dashboard_by_slug(self, slug):
+        return {"id": 7}
+
+    def export_dashboard(self, did):
+        return b"backup-zip"
+
+    def dashboard_charts(self, did):
+        self._maybe_crash("dashboard_charts")
+        return [{"slice_name": "Orders"}]
+
+    def charts_by_uuids(self, mapping):
+        return {u: {"id": 11, "slice_name": n} for u, n in mapping.items()}
+
+    def import_dashboard_bundle(self, bundle, overwrite=True):
+        self._maybe_crash("import_dashboard_bundle")
+
+        class R:
+            status_code = 200
+            text = ""
+        return R()
+
+
+@pytest.fixture
+def flow(monkeypatch, tmp_path):
+    """Isolate apply's orchestration: resolution, compile and restore stubbed."""
+    monkeypatch.setenv("CHARTWRIGHT_BACKUP_DIR", str(tmp_path))
+    monkeypatch.setattr(ap, "resolve", lambda spec, client: Resolution())
+    monkeypatch.setattr(ap, "_ownership_guard", lambda spec, client: None)
+    monkeypatch.setattr(ap, "_roundtrip_dataset_files", lambda res, client: {})
+    monkeypatch.setattr(ap, "compile_bundle", lambda spec, res, extra_files=None: b"bundle")
+    monkeypatch.setattr(ap, "_apply_filter_scopes", lambda spec, client, did: [])
+    monkeypatch.setattr(ap, "smoke", lambda spec, res, client: [])
+    restores = []
+
+    def fake_restore(data, slug, client):
+        restores.append(data)
+        return ap.ApplyReport(ok=True, stage="done")
+
+    monkeypatch.setattr(ap, "restore_bundle", fake_restore)
+    return restores
+
+
+def test_a_rejected_chart_update_restores_the_backup(flow, monkeypatch):
+    monkeypatch.setattr(ap, "_update_owned_charts_in_place",
+                        lambda *a, **k: ([], ["chart 'Orders': update PUT HTTP 500 boom"]))
+    rep = ap.apply(load_spec(SPEC), StubClient(), "dev")
+    assert not rep.ok and rep.stage == "import"
+    assert flow == [b"backup-zip"]
+    assert any("AUTO-RESTORED" in w for w in rep.warnings)
+    assert rep.backup
+
+
+def test_an_unexpected_error_during_import_restores_and_reports(flow, monkeypatch):
+    rep = ap.apply(load_spec(SPEC), StubClient(crash_on="import_dashboard_bundle"), "dev")
+    assert not rep.ok and rep.stage == "import"
+    assert "RuntimeError" in rep.import_detail
+    assert flow == [b"backup-zip"]
+    assert rep.backup
+
+
+def test_an_unexpected_error_after_import_reports_without_restoring(flow, monkeypatch):
+    monkeypatch.setattr(ap, "_update_owned_charts_in_place", lambda *a, **k: (["Orders"], []))
+    calls = {"n": 0}
+    client = StubClient()
+    real = client.dashboard_charts
+
+    def second_call_crashes(did):
+        calls["n"] += 1
+        if calls["n"] == 2:              # call 1 is the prepare-stage stale check
+            raise RuntimeError("bug in linkage")
+        return real(did)
+
+    client.dashboard_charts = second_call_crashes
+    rep = ap.apply(load_spec(SPEC), client, "dev")
+    assert not rep.ok and rep.stage == "linkage"
+    assert "RuntimeError" in rep.import_detail
+    assert flow == []                    # later steps leave the new version live
+    assert rep.backup
+
+
+def test_backups_written_in_the_same_second_never_overwrite(tmp_path):
+    a = ap.write_backup(tmp_path, b"first")
+    b = ap.write_backup(tmp_path, b"second")
+    assert a != b
+    assert a.read_bytes() == b"first" and b.read_bytes() == b"second"
+    assert sorted(p.name for p in tmp_path.iterdir()) == [a.name, b.name]

tools/faultline.py

@@ -1,13 +1,15 @@
 """Fault injection: every apply-stage boundary must fail SAFE.
 
 A FaultyClient delegates to the real client but trips an armed method with
-either a typed SupersetAPIError (what a real network drop surfaces as after
-client._send's conversion) or a fake HTTP 500 (the importer's failure shape).
+a typed SupersetAPIError (what a real network drop surfaces as after
+client._send's conversion), a fake HTTP 500 (a rejected request), or a
+RuntimeError (a bug in the tool itself, not a Superset error).
 For each fault we assert the contract:
   - apply returns a TYPED report (never a traceback) naming the right stage;
   - report.backup is set whenever mutation had begun on an existing dashboard;
-  - auto-restore fires for prepare/import faults and the dashboard is BACK to
-    its pre-apply state (plan against the previous spec is clean);
+  - auto-restore fires for prepare/import faults (in-place chart updates
+    included), whatever the error type, and the dashboard is BACK to its
+    pre-apply state (plan against the previous spec is clean);
   - later-stage faults (linkage/scope/smoke) leave a recoverable dashboard:
     a clean re-apply converges (plan clean, ids stable).
 Also proves restore completeness: `restore_bundle` rolls back surviving
@@ -68,6 +70,8 @@ def wrapper(*a, **kw):
                 self.tripped = True
                 if self._kind == "http500":
                     return FakeResponse()
+                if self._kind == "crash":
+                    raise RuntimeError(f"injected bug in {name}")
                 raise SupersetAPIError(f"injected fault in {name}", None, None)
             return attr(*a, **kw)
 
@@ -178,6 +182,9 @@ def main(argv: list[str] | None = None) -> int:
         ("delete_chart",            "error",   "prepare", True,  1),  # stale-owned deletion (v2 drops a chart)
         ("import_dashboard_bundle", "http500", "import",  True,  1),  # importer 500
         ("import_dashboard_bundle", "error",   "import",  True,  1),  # network drop mid-import
+        ("import_dashboard_bundle", "crash",   "import",  True,  1),  # a bug, not a Superset error
+        ("put_json",                "http500", "import",  True,  1),  # in-place chart update rejected
+        ("put_json",                "error",   "import",  True,  1),  # network drop mid chart update
         ("dashboard_charts",        "error",   "linkage", False, 2),  # call#1 is the prepare-stage stale check
         ("chart_data",              "error",   "smoke",   False, 1),  # smoke query
     ]