extend 502/504 failure logging (#268)

* 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

* eddited after rewiew:
client_push.py removed unnecessary code, specified exceptions, added type hints
test_push_logging.py changeed to use real job


* changes based on review: try-except should catch more geodiff errors and using real MerginProject in test_push_logging.py

* black

Author: DanChov

mergin/client_push.py

@@ -15,6 +15,13 @@
 import tempfile
 import concurrent.futures
 import os
+from pygeodiff import (
+    GeoDiff,
+    GeoDiffLibError,
+    GeoDiffLibConflictError,
+    GeoDiffLibUnsupportedChangeError,
+    GeoDiffLibVersionError,
+)
 
 from .common import UPLOAD_CHUNK_SIZE, ClientError
 from .merginproject import MerginProject
@@ -268,14 +275,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 +327,25 @@ def push_project_finalize(job):
     job.mp.log.info("--- push finished - new project version " + job.server_resp["version"])
 
 
+def _geodiff_changes_count(mp: MerginProject, diff_rel_path: str):
+    """
+    Best-effort: return number of changes in the .gpkg diff (int) or None.
+    Never raises – diagnostics/logging must not fail.
+    """
+
+    diff_abs = mp.fpath_meta(diff_rel_path)
+    try:
+        return GeoDiff().changes_count(diff_abs)
+    except (
+        GeoDiffLibError,
+        GeoDiffLibConflictError,
+        GeoDiffLibUnsupportedChangeError,
+        GeoDiffLibVersionError,
+        FileNotFoundError,
+    ):
+        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,98 @@
+from types import SimpleNamespace
+from pathlib import Path
+import logging
+import tempfile
+from unittest.mock import MagicMock
+import pytest
+
+from pygeodiff import GeoDiff
+from mergin.client_push import push_project_finalize, UploadJob
+from mergin.common import ClientError
+from mergin.merginproject import MerginProject
+from mergin.client import MerginClient
+
+
+@pytest.mark.parametrize("status_code", [502, 504])
+def test_push_finalize_logs_on_5xx_real_diff(caplog, status_code, tmp_path):
+    # test data
+    data_dir = Path(__file__).resolve().parent / "test_data"
+    base = data_dir / "base.gpkg"
+    modified = data_dir / "inserted_1_A.gpkg"
+    assert base.exists() and modified.exists()
+
+    # real MerginProject in temp dir
+    proj_dir = tmp_path / "proj"
+    meta_dir = proj_dir / ".mergin"
+    meta_dir.mkdir(parents=True)
+    mp = MerginProject(str(proj_dir))
+
+    # route MP logs into pytest caplog
+    logger = logging.getLogger("mergin_test")
+    logger.setLevel(logging.DEBUG)
+    logger.propagate = True
+    caplog.set_level(logging.ERROR, logger="mergin_test")
+    mp.log = logger
+
+    # generate a real diff into .mergin/
+    diff_path = meta_dir / "base_to_inserted_1_A.diff"
+    GeoDiff().create_changeset(str(base), str(modified), str(diff_path))
+    diff_rel = diff_path.name
+    diff_size = diff_path.stat().st_size
+    file_size = modified.stat().st_size
+
+    # mock MerginClient: only patch post(); simulate 5xx on finish
+    tx = "tx-1"
+
+    def mc_post(url, *args, **kwargs):
+        if url.endswith(f"/v1/project/push/finish/{tx}"):
+            err = ClientError("Gateway error")
+            setattr(err, "http_error", status_code)  # emulate HTTP code on the exception
+            raise err
+        if url.endswith(f"/v1/project/push/cancel/{tx}"):
+            return SimpleNamespace(msg="cancelled")
+        return SimpleNamespace(msg="ok")
+
+    mc = MagicMock(spec=MerginClient)
+    mc.post.side_effect = mc_post
+
+    tmp_dir = tempfile.TemporaryDirectory(prefix="python-api-client-test-")
+
+    # build a real UploadJob that references the diff/file sizes
+    job = UploadJob(
+        project_path="u/p",
+        changes={
+            "added": [],
+            "updated": [
+                {
+                    "path": modified.name,
+                    "size": file_size,
+                    "diff": {"path": diff_rel, "size": diff_size},
+                    "chunks": [1],
+                }
+            ],
+            "removed": [],
+        },
+        transaction_id=tx,
+        mp=mp,
+        mc=mc,
+        tmp_dir=tmp_dir,
+    )
+
+    job.total_size = 1234
+    job.transferred_size = 1234
+    job.upload_queue_items = [1]
+    job.executor = SimpleNamespace(shutdown=lambda wait=True: None)
+    job.futures = [SimpleNamespace(done=lambda: True, exception=lambda: None, running=lambda: False)]
+    job.server_resp = {"version": "n/a"}
+
+    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
+    assert ("changes=" in text) or ("changes=n/a" in text)

scripts/update_version.py

@@ -4,7 +4,7 @@
 
 
 def replace_in_file(filepath, regex, sub):
-    with open(filepath, 'r') as f:
+    with open(filepath, "r") as f:
         content = f.read()
 
     content_new = re.sub(regex, sub, content, flags=re.M)
@@ -15,14 +15,14 @@ def replace_in_file(filepath, regex, sub):
 
 dir_path = os.path.dirname(os.path.realpath(__file__))
 parser = argparse.ArgumentParser()
-parser.add_argument('--version', help='version to replace')
+parser.add_argument("--version", help="version to replace")
 args = parser.parse_args()
 ver = args.version
 print("using version " + ver)
 
 about_file = os.path.join(dir_path, os.pardir, "mergin", "version.py")
 print("patching " + about_file)
-replace_in_file(about_file, "__version__\s=\s\".*", "__version__ = \"" + ver + "\"")
+replace_in_file(about_file, '__version__\s=\s".*', '__version__ = "' + ver + '"')
 
 setup_file = os.path.join(dir_path, os.pardir, "setup.py")
 print("patching " + setup_file)

setup.py

@@ -4,35 +4,31 @@
 from setuptools import setup, find_packages
 
 setup(
-    name='mergin-client',
-    version='0.10.5',
-    url='https://github.com/MerginMaps/python-api-client',
-    license='MIT',
-    author='Lutra Consulting Ltd.',
-    author_email='info@merginmaps.com',
-    description='Mergin Maps utils and client',
-    long_description='Mergin Maps utils and client',
-
+    name="mergin-client",
+    version="0.10.5",
+    url="https://github.com/MerginMaps/python-api-client",
+    license="MIT",
+    author="Lutra Consulting Ltd.",
+    author_email="info@merginmaps.com",
+    description="Mergin Maps utils and client",
+    long_description="Mergin Maps utils and client",
     packages=find_packages(),
-
-    platforms='any',
+    platforms="any",
     install_requires=[
-        'python-dateutil==2.8.2',
-        'pygeodiff==2.0.4',
-        'pytz==2022.1',
-        'click==8.1.3',
+        "python-dateutil==2.8.2",
+        "pygeodiff==2.0.4",
+        "pytz==2022.1",
+        "click==8.1.3",
     ],
-
     entry_points={
-        'console_scripts': ['mergin=mergin.cli:cli'],
+        "console_scripts": ["mergin=mergin.cli:cli"],
     },
-
     classifiers=[
-        'Development Status :: 5 - Production/Stable',
-        'Intended Audience :: Developers',
-        'License :: OSI Approved :: MIT License',
-        'Operating System :: OS Independent',
-        'Programming Language :: Python :: 3'
+        "Development Status :: 5 - Production/Stable",
+        "Intended Audience :: Developers",
+        "License :: OSI Approved :: MIT License",
+        "Operating System :: OS Independent",
+        "Programming Language :: Python :: 3",
     ],
-    package_data={'mergin': ['cert.pem']}
+    package_data={"mergin": ["cert.pem"]},
 )