diff --git a/app/backup.py b/app/backup.py index 0bb423d..b196db4 100644 --- a/app/backup.py +++ b/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) diff --git a/tests/test_controller_backup.py b/tests/test_controller_backup.py index 201033d..c2b9016 100644 --- a/tests/test_controller_backup.py +++ b/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