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()