Skip to content

fix(saude): never reuse or publish a partial CKAN download - #353

Closed
devgtv wants to merge 1 commit into
AlertaDengue:mainfrom
devgtv:fix/saude-partial-download-reuse
Closed

devgtv wants to merge 1 commit into
AlertaDengue:mainfrom
devgtv:fix/saude-partial-download-reuse

Conversation

@devgtv

@devgtv devgtv commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

download_resource() short-circuits on dest_path.exists() but streamed directly into that path:

if dest_path.exists() and not overwrite:
    return dest_path
...
with dest_path.open("wb") as fh:      # truncated file survives a mid-stream drop
    async for chunk in response.aiter_bytes(...):
        fh.write(chunk)

The sync retry loop could not recover from this. _download_raw_with_retry names its temp file <uuid8>-<basename>, but CKAN/Saude ignores the requested filename and writes filename_for(resource, ...) inside output.parent — as _download_once's own docstring states:

Some origins (e.g. CKAN/Saude resources) ignore the requested filename and write to their own derived name in output.parent

So the retry's self._cleanup_local(output) deleted a path that never existed, and download_resource then immediately returned the truncated file. The engine hashed it, converted it, and uploaded it to S3 as the official artifact.

A second, related hazard: all Saude workers share management/tmp as dest_dir, and filename_for derives the name purely from resource.name. Two packages publishing a resource with the same name resolve to the same dest_path, and two concurrent workers interleave open("wb") writes into one file.

Fix

download_resource() streams into a sibling .<name>.<uuid8>.part file and os.replace()s it on success, unlinking it on any BaseException. The rename is atomic, so concurrent writers can no longer interleave.

_download_raw_with_retry() gives each attempt its own scratch directory and removes the whole directory on retry — the only reliable cleanup when the origin chose the filename itself. No directory is created for the final attempt, so nothing is left behind. _cleanup_stale_tmp() also sweeps these directories (recognised by the new _is_attempt_dir(): 8 hex chars, which nothing else in that tmp area uses), so a killed run does not leak them into the next one.

Tests

test_download.py — new _FlakyTransport yields real bytes and then raises ReadError, so the failure happens after the destination has been written to:

  • failure mid-stream leaves no file and no .part scratch
  • a retry after a failure downloads the full content (previously returned the partial file)
  • an HTTP error leaves no file
  • a complete download leaves no scratch file

test_sync_more.py — new TestDownloadRawWithRetry:

  • a failed attempt does not leak the origin-derived file across retries, and each attempt gets a distinct directory
  • a successful retry returns the fresh path with complete bytes
  • _is_attempt_dir accepts only hex-8 names
  • _cleanup_stale_tmp sweeps leftover attempt directories

Verified the two download tests fail against the previous code.

  • 1741 passed, 6 skipped
  • black and isort clean

download_resource() short-circuits on `dest_path.exists()` but streamed
straight into that path, so a connection dropped mid-transfer left a
truncated file behind. The next attempt returned it as a complete
download.

The sync retry loop could not recover from this: _download_raw_with_retry
names its temp file `<uuid8>-<basename>`, while CKAN/Saude ignores the
requested filename and writes `filename_for(resource, ...)` inside
`output.parent` (as _download_once's own docstring notes). The retry's
cleanup deleted a path that never existed, leaving the partial file in
place for the next attempt to pick up. The engine then hashed it,
converted it and uploaded it to S3 as the official artifact.

Two changes:

download_resource() streams into a sibling `.<name>.<uuid8>.part` file
and os.replace()s it on success, removing it on any BaseException. The
rename is also atomic, so two Saude packages publishing a resource with
the same name -- which resolve to the same dest_path in a shared tmp dir
-- can no longer interleave writes into one file.

_download_raw_with_retry() gives each attempt its own scratch directory
and removes the whole directory on retry, which is the only reliable
cleanup when the origin chose the filename. No directory is created for
the final attempt, so nothing is left behind. _cleanup_stale_tmp() also
sweeps these directories, recognised by _is_attempt_dir(), so a killed
run does not leak them into the next one.
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