Skip to content

Resume an interrupted download across runs, safely #15

Description

@gaurav

Background

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:

  1. When a download starts writing, record the response's ETag (else Last-Modified) in a
    sidecar beside the .tmp.
  2. 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.
  3. 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 .tmp and 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.
  • A 206 resumed from a sidecar must still pass the size check added in PR Add babel-explorer: a CLI for querying Babel cross-references via DuckDB and NodeNorm #1, and
    _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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions