From f1ae685c92414bade70aa91ad7b6f43b25fba571 Mon Sep 17 00:00:00 2001 From: devgtv Date: Tue, 29 Sep 2026 15:27:54 -0300 Subject: [PATCH] fix(sync): avoid UnboundLocalError in the upload_file error handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upload_file()'s `except BaseException` handler cleans up both the raw download and the converted parquet, but only parquet_file was pre-initialised. raw_path is first bound at the `_download_raw_with_retry` call, so any failure raised before that point made the handler itself raise `UnboundLocalError: cannot access local variable 'raw_path'`. That replaced the real exception with a misleading message, since `_reprocess` surfaces whatever the handler propagates — the operator saw "cannot access local variable 'raw_path'" instead of the actual cause. The temp file was also orphaned, as the cleanup aborted before running. Both names are now bound to None before the try block and the handler guards on raw_path, matching the existing parquet_file handling. Failures before the download now propagate their original type and message. Tests cover both paths: an error raised by _ensure_management_columns (before the download) propagates unchanged, and an error raised while hashing the download (after it) still removes the temp file. --- pysus/management/sync.py | 7 ++- pysus/tests/management/test_sync_more.py | 54 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/pysus/management/sync.py b/pysus/management/sync.py index fa84db25..fda0ccb0 100644 --- a/pysus/management/sync.py +++ b/pysus/management/sync.py @@ -454,6 +454,10 @@ async def upload_file( columns_conn = columns_adapter.raw_connection() connections = (central_conn, dataset_conn, columns_conn) + # Both are read by the `except` handler below, which runs for + # failures raised anywhere in the block — including the ones that + # happen before either name is bound. + raw_path: Path | None = None parquet_file = None try: with central_conn, dataset_conn, columns_conn: @@ -576,7 +580,8 @@ def _upload_callback(processed: int, total: int) -> None: conn.close() except Exception: # noqa pass - self._cleanup_local(raw_path) + if raw_path is not None: + self._cleanup_local(raw_path) if parquet_file is not None: self._cleanup_local(parquet_file.path) raise exc diff --git a/pysus/tests/management/test_sync_more.py b/pysus/tests/management/test_sync_more.py index 70df5ad9..d2c4a48e 100644 --- a/pysus/tests/management/test_sync_more.py +++ b/pysus/tests/management/test_sync_more.py @@ -524,6 +524,60 @@ async def test_requires_ducklake(self, engine): with pytest.raises(Exception, match="not connected"): await engine.upload_file(f) + @pytest.mark.asyncio + async def test_error_before_download_propagates_original( + self, engine, tmp_path + ): + """A failure before ``raw_path`` is bound must not be masked. + + The ``except BaseException`` handler cleans up ``raw_path``. When + the failure happens earlier in the block (here, the ALTER TABLE + issued by ``_ensure_management_columns``), the name is unbound and + the handler itself raised ``UnboundLocalError``, replacing the real + cause with a misleading message. + """ + engine._ducklake = self._ducklake() + + writer = self._writer() + writer._ensure_management_columns.side_effect = RuntimeError( + "alter table blew up" + ) + + with patch.object( + SyncEngine, "writer", new_callable=PropertyMock + ) as prop: + prop.return_value = writer + with pytest.raises(RuntimeError, match="alter table blew up"): + await engine.upload_file(_remote_file()) + + @pytest.mark.asyncio + async def test_error_after_download_cleans_up_raw_file( + self, engine, tmp_path + ): + """A failure after the download must still remove the temp file.""" + raw_path = tmp_path / "X.dbc" + raw_path.write_bytes(b"partial") + + engine._ducklake = self._ducklake() + engine._download_raw_with_retry = AsyncMock(return_value=raw_path) + + writer = self._writer() + writer.get_file_full.return_value = None + with ( + patch.object( + SyncEngine, "writer", new_callable=PropertyMock + ) as prop, + patch( + "pysus.management.sync.sha256_of", + side_effect=OSError("read failed"), + ), + ): + prop.return_value = writer + with pytest.raises(OSError, match="read failed"): + await engine.upload_file(_remote_file()) + + assert not raw_path.exists() + @pytest.mark.asyncio async def test_skips_when_current(self, engine, tmp_path): engine._ducklake = self._ducklake()