Skip to content

fix(sync): avoid UnboundLocalError in the upload_file error handler - #350

Closed
devgtv wants to merge 1 commit into
AlertaDengue:mainfrom
devgtv:fix/sync-upload-file-unbound-raw-path
Closed

devgtv wants to merge 1 commit into
AlertaDengue:mainfrom
devgtv:fix/sync-upload-file-unbound-raw-path

Conversation

@devgtv

@devgtv devgtv commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

SyncEngine.upload_file() ends with an except BaseException handler that cleans up both the raw download and the converted parquet:

except BaseException as exc:  # noqa
    for conn in connections:
        try:
            conn.close()
        except Exception:  # noqa
            pass
    self._cleanup_local(raw_path)   # <-- raw_path may be unbound here
    if parquet_file is not None:
        self._cleanup_local(parquet_file.path)
    raise exc

parquet_file = None was pre-initialised, but raw_path was not — its first binding is the _download_raw_with_retry() call partway into the block. Any failure raised before that point made the handler itself raise UnboundLocalError: cannot access local variable 'raw_path'.

Impact

The handler's own exception replaced the real one, because _reprocess surfaces whatever propagates. A failure in writer._ensure_management_columns() (the ALTER TABLE), in get_file_full(), or on a broken connection was reported to the operator as:

failed: X.dbc: cannot access local variable 'raw_path'

which points at PySUS internals instead of the actual cause. The cleanup also aborted before running, so the temp file was orphaned on disk.

Fix

Both names are bound to None before the try block, and the handler guards on raw_path the same way it already did for parquet_file.

Tests

Two tests added to TestUploadFile:

  • test_error_before_download_propagates_original — an error from _ensure_management_columns propagates with its original type and message
  • test_error_after_download_cleans_up_raw_file — an error while hashing the download still removes the temp file

I verified both fail with UnboundLocalError against the unpatched handler and pass after the fix.

  • 1735 passed, 6 skipped
  • black and isort clean

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants