Skip to content

download: don't leave half-extracted parts when a download is interrupted, fix #1633 - #1675

Open
Slach wants to merge 1 commit into
masterfrom
fix/download-extract-parts-atomically
Open

Slach wants to merge 1 commit into
masterfrom
fix/download-extract-parts-atomically

Conversation

@Slach

@Slach Slach commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Reopened from #1665 by @RyoKusnadi (original fork was deleted); commit cherry-picked onto current master with authorship preserved.

Problem

After an interrupted download of an incremental backup, running download again can finish without an error, but restore --data then fails with code: 226 ... Marks file ... doesn't exist on some parts. Those parts were required parts of the full backup, and they were only half downloaded.

How it happens: download stopped (killed container, cancelled run) while DownloadCompressedStream was still extracting a required part into backup/<required>/shadow/<db>/<table>/<disk>/<part>. The half-extracted directory stays. On the next run, downloadDiffParts (pkg/backup/download.go, the else branch after os.Stat(existsPath)) sees the directory, takes it as complete, and hard-links it into the incremental backup. This also happens without --resume, for example after delete local <incremental> and a new download.

Cause

DownloadCompressedStream in pkg/storage/general.go extracted straight into the final directory and did not remove the files it had already written when it failed or was cancelled.

Fix

DownloadCompressedStream now extracts into a temporary directory inside localPath (.tmp_extract_*) and moves the top-level entries into place only after the whole archive was read. On any error the temporary directory is removed. So an existing part directory is complete. An entry already in localPath (a leftover from an older version or an earlier attempt) is replaced. This is step 1 of the proposal in the issue; it fixes all callers (required parts, own parts, backup-related dirs) at one place.

Scope

  • Not changed: step 2 of the issue (in downloadDiffParts, remove an existing existsPath that is not in the resumable state). Without --resume that state is empty, so it would re-download every part of an already complete required backup. Leftovers written by older versions are still trusted. Happy to do it as a follow-up if you prefer.
  • If the process is killed hard (not an error return), a .tmp_extract_* directory can stay inside the table directory. It is not a part directory, so it is not linked or restored.

Testing

New tests in pkg/storage/download_atomic_test.go (temp dirs, in-memory tar, no ClickHouse):

  • TestDownloadCompressedStreamInterruptedLeavesNoPartDir: stream cut inside data.bin. Before the fix: Should be empty, but was [20261001_1_1_0]. After: passes.
  • TestDownloadCompressedStreamMovesCompletePartIntoPlace: complete archive replaces an old stale part directory. Before the fix: elements differ (stale.bin stayed). After: passes.
  • go test ./pkg/storage/ passes, go vet ./pkg/storage/ and gofmt -l are clean.
  • Not run: integration/testflows tests (need a live ClickHouse) and the full suite.

Risk

One extra rename per top-level entry after each archive, on the same filesystem. No config or format change. Revert the commit to roll back.

Fixes #1633

🤖 Generated with Claude Code

…1633

Signed-off-by: Ryo Kusnadi <ryokusnadi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants