Lougarou commented on code in PR #67076:
URL: https://github.com/apache/airflow/pull/67076#discussion_r3299052996
##########
providers/google/src/airflow/providers/google/marketing_platform/operators/bid_manager.py:
##########
@@ -338,19 +340,25 @@ def redirect_request(self, req, fp, code, msg, headers,
newurl):
no_redirect_opener = urllib.request.build_opener(_NoRedirect)
self.log.info("Starting downloading report %s", self.report_id)
with tempfile.NamedTemporaryFile(delete=False) as temp_file:
- with no_redirect_opener.open(file_url) as response: # nosec
- shutil.copyfileobj(response, temp_file, length=self.chunk_size)
-
- temp_file.flush()
- # Upload the local file to bucket
- bucket_name = self._set_bucket_name(self.bucket_name)
- gcs_hook.upload(
- bucket_name=bucket_name,
- object_name=report_name,
- gzip=self.gzip,
- filename=temp_file.name,
- mime_type="text/csv",
- )
+ try:
+ with no_redirect_opener.open(file_url) as response: # nosec
+ shutil.copyfileobj(response, temp_file,
length=self.chunk_size)
+
+ temp_file.flush()
+ # Upload the local file to bucket
+ bucket_name = self._set_bucket_name(self.bucket_name)
+ gcs_hook.upload(
+ bucket_name=bucket_name,
+ object_name=report_name,
+ gzip=self.gzip,
+ filename=temp_file.name,
+ mime_type="text/csv",
+ )
+ finally:
+ # Always remove the unencrypted report left in the temp
directory,
+ # on both the success and the failure path.
+ with contextlib.suppress(OSError):
+ os.unlink(temp_file.name)
Review Comment:
Good catch @shahar1 — addressed in 5c6cdd76. The fix has two parts so the
silent-swallow concern doesn't come back through a different door:
1. **Close before unlink.** `temp_file.close()` is now called inside the
`try` block (and idempotently again in `finally`) so the handle is released
before `os.unlink`. On Windows, `NamedTemporaryFile` keeps the file open until
close — without this, unlink would `PermissionError` exactly as you described.
2. **Don't silently swallow.** Replaced `contextlib.suppress(OSError)` with
an explicit:
- `except FileNotFoundError: pass` — benign, file already gone
- `except OSError as e: self.log.warning(...)` — anything else
(antivirus, NFS EBUSY, …) is now observable instead of hidden
Added three new tests covering the contract: close-before-unlink ordering,
log warning on `PermissionError`, and silent skip on `FileNotFoundError`.
Verified all five cleanup tests pass standalone in a Python 3.10 container.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
