You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #1 removed cross-run resume of partial downloads. get_downloaded_file() now deletes any
leftover .tmp before starting, and cleans up on BaseException so a Ctrl-C leaves nothing
behind.
That was the right call for correctness. Resume is by byte offset, the only way to reach the
download block at all is that the remote bytes changed, and an orphaned .tmp carries no
record of which version its bytes came from — so resuming it appended the new file's tail to the
old file's prefix. _save_meta() then stamped the splice with the correct new ETag, so every
later freshness check passed and the corruption was permanent until the Babel release changed.
The cost is real, though: Concord.parquet and Identifiers.parquet are multi-gigabyte files, and a
Ctrl-C or a dropped connection that outlasts the retry loop now means starting from zero. In-run
retries still resume, so this only bites across process invocations.
What would make it safe
Persist the validator next to the partial file, so a later run can prove the bytes on disk belong
to the file it is about to fetch:
When a download starts writing, record the response's ETag (else Last-Modified) in a
sidecar beside the .tmp.
On a later run, if both a .tmp and its sidecar are present, resume with If-Range: <stored validator>. A changed file answers 200, and the existing
"server doesn't support resume" branch already truncates and restarts — so the unsafe path
collapses into a path we already handle.
With no sidecar, or no validator in it, keep today's behaviour and delete the .tmp.
Pitfalls found while scoping this
Cleanup semantics invert. The except BaseException handler in get_downloaded_file()
currently exists to guarantee no resumable partial survives an interrupt. It would have to keep
the .tmpand its sidecar instead, while still discarding both when the failure means the
bytes are untrustworthy. Getting this backwards silently reintroduces the original bug.
sync_cache_version() globs duckdb/*.meta, which would also match a *.tmp.meta
sidecar and hand _load_meta() a path that is not a cached Parquet. The sweep needs to exclude
them, and delete them alongside the .tmp files it already removes.
Orphan sidecars need a lifetime. A .tmp.meta whose .tmp is gone (or vice versa) should
be treated as no sidecar at all, not as a licence to resume.
It re-enters exactly the code path that PR just hardened, inverts the interrupt-cleanup rule, and
adds a new on-disk file with its own lifecycle — enough surface that it deserves its own review
rather than riding along on the fixes.
See CLAUDE.md § "Partial downloads" for the rules any implementation has to keep.
Background
PR #1 removed cross-run resume of partial downloads.
get_downloaded_file()now deletes anyleftover
.tmpbefore starting, and cleans up onBaseExceptionso a Ctrl-C leaves nothingbehind.
That was the right call for correctness. Resume is by byte offset, the only way to reach the
download block at all is that the remote bytes changed, and an orphaned
.tmpcarries norecord of which version its bytes came from — so resuming it appended the new file's tail to the
old file's prefix.
_save_meta()then stamped the splice with the correct new ETag, so everylater freshness check passed and the corruption was permanent until the Babel release changed.
The cost is real, though: Concord.parquet and Identifiers.parquet are multi-gigabyte files, and a
Ctrl-C or a dropped connection that outlasts the retry loop now means starting from zero. In-run
retries still resume, so this only bites across process invocations.
What would make it safe
Persist the validator next to the partial file, so a later run can prove the bytes on disk belong
to the file it is about to fetch:
ETag(elseLast-Modified) in asidecar beside the
.tmp..tmpand its sidecar are present, resume withIf-Range: <stored validator>. A changed file answers 200, and the existing"server doesn't support resume" branch already truncates and restarts — so the unsafe path
collapses into a path we already handle.
.tmp.Pitfalls found while scoping this
except BaseExceptionhandler inget_downloaded_file()currently exists to guarantee no resumable partial survives an interrupt. It would have to keep
the
.tmpand its sidecar instead, while still discarding both when the failure means thebytes are untrustworthy. Getting this backwards silently reintroduces the original bug.
sync_cache_version()globsduckdb/*.meta, which would also match a*.tmp.metasidecar and hand
_load_meta()a path that is not a cached Parquet. The sweep needs to excludethem, and delete them alongside the
.tmpfiles it already removes..tmp.metawhose.tmpis gone (or vice versa) shouldbe treated as no sidecar at all, not as a licence to resume.
_full_content_length()must keep recording the whole file's length rather than the range's.Why not in PR #1
It re-enters exactly the code path that PR just hardened, inverts the interrupt-cleanup rule, and
adds a new on-disk file with its own lifecycle — enough surface that it deserves its own review
rather than riding along on the fixes.
See
CLAUDE.md§ "Partial downloads" for the rules any implementation has to keep.