Merge pull request opensandbox-group#842 from ninan-nn/feature/updates_12319023

feat(server): refine deleting snapshots

Author: Pangjiping

server/opensandbox_server/services/snapshot_service.py

@@ -196,9 +196,6 @@ def delete_snapshot(self, snapshot_id: str) -> None:
                 },
             )
 
-        if record.status.state == SnapshotState.DELETING:
-            return
-
         if record.status.state == SnapshotState.CREATING:
             raise HTTPException(
                 status_code=status.HTTP_409_CONFLICT,
@@ -208,6 +205,11 @@ def delete_snapshot(self, snapshot_id: str) -> None:
                 },
             )
 
+        if record.status.state != SnapshotState.DELETING:
+            record = self._mark_snapshot_deleting(record)
+            if record is None:
+                return
+
         self._snapshot_runtime.delete_snapshot(
             snapshot_id,
             image=record.restore_config.image,
@@ -230,6 +232,43 @@ def _default_pagination():
 
         return PaginationRequest()
 
+    def _mark_snapshot_deleting(self, record: SnapshotRecord) -> SnapshotRecord | None:
+        now = datetime.now(timezone.utc)
+        deleting_record = SnapshotRecord(
+            id=record.id,
+            source_sandbox_id=record.source_sandbox_id,
+            name=record.name,
+            description=record.description,
+            restore_config=record.restore_config,
+            status=SnapshotStatusRecord(
+                state=SnapshotState.DELETING,
+                reason="snapshot_delete_requested",
+                message="Snapshot deletion requested.",
+                last_transition_at=now,
+            ),
+            created_at=record.created_at,
+            updated_at=now,
+        )
+        if self._snapshot_repository.update_if_state(
+            deleting_record,
+            record.status.state,
+        ):
+            return deleting_record
+
+        current_record = self._snapshot_repository.get(record.id)
+        if current_record is None:
+            return None
+        if current_record.status.state == SnapshotState.DELETING:
+            return current_record
+
+        raise HTTPException(
+            status_code=status.HTTP_409_CONFLICT,
+            detail={
+                "code": "SNAPSHOT::INVALID_STATE",
+                "message": f"Snapshot {record.id} changed state and cannot be deleted",
+            },
+        )
+
     def _create_snapshot_worker(self, record: SnapshotRecord) -> None:
         try:
             runtime_status = self._snapshot_runtime.create_snapshot(

server/tests/test_snapshot_service.py

@@ -381,7 +381,14 @@ def create_snapshot(snapshot_id: str, sandbox_id: str):
         runtime.calls.append((snapshot_id, sandbox_id))
         return ready_status
 
+    def delete_snapshot(snapshot_id: str, image: str | None = None) -> None:
+        stored = repo.get(snapshot_id)
+        assert stored is not None
+        assert stored.status.state == SnapshotState.DELETING
+        runtime.delete_calls.append((snapshot_id, image))
+
     runtime.create_snapshot = create_snapshot
+    runtime.delete_snapshot = delete_snapshot
     created = service.create_snapshot("sbx-001", CreateSnapshotRequest(name="checkpoint"))
 
     service.delete_snapshot(created.id)
@@ -420,8 +427,49 @@ def delete_snapshot(snapshot_id: str, image: str | None = None) -> None:
     with pytest.raises(HTTPException) as exc_info:
         service.delete_snapshot("snap-in-use")
 
+    stored = repo.get("snap-in-use")
     assert exc_info.value.status_code == 409
-    assert repo.get("snap-in-use") is not None
+    assert stored is not None
+    assert stored.status.state == SnapshotState.DELETING
+
+
+def test_snapshot_service_recovers_delete_after_runtime_cleanup_succeeds(tmp_path) -> None:
+    repo = SQLiteSnapshotRepository(tmp_path / "snapshots.db")
+    runtime = StubSnapshotRuntime()
+    service = PersistedSnapshotService(
+        repo,
+        StubSandboxService(),
+        snapshot_runtime=runtime,
+        snapshot_executor=ImmediateExecutor(),
+    )
+
+    record = _snapshot_record(
+        "snap-delete-crash",
+        SnapshotState.READY,
+        image="opensandbox-snapshots:snap-delete-crash",
+    )
+    repo.create(record)
+
+    original_delete = repo.delete
+
+    def crash_delete(snapshot_id: str) -> None:
+        raise RuntimeError("simulated metadata delete crash")
+
+    repo.delete = crash_delete
+    with pytest.raises(RuntimeError, match="simulated metadata delete crash"):
+        service.delete_snapshot("snap-delete-crash")
+
+    stored = repo.get("snap-delete-crash")
+    assert stored is not None
+    assert stored.status.state == SnapshotState.DELETING
+    assert runtime.delete_calls == [("snap-delete-crash", "opensandbox-snapshots:snap-delete-crash")]
+
+    repo.delete = original_delete
+    recovery_runtime = StubSnapshotRuntime()
+    PersistedSnapshotService(repo, StubSandboxService(), snapshot_runtime=recovery_runtime)
+
+    assert recovery_runtime.delete_calls == [("snap-delete-crash", "opensandbox-snapshots:snap-delete-crash")]
+    assert repo.get("snap-delete-crash") is None
 
 
 def test_snapshot_service_worker_cleans_up_snapshot_deleted_during_creation(tmp_path) -> None: