feat(push): extend 502/504 failure logging with upload summary and file list; include gpkg diff size and change count via safe helper; add real-diff test; no change to success path

Author: DanChov

mergin/client_push.py

@@ -268,14 +268,31 @@ def push_project_finalize(job):
             resp = job.mc.post("/v1/project/push/finish/%s" % job.transaction_id)
             job.server_resp = json.load(resp)
         except ClientError as err:
-            # Log additional metadata on server error 502 or 504
-            if hasattr(err, "http_error") and err.http_error in (502, 504):
+            # Log additional metadata on server error 502 or 504 (extended logging only)
+            http_code = getattr(err, "http_error", None)
+            if http_code in (502, 504):
                 job.mp.log.error(
-                    f"Push failed with HTTP error {err.http_error}. "
+                    f"Push failed with HTTP error {http_code}. "
                     f"Upload details: {len(job.upload_queue_items)} file chunks, total size {job.total_size} bytes."
                 )
+                job.mp.log.error("Files:")
+                for f in job.changes.get("added", []) + job.changes.get("updated", []):
+                    path = f.get("path", "<unknown>")
+                    size = f.get("size", "?")
+                    if "diff" in f:
+                        diff_info = f.get("diff", {})
+                        diff_size = diff_info.get("size", "?")
+                        # best-effort: number of geodiff changes (if available)
+                        changes_cnt = _geodiff_changes_count(job.mp, diff_info.get("path"))
+                        if changes_cnt is None:
+                            job.mp.log.error(f"  - {path}, size={size}, diff_size={diff_size}, changes=n/a")
+                        else:
+                            job.mp.log.error(f"  - {path}, size={size}, diff_size={diff_size}, changes={changes_cnt}")
+                    else:
+                        job.mp.log.error(f"  - {path}, size={size}")
+
             # server returns various error messages with filename or something generic
-            # it would be better if it returned list of failed files (and reasons) whenever possible    
+            # it would be better if it returned list of failed files (and reasons) whenever possible
             job.mp.log.error("--- push finish failed! " + str(err))
 
             # if push finish fails, the transaction is not killed, so we
@@ -303,6 +320,36 @@ def push_project_finalize(job):
     job.mp.log.info("--- push finished - new project version " + job.server_resp["version"])
 
 
+def _geodiff_changes_count(mp, diff_rel_path):
+    """
+    Best-effort: return number of changes in the .gpkg diff (int) or None.
+    Never raises – diagnostics/logging must not fail.
+    """
+    if not diff_rel_path:
+        return None
+    try:
+        diff_abs = mp.fpath_meta(diff_rel_path)
+    except Exception:
+        return None
+
+    try:
+        from pygeodiff import GeoDiff
+        return GeoDiff().changes_count(diff_abs)
+    except Exception:
+        pass
+
+    try:
+        import geodiff  # optional fallback
+        if hasattr(geodiff, "changes_count"):
+            try:
+                return geodiff.changes_count(diff_abs)
+            except Exception:
+                return None
+    except Exception:
+        return None
+
+    return None
+
 def push_project_cancel(job):
     """
     To be called (from main thread) to cancel a job that has uploads in progress.

mergin/test/test_push_logging.py

@@ -0,0 +1,108 @@
+import logging
+import tempfile
+from pathlib import Path
+from types import SimpleNamespace
+
+import pytest
+
+
+@pytest.mark.parametrize("status_code", [502, 504])
+def test_push_finalize_logs_on_5xx_real_diff(caplog, status_code):
+    # geodiff is required – if missing, skip the test gracefully
+    pytest.importorskip("pygeodiff")
+    from pygeodiff import GeoDiff
+
+    from mergin.client_push import push_project_finalize
+    from mergin.common import ClientError
+
+    # --- exact paths according to your repo structure ---
+    data_dir = Path(__file__).resolve().parent / "test_data"
+    base = data_dir / "base.gpkg"
+    modified = data_dir / "inserted_1_A.gpkg"
+
+    # if something is missing, print the directory contents and FAIL
+    if not base.exists() or not modified.exists():
+        print("[DEBUG] data_dir:", data_dir)
+        print("[DEBUG] data_dir.exists():", data_dir.exists())
+        if data_dir.exists():
+            print("[DEBUG] contents:", [p.name for p in sorted(data_dir.iterdir())])
+        pytest.fail(f"Expected files are not available: base={base.exists()} inserted_1_A={modified.exists()}")
+
+    # --- create a real .diff (changeset) ---
+    with tempfile.TemporaryDirectory(prefix="geodiff-") as tmpdir:
+        tmpdir = Path(tmpdir)
+        diff_path = tmpdir / "base_to_inserted_1_A.diff"
+
+        gd = GeoDiff()
+        gd.create_changeset(str(base), str(modified), str(diff_path))
+        diff_size = diff_path.stat().st_size
+        file_size = modified.stat().st_size
+
+        # --- logger captured by caplog ---
+        logger = logging.getLogger("mergin_test")
+        logger.setLevel(logging.DEBUG)
+        logger.propagate = True
+
+        # --- fake mc.post: /finish -> 5xx, /cancel -> ok ---
+        def fake_post(url, *args, **kwargs):
+            if "/finish/" in url:
+                err = ClientError("Gateway error")
+                setattr(err, "http_error", status_code)
+                raise err
+            if "/cancel/" in url:
+                return SimpleNamespace(msg="cancelled")
+            return SimpleNamespace(msg="ok")
+
+        # --- fake future/executor (no exceptions) ---
+        fake_future = SimpleNamespace(done=lambda: True, exception=lambda: None, running=lambda: False)
+        fake_executor = SimpleNamespace(shutdown=lambda wait=True: None)
+
+        # mp.fpath_meta should resolve the relative diff path to an absolute path
+        def fpath_meta(rel):
+            return str(diff_path) if rel == diff_path.name else rel
+
+        # --- minimal job – only what finalize() uses ---
+        job = SimpleNamespace(
+            executor=fake_executor,
+            futures=[fake_future],
+            transferred_size=1234,
+            total_size=1234,
+            upload_queue_items=[1],
+            transaction_id="tx-1",
+            mp=SimpleNamespace(
+                log=logger,
+                update_metadata=lambda *a, **k: None,
+                apply_push_changes=lambda *a, **k: None,
+                fpath_meta=fpath_meta,
+            ),
+            mc=SimpleNamespace(post=fake_post),
+            changes={
+                "added": [],
+                "updated": [{
+                    "path": modified.name,
+                    "size": file_size,
+                    "diff": {"path": diff_path.name, "size": diff_size},
+                    "chunks": [1],
+                }],
+                "removed": [],
+            },
+            tmp_dir=SimpleNamespace(cleanup=lambda: None),
+        )
+
+        # capture ERROR-level logs from "mergin_test"
+        caplog.set_level(logging.ERROR, logger="mergin_test")
+
+        # --- call production logic (expecting 5xx) ---
+        with pytest.raises(ClientError):
+            push_project_finalize(job)
+
+        text = caplog.text
+
+        assert f"Push failed with HTTP error {status_code}" in text
+        assert "Upload details:" in text
+        assert "Files:" in text
+        assert modified.name in text
+        assert f"size={file_size}" in text
+        assert f"diff_size={diff_size}" in text
+        # helper may compute changes, or log changes=n/a – accept both
+        assert ("changes=" in text) or ("changes=n/a" in text)