fix(restore): treat a 404 restore-result ack as terminal so cross-greffer migration leftovers self-clean

app/backup.py

@@ -534,10 +534,32 @@ def _restart(settings, instance_id: str) -> None:
     )
 
 
-def _post_callback(settings, instance_id: str, action: str, payload: dict) -> bool:
-    """POST the result to the manager (``X-Greffer-Token``); returns True iff the
-    manager acked (2xx). Never raises -- a lost callback is recovered by the
-    manager reaper / greffer boot reconciliation."""
+# Callback delivery outcomes (see ``_post_callback``).
+_CB_ACKED = "acked"  # manager 2xx -- the op is recorded and finalized
+_CB_GONE = "gone"    # manager 404 -- no such instance/run; retrying is pointless
+_CB_RETRY = "retry"  # 5xx / 403 / other / network -- transient, retry later
+
+
+def _post_callback(settings, instance_id: str, action: str, payload: dict) -> str:
+    """POST the result to the manager (``X-Greffer-Token``). Returns one of:
+
+    - ``_CB_ACKED``: manager 2xx -- the op is finalized.
+    - ``_CB_GONE``:  manager 404 -- the manager has no record of this run (or the
+      instance is gone), so the outcome is terminal and retrying is pointless. The
+      canonical source is a cross-greffer migration's internal restore, which runs
+      with NO manager RestoreRun (manager migration_services polls the greffer's
+      durable restore-status instead); once the migration has cut the instance's FK
+      over to this greffer, a reconcile re-post finalize-misses and 404s here.
+    - ``_CB_RETRY``: 5xx / network / 403 / any other status -- recover later via the
+      manager reaper / greffer boot reconciliation.
+
+    A 403 is deliberately ``_CB_RETRY``, NOT ``_CB_GONE``: during a live migration
+    the instance's FK is still on the SOURCE greffer, so this (target) greffer's
+    token mismatches and the in-flight restore callback 403s -- yet the manager is
+    actively polling our durable restore-status through exactly that window, so the
+    state file MUST survive a 403. (A 403 can also be a transient token-acceptance
+    race.) Only a 404 -- which for a migration arrives only AFTER cutover, when the
+    manager no longer needs the file -- is safe to treat as terminal. Never raises."""
     try:
         resp = requests.post(
             f"{settings.greffon_base_server}/api/greffer/instances/{instance_id}/{action}/",
@@ -546,10 +568,27 @@ def _post_callback(settings, instance_id: str, action: str, payload: dict) -> bo
             verify=settings.greffer_ssl_verify,
             timeout=_HTTP_TIMEOUT,
         )
-        return 200 <= resp.status_code < 300
     except requests.RequestException:
         logger.warning("backup_callback_failed instance=%s action=%s", instance_id, action)
-        return False
+        return _CB_RETRY
+    if 200 <= resp.status_code < 300:
+        return _CB_ACKED
+    if resp.status_code == 404:
+        logger.info(
+            "backup_callback_gone instance=%s action=%s -- no manager record, "
+            "dropping durable state", instance_id, action)
+        return _CB_GONE
+    logger.warning(
+        "backup_callback_unacked instance=%s action=%s status=%s",
+        instance_id, action, resp.status_code)
+    return _CB_RETRY
+
+
+def _callback_settled(outcome: str) -> bool:
+    """A durable restore-state file is safe to drop once the callback is settled:
+    the manager acked it (``_CB_ACKED``) or has no record of the run (``_CB_GONE``,
+    a 404). A transient outcome (``_CB_RETRY``) keeps the file for boot reconciliation."""
+    return outcome in (_CB_ACKED, _CB_GONE)
 
 
 def _instance_dir(settings, instance_id: str) -> Path:
@@ -618,7 +657,8 @@ def reconcile_on_boot(settings) -> None:
                 payload = json.loads(state_file.read_text())
             except (OSError, ValueError):
                 continue
-            if _post_callback(settings, instance_id, "restore-result", payload):
+            outcome = _post_callback(settings, instance_id, "restore-result", payload)
+            if _callback_settled(outcome):
                 _remove(state_file)
 
 
@@ -913,11 +953,14 @@ def restore_instance(settings, instance_id: str, restic_snapshot_id: str,
                 compose.stop({"id": instance_id})
             except Exception:  # noqa: BLE001
                 logger.exception("restore_db_abort_stop_failed instance=%s", instance_id)
-        # Durable restore-state, kept until the manager acks (boot reconciliation
-        # re-posts a lost callback so an overwritten instance is never stranded).
+        # Durable restore-state, kept until the callback settles (boot reconciliation
+        # re-posts a lost callback so an overwritten instance is never stranded). A
+        # migration-internal restore 403s here (FK still on the source greffer) -> NOT
+        # settled -> kept, so the manager can poll restore-status through this window.
         state_path = _restore_state_path(settings, instance_id, restore_id)
         _write_json(state_path, payload)
-        if _post_callback(settings, instance_id, "restore-result", payload):
+        outcome = _post_callback(settings, instance_id, "restore-result", payload)
+        if _callback_settled(outcome):
             _remove(state_path)
 
 

tests/test_controller_backup.py

@@ -194,19 +194,34 @@ def test_spawn_backup_busy_raises(monkeypatch):
 
 # ---- callback ack + crash recovery ----------------------------------------
 
-def test_post_callback_returns_ack(monkeypatch):
+def test_post_callback_classifies_status(monkeypatch):
     s = _settings()
-    monkeypatch.setattr(backup.requests, "post",
-                        lambda *a, **k: mock.Mock(status_code=200))
-    assert backup._post_callback(s, "i", "backup-result", {}) is True
-    monkeypatch.setattr(backup.requests, "post",
-                        lambda *a, **k: mock.Mock(status_code=500))
-    assert backup._post_callback(s, "i", "backup-result", {}) is False
+
+    def _status(code):
+        monkeypatch.setattr(backup.requests, "post",
+                            lambda *a, **k: mock.Mock(status_code=code))
+        return backup._post_callback(s, "i", "backup-result", {})
+
+    assert _status(200) == backup._CB_ACKED
+    assert _status(204) == backup._CB_ACKED
+    # 404 == the manager has no record of this run -> terminal, safe to drop state.
+    assert _status(404) == backup._CB_GONE
+    # 403 == ownership/token mismatch (the live cross-greffer migration window).
+    # MUST stay retryable so the durable restore-state survives for the manager poll.
+    assert _status(403) == backup._CB_RETRY
+    assert _status(500) == backup._CB_RETRY
+    assert _status(400) == backup._CB_RETRY  # other 4xx are not terminal either
 
     def _raise(*a, **k):
         raise backup.requests.ConnectionError()
     monkeypatch.setattr(backup.requests, "post", _raise)
-    assert backup._post_callback(s, "i", "backup-result", {}) is False
+    assert backup._post_callback(s, "i", "backup-result", {}) == backup._CB_RETRY
+
+
+def test_callback_settled_only_acked_or_gone():
+    assert backup._callback_settled(backup._CB_ACKED) is True
+    assert backup._callback_settled(backup._CB_GONE) is True
+    assert backup._callback_settled(backup._CB_RETRY) is False
 
 
 def test_restore_status_reads_durable_state(tmp_path):
@@ -272,7 +287,36 @@ def test_reconcile_reposts_lost_restore_callback(tmp_path, monkeypatch):
     posts = []
     monkeypatch.setattr(
         backup, "_post_callback",
-        lambda settings, iid, action, payload: posts.append(action) or True)
+        lambda settings, iid, action, payload: posts.append(action) or backup._CB_ACKED)
     backup.reconcile_on_boot(s)
     assert posts == ["restore-result"]
     assert not (inst / ".restore_r1.json").exists()  # removed on ack
+
+
+def test_reconcile_drops_restore_state_on_404_gone(tmp_path, monkeypatch):
+    # The cross-greffer migration leftover: after cutover the re-post 404s (no
+    # manager RestoreRun). 404 is terminal -> drop the orphan state file.
+    s = _settings(greffon_path=str(tmp_path))
+    inst = tmp_path / "i"
+    inst.mkdir()
+    (inst / ".restore_r1.json").write_text('{"restore_id": "r1", "status": "success"}')
+    monkeypatch.setattr(
+        backup, "_post_callback",
+        lambda settings, iid, action, payload: backup._CB_GONE)
+    backup.reconcile_on_boot(s)
+    assert not (inst / ".restore_r1.json").exists()  # dropped on terminal 404
+
+
+def test_reconcile_keeps_restore_state_on_403_retry(tmp_path, monkeypatch):
+    # REGRESSION GUARD: a 403 is the LIVE migration window (FK still on the source
+    # greffer) -- the manager polls our durable restore-status through it, so the
+    # state file MUST survive. A 403 must never be treated as terminal.
+    s = _settings(greffon_path=str(tmp_path))
+    inst = tmp_path / "i"
+    inst.mkdir()
+    (inst / ".restore_r1.json").write_text('{"restore_id": "r1", "status": "success"}')
+    monkeypatch.setattr(
+        backup, "_post_callback",
+        lambda settings, iid, action, payload: backup._CB_RETRY)
+    backup.reconcile_on_boot(s)
+    assert (inst / ".restore_r1.json").exists()  # kept for the manager poll / retry